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

chore(gomod): Bumping AWS SDK version #714

Merged
merged 1 commit into from
Aug 6, 2020
Merged

chore(gomod): Bumping AWS SDK version #714

merged 1 commit into from
Aug 6, 2020

Conversation

sc250024
Copy link
Contributor

@sc250024 sc250024 commented Aug 4, 2020

Bumps the AWS SDK version to support more types of credential chaining.

Description

It seems that the following issues:

Could potentially be helped with a simple bump of the AWS SDK version. Specifically my issue in the following comments...

...was resolved by simply bumping the AWS SDK version, compiling, and testing it out.

Motivation and Context

Credential chaining did not work for me even though it should. My AWS credentials are obtained via gimme-aws-creds through Okta. This means that the ~/.aws/credentials file uses temporary aws_access_key_id and aws_secret_access_key keys.

Using AWS_PROFILE=development uses another profile as a source_profile called base; the base profile is the one that has the credentials obtained through the gimme-aws-creds CLI tool.

Here are some example files:

~/.aws/credentials
[base]
aws_access_key_id = <redacted>
aws_secret_access_key = <redacted>
aws_session_token = <redacted>
aws_security_token = <redacted>
~/.aws/config
[profile base]
cli_pager =
region = us-east-1
output = json

[profile development]
cli_pager =
source_profile = base
role_arn = arn:aws:iam::111122223333:role/Administrator
region = us-east-1
output = json

The following error occurred when trying to create a new file:

$ AWS_PROFILE=development sops --verbose test.yaml

[AWSKMS]     INFO[0005] Encryption failed                             arn="arn:aws:kms:us-east-1:111122223333:key/11111c1d-b2c8-437d-ae4b-d00a123ccc45"
Error encrypting the data key with one or more master keys: [failed to encrypt new data key with master key "arn:aws:kms:us-east-1:111122223333:key/11111c1d-b2c8-437d-ae4b-d00a123ccc45": Failed to call KMS encryption service: NoCredentialProviders: no valid providers in chain. Deprecated.
    For verbose messaging see aws.Config.CredentialsChainVerboseErrors]

Which made no sense because the AWS SDK solved this a while ago. The version of the AWS SDK Sops was using before this PR v1.23.13 is now almost 1 year old.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@sc250024
Copy link
Contributor Author

sc250024 commented Aug 4, 2020

@autrilla It looks like there's some additional mock functions that are needed in kms/mocks/KMSAPI.go. Any hint on how to get those started?

@autrilla
Copy link
Contributor

autrilla commented Aug 4, 2020

@autrilla It looks like there's some additional mock functions that are needed in kms/mocks/KMSAPI.go. Any hint on how to get those started?

make mock should do the trick.

@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2020

Codecov Report

Merging #714 into develop will increase coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #714      +/-   ##
===========================================
+ Coverage    36.28%   36.44%   +0.15%     
===========================================
  Files           22       22              
  Lines         3205     3205              
===========================================
+ Hits          1163     1168       +5     
+ Misses        1922     1918       -4     
+ Partials       120      119       -1     
Impacted Files Coverage Δ
stores/flatten.go 91.52% <0.00%> (+4.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f78682c...7c38e5d. Read the comment docs.

@sc250024
Copy link
Contributor Author

sc250024 commented Aug 4, 2020

@autrilla It looks like there's some additional mock functions that are needed in kms/mocks/KMSAPI.go. Any hint on how to get those started?

make mock should do the trick.

Ah cool, thank you! I just pushed the last commit after resolving Go dependencies. It's good to go from my side, let me know what you think.

@autrilla autrilla self-requested a review August 5, 2020 10:36
Copy link
Contributor

@autrilla autrilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :). Glad such a simple change fixes the issues.

@autrilla
Copy link
Contributor

autrilla commented Aug 5, 2020

Oh, woops, you need to make this merge to develop instead of master.

@sc250024 sc250024 changed the base branch from master to develop August 5, 2020 11:14
@sc250024
Copy link
Contributor Author

sc250024 commented Aug 5, 2020

Oh, woops, you need to make this merge to develop instead of master.

Done; I also rebased against the develop branch to keep it cleaner. Thank you for the approval 🙏

@sc250024 sc250024 requested a review from autrilla August 5, 2020 12:22
@sc250024
Copy link
Contributor Author

sc250024 commented Aug 6, 2020

@autrilla Is there anything else that's needed after the rebase?

@autrilla
Copy link
Contributor

autrilla commented Aug 6, 2020

Nope, LGTM. Thanks!

@autrilla autrilla merged commit 4bd640e into getsops:develop Aug 6, 2020
@sc250024 sc250024 deleted the chore-AwsSdkBump branch August 6, 2020 16:10
@sc250024
Copy link
Contributor Author

sc250024 commented Aug 6, 2020

@autrilla Thank you sir!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants