Issue 805 - support govcloud role in kms+role format

[ryan-dyer-sp]

Fixes #805
AWS supports partitions other than "aws" and not sure we really care the format of the role beyond the +, but didnt want to go that far with the change. If the role isnt correct, the encryption will fail whether its a typo or some other reason.

[felixfontein]

I'm not familiar with AWS and these strings, but your examples in #805 have both arn:aws-us-gov:kms:... and arn:aws-us-gov:iam::... in them, so I guess it would be better to use a regex here, something like +arn:aws[^:]*:iam::.

[felixfontein]

Or maybe arn:aws(-[a-z\-]+)?:iam:: (see https://github.com/ansible-collections/community.aws/blob/main/plugins/modules/lambda.py#L393).

[ryan-dyer-sp]

I'm not aware of strings.Index supporting regex.

IMO none of this is necessary to begin with. It shouldnt be up to sops to "validate" that the role is correct. All sops should be doing is splitting the string from kms+role to a distinct kms and role. As KMS key arns dont contain a + character, the first + in a string is a clear indication of a split needing to occur. The attempt for sops to validate what is a valid role and doing so incorrectly is why this PR is needed to begin with. At least if someone puts in jibberish for the role, they will likely obtain a better error response from AWS than the current one where it is a valid role and you really have to pay attention to the message in order to see that sops isnt doing things as it should.

[felixfontein]

I don't think strings.Index does that. But I'm sure golang's stdlib offers some way to search for an regex in a string.

About this probably not being necessary: could well be, but I cannot judge that. IMO the chances of this getting merged are higher if it is as close to the original as possible, and searching for a regex is IMO more close to the original than your change, since it increases the chance of finding the wrong position.

[ryan-dyer-sp]

@felixfontein If you're not familiar with aws arn string and can't judge the merits of my comments for why I've made the changes that I have, then I don't see why you're reviewing this or why I should make changes to the PR based on your comments. The entire basis of this PR hinges around a bug which exists because whomever made the initial commit didn't fully understand AWS arn strings, their formats, and made a decision that sops should somehow be responsible for doing a partial validation of the string format.

[autrilla]

@ryan-dyer-sp please try to be respectful.

I agree with @felixfontein here. Using a regex would be better. Or split on the +, but:

they will likely obtain a better error response from AWS than the current one

Have you validated this?

[ryan-dyer-sp]

@ryan-dyer-sp please try to be respectful.

I agree with @felixfontein here. Using a regex would be better. Or split on the +, but:

they will likely obtain a better error response from AWS than the current one

Have you validated this?

The current error is this 'Invalid arn arn:aws-us-gov:kms:us-gov-west-1:account:key/key+arn:aws-us-gov:iam::account:role/test' Anything is better than this. This message requires someone looking into this code to understand how that error even came about. One who follows the existing documentation could have things work for 'aws' partitions, but not work for non-aws partitions.

As KMS keys can't contain +, I'm modifying the split to simply be on the + and dropping the additional pattern matching entirely. This is how the documentation says it should work to begin with.

[farvour]

Can we merge this? Is there a reason why this is 4 years old but never got merged?

[mfann-or]

Echoing @farvour -- why can't this be merged?

It effectively renders SOPS useless to anyone using AWS GovCloud (aws-us-gov) or AWS China (aws-cn) partitions.