fix(secretsmanager): `arnForPolicies` evaluates to the partial ARN if accessed from a cross-env stack #26308

gshpychka · 2023-07-10T16:54:19Z

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

gitpod-io · 2023-07-10T16:54:29Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

gshpychka · 2023-07-10T18:03:07Z

Not sure how to go about an integration test - I've written one but then realized that the AWS account number has to be provided (and included in the snapshots) since the stacks have to be environment-specific - what is the process in this case?
Clarification Request

gshpychka · 2023-07-11T10:47:13Z

Hm, the integ tests failed because

      {
       "Action": [
        "secretsmanager:DescribeSecret",
        "secretsmanager:GetSecretValue"
       ],
       "Effect": "Allow",
       "Principal": {
        "AWS": {
         "Fn::Join": [
          "",
          [
           "arn:",
           {
            "Ref": "AWS::Partition"
           },
           ":iam::",
           {
            "Ref": "AWS::AccountId"
           },
           ":root"
          ]
         ]
        },
        "Service": "ecs-tasks.amazonaws.com"
       },
       "Resource": {
        "Ref": "testsecretAttachment19AD251F"
       }
      }

changed to

      {
       "Action": [
        "secretsmanager:DescribeSecret",
        "secretsmanager:GetSecretValue"
       ],
       "Effect": "Allow",
       "Principal": {
        "AWS": {
         "Fn::Join": [
          "",
          [
           "arn:",
           {
            "Ref": "AWS::Partition"
           },
           ":iam::",
           {
            "Ref": "AWS::AccountId"
           },
           ":root"
          ]
         ]
        }
       },
       "Resource": {
        "Ref": "testsecretAttachment19AD251F"
       }
      },
      {
       "Action": [
        "secretsmanager:DescribeSecret",
        "secretsmanager:GetSecretValue"
       ],
       "Effect": "Allow",
       "Principal": {
        "Service": "ecs-tasks.amazonaws.com"
       },
       "Resource": {
        "Ref": "testsecretAttachment19AD251F"
       }
      }

Any idea why this could be?

packages/aws-cdk-lib/aws-secretsmanager/lib/secret.ts

mrgrain · 2023-07-11T11:14:12Z

(Sorry I accidentally closed this)

mrgrain · 2023-07-11T11:21:25Z

Hm, the integ tests failed because

Which test is this?

Any idea why this could be?

I suspect the policy deduplication mechanism (search for mergeStatements) doesn't think these are combinable anymore. Probably because it is now a lazy token object which are not going to be equal to each other.

gshpychka · 2023-07-11T11:34:30Z

Which test is this?

It is aws-rds/test/integ.serverless-cluster-secret-rotation-custom-names and aws-rds/test/integ.serverless-cluster-secret-rotation

This would make it a breaing change, though.

Regarding an integration test for the fix itself - I'm not really comfortable with making it exempt, but not sure how a env-specific integ test can possibly be implemented.

mrgrain · 2023-07-11T19:23:58Z

Which test is this?

It is aws-rds/test/integ.serverless-cluster-secret-rotation-custom-names and aws-rds/test/integ.serverless-cluster-secret-rotation

This would make it a breaing change, though.

Regarding an integration test for the fix itself - I'm not really comfortable with making it exempt, but not sure how a env-specific integ test can possibly be implemented.

Back to square one then.

Currently, Secret.secretFullArn returns the partial ARN if the secret is referenced between cross-env stacks.

Have you figured out why this happens?

gshpychka · 2023-07-11T19:35:24Z

Have you figured out why this happens?

I believe it's because the default implementation of secretFullArn defined in BaseSecret returns this.secretArn and that doesn't change when a secret is used across envs. It seems that this is only overridden in .fromSecret* methods.And secretArn uses Resource.getResourceArnAttribute which in across-env case returns a formatted ARN and ref otherwise.

gshpychka · 2023-07-12T13:53:28Z

Have you figured out why this happens?

I believe it's because the default implementation of secretFullArn defined in BaseSecret returns this.secretArn and that doesn't change when a secret is used across envs. It seems that this is only overridden in .fromSecret* methods.And secretArn uses Resource.getResourceArnAttribute which in across-env case returns a formatted ARN and ref otherwise.

We could change secretFullArn to use a custom function instead of Resource.getResourceArnAttribute and return something else in case of cross-env access, but what do we return?

