Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grantRead on a Secret should also grant DescribeSecret #6444

Closed
1 of 2 tasks
jls-tschanzc opened this issue Feb 25, 2020 · 0 comments · Fixed by #8409
Closed
1 of 2 tasks

grantRead on a Secret should also grant DescribeSecret #6444

jls-tschanzc opened this issue Feb 25, 2020 · 0 comments · Fixed by #8409
Assignees
Labels
@aws-cdk/aws-secretsmanager Related to AWS Secrets Manager effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md

Comments

@jls-tschanzc
Copy link

The grantRead method will only give permission for the secretsmanager:GetSecretValue action on a secret and won't also grant permission for the secretsmanager:DescribeSecret action.

This is unintuitive as granting permission for retrieval should also grant permission to read the metadata.

Use Case

Creating an RDS DatabaseInstance will create a DatabaseSecret in the Secrets Manager. This secret can be used by the aws-secretsmanager-jdbc package to connect to the database.

When having e.g. a Spring Boot application running on ECS, you can grant the task role read access to the secret using:

appDB.secret?.grantRead(service.taskDefinition.taskRole);

But this is not enough; the aws-secretsmanager-jdbc package will try to interact with the secret using the secretsmanager:DescribeSecret action as well. So apart from the grantRead above, you will need to attach an inline policy to grant the missing action:

service.taskDefinition.taskRole.attachInlinePolicy(
    new Policy(this, 'DBSecretPolicy', {
      statements: [
        new PolicyStatement({
          effect: Effect.ALLOW,
          resources: [appDB.secret?.secretArn || ''],
          actions: ['secretsmanager:DescribeSecret'],
        }),
      ],
    })
);

Proposed Solution

I would expect grantRead to give full read access to the secret and it's metadata to the specified role. So the grantRead method should grant permissions for both secretsmanager:GetSecretValue and secretsmanager:DescribeSecret.

Otherwise further permission functions like grantReadDescribe might be useful.

Other

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@jls-tschanzc jls-tschanzc added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Feb 25, 2020
@SomayaB SomayaB added the @aws-cdk/aws-secretsmanager Related to AWS Secrets Manager label Feb 27, 2020
@skinny85 skinny85 added the effort/small Small work item – less than a day of effort label Mar 11, 2020
@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label May 19, 2020
@skinny85 skinny85 added the good first issue Related to contributions. See CONTRIBUTING.md label Jun 1, 2020
@mergify mergify bot closed this as completed in #8409 Jun 6, 2020
mergify bot pushed a commit that referenced this issue Jun 6, 2020
…rmissions (#8409)

`Secret.grantRead()` now gives permission for `secretmanager:DescribeSecret` and `secretmanager:GetSecretValue`,
instead of only `secretmanager:GetSecretValue`. 

Fixes #6444 
Fixes #7953 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-secretsmanager Related to AWS Secrets Manager effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants