[REVIEW] alias fix, needs test #541
Conversation
Codecov Report
@@ Coverage Diff @@
## master #541 +/- ##
=========================================
- Coverage 36.46% 36.4% -0.07%
=========================================
Files 20 20
Lines 2863 2868 +5
=========================================
Hits 1044 1044
- Misses 1725 1730 +5
Partials 94 94
Continue to review full report at Codecov.
|
kms/keysource.go
Outdated
| } else { | ||
|
|
||
| config := aws.Config{Region: aws.String(matches[1])} | ||
| if alias_matches != nil { | ||
| // its an alias the region is irrelevant | ||
| config := aws.Config{Region: aws.String(matches[0])} | ||
| } else { | ||
| return nil, fmt.Errorf("No valid ARN found in %q", key.Arn) | ||
| } | ||
| } |
There was a problem hiding this comment.
Unnest this into an else if and an else.
There was a problem hiding this comment.
Thanks! I'm not sure about my butchering the arn pattern to force the region into the alias?
There was a problem hiding this comment.
Have you tested what happens if you provide a bogus region to the alias? e.g. blah:alias/foo.
I don't currently have an AWS account set up so this is not particularly easy for me to test :(
|
I imagine a ErrSharedConfigInvalidCredSource would be thrown? But in truth
no, in fact I've not yet written any tests for this component as I'm
floating the idea to get some feedback on the need for alias support?
…On Tue, Oct 8, 2019 at 9:44 PM Adrian Utrilla ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In kms/keysource.go
<#541 (comment)>:
> + } else {
- config := aws.Config{Region: aws.String(matches[1])}
+ if alias_matches != nil {
+ // its an alias the region is irrelevant
+ config := aws.Config{Region: aws.String(matches[0])}
+ } else {
+ return nil, fmt.Errorf("No valid ARN found in %q", key.Arn)
+ }
+ }
Have you tested what happens if you provide a bogus region to the alias?
e.g. blah:alias/foo.
I don't currently have an AWS account set up so this is not particularly
easy for me to test :(
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#541>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADA6MR5G3TT3DCUODOQL6DQNTPIHANCNFSM4I2WPU4Q>
.
|
|
I don't mean automated tests, just whether you've tested that this actually works with aliases, and what happens when you provide either the wrong region, or a region that doesn't exist. I'm not asking about what behavior should be implemented, I'm just wondering what the behavior of the AWS SDK is. |
sops should support aws kms alias in the same way the aws cli does as per #381
I've added a simple check, please review and ill try add tests for it (first go contribution, go easy please)