Sanitize ssm paths (#874)

edwardfoyle · web-flow · commit 5678ab4d44eb · 2024-01-05T16:49:38.000-08:00
diff --git a/.changeset/chatty-balloons-sing.md b/.changeset/chatty-balloons-sing.md
@@ -0,0 +1,9 @@
+---
+'@aws-amplify/platform-core': minor
+---
+
+Sanitize invalid characters when constructing SSM parameter paths.
+Uses the same convention that is used for sanitizing stack names.
+
+**NOTE:** Any secrets created before this change will no longer be found.
+Recreate sandbox secrets using `npx amplify sandbox secret set` and recreate branch secrets in the Amplify console.
diff --git a/.changeset/early-maps-live.md b/.changeset/early-maps-live.md
@@ -0,0 +1,7 @@
+---
+'@aws-amplify/backend-function': minor
+'@aws-amplify/backend-secret': minor
+'@aws-amplify/backend': minor
+---
+
+Consume parameter resolution changes from @aws-amplify/platform-core
diff --git a/packages/backend-function/src/factory.test.ts b/packages/backend-function/src/factory.test.ts
@@ -280,7 +280,8 @@ void describe('AmplifyFunctionFactory', () => {
             TEST_VAR: 'testValue',
             TEST_SECRET: '<value will be resolved during runtime>',
             AMPLIFY_SECRET_PATHS: JSON.stringify({
-              TEST_SECRET: '/amplify/testBackendId/testBranchName/secretValue',
+              TEST_SECRET:
+                '/amplify/testBackendId/testBranchName-branch-e482a1c36f/secretValue',
             }),
           },
         },
diff --git a/packages/backend-secret/src/ssm_secret.test.ts b/packages/backend-secret/src/ssm_secret.test.ts
@@ -19,7 +19,7 @@ const testSecretName = 'testSecretName';
 const testSecretValue = 'testSecretValue';
 const testSecretLastUpdated = new Date(1234567);
 const testSecretVersion = 20;
-const testBranchPath = `/amplify/${testBackendId}/${testBranchName}`;
+const testBranchPath = `/amplify/${testBackendId}/${testBranchName}-branch-e482a1c36f`;
 const testSharedPath = `/amplify/${shared}/${testBackendId}`;
 const testBranchSecretFullNamePath = `${testBranchPath}/${testSecretName}`;
 const testSharedSecretFullNamePath = `${testSharedPath}/${testSecretName}`;
diff --git a/packages/backend/src/engine/backend-secret/backend_secret_fetcher_factory.ts b/packages/backend/src/engine/backend-secret/backend_secret_fetcher_factory.ts
@@ -58,6 +58,7 @@ export class BackendSecretFetcherFactory {
     const customResourceProps: SecretResourceProps = {
       namespace: backendIdentifier.namespace,
       name: backendIdentifier.name,
+      type: backendIdentifier.type,
       secretName: secretName,
     };
 
diff --git a/packages/backend/src/engine/backend-secret/backend_secret_fetcher_provider_factory.ts b/packages/backend/src/engine/backend-secret/backend_secret_fetcher_provider_factory.ts
@@ -7,6 +7,7 @@ import { Runtime as LambdaRuntime } from 'aws-cdk-lib/aws-lambda';
 import { Provider } from 'aws-cdk-lib/custom-resources';
 import { fileURLToPath } from 'url';
 import { BackendIdentifier } from '@aws-amplify/plugin-types';
+import { ParameterPathConversions } from '@aws-amplify/platform-core';
 
 const filename = fileURLToPath(import.meta.url);
 const dirname = path.dirname(filename);
@@ -41,13 +42,19 @@ export class BackendSecretFetcherProviderFactory {
       handler: 'handler',
     });
 
+    const backendParameterPrefix =
+      ParameterPathConversions.toParameterPrefix(backendIdentifier);
+    const sharedParameterPrefix = ParameterPathConversions.toParameterPrefix(
+      backendIdentifier.namespace
+    );
+
     secretLambda.grantPrincipal.addToPrincipalPolicy(
       new iam.PolicyStatement({
         effect: iam.Effect.ALLOW,
         actions: ['ssm:GetParameter'],
         resources: [
-          `arn:aws:ssm:*:*:parameter/amplify/${backendIdentifier.namespace}/${backendIdentifier.name}/*`,
-          `arn:aws:ssm:*:*:parameter/amplify/shared/${backendIdentifier.namespace}/*`,
+          `arn:aws:ssm:*:*:parameter${backendParameterPrefix}/*`,
+          `arn:aws:ssm:*:*:parameter${sharedParameterPrefix}/*`,
         ],
       })
     );
diff --git a/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher.test.ts b/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher.test.ts
@@ -42,8 +42,7 @@ const customResourceEventCommon = {
   PhysicalResourceId: 'physicalId',
   ResourceType: 'AWS::CloudFormation::CustomResource',
   ResourceProperties: {
-    namespace: testBackendId,
-    name: testBranchName,
+    ...testBackendIdentifier,
     secretName: testSecretName,
     ServiceToken: 'token',
   },
diff --git a/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher.ts b/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher.ts
@@ -56,7 +56,7 @@ export const handleCreateUpdateEvent = async (
       {
         namespace: props.namespace,
         name: props.name,
-        type: 'branch',
+        type: props.type,
       },
       {
         name: props.secretName,
@@ -74,7 +74,7 @@ export const handleCreateUpdateEvent = async (
     }
   }
 
-  // if the secret is not available in branch path, retrieve it at app-level.
+  // if the secret is not available in branch path, try retrieving it at the app-level.
   if (!secret) {
     try {
       const resp = await secretClient.getSecret(props.namespace, {
diff --git a/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher_types.ts b/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher_types.ts
@@ -1,5 +1,5 @@
-export type SecretResourceProps = {
-  namespace: string;
-  name: string;
+import { BackendIdentifier } from '@aws-amplify/plugin-types';
+
+export type SecretResourceProps = Omit<BackendIdentifier, 'hash'> & {
   secretName: string;
 };
diff --git a/packages/platform-core/src/backend_identifier_conversions.test.ts b/packages/platform-core/src/backend_identifier_conversions.test.ts
@@ -4,7 +4,7 @@ import assert from 'node:assert';
 import { BackendIdentifier } from '@aws-amplify/plugin-types';
 
 void describe('toStackName', () => {
-  void it('removes non-alphanumeric chars from namespace and instance', () => {
+  void it('removes non-alphanumeric chars from namespace and name', () => {
     const actual = BackendIdentifierConversions.toStackName({
       namespace: 't-_e.s,@ t--T@,,  !@#$%^&*(){}:":<>?/|\\[]   H/I. .S',
       name: 't-_h.i,@ ng1-&&%2@@3-   _',
diff --git a/packages/platform-core/src/parameter_path_conversions.test.ts b/packages/platform-core/src/parameter_path_conversions.test.ts
@@ -9,13 +9,75 @@ void describe('toParameterPrefix', () => {
       name: 'userName',
       type: 'sandbox',
     });
-    assert.equal(actual, '/amplify/reasonableName/userName');
+    assert.equal(actual, '/amplify/reasonableName/userName-sandbox-99c4c0494a');
   });
 
   void it('passes through values for shared parameter', () => {
     const actual = ParameterPathConversions.toParameterPrefix('someAppId');
     assert.equal(actual, '/amplify/shared/someAppId');
   });
+
+  void it('removes non-alphanumeric chars from namespace and name', () => {
+    const actual = ParameterPathConversions.toParameterPrefix({
+      namespace: 't-_e.s,@ t--T@,,  !@#$%^&*(){}:":<>?/|\\[]   H/I. .S',
+      name: 't-_h.i,@ ng1-&&%2@@3-   _',
+      type: 'branch',
+    });
+    // eslint-disable-next-line spellcheck/spell-checker
+    assert.equal(actual, '/amplify/testTHIS/thing123-branch-70899a9dd1');
+  });
+
+  void it('truncates long name', () => {
+    const actual = ParameterPathConversions.toParameterPrefix({
+      namespace: 'reasonableName',
+      name: 'InsanelyLongUserNameProvidedByCustomerDoNotKnowWhatCustomersAreThinkingWhenChoosingThisGreatBigName',
+      type: 'sandbox',
+    });
+    assert.equal(
+      actual,
+      '/amplify/reasonableName/InsanelyLongUserNameProvidedByCustomerDoNotKnowWha-sandbox-25ed708259'
+    );
+  });
+
+  void it('truncates long namespace', () => {
+    const actual = ParameterPathConversions.toParameterPrefix({
+      namespace:
+        'InsanelyLongNamespaceProvidedByCustomerDoNotKnowWhatCustomersAreThinkingWhenChoosingThisGreatBigNameButItIsStillTheoreticallyPossible',
+      name: 'userName',
+      type: 'sandbox',
+    });
+    assert.equal(
+      actual,
+      '/amplify/InsanelyLongNamespaceProvidedByCustomerDoNotKnowWhatCustomersAreThinkingWhenChoosingThisGre/userName-sandbox-119c592989'
+    );
+    assert.ok(actual.length <= 128);
+  });
+
+  void it('truncates long namespace and name', () => {
+    const actual = ParameterPathConversions.toParameterPrefix({
+      namespace:
+        'InsanelyLongNameProvidedByCustomerDoNotKnowWhatCustomersAreThinkingWhenChoosingThisGreatBigNameButItIsStillTheoreticallyPossible',
+      name: 'InsanelyLongUserNameProvidedByCustomerDoNotKnowWhatCustomersAreThinkingWhenChoosingThisGreatBigName',
+
+      type: 'sandbox',
+    });
+    assert.equal(
+      actual,
+      // eslint-disable-next-line spellcheck/spell-checker
+      '/amplify/InsanelyLongNameProvidedByCustomerDoNotKnowWhatCu/InsanelyLongUserNameProvidedByCustomerDoNotKnowWha-sandbox-4abbed907b'
+    );
+    assert.ok(actual.length <= 128);
+  });
+
+  void it('passes through values within the constraints', () => {
+    const actual = ParameterPathConversions.toParameterPrefix({
+      namespace: 'reasonableName',
+      name: 'userName',
+      type: 'sandbox',
+      hash: 'testHash',
+    });
+    assert.equal(actual, '/amplify/reasonableName/userName-sandbox-testHash');
+  });
 });
 
 void describe('toParameterFullPath', () => {
@@ -28,7 +90,10 @@ void describe('toParameterFullPath', () => {
       },
       'secretName'
     );
-    assert.equal(actual, '/amplify/reasonableName/userName/secretName');
+    assert.equal(
+      actual,
+      '/amplify/reasonableName/userName-sandbox-99c4c0494a/secretName'
+    );
   });
 
   void it('passes through values for shared parameter', () => {
diff --git a/packages/platform-core/src/parameter_path_conversions.ts b/packages/platform-core/src/parameter_path_conversions.ts
@@ -1,4 +1,5 @@
 import { AppId, BackendIdentifier } from '@aws-amplify/plugin-types';
+import { BackendIdentifierConversions } from './backend_identifier_conversions.js';
 
 const SHARED_SECRET = 'shared';
 
@@ -11,7 +12,7 @@ export class ParameterPathConversions {
    */
   static toParameterPrefix(backendId: BackendIdentifier | AppId): string {
     if (typeof backendId === 'object') {
-      return getBranchParameterPrefix(backendId);
+      return getBackendParameterPrefix(backendId);
     }
     return getSharedParameterPrefix(backendId);
   }
@@ -24,7 +25,7 @@ export class ParameterPathConversions {
     secretName: string
   ): string {
     if (typeof backendId === 'object') {
-      return getBranchParameterFullPath(backendId, secretName);
+      return getBackendParameterFullPath(backendId, secretName);
     }
     return getSharedParameterFullPath(backendId, secretName);
   }
@@ -33,18 +34,28 @@ export class ParameterPathConversions {
 /**
  * Get a branch-specific parameter prefix.
  */
-const getBranchParameterPrefix = (parts: BackendIdentifier): string => {
-  return `/amplify/${parts.namespace}/${parts.name}`;
+const getBackendParameterPrefix = (parts: BackendIdentifier): string => {
+  // round trip the backend id through the stack name conversion to ensure we are applying the same sanitization to SSM paths
+  const sanitizedBackendId = BackendIdentifierConversions.fromStackName(
+    BackendIdentifierConversions.toStackName(parts)
+  );
+  if (!sanitizedBackendId || !sanitizedBackendId.hash) {
+    // this *should* never happen
+    throw new Error(
+      `Could not sanitize the backendId to construct the parameter path`
+    );
+  }
+  return `/amplify/${sanitizedBackendId.namespace}/${sanitizedBackendId.name}-${sanitizedBackendId.type}-${sanitizedBackendId.hash}`;
 };
 
 /**
  * Get a branch-specific parameter full path.
  */
-const getBranchParameterFullPath = (
+const getBackendParameterFullPath = (
   backendIdentifier: BackendIdentifier,
   secretName: string
 ): string => {
-  return `${getBranchParameterPrefix(backendIdentifier)}/${secretName}`;
+  return `${getBackendParameterPrefix(backendIdentifier)}/${secretName}`;
 };
 
 /**

Original file line number	Diff line number	Diff line change
`@@ -56,7 +56,7 @@ export const handleCreateUpdateEvent = async (`
`56`	`56`	`{`
`57`	`57`	`namespace: props.namespace,`
`58`	`58`	`name: props.name,`
`59`		`- type: 'branch',`
	`59`	`+ type: props.type,`
`60`	`60`	`},`
`61`	`61`	`{`
`62`	`62`	`name: props.secretName,`
`@@ -74,7 +74,7 @@ export const handleCreateUpdateEvent = async (`
`74`	`74`	`}`
`75`	`75`	`}`
`76`	`76`
`77`		`- // if the secret is not available in branch path, retrieve it at app-level.`
	`77`	`+ // if the secret is not available in branch path, try retrieving it at the app-level.`
`78`	`78`	`if (!secret) {`
`79`	`79`	`try {`
`80`	`80`	`const resp = await secretClient.getSecret(props.namespace, {`