Skip to content

Commit

Permalink
fix(secretmanager) Allow grantRead in the same account
Browse files Browse the repository at this point in the history
  • Loading branch information
kaiz-io committed Jan 9, 2022
1 parent cac11bb commit 3603de0
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 2 deletions.
6 changes: 4 additions & 2 deletions packages/@aws-cdk/aws-secretsmanager/lib/secret.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as iam from '@aws-cdk/aws-iam';
import * as kms from '@aws-cdk/aws-kms';
import { ArnFormat, FeatureFlags, Fn, IResource, Lazy, RemovalPolicy, Resource, SecretValue, Stack, Token } from '@aws-cdk/core';
import { ArnFormat, FeatureFlags, Fn, IResource, Lazy, RemovalPolicy, Resource, SecretValue, Stack, Token, TokenComparison } from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { IConstruct, Construct } from 'constructs';
import { ResourcePolicy } from './policy';
Expand Down Expand Up @@ -306,8 +306,10 @@ abstract class SecretBase extends Resource implements ISecret {
);
}

const crossAccount = Token.compareStrings(Stack.of(this).account, grantee.grantPrincipal.principalAccount || '');

// Throw if secret is not imported and it's shared cross account and no KMS key is provided
if (this instanceof Secret && result.resourceStatement && !this.encryptionKey) {
if (this instanceof Secret && result.resourceStatement && (!this.encryptionKey && crossAccount === TokenComparison.DIFFERENT)) {
throw new Error('KMS Key must be provided for cross account access to Secret');
}

Expand Down
40 changes: 40 additions & 0 deletions packages/@aws-cdk/aws-secretsmanager/test/secret.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,46 @@ describe('secretStringBeta1', () => {
});

test('grantRead', () => {
// GIVEN
const secret = new secretsmanager.Secret(stack, 'Secret');
const role = new iam.Role(stack, 'Role', { assumedBy: new iam.AccountRootPrincipal() });

// WHEN
secret.grantRead(role);

// THEN
expect(stack).toHaveResource('AWS::IAM::Policy', {
PolicyDocument: {
Version: '2012-10-17',
Statement: [{
Action: [
'secretsmanager:GetSecretValue',
'secretsmanager:DescribeSecret',
],
Effect: 'Allow',
Resource: { Ref: 'SecretA720EF05' },
}],
},
});
});

test('Error when grantRead with different role and no KMS', () => {
// GIVEN
const testStack = new cdk.Stack(app, 'TestStack', {
env: {
account: '123456789012',
},
});
const secret = new secretsmanager.Secret(testStack, 'Secret');
const role = iam.Role.fromRoleArn(testStack, 'RoleFromArn', 'arn:aws:iam::111111111111:role/SomeRole');

// THEN
expect(() => {
secret.grantRead(role);
}).toThrowError('KMS Key must be provided for cross account access to Secret');
});

test('grantRead with KMS Key', () => {
// GIVEN
const key = new kms.Key(stack, 'KMS');
const secret = new secretsmanager.Secret(stack, 'Secret', { encryptionKey: key });
Expand Down

0 comments on commit 3603de0

Please sign in to comment.