Feat: Add support for AWS Signing Version 4A #5489
Conversation
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
@philipaconrad I'm curious on your thoughts on creating the |
jwineinger
force-pushed
the
aws-sig-v4a
branch
3 times, most recently
from
3191710
to
faea7c2
Compare
jwineinger
changed the title
jwineinger
force-pushed
the
aws-sig-v4a
branch
from
351e97e
to
605f91a
Compare
|
Hello @jwineinger! In reply to some of your questions:
That was done because there was only 1x cloud provider with 1x feature to worry about: AWS, with Sig v4 request signing. It seemed a bit excessive at the time to carve out another directory just for that. Now that there's 2 signing algorithms, giving it a sub-package unto itself makes more sense. The reason for the "AWS" naming prefix was precisely because everything was nested under |
|
@jwineinger This is a chunky PR (+/- around 1500 LOC!) that I want take the time to properly review, but my initial impression from skimming over the code is a very positive one. 🙂 I'm planning to carve out some time early next week to specifically go and double-check over the trickier parts, especially the crypto bits, tests, and anything mangling header values. I suspect the crypto parts will be simple enough, since it looks like you simply ported the ECC utilities over straight from the AWS SDK. Early Comments:
Thank you for your contribution, and I'll see about getting some more detailed CR comments up for you next week! 😄 |
yeah
I saw those as well. It seems to be only on go 1.17, which is a little strange to me. Go 1.18 and 1.19 (locally) all pass. I did start digging into them a little on Friday and hopefully I'll get more time to finish that early this week. I am able to repro the failures using 1.17 locally. It seems that the generated signature is different on 1.17 than 1.18/1.19. |
|
It appears the test failures between 1.17 and 1.18 are caused by a go crypto library change, specifically in the generation of the key used to initialize the AES cypher. In go 1.17, it calculates that it needs 128 bits / 16 bytes of entropy (https://github.com/golang/go/blob/go1.17.13/src/crypto/ecdsa/ecdsa.go#L204-L208) because the derived private key uses a 256 bit curve (https://github.com/open-policy-agent/opa/blob/0e06fb332fdda0d00e0ad0e55b63f904eb42f9fb/internal/providers/aws/signing_v4a.go#L75-L120), which is used in the key generation (https://github.com/golang/go/blob/go1.17.13/src/crypto/ecdsa/ecdsa.go#L217). In go 1.18, the implementation was changed to always get 256 bits of entropy (https://github.com/golang/go/blob/go1.18.9/src/crypto/ecdsa/ecdsa.go#L209). This changes the key, and thus the AES cipher, and thus the resulting signature. It seems a bit odd that the same derived key could produce different signatures, though I'm no crypto expert. I may do some testing against AWS and see if they accept both. |
|
Interestingly, both go 1.17.13 and 1.18.9 produce binaries that sign requests in a way that AWS accepts. So I guess the thing to do is have the tests expect different signatures between go versions. |
jwineinger
force-pushed
the
aws-sig-v4a
branch
from
95f4d16
to
0e06fb3
Compare
| expectedAuthorization: []string{ | ||
| // this signature is for go 1.18+, which changed crypto/ecdsa so signatures differ from go 1.17 | ||
| "AWS4-ECDSA-P256-SHA256 Credential=MYAWSACCESSKEYGOESHERE/20190424/execute-api/aws4_request, " + | ||
| "SignedHeaders=content-length;content-type;host;x-amz-content-sha256;x-amz-date;x-amz-region-set;x-amz-security-token, " + | ||
| "Signature=30450221009f3b0cda178456dfd1bec61b78bdbd115c0cf497eaa52c58bbb2850ad9c49c3002207009cb88a1219a4a6626056c31823a6b5bc2728bc88bc98a06e12e1148482c94", | ||
| // this signature is for go 1.17. Remove this and only test for a single value when OPA drops go 1.17 | ||
| "AWS4-ECDSA-P256-SHA256 Credential=MYAWSACCESSKEYGOESHERE/20190424/execute-api/aws4_request, " + | ||
| "SignedHeaders=content-length;content-type;host;x-amz-content-sha256;x-amz-date;x-amz-region-set;x-amz-security-token, " + | ||
| "Signature=304602210088b5a5ccf9e37aac765f7e6bf0507577eb1b919b80bc3c385b8856c7ab7a9912022100f4c558e36be338c9644240b722e06333ea9a5305b2e638d56ad0105995c9b1f7", | ||
| }, |
There was a problem hiding this comment.
This is excellent, and solves the problem with 2x signature results being possible. 👍
Well, that's horrifying. 😅 Thank you for digging into that issue, and for coming up with a workaround. |
There was a problem hiding this comment.
@jwineinger This PR looks good to me! 😄
Thank you for putting in the time to investigate the test signature differences.
AWS is rolling out an extension to SigV4 called Signature Version 4A (SigV4A) which enables signatures that are valid in more than one AWS Region. This is required for signing multi-region API requests, for example with Amazon S3 Multi-Region Access Points (MRAP). This lets OPA use an S3 MRAP as a bundle source that is latency optimized by geography and also highly-available / resilient against regional outages. This commit adds SigV4A support with an implementation that is a modified version of internal code from the aws-sdk-go-v2 project: https://github.com/aws/aws-sdk-go-v2/tree/93c3f18/internal/v4a. That approach felt far safer than trying to re-implement the necessary crypto functions from scratch. Note: the aws-sdk-go-v2 project is Apache 2 licensed, so copying and modifying their code seems fine according to section 4. I've attempted to satisfy the conditions of that section by: * 4.b: adding a comment line at the top of internal/providers/aws/signing_v4a.go * 4.d: including the NOTICES.txt file from aws-sdk-go-v2 in the internal/providers/aws/ folder This also makes an internal/providers/aws package that contains both the existing V4 signing code as well as the V4A signing code being added. This seemed reasonable for code organization and because this is internal code. Fixes open-policy-agent#5429 Signed-off-by: Jay Wineinger <jawineinger@spscommerce.com>
Signed-off-by: Jay Wineinger <jawineinger@spscommerce.com>
…cdsa changes Signed-off-by: Jay Wineinger <jawineinger@spscommerce.com>
Signed-off-by: Jay Wineinger <jawineinger@spscommerce.com>
philipaconrad
force-pushed
the
aws-sig-v4a
branch
from
0e06fb3
to
febf55d
Compare
AWS is rolling out an extension to SigV4 called Signature Version 4A (SigV4A) which enables signatures that are valid in more than one AWS Region. This is required for signing multi-region API requests, for example with Amazon S3 Multi-Region Access Points (MRAP). This lets OPA use an S3 MRAP as a bundle source that is latency optimized by geography and also highly-available / resilient against regional outages.
This commit adds SigV4A support with an implementation that is a modified version of internal code from the aws-sdk-go-v2 project: https://github.com/aws/aws-sdk-go-v2/tree/93c3f18/internal/v4a. That approach felt far safer than trying to re-implement the necessary crypto functions from scratch. Note: the aws-sdk-go-v2 project is Apache 2 licensed, so copying and modifying their code seems fine according to section 4. I've attempted to satisfy the conditions of that section by:
This also makes an internal/providers/aws package that contains both the existing V4 signing code as well as the V4A signing code being added. This seemed reasonable for code organization and because this is internal code.
Fixes #5429