Allow aws profile setting from metadata to be overridden. #1659
Conversation
When decrypting, sops uses the AWS profile setting stored in the encrypted file metadata. This is a problem as the profile can change from user to user. This change will allow the AWS profile setting to be overridden by the '--aws-profile' flag and the AWS_PROFILE environment variable, in that order of precedence. The metadata value is used as a last resort only. Signed-off-by: Leandro Martelli <[email protected]>
There was a problem hiding this comment.
Thanks for your contribution! I'm not familiar with AWS' KMS, so I cannot comment on whether this makes sense, but I have some general things.
One is that this looks like a potential breaking change to me, since a lot of AWS users might have AWS_PROFILE already set (it's an official AWS environment variable), and for them SOPS might suddenly behave differently and stop decrypting files that it was able to decrypt before. Or is this not possible for some reason? (I really don't know.)
| @@ -251,6 +252,22 @@ func LoadEncryptedFileWithBugFixes(opts GenericDecryptOpts) (*sops.Tree, error) | |||
| } | |||
| } | |||
|
|
|||
| awsProfile := os.Getenv("AWS_PROFILE") | |||
| if opts.UseAwsProfile != "" { | |||
| awsProfile = opts.UseAwsProfile | |||
There was a problem hiding this comment.
I don't think this is the right place to look at environment variables. This is a library function deep down the call tree.
Maybe adding EnvVar: "AWS_PROFILE", to the CLI option would be a better place to handle this? This might have other unintended consequences, though.
There was a problem hiding this comment.
Agreed. I can move this logic up in the stack.
| @@ -796,6 +800,7 @@ func main() { | |||
| KeyServices: svcs, | |||
| DecryptionOrder: order, | |||
| IgnoreMAC: c.Bool("ignore-mac"), | |||
| UseAwsProfile: c.String("aws-profile"), | |||
There was a problem hiding this comment.
There are four instances of decryptOpts creations in this file. You need to change all of them, not just one.
First, thanks for the comments and the time to review it. :) Yeah, I'm also no sure how people usually go about it, but it was unexpected for me to have a profile be determined by the encrypted file. Since AWS_PROFILE is quite standard for AWS tooling, this felt like the natural fix. But I get it might break for some people. I believe we could split this into a few steps:
If we decide to move towards having AWS_PROFILE be authoritative:
I'd expect people to support this, as it might make life easier by not having to sync profile names across environments, specially around ci/cd pipelines. How does it sound? |
When decrypting, sops uses the AWS profile setting stored in the encrypted file metadata. This is a problem as the profile can change from user to user.
This change will allow the AWS profile metadata setting to be overridden by the '--aws-profile' flag and the AWS_PROFILE environment variable, in that order of precedence. The metadata value is used as a last resort only.