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

feat(iam): generate AccessKeys #18180

Merged
merged 8 commits into from
Jan 11, 2022
Merged

Conversation

laurelmay
Copy link
Contributor

@laurelmay laurelmay commented Dec 26, 2021

This adds an L2 resource for creating IAM access keys. Instructions for
creating access keys are added to the README near the information on
creating users. Tests are added (including an integration test) and
locations elsewhere in the CDK where CfnAccessKey was used have been
updated to leverage the new L2 construct (which required changes in the
secretsmanager and apigatewayv2-authorizers packages).

Excludes were added for two awslint rules. Access Keys don't support
specifying physical names, so having such a property is impossible.
Additionally, since the primary value of an AWS::IAM::AccessKey is to
gain access to the SecretAccessKey value, a fromXXX static method
doesn't seem to make a lot of sense (because ideally you'd just pull that
from a Secret anyway if it was required in the app).

I looked into integrating with secretsmanager.Secret as part of this PR;
however, at this time it's currently experimental to support strings via
tokens and the experimental resource's documentation isn't available so it
seemed suboptimal to do that integration.

Resolves: #8432


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Dec 26, 2021

@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Dec 26, 2021
@laurelmay laurelmay force-pushed the feature/iam-accesskey-l2 branch from 84a9543 to 0a16ca3 Compare December 26, 2021 02:01
This adds an L2 resource for creating IAM access keys. Instructions for
creating access keys are added to the README near the information on
creating users. Tests are added (including an integration test) and
locations elsewhere in the CDK where `CfnAccessKey` was used have been
updated to leverage the new L2 construct (which required changes in the
`secretsmanager` and `apigatewayv2-authorizers` packages).

Excludes were added for two `awslint` rules. Access Keys don't support
specifying physical names, so having such a property is impossible.
Additionally, since the primary value of an `AWS::IAM::AccessKey` is to
gain access to the `SecretAccessKey` value, a `fromXXX` static method
doesn't seem to make a lot of sense (because ideally you'd just pull that
from a Secret anyway if it was required in the app).
@laurelmay laurelmay force-pushed the feature/iam-accesskey-l2 branch from 0a16ca3 to a4afd62 Compare December 27, 2021 14:39
@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 3, 2022

Thanks for this!

I'm all good with it, except I do have a slight concern about the accessKey.secretAccessKey attribute. The use as demonstrated in the integ test (sticking the value directly into a CfnOutput) is unsafe, and we should see if there's a way we can programmatically discourage that.

Hold on while we discuss internally.

@laurelmay
Copy link
Contributor Author

Thanks! I looked into using SecretValue.plainText but that doesn't seem to be used directly throughout the library (outside of tests). It also doesn't really prevent outputting the value (though it does require an extra .toString() call). Maybe that at least gets the point across a little better?

It's a little unfortunate that with the L1 construct it was being output and that there aren't more protections at the CloudFormation level to discourage that, but it'd be awesome to at least provide that safety at a higher level in the CDK codebase.

Looking forward to seeing what those internal discussions yield and how this construct can be safer!

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 3, 2022

Yes I think wrapping it in a SecretValue might do the job--though we have to review SecretValue and its API. Unfortunately a toString() is not a good enough barrier. That might accidentally get called implicitly. I'd prefer an explicit method like value.unsafeUnwrap() or something.

This doesn't achieve a lot by itself just yet, but it will if we change
`SecretValue` a little.
@rix0rrr rix0rrr changed the title feat(iam): add L2 AccessKey resource feat(iam): generate AccessKeys Jan 5, 2022
rix0rrr
rix0rrr previously approved these changes Jan 5, 2022
@mergify mergify bot dismissed rix0rrr’s stale review January 5, 2022 14:29

Pull request has been modified.

rix0rrr
rix0rrr previously approved these changes Jan 10, 2022
@mergify mergify bot dismissed rix0rrr’s stale review January 10, 2022 15:30

Pull request has been modified.

`toString()` needs to be called on the `SecretValue` because it
requires a string argument. And the same needs to be done in with JSON,
otherwise the `toJSON` in `Intrinsic` seems to be used and that does
not result in a token being returned.
It seems `.toString()` needs to be called on `SecretValue`s being
`JSON.stringify`-ed.
This adds the new required call to `.toString()` to retrieve the value.
@mergify
Copy link
Contributor

mergify bot commented Jan 11, 2022

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 9874f21
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit beb5706 into aws:master Jan 11, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 11, 2022

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

@laurelmay laurelmay deleted the feature/iam-accesskey-l2 branch January 11, 2022 19:27
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
This adds an L2 resource for creating IAM access keys. Instructions for
creating access keys are added to the README near the information on
creating users. Tests are added (including an integration test) and
locations elsewhere in the CDK where `CfnAccessKey` was used have been
updated to leverage the new L2 construct (which required changes in the
`secretsmanager` and `apigatewayv2-authorizers` packages).

Excludes were added for two `awslint` rules. Access Keys don't support
specifying physical names, so having such a property is impossible.
Additionally, since the primary value of an `AWS::IAM::AccessKey` is to
gain access to the `SecretAccessKey` value, a `fromXXX` static method
doesn't seem to make a lot of sense (because ideally you'd just pull that
from a Secret anyway if it was required in the app).

I looked into integrating with `secretsmanager.Secret` as part of this PR;
however, at this time it's currently experimental to support strings via
tokens and the experimental resource's documentation isn't available so it
seemed suboptimal to do that integration.

Resolves: aws#8432

----

*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-iam Related to AWS Identity and Access Management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement CDK AccessKey (construct)
3 participants