Skip to content

Commit

Permalink
fix(secretsmanager): Secret.fromSecretName doesn't work with ECS
Browse files Browse the repository at this point in the history
The ability to import and reference a Secret purely by the secret name was
introduced in #10309. One of the original requests was modelled after the
integration with CodeBuild, where either the secret name or the full ARN
-- including the SecretsManager-provided suffix -- were accepted, but not a
"partial" ARN without the suffix. To ease integrations with other services
in this case, the `secretArn` was defined as returning the `secretName` for
these secrets imported by name.

However, other services -- like ECS -- require that an ARN format is provided,
even as a partial ARN. This introduces a dual behavior where sometimes the
secretName works and partial ARN fails, and other times the partial ARN works
and the secretName fails.

This change introduces an option to the `fromSecretName` factory to control this
behavior, so users can set up the secret properly for the service they are
integrating with.

*Disclaimer:* - I don't *love* this, and am very open to feedback on alternative
approaches that would also be backwards compatible.

Related changes -- I improved the suffix-strippiung logic of `parseSecretName`
to only strip a suffix if it's exactly 6 characters long, as all SecretsManager
suffixes are 6 characters. This prevents accidentally stripping the last word
off of a hyphenated secret name like 'github-token'.

fixes #10519
  • Loading branch information
njlynch committed Oct 22, 2020
1 parent a73a4ee commit 3744e8b
Show file tree
Hide file tree
Showing 4 changed files with 132 additions and 10 deletions.
13 changes: 11 additions & 2 deletions packages/@aws-cdk/aws-secretsmanager/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -163,17 +163,26 @@ Existing secrets can be imported by ARN, name, and other attributes (including t
Secrets imported by name can used the short-form of the name (without the SecretsManager-provided suffx);
the secret name must exist in the same account and region as the stack.
Importing by name makes it easier to reference secrets created in different regions, each with their own suffix and ARN.
However, different services have different requirements for referencing secrets by name. The `supportPartialSecretArn` changes
how the `secretArn` is calculated from the `secretName`.

```ts
import * as kms from '@aws-cdk/aws-kms';

const secretArn = 'arn:aws:secretsmanager:eu-west-1:111111111111:secret:MySecret-f3gDy9';
const encryptionKey = kms.Key.fromKeyArn(stack, 'MyEncKey', 'arn:aws:kms:eu-west-1:111111111111:key/21c4b39b-fde2-4273-9ac0-d9bb5c0d0030');
const mySecretFromArn = secretsmanager.Secret.fromSecretArn(stack, 'SecretFromArn', secretArn);
const mySecretFromName = secretsmanager.Secret.fromSecretName(stack, 'SecretFromName', 'MySecret') // Note: the -f3gDy9 suffix is optional
const mySecretFromAttrs = secretsmanager.Secret.fromSecretAttributes(stack, 'SecretFromAttributes', {
secretArn,
encryptionKey,
secretName: 'MySecret', // Optional, will be calculated from the ARN
});

// The secretArn will be the secretName. Works with services like CodeBuild.
const mySecretFromName = secretsmanager.Secret.fromSecretName(stack, 'SecretFromName', 'MySecret');
// The secretArn will be in ARN format, but without the SecretsManager suffix
// (e.g., 'arn:aws:secretsmanager:eu-west-1:111111111111:secret:MySecret')
// Works with services like ECS.
const mySecretFromName = secretsmanager.Secret.fromSecretName(stack, 'SecretFromName', 'MySecret', {
supportPartialSecretArn: true,
});
```
38 changes: 30 additions & 8 deletions packages/@aws-cdk/aws-secretsmanager/lib/secret.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,20 @@ abstract class SecretBase extends Resource implements ISecret {
protected get arnForPolicies() { return this.secretArn; }
}

/** Options for `Secret.fromSecretName` */
export interface SecretFromSecretNameOptions {
/**
* Whether the secret imported by name will respond with a "partial" ARN without the Secrets Manager suffix.
* If this is set, `secretArn` will return a composed ARN without the Secrets Manager suffix.
* If not set, 'secretArn' will return the `secretName`.
* Some service integrations accept either the `secretName` or the full ARN with the suffix, whereas others
* accept the partial ARN. This parameter allows controlling which version of the ARN the service receives.
*
* @default - false
*/
readonly supportPartialSecretArn?: boolean;
}

/**
* Creates a new secret in AWS SecretsManager.
*/
Expand All @@ -250,19 +264,23 @@ export class Secret extends SecretBase {
* Imports a secret by secret name; the ARN of the Secret will be set to the secret name.
* A secret with this name must exist in the same account & region.
*/
public static fromSecretName(scope: Construct, id: string, secretName: string): ISecret {
public static fromSecretName(scope: Construct, id: string, secretName: string, options: SecretFromSecretNameOptions = {}): ISecret {
return new class extends SecretBase {
public readonly encryptionKey = undefined;
public readonly secretArn = secretName;
public readonly secretArn = options.supportPartialSecretArn ? this.partialArn : secretName;
public readonly secretName = secretName;
protected readonly autoCreatePolicy = false;
// Overrides the secretArn for grant* methods, where the secretArn must be in ARN format.
// Also adds a wildcard to the resource name to support the SecretsManager-provided suffix.
protected get arnForPolicies() {
return this.partialArn + '*';
}
// Creates a "partial" ARN from the secret name. The "full" ARN would include the SecretsManager-provided suffix.
private get partialArn(): string {
return Stack.of(this).formatArn({
service: 'secretsmanager',
resource: 'secret',
resourceName: this.secretName + '*',
resourceName: secretName,
sep: ':',
});
}
Expand Down Expand Up @@ -299,8 +317,8 @@ export class Secret extends SecretBase {
});

if (props.generateSecretString &&
(props.generateSecretString.secretStringTemplate || props.generateSecretString.generateStringKey) &&
!(props.generateSecretString.secretStringTemplate && props.generateSecretString.generateStringKey)) {
(props.generateSecretString.secretStringTemplate || props.generateSecretString.generateStringKey) &&
!(props.generateSecretString.secretStringTemplate && props.generateSecretString.generateStringKey)) {
throw new Error('`secretStringTemplate` and `generateStringKey` must be specified together.');
}

Expand Down Expand Up @@ -604,9 +622,13 @@ function parseSecretName(construct: IConstruct, secretArn: string) {
return resourceName;
}

// Secret resource names are in the format `${secretName}-${SecretsManager suffix}`
// If there is no hyphen, assume no suffix was provided, and return the whole name.
return resourceName.substr(0, resourceName.lastIndexOf('-')) || resourceName;
// Secret resource names are in the format `${secretName}-${6-character SecretsManager suffix}`
// If there is no hyphen (or 6-character suffix) assume no suffix was provided, and return the whole name.
const lastHyphenIndex = resourceName.lastIndexOf('-');
if (lastHyphenIndex !== -1 && resourceName.substr(lastHyphenIndex + 1).length === 6) {
return resourceName.substr(0, lastHyphenIndex);
}
return resourceName;
}
throw new Error('invalid ARN format; no secret name provided');
}
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-secretsmanager/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@
"awslint": {
"exclude": [
"attribute-tag:@aws-cdk/aws-secretsmanager.Secret.secretName",
"from-signature:@aws-cdk/aws-secretsmanager.Secret.fromSecretName",
"from-signature:@aws-cdk/aws-secretsmanager.SecretTargetAttachment.fromSecretTargetAttachmentSecretArn",
"from-attributes:fromSecretTargetAttachmentAttributes",
"props-physical-name:@aws-cdk/aws-secretsmanager.RotationScheduleProps",
Expand Down
90 changes: 90 additions & 0 deletions packages/@aws-cdk/aws-secretsmanager/test/secret.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,18 @@ test('import by secretArn supports secret ARNs without suffixes', () => {
expect(secret.secretName).toBe('MySecret');
});

test('import by secretArn does not strip suffixes unless the suffix length is six', () => {
// GIVEN
const arnWith5CharacterSuffix = 'arn:aws:secretsmanager:eu-west-1:111111111111:secret:github-token';
const arnWith6CharacterSuffix = 'arn:aws:secretsmanager:eu-west-1:111111111111:secret:github-token-f3gDy9';
const arnWithMultiple6CharacterSuffix = 'arn:aws:secretsmanager:eu-west-1:111111111111:secret:github-token-f3gDy9-acb123';

// THEN
expect(secretsmanager.Secret.fromSecretArn(stack, 'Secret5', arnWith5CharacterSuffix).secretName).toEqual('github-token');
expect(secretsmanager.Secret.fromSecretArn(stack, 'Secret6', arnWith6CharacterSuffix).secretName).toEqual('github-token');
expect(secretsmanager.Secret.fromSecretArn(stack, 'Secret6Twice', arnWithMultiple6CharacterSuffix).secretName).toEqual('github-token-f3gDy9');
});

test('import by secretArn supports tokens for ARNs', () => {
// GIVEN
const app = new cdk.App();
Expand Down Expand Up @@ -511,6 +523,40 @@ test('import by secret name', () => {
expect(stack.resolve(secret.secretValueFromJson('password'))).toBe(`{{resolve:secretsmanager:${secretName}:SecretString:password::}}`);
});

test('import by secret name - supportPartialSecretArn', () => {
// GIVEN
const secretName = 'MySecret';

// WHEN
const secret = secretsmanager.Secret.fromSecretName(stack, 'Secret', secretName, { supportPartialSecretArn: true });

// THEN
expect(secret.secretArn).toBe(`arn:${stack.partition}:secretsmanager:${stack.region}:${stack.account}:secret:MySecret`);
expect(secret.secretName).toBe(secretName);
expect(stack.resolve(secret.secretValue)).toEqual({
'Fn::Join': ['', [
'{{resolve:secretsmanager:arn:',
{ Ref: 'AWS::Partition' },
':secretsmanager:',
{ Ref: 'AWS::Region' },
':',
{ Ref: 'AWS::AccountId' },
':secret:MySecret:SecretString:::}}',
]],
});
expect(stack.resolve(secret.secretValueFromJson('password'))).toEqual({
'Fn::Join': ['', [
'{{resolve:secretsmanager:arn:',
{ Ref: 'AWS::Partition' },
':secretsmanager:',
{ Ref: 'AWS::Region' },
':',
{ Ref: 'AWS::AccountId' },
':secret:MySecret:SecretString:password::}}',
]],
});
});

test('import by secret name with grants', () => {
// GIVEN
const role = new iam.Role(stack, 'Role', { assumedBy: new iam.AccountRootPrincipal() });
Expand Down Expand Up @@ -555,6 +601,50 @@ test('import by secret name with grants', () => {
});
});

test('import by secret name with grants - supportPartialSecretArn', () => {
// GIVEN
const role = new iam.Role(stack, 'Role', { assumedBy: new iam.AccountRootPrincipal() });
const secret = secretsmanager.Secret.fromSecretName(stack, 'Secret', 'MySecret', { supportPartialSecretArn: true });

// WHEN
secret.grantRead(role);
secret.grantWrite(role);

// THEN
const expectedSecretReference = {
'Fn::Join': ['', [
'arn:',
{ Ref: 'AWS::Partition' },
':secretsmanager:',
{ Ref: 'AWS::Region' },
':',
{ Ref: 'AWS::AccountId' },
':secret:MySecret*',
]],
};
expect(stack).toHaveResource('AWS::IAM::Policy', {
PolicyDocument: {
Version: '2012-10-17',
Statement: [{
Action: [
'secretsmanager:GetSecretValue',
'secretsmanager:DescribeSecret',
],
Effect: 'Allow',
Resource: expectedSecretReference,
},
{
Action: [
'secretsmanager:PutSecretValue',
'secretsmanager:UpdateSecret',
],
Effect: 'Allow',
Resource: expectedSecretReference,
}],
},
});
});

test('can attach a secret with attach()', () => {
// GIVEN
const secret = new secretsmanager.Secret(stack, 'Secret');
Expand Down

0 comments on commit 3744e8b

Please sign in to comment.