-
Notifications
You must be signed in to change notification settings - Fork 311
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
chore(test): replace AWS creds with fictitious samples #1325
Conversation
I see the conventional commits validation failed and will adjust this afternoon. |
@johnkrah-aws LGTM after commit check passes Thank you for this contribution! |
IIRC these were copied from the AWS test suite. There is no reason to change them @clundin25 |
@lsirac which test suite are you referencing? Either way, this change seems to mostly be cosmetic. |
We recommend (1) removing these secrets from code and (2) rotating the account access secret key to prevent unauthorized use by anyone who has seen the credentials written here. Consider using AWS Secrets Manager to store and access credentials, here’s a more detailed explanation https://maturitymodel.security.aws.dev/en/2.-foundational/dont-store-secrets-in-code/ why this is a best practice and a helpful guide https://docs.aws.amazon.com/secretsmanager/latest/userguide/hardcoded.html how to implement. Alternately in case these are purely test strings we recommend using a recognizably mocked value to assure that there is no accidental disclosure of credentials suggested by https://docs.aws.amazon.com/STS/latest/APIReference/API_GetAccessKeyInfo.html and https://docs.aws.amazon.com/STS/latest/APIReference/API_GetSessionToken.html.
updated commit message to follow conventional commits rule, its automatic check has passed now. correct this PR does not change any feature/behavior of the library only addresses hard coded secrets finding. |
@johnkrah-aws will the proposed change prevent this codebase from appearing as a false-signal in scanners searching for real committed AWS credentials? |
yes @clundin25 that's correct. scanners that look for AWS account secrets do find the hard coded creds in this test file, and do/should not find the example creds. we also fork this library internally and our AWS Code Guru static analysis flagged this instance for us. and I have confirmed on our internal fork that the proposed fix resolved the AWS Code Guru finding. I can't guarantee for all code scanners in the world, but if they are following public best practices, api docs, and/or partner guidance for real secrets and sample/mock/fictitious secrets then should be clear sailing. |
The other auth libs use these creds too btw. I also couldn't find it in the aws test suite @clundin25 |
We recommend (1) removing these secrets from code and (2) rotating the account access secret key to prevent unauthorized use by anyone who has seen the credentials written here. Consider using AWS Secrets Manager to store and access credentials, here’s a more detailed explanation https://maturitymodel.security.aws.dev/en/2.-foundational/dont-store-secrets-in-code/ why this is a best practice and a helpful guide
https://docs.aws.amazon.com/secretsmanager/latest/userguide/hardcoded.html how to implement.
Alternately in case these are purely test strings we recommend using a recognizably mocked value to assure that there is no accidental disclosure of credentials suggested by
https://docs.aws.amazon.com/STS/latest/APIReference/API_GetAccessKeyInfo.html and
https://docs.aws.amazon.com/STS/latest/APIReference/API_GetSessionToken.html.
Fixes #1324
Contribution guide checklist
[x] have an organization and individual contributor license agreement on file
[x] forked repo and dev/test on my fork, don't believe a doc is needed for this test config change
[x] added details in the commit message and duplicated above ^^
[x] submitting this pull request
[x] have not modified nor made a new feature
[x] full nox test suite verified working on python 3.7 and 3.8 in linux, since the change is string literal assignment with no declarations I don't expect any difference across os/py versions
[x] test coverage remains 100%, no added lines of code
let me know any changes I can make to improve the quality of this PR, thanks!