providers/aws: Refactor + Fix 2x Authorization header append issue. (o…

…pen-policy-agent#5475)

This commit refactors the shared AWS Sig v4 signing code, specifically
to prevent the issue behind open-policy-agent#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: open-policy-agent#5472

Signed-off-by: Philip Conrad <[email protected]>

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
 }

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

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)

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.