mrgrain · 2023-07-12T14:14:04Z

We could change secretFullArn to use a custom function instead of Resource.getResourceArnAttribute and return something else in case of cross-env access, but what do we return?

Hm, so I guess that makes secretFullArn broken whenever we have a cross stack reference. CR would be one way, could we also deal with this special case using import/exports or a SSM parameter? For both we should have some code doing something similar already.

gshpychka · 2023-07-13T14:39:02Z

Hm, so I guess that makes secretFullArn broken whenever we have a cross stack reference.

No, it works fine with cross-stack references if in the same environment (using Cfn imports under the hood). The relevant logic for cross-stack references is implemented elsewhere.

mrgrain · 2023-07-13T14:41:00Z

No, it works fine with cross-stack references if in the same environment (using Cfn imports under the hood). The relevant logic for cross-stack references is implemented elsewhere.

Yeah, but could we use the same/similar logic for cross-env?

gshpychka · 2023-07-13T14:55:07Z

No, it works fine with cross-stack references if in the same environment (using Cfn imports under the hood). The relevant logic for cross-stack references is implemented elsewhere.

Yeah, but could we use the same/similar logic for cross-env?

Well, Cfn imports don't work cross-env. The cross-env logic is just "resolve the references at synth time".

What do you think we should be focusing on (I'm getting a bit lost in the discussion)?
Do you think the IAM policy merging change could be acceptable? Because whatever we do, seems that there's no way around this.

gshpychka · 2023-07-13T15:14:35Z

Yeah, but could we use the same/similar logic for cross-env?

To make sure we're on the same page - detecting that it's a cross-env reference is not the issue - that's easy. Returning a correct arnForPolicies in this case is easy as well - this PR does that.

We can have secretFullArn return a Lazy as well - and then we can implement the reference with a CR.

But the issue with merging in IAM policies would still be there in both of these cases, as far as I understand.

mrgrain · 2023-07-13T16:08:21Z

Yeah, but could we use the same/similar logic for cross-env?

To make sure we're on the same page - detecting that it's a cross-env reference is not the issue - that's easy. Returning a correct arnForPolicies in this case is easy as well - this PR does that.

We can have secretFullArn return a Lazy as well - and then we can implement the reference with a CR.

But the issue with merging in IAM policies would still be there in both of these cases, as far as I understand.

I'm with you now. Having arnForPolicies a Lazy is probably okay then. I'll need to set aside a bit to look into the merging code if there is a way around it.

mrgrain · 2023-07-21T16:50:09Z

~~@gshpychka I can't figure out how to push to your branch.~~

Nervermind. Figured it out.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

mergify · 2023-07-24T22:39:14Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-07-24T23:30:43Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: d043a29
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-07-24T23:31:07Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

… accessed from a cross-env stack (aws#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 aws#22468. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

rv2673 · 2023-08-18T14:47:09Z

@gshpychka @mrgrain This unfortunately break my app.
However secrets Constructs created through fromSecretCompleteArn, that are used cross region, break completely with this change, since it is treating them as a partialArn.

I believe this is because the condition !this.secretFullArn is used in or condition and is never reached since the cross region condition is short circuiting before secretFullArn is actually checked. So even if full arn is filled it is treated as partialArn.

So either the if block should only be entered if fullArn is undefined and cross-region or arnForPolicies should be overriden in class returned fromSecretAttributes.

This is a nice fix for secrets created(not imported) cross-region which would reduce some complexity in my apps since it has this case too.

mrgrain · 2023-08-18T15:12:19Z

@rv2673 Thanks for letting us know! Would you mind creating a new issue for this and include a code sample for the reproduction. Thank you.

rv2673 · 2023-08-18T17:08:10Z

@rv2673 Thanks for letting us know! Would you mind creating a new issue for this and include a code sample for the reproduction. Thank you.

Done #26811

gshpychka added 2 commits July 10, 2023 19:33

add unit test

d36f2a8

add suffix to ARN if cross-env

dd61f42

aws-cdk-automation requested a review from a team July 10, 2023 16:54

github-actions bot added repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Jul 10, 2023

aws-cdk-automation previously requested changes Jul 10, 2023

View reviewed changes

gshpychka added 2 commits July 10, 2023 21:20

Merge branch 'main' into master

d888020

return secretFullArn explicitly

4f23619

mrgrain reviewed Jul 11, 2023

View reviewed changes

packages/aws-cdk-lib/aws-secretsmanager/lib/secret.ts Outdated Show resolved Hide resolved

mrgrain closed this Jul 11, 2023

mrgrain reopened this Jul 11, 2023

aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Jul 11, 2023

mrgrain added effort/medium Medium work item – several days of effort and removed effort/small Small work item – less than a day of effort labels Jul 11, 2023

use the same lazy everywhere

918748a

Merge branch 'main' into master

a72cdcd

mrgrain added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run labels Jul 21, 2023

mrgrain marked this pull request as ready for review July 24, 2023 22:38

mrgrain approved these changes Jul 24, 2023

View reviewed changes

Merge branch 'main' into master

d043a29

mergify bot merged commit 0e808d8 into aws:main Jul 24, 2023

github-actions bot mentioned this pull request Aug 1, 2023

Monthly PRs metrics report - July 2023 #26583

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(secretsmanager): `arnForPolicies` evaluates to the partial ARN if accessed from a cross-env stack #26308

fix(secretsmanager): `arnForPolicies` evaluates to the partial ARN if accessed from a cross-env stack #26308

gshpychka commented Jul 10, 2023 •

edited

Loading

gitpod-io bot commented Jul 10, 2023

aws-cdk-automation left a comment

gshpychka commented Jul 10, 2023 •

edited

Loading

gshpychka commented Jul 11, 2023

mrgrain commented Jul 11, 2023 •

edited

Loading

mrgrain commented Jul 11, 2023

gshpychka commented Jul 11, 2023

mrgrain commented Jul 11, 2023

gshpychka commented Jul 11, 2023 •

edited

Loading

gshpychka commented Jul 12, 2023

mrgrain commented Jul 12, 2023

gshpychka commented Jul 13, 2023

mrgrain commented Jul 13, 2023

gshpychka commented Jul 13, 2023 •

edited

Loading

gshpychka commented Jul 13, 2023

mrgrain commented Jul 13, 2023

mrgrain commented Jul 21, 2023 •

edited

Loading

mergify bot commented Jul 24, 2023

aws-cdk-automation commented Jul 24, 2023

mergify bot commented Jul 24, 2023

rv2673 commented Aug 18, 2023

mrgrain commented Aug 18, 2023

rv2673 commented Aug 18, 2023

fix(secretsmanager): arnForPolicies evaluates to the partial ARN if accessed from a cross-env stack #26308

fix(secretsmanager): arnForPolicies evaluates to the partial ARN if accessed from a cross-env stack #26308

Conversation

gshpychka commented Jul 10, 2023 • edited Loading

gitpod-io bot commented Jul 10, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

gshpychka commented Jul 10, 2023 • edited Loading

gshpychka commented Jul 11, 2023

mrgrain commented Jul 11, 2023 • edited Loading

mrgrain commented Jul 11, 2023

gshpychka commented Jul 11, 2023

mrgrain commented Jul 11, 2023

gshpychka commented Jul 11, 2023 • edited Loading

gshpychka commented Jul 12, 2023

mrgrain commented Jul 12, 2023

gshpychka commented Jul 13, 2023

mrgrain commented Jul 13, 2023

gshpychka commented Jul 13, 2023 • edited Loading

gshpychka commented Jul 13, 2023

mrgrain commented Jul 13, 2023

mrgrain commented Jul 21, 2023 • edited Loading

mergify bot commented Jul 24, 2023

aws-cdk-automation commented Jul 24, 2023

AWS CodeBuild CI Report

mergify bot commented Jul 24, 2023

rv2673 commented Aug 18, 2023

mrgrain commented Aug 18, 2023

rv2673 commented Aug 18, 2023

fix(secretsmanager): `arnForPolicies` evaluates to the partial ARN if accessed from a cross-env stack #26308

fix(secretsmanager): `arnForPolicies` evaluates to the partial ARN if accessed from a cross-env stack #26308

gshpychka commented Jul 10, 2023 •

edited

Loading

gshpychka commented Jul 10, 2023 •

edited

Loading

mrgrain commented Jul 11, 2023 •

edited

Loading

gshpychka commented Jul 11, 2023 •

edited

Loading

gshpychka commented Jul 13, 2023 •

edited

Loading

mrgrain commented Jul 21, 2023 •

edited

Loading