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?

No it's not necessary here. This was lifted from smithy-go httpbinding -- will remove it here.

nit: this is a create queue error, not a list queue error

can we also add a test for double header value, like x-amz-meta=a,b?

There's a double header in one of these cases but i'll make it its own thing

Wait--isn't this that? x-amz-foo has two values in this one.

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(""))

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"

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

That's already happening on 134 - Query() turns URL.RawQuery into a map, RawQuery is already in encoded form.

Added a test that demonstrates this.

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?

I wanted to expose it as const, if it was []byte it'd have to be var UnsignedPayload = blah.

I'd still argue that that is more ergonomic, although making it var means people may be able to change it by mistake

maybe this?

func UnsignedPayload() []byte {
    return []byte("UNSIGNED-PAYLOAD")
}

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

nit: would be nice to also show these parts for debugging

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?

2-3 minutes in my own testing, it seems.

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

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.

Why not use https://pkg.go.dev/crypto/subtle#ConstantTimeCompare ?

They're not entirely the same. The "breakout" case is c > N-2, ConstantTimeCompare can only tell if equal or not equal.

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

• .NET
• Python
• Java

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

FWIW, this derivation component is ripped from the internal go v2 implementation, which previously had an AppSec review.

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.

nit: can we say instead if n > math.MaxInt32