Fix ToString for AWS KMS to include role, context, and profile #1733
Conversation
Signed-off-by: Felix Fontein <[email protected]>
There was a problem hiding this comment.
Hey Felix,
first of all, thank you for creating this fix. Sorry that I broke stuff for others with my change.
The PR looks good to me.
I still am no AWS guy, so my comments on it can not cover the behavior of the code after your changes. I cannot judge if it is working correctly afterwards, but I understood the #1580 (comment) to match what you implemented now.
🚀
| @@ -538,9 +553,10 @@ func TestKeyGroupsForFileWithGroups(t *testing.T) { | |||
| conf, err := parseCreationRuleForFile(parseConfigFile(sampleConfigWithGroups, t), "/conf/path", "whatever", nil) | |||
| assert.Nil(t, err) | |||
| assert.Equal(t, "bar", conf.KeyGroups[0][0].ToString()) | |||
| assert.Equal(t, "foo", conf.KeyGroups[0][1].ToString()) | |||
| assert.Equal(t, "foo||bar", conf.KeyGroups[0][1].ToString()) | |||
| assert.Equal(t, "foo|baz:bam", conf.KeyGroups[0][2].ToString()) | |||
There was a problem hiding this comment.
I think there should be a test-case where both context and profile are set?
There was a problem hiding this comment.
I've added some more tests.
kms/keysource.go
Outdated
| if key.AwsProfile != "" { | ||
| return fmt.Sprintf("%s|%s|%s", arnRole, context, key.AwsProfile) | ||
| } | ||
| if len(key.EncryptionContext) > 0 { |
There was a problem hiding this comment.
| if len(key.EncryptionContext) > 0 { | |
| if context != "" { |
Just a thought, that the kmsContextToString could return something even if the length of the EncryptionContext is zero. It doesn’t right now, but maybe in the future?
Checking if the string is empty is also more consistent with the other if statements.
There was a problem hiding this comment.
I don't think kmsContextToString() should ever return anything than the empty string for an empty EncryptionContext.
Signed-off-by: Felix Fontein <[email protected]>
Follow-up for #1493, in particular #1493 (comment). Turns out the other fields were important as well.
This not only applies to #1493, but also other operations where
kms.ToStringis used, for example therotatesubcommand, theupdatekeyssubcommand, and thepublishsubcommand (the two latter useDiffKeyGroups, which relies onToStringto identify a full key).Fixes #1580.