Don't read AWS env vars #5974
Don't read AWS env vars #5974
Conversation
vault/seal/awskms/awskms.go
Outdated
| k.accessKey = accessKey | ||
| } | ||
| // Check and set AWS access key, secret key, and session token | ||
| var accessKey, secretKey, sessionToken string |
There was a problem hiding this comment.
My code editor complains that these aren't actually read below.
There was a problem hiding this comment.
Fails to make for me with:
Stderr: # github.com/hashicorp/vault/vault/seal/awskms
../../../../../GO/src/github.com/hashicorp/vault/vault/seal/awskms/awskms.go:103:6: accessKey declared and not used
../../../../../GO/src/github.com/hashicorp/vault/vault/seal/awskms/awskms.go:103:17: secretKey declared and not used
../../../../../GO/src/github.com/hashicorp/vault/vault/seal/awskms/awskms.go:103:28: sessionToken declared and not used```
vault/seal/awskms/awskms.go
Outdated
| } | ||
| // Check and set AWS access key, secret key, and session token | ||
| var accessKey, secretKey, sessionToken string | ||
| if os.Getenv("AWS_ACCESS_KEY_ID") == "" { |
There was a problem hiding this comment.
I think that since we use credsConfig.GenerateCredentialChain() below, and that includes adding an Env cred provider if the AWS key is unset, we can just do away with all fussing with pulling AWS environment variables here and let the Env provider do the right thing.
There was a problem hiding this comment.
Looks like the Env provider supports pulling specifically the access key, secret key, and session token, but nothing else.
physical/dynamodb/dynamodb.go
Outdated
| secretKey = conf["secret_key"] | ||
| } | ||
| sessionToken := os.Getenv("AWS_SESSION_TOKEN") | ||
| if sessionToken == "" { | ||
| if os.Getenv("AWS_SESSION_TOKEN") == "" { |
There was a problem hiding this comment.
We should just drop pulling all three of these here too.
There was a problem hiding this comment.
The point of keeping it there was so that if you overrode the config with env vars we'd prioritize the env vars -- note that awsutil has the ordering static, env, shared, iam. Whether this is the right thing to do or not is debatable, but it's also how it's worked up to this point so I'm not sure changing them is the right move.
There was a problem hiding this comment.
Oh I see! Maybe it would be nice to add a comment to that effect, then. But yes, I looked through the code in GenerateCredentialChain and I see why you're doing that but still using the method, that method does a lot of work.
There was a problem hiding this comment.
Yeah, I've been trying to think about whether we should change this. I'm starting to think we should.
There was a problem hiding this comment.
This code looks good as-is, up to you whether to add a comment explaining why the env vars are checked. I do note there's a test failure on the code but the failure is on master too, so it's unrelated to this change. Just posting it as an FYI.
--- FAIL: TestAWSKMSSeal (0.00s)
awskms_test.go:22: expected error when AWSKMSSeal key ID is not provided
Let AWS SDK env cred chain provider do it for us Fixes #5965
Let AWS SDK env cred chain provider do it for us
Fixes #5965