From 9af3b22380061d9ed4bb5ce11ab7070111203ba0 Mon Sep 17 00:00:00 2001 From: Philip Conrad Date: Mon, 12 Dec 2022 10:38:21 -0500 Subject: [PATCH] providers/aws: Refactor + Fix 2x Authorization header append issue. (#5475) This commit refactors the shared AWS Sig v4 signing code, specifically to prevent the issue behind #5472. The underlying problem for was that the `"Authorization"` header was being appended *twice* to the request, but only for the AWS REST plugin, because the value was pulled twice from the signed headers map. This was not caught by the unit tests, because the REST plugin's unit tests all assumed the header was single-valued and canonicalized. We now explicitly test for that condition in the unit tests, and the signing code now returns the AWS headers map separately from the value for the `"Authorization"` header, reducing the potential for this mistake to happen in the future. Fixes: #5472 Signed-off-by: Philip Conrad (cherry picked from commit 1d1cb357ae943818b77921a18192b297daf327e4) --- internal/providers/aws.go | 33 +++++++++++++-------------------- plugins/rest/aws.go | 8 ++++---- plugins/rest/aws_test.go | 4 ++++ topdown/providers.go | 6 +++--- 4 files changed, 24 insertions(+), 27 deletions(-) diff --git a/internal/providers/aws.go b/internal/providers/aws.go index 0d40a66879..4bd5342ecc 100644 --- a/internal/providers/aws.go +++ b/internal/providers/aws.go @@ -74,8 +74,8 @@ func sortKeys(strMap map[string][]string) []string { return keys } -// AWSSignV4 modifies a map[string][]string of headers to include an AWS V4 signature based on the config/credentials provided. -func AWSSignV4(headers map[string][]string, method string, theURL *url.URL, body []byte, service string, awsCreds AWSCredentials, theTime time.Time) map[string][]string { +// AWSSignV4 modifies a map[string][]string of headers to generate an AWS V4 signature + headers based on the config/credentials provided. +func AWSSignV4(headers map[string][]string, method string, theURL *url.URL, body []byte, service string, awsCreds AWSCredentials, theTime time.Time) (string, map[string]string) { // General ref. https://docs.aws.amazon.com/general/latest/gr/sigv4_signing.html // S3 ref. https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-auth-using-authorization-header.html // APIGateway ref. https://docs.aws.amazon.com/apigateway/api-reference/signing-requests/ @@ -104,16 +104,18 @@ func AWSSignV4(headers map[string][]string, method string, theURL *url.URL, body } headersToSign := map[string][]string{} - // sign all of the aws headers that aren't on the ignore list. + // sign all of the aws headers. + for k, v := range awsHeaders { + headersToSign[k] = []string{v} + } + + // sign all of the request's headers, except for those in the ignore list for k, v := range headers { lowercaseHeader := strings.ToLower(k) if _, ok := awsSigv4IgnoredHeaders[lowercaseHeader]; !ok { headersToSign[lowercaseHeader] = v } } - for k, v := range awsHeaders { - headersToSign[k] = []string{v} - } // the "canonical request" is the normalized version of the AWS service access // that we're attempting to perform; in this case, a GET from an S3 bucket @@ -153,19 +155,10 @@ func AWSSignV4(headers map[string][]string, method string, theURL *url.URL, body // required format of Authorization header; n.b. the access key corresponding to // the secret key is included here - authHdr := "AWS4-HMAC-SHA256 Credential=" + awsCreds.AccessKey + "/" + dateNow - authHdr += "/" + awsCreds.RegionName + "/" + service + "/aws4_request," - authHdr += "SignedHeaders=" + headerList + "," - authHdr += "Signature=" + fmt.Sprintf("%x", signature) - - // add the computed Authorization - out := make(map[string][]string, len(awsHeaders)+1) - out["Authorization"] = []string{authHdr} - - // populate the other signed headers into the request - for k, v := range awsHeaders { - out[k] = []string{v} - } + authHeader := "AWS4-HMAC-SHA256 Credential=" + awsCreds.AccessKey + "/" + dateNow + authHeader += "/" + awsCreds.RegionName + "/" + service + "/aws4_request," + authHeader += "SignedHeaders=" + headerList + "," + authHeader += "Signature=" + fmt.Sprintf("%x", signature) - return out + return authHeader, awsHeaders } diff --git a/plugins/rest/aws.go b/plugins/rest/aws.go index 059fbd885c..3561448fd0 100644 --- a/plugins/rest/aws.go +++ b/plugins/rest/aws.go @@ -507,11 +507,11 @@ func signV4(req *http.Request, service string, credService awsCredentialService, now := theTime.UTC() - signedHeaders := providers.AWSSignV4(req.Header, req.Method, req.URL, body, service, creds, now) + authHeader, awsHeaders := providers.AWSSignV4(req.Header, req.Method, req.URL, body, service, creds, now) - req.Header.Set("Authorization", strings.Join(signedHeaders["Authorization"], "")) - for k, v := range signedHeaders { - req.Header.Add(k, strings.Join(v, ",")) + req.Header.Set("Authorization", authHeader) + for k, v := range awsHeaders { + req.Header.Add(k, v) } return nil diff --git a/plugins/rest/aws_test.go b/plugins/rest/aws_test.go index 792d16fa88..a80e320071 100644 --- a/plugins/rest/aws_test.go +++ b/plugins/rest/aws_test.go @@ -717,6 +717,10 @@ func TestV4SigningWithMultiValueHeaders(t *testing.T) { "AWS4-HMAC-SHA256 Credential=MYAWSACCESSKEYGOESHERE/20190424/us-east-1/execute-api/aws4_request,"+ "SignedHeaders=accept;host;x-amz-date;x-amz-security-token,"+ "Signature=0237b0c789cad36212f0efba70c02549e1f659ab9caaca16423930cc7236c046", t) + // Ensure 'authorization' is not multi-valued. + if len(req.Header.Values("Authorization")) != 1 { + t.Fatal("Authorization header is multi-valued. This will break AWS v4 signing.") + } // The multi-value headers are preserved assertEq(req.Header.Values("Accept")[0], "text/plain", t) assertEq(req.Header.Values("Accept")[1], "text/html", t) diff --git a/topdown/providers.go b/topdown/providers.go index 8d988eab2c..26e087d7c6 100644 --- a/topdown/providers.go +++ b/topdown/providers.go @@ -7,7 +7,6 @@ package topdown import ( "encoding/json" "net/url" - "strings" "time" "github.com/open-policy-agent/opa/ast" @@ -173,10 +172,11 @@ func builtinAWSSigV4SignReq(ctx BuiltinContext, operands []*ast.Term, iter func( } // Sign the request object's headers, and reconstruct the headers map. - signedHeadersMap := providers.AWSSignV4(objectToMap(headers), method, theURL, body, service, awsCreds, signingTimestamp) + authHeader, signedHeadersMap := providers.AWSSignV4(objectToMap(headers), method, theURL, body, service, awsCreds, signingTimestamp) signedHeadersObj := ast.NewObject() + signedHeadersObj.Insert(ast.StringTerm("Authorization"), ast.StringTerm(authHeader)) for k, v := range signedHeadersMap { - signedHeadersObj.Insert(ast.StringTerm(k), ast.StringTerm(strings.Join(v, ","))) + signedHeadersObj.Insert(ast.StringTerm(k), ast.StringTerm(v)) } // Create new request object with updated headers.