introduce new aws-http-auth module which implements sigv4 and sigv4a #541
Conversation
aws-http-auth/internal/v4/strings.go
Outdated
| } | ||
|
|
||
| // uriEncode implements "Amazon-style" URL escaping. | ||
| func uriEncode(path string, encodeSep bool) string { |
There was a problem hiding this comment.
I only see this being called from buildCanonicalRequest and encodeSep is always set to false. Is there another future use that will use encodeSep as true?
There was a problem hiding this comment.
No it's not necessary here. This was lifted from smithy-go httpbinding -- will remove it here.
aws-http-auth/sigv4/e2e_test.go
Outdated
| QueueName: queueName, | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("list queues: %v", err) |
There was a problem hiding this comment.
nit: this is a create queue error, not a list queue error
| } | ||
| } | ||
|
|
||
| func TestBuildCanonicalRequest_DoubleHeader(t *testing.T) { |
There was a problem hiding this comment.
can we also add a test for double header value, like x-amz-meta=a,b?
There was a problem hiding this comment.
There's a double header in one of these cases but i'll make it its own thing
There was a problem hiding this comment.
Wait--isn't this that? x-amz-foo has two values in this one.
aws-http-auth/internal/v4/signer.go
Outdated
|
|
||
| rs, ok := s.Request.Body.(io.ReadSeeker) | ||
| if !ok || s.Options.DisableImplicitPayloadHashing { | ||
| s.PayloadHash = []byte(v4.UnsignedPayload) |
There was a problem hiding this comment.
Wouldn't a request with no payload needs to hash the empty string instead of the magic string?
If there is no payload in the request, you compute a hash of the empty string, such as when you retrieve an object by using a GET request, there is nothing in the payload.
Hex(SHA256Hash(""))
There was a problem hiding this comment.
Yes but that's not what this case is checking. This is just saying "if we can't compute the payload ourselves (or they shut it off) do explicit unsigned"
| for key := range query { | ||
| sort.Strings(query[key]) | ||
| } | ||
| canonQuery := strings.Replace(query.Encode(), "+", "%20", -1) |
There was a problem hiding this comment.
Docs say we need to do sorting after encoding
You must also sort the parameters in the canonical query string alphabetically by key name. The sorting occurs after encoding
There was a problem hiding this comment.
That's already happening on 134 - Query() turns URL.RawQuery into a map, RawQuery is already in encoded form.
There was a problem hiding this comment.
Added a test that demonstrates this.
aws-http-auth/sigv4/sigv4_test.go
Outdated
| "explicit unsigned payload": { | ||
| Input: &SignRequestInput{ | ||
| Request: newRequest(seekable("{}")), | ||
| PayloadHash: []byte(v4.UnsignedPayload), |
There was a problem hiding this comment.
customers are expected to set this value always wrapped in bytes. Should we provide this as UnsignedPayloadHash instead so you can do PayloadHash: v4.UnsignedPayloadHash instead of them having to wrap it on a byte?
There was a problem hiding this comment.
I wanted to expose it as const, if it was []byte it'd have to be var UnsignedPayload = blah.
There was a problem hiding this comment.
I'd still argue that that is more ergonomic, although making it var means people may be able to change it by mistake
There was a problem hiding this comment.
maybe this?
func UnsignedPayload() []byte {
return []byte("UNSIGNED-PAYLOAD")
}There was a problem hiding this comment.
That works for me, although since I'm newer to Go I'll let you be the judge of what is more ergonomic for callers
aws-http-auth/sigv4a/sigv4a_test.go
Outdated
|
|
||
| parts := strings.Split(auth, ", ") | ||
| if len(parts) != 3 { | ||
| err = fmt.Errorf("auth parts have non-3 size %d", len(parts)) |
There was a problem hiding this comment.
nit: would be nice to also show these parts for debugging
| // 1. creates a bucket in us-east-1 | ||
| // 2. creates an MRAP that points to that bucket | ||
| // 3. polls MRAP status until created | ||
| // 4. puts object to MRAP |
There was a problem hiding this comment.
So I suspect is only up to this point that we check sigv4a, right?
Depending on how much "a while" is "a while", I could lean to instead create a bucket if not exists without deleting the bucket at the end, so we skip the lag of MRAP creation. This still validates what we want to validate, right?
There was a problem hiding this comment.
2-3 minutes in my own testing, it seems.
There was a problem hiding this comment.
2-3 minutes is fine for more "correctness", but if we ever see this taking longer I wouldn't be opposed to just check if bucket exists and re-using it with a hardcoded name
There was a problem hiding this comment.
Yeah I did consider adding an env for basically "MRAP_ALIAS" - i.e. if that's set, just bypass the whole setup/teardown and just call against that - but by the time I got this test written I was so done with it that I moved on to other stuff 😂
We can put that in now if you feel strongly that it should be an option.
| } | ||
|
|
||
| // constant-time byte slice compare | ||
| func cmpConst(x, y []byte) (int, error) { |
There was a problem hiding this comment.
Why not use https://pkg.go.dev/crypto/subtle#ConstantTimeCompare ?
There was a problem hiding this comment.
They're not entirely the same. The "breakout" case is c > N-2, ConstantTimeCompare can only tell if equal or not equal.
| // AccessKey and SecretKey pair. | ||
| // | ||
| // Based on FIPS.186-4 Appendix B.4.2 | ||
| func derivePrivateKey(creds credentials.Credentials) (*ecdsa.PrivateKey, error) { |
There was a problem hiding this comment.
I'll start taking a look at this compared with the docs you referenced, but I'll take time and I'm concerned since these are crypto functions that I'm not familiar with. Looking at what other SDKs do, they basically import the CRT and call it a day, which we don't want to do
We can test correctness by "we were able to call the service and it works" but we may need some extra eyes on this. Curious to hear your thoughts on it
There was a problem hiding this comment.
FWIW, this derivation component is ripped from the internal go v2 implementation, which previously had an AppSec review.
There was a problem hiding this comment.
But as far as testing correctness goes - yeah, not sure we can do any better than we were doing before in the SDK. In fact we're technically probably testing it better now since we have a dedicated e2e test for it.
| func deriveHMACKey(hash func() hash.Hash, bitLen int, key []byte, label, context []byte) ([]byte, error) { | ||
| // verify that we won't overflow the counter | ||
| n := int64(math.Ceil((float64(bitLen) / 8) / float64(hash().Size()))) | ||
| if n > 0x7FFFFFFF { |
There was a problem hiding this comment.
nit: can we say instead if n > math.MaxInt32
Closes #473
Closes #474
Closes aws/aws-sdk-go-v2#2634