Skip to content

Commit

Permalink
fix(secretsmanager): arnForPolicies evaluates to the partial ARN if…
Browse files Browse the repository at this point in the history
… accessed from a cross-env stack (#26308)

Currently, `Secret.secretFullArn` returns the partial ARN if the secret is referenced between cross-env stacks.
An obvious practical implication is that `grant*` methods will produce an incorrect ARN for the IAM policies, since a full ARN is required for cross-environment access.

This PR partially fixes the issue - I reimplemented `arnForPolicies` to be lazily evaluated. It checks if the value is being accessed across environments and adds `-??????` to the ARN if it is.

Now, this does not solve the underlying issue of `secretFullArn` returning the partial ARN. While it should return undefined, we have to check how the prop is accessed (same environment or cross-env) before we know whether to return the ARN or `undefined`. If we use a `Lazy` here, it still cannot return `undefined` (only `any` as the closest thing).

So I don't think the underlying cause could be solved currently, that's why I opted for this partial fix that would address the most practical consequence of the bug.
 
This is a partial fix for #22468.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
gshpychka authored Jul 24, 2023
1 parent 1d9d808 commit 0e808d8
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 3 deletions.
36 changes: 35 additions & 1 deletion packages/aws-cdk-lib/aws-iam/test/merge-statements.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { App, Stack } from '../../core';
import { App, Lazy, Stack } from '../../core';
import * as iam from '../lib';
import { PolicyStatement } from '../lib';

Expand Down Expand Up @@ -490,6 +490,40 @@ test('lazily generated statements are merged correctly', () => {
]);
});

test('merge statements if resource is a lazy', () => {
const stack = new Stack();
const user1 = new iam.User(stack, 'User1');
const user2 = new iam.User(stack, 'User2');
const resourceToken = Lazy.string({
produce: () => 'a',
});

assertMerged([
new iam.PolicyStatement({
resources: [resourceToken],
actions: ['service:Action'],
principals: [user1],
}),
new iam.PolicyStatement({
resources: [resourceToken],
actions: ['service:Action'],
principals: [user2],
}),
], [
{
Effect: 'Allow',
Resource: 'a',
Action: 'service:Action',
Principal: {
AWS: [
{ 'Fn::GetAtt': ['User1E278A736', 'Arn'] },
{ 'Fn::GetAtt': ['User21F1486D1', 'Arn'] },
],
},
},
]);
});

function assertNoMerge(statements: iam.PolicyStatement[]) {
const app = new App();
const stack = new Stack(app, 'Stack');
Expand Down
17 changes: 15 additions & 2 deletions packages/aws-cdk-lib/aws-secretsmanager/lib/secret.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { RotationSchedule, RotationScheduleOptions } from './rotation-schedule';
import * as secretsmanager from './secretsmanager.generated';
import * as iam from '../../aws-iam';
import * as kms from '../../aws-kms';
import { ArnFormat, FeatureFlags, Fn, IResource, Lazy, RemovalPolicy, Resource, ResourceProps, SecretValue, Stack, Token, TokenComparison } from '../../core';
import { ArnFormat, FeatureFlags, Fn, IResolveContext, IResource, Lazy, RemovalPolicy, Resource, ResourceProps, SecretValue, Stack, Token, TokenComparison } from '../../core';
import * as cxapi from '../../cx-api';

const SECRET_SYMBOL = Symbol.for('@aws-cdk/secretsmanager.Secret');
Expand Down Expand Up @@ -340,9 +340,22 @@ abstract class SecretBase extends Resource implements ISecret {
protected abstract readonly autoCreatePolicy: boolean;

private policy?: ResourcePolicy;
private _arnForPolicies: string;

constructor(scope: Construct, id: string, props: ResourceProps = {}) {
super(scope, id, props);
this._arnForPolicies = Lazy.uncachedString({
produce: (context: IResolveContext) => {
const consumingStack = Stack.of(context.scope);
if (this.stack.account !== consumingStack.account ||
(this.stack.region !== consumingStack.region &&
!consumingStack._crossRegionReferences) || !this.secretFullArn) {
return `${this.secretArn}-??????`;
} else {
return this.secretFullArn;
}
},
});

this.node.addValidation({ validate: () => this.policy?.document.validateForResourcePolicy() ?? [] });
}
Expand Down Expand Up @@ -450,7 +463,7 @@ abstract class SecretBase extends Resource implements ISecret {
* then we need to add a suffix to capture the full ARN's format.
*/
protected get arnForPolicies() {
return this.secretFullArn ? this.secretFullArn : `${this.secretArn}-??????`;
return this._arnForPolicies;
}

/**
Expand Down
33 changes: 33 additions & 0 deletions packages/aws-cdk-lib/aws-secretsmanager/test/secret.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1357,3 +1357,36 @@ describe('secretObjectValue', () => {
});
});
});

test('cross-environment grant with direct object reference', () => {
// GIVEN
const producerStack = new cdk.Stack(app, 'ProducerStack', { env: { region: 'foo', account: '1111111111' } });
const consumerStack = new cdk.Stack(app, 'ConsumerStack', { env: { region: 'bar', account: '1111111111' } });
const secret = new secretsmanager.Secret(producerStack, 'Secret', { secretName: 'MySecret' });
const role = new iam.Role(consumerStack, 'Role', { assumedBy: new iam.AccountRootPrincipal() });

// WHEN
secret.grantRead(role);

// THEN
Template.fromStack(consumerStack).hasResourceProperties('AWS::IAM::Policy', {
PolicyDocument: {
Version: '2012-10-17',
Statement: [{
Action: [
'secretsmanager:GetSecretValue',
'secretsmanager:DescribeSecret',
],
Effect: 'Allow',
Resource: {
'Fn::Join': ['', [
'arn:',
{ Ref: 'AWS::Partition' },
':secretsmanager:foo:1111111111:secret:MySecret-??????',
]],
},
}],
},
});

});

0 comments on commit 0e808d8

Please sign in to comment.