Merge pull request #1432 from cyberark/fix/aws-connector

Fix(aws-connector): Limit signing to signed headers from original request

.gitleaks.toml

@@ -15,8 +15,8 @@ description = "AWS Client ID"
 regex = '''(A3T[A-Z0-9]|AKIA|AGPA|AIDA|AROA|AIPA|ANPA|ANVA|ASIA)[A-Z0-9]{16}'''
 tags = ["key", "AWS"]
 [[rules.whitelist]]
-description = "sample AWS key in AWS HTTP connector comments"
-regex = '''AKIAJC5FABNOFVBKRWHA'''
+description = "sample AWS key in AWS HTTP connector"
+regex = '''AKIAIOSFODNN7EXAMPLE'''
 [[rules.whitelist]]
 description = "since-removed sample AWS key"
 regex = '''AKIAJADDJE4Q4JVX3HAA'''
@@ -214,6 +214,7 @@ files = [
   "test/ssh/id_(.*)", # since-removed ssh test certs
   "test/util/ssl/(.*)", # test ssl certs
   "internal/plugin/connectors/tcp/mssql/connection_details_test.go", # fake cert string
+  "internal/plugin/connectors/http/aws/(.*)", # fake AWS credentials
 ]
 # As of v4, gitleaks can whitelist paths to accommodate no longer using
 # paths in the `files` whitelist.
@@ -232,10 +233,11 @@ paths = [
   "test/connector/tcp/mssql/certs",
   "test/ssh",
   "test/util/ssl",
-  "internal/plugin/connectors/tcp/mssql"
+  "internal/plugin/connectors/tcp/mssql",
+  "internal/plugin/connectors/http/aws",
 ]
 regexes = [
-  "AKIAJC5FABNOFVBKRWHA", # sample AWS key in AWS HTTP connector comments
+  "AKIAIOSFODNN7EXAMPLE", # sample AWS key in AWS HTTP connector
   "AKIAJADDJE4Q4JVX3HAA", # since-removed sample AWS key
   "SuperSecure", # dummy password used in conjur integration test docker-compose
 ]

CHANGELOG.md

@@ -6,6 +6,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## [Unreleased]
 
+### Fixed
+- Request-signing on the AWS connector was updated to address a bug that was
+  causing failed integrity checks, where the request-signing by Secretless was
+  incorporating more headers than were used on the original request-signing. The
+  fix limits the headers used by Secretless to those used in the original
+  request. [cyberark/secretless-broker#1432](https://github.com/cyberark/secretless-broker/issues/1432)
+
 ### Security
 - Updated containerd to v1.4.11 to close CVE-2020-15257 (Not vulnerable)
   [cyberark/secretless-broker#1431](https://github.com/cyberark/secretless-broker/pull/1431)

internal/plugin/connectors/http/aws/aws.go

@@ -6,7 +6,6 @@ import (
 	"io/ioutil"
 	gohttp "net/http"
 	"net/url"
-	"regexp"
 	"strings"
 	"time"
 
@@ -20,15 +19,6 @@ import (
 // From https://github.com/aws/aws-sdk-go/blob/master/aws/signer/v4/v4.go#L77
 const timeFormat = "20060102T150405Z"
 
-// reForCredentialComponent matches headers strings like:
-//
-//     Credential=AKIAJC5FABNOFVBKRWHA/20171103/us-east-1/ec2/aws4_request
-//
-// See https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-auth-using-authorization-header.html
-var reForCredentialComponent = regexp.MustCompile(
-	`^Credential=\w+\/\d+\/([\w-_]+)\/(\w+)\/aws4_request$`,
-)
-
 // newAmzDate parses a date string using the AWS signer time format
 func newAmzDate(amzDateStr string) (time.Time, error) {
 	if amzDateStr == "" {
@@ -39,52 +29,52 @@ func newAmzDate(amzDateStr string) (time.Time, error) {
 }
 
 // requestMetadataFromAuthz parses an authorization header string and create a
-// requestMetadata instance populated with the associated region and service
-// name
+// requestMetadata instance populated with the associated region, service
+// name and signed headers
 func requestMetadataFromAuthz(authorizationStr string) (*requestMetadata, error) {
-	// extract credentials section of authorization header
-	credentialsComponent, err := extractCredentialsComponent(authorizationStr)
-	if err != nil {
-		return nil, err
+	// Parse the following (line breaks added for readability):
+	// AWS4-HMAC-SHA256 \
+	// Credential=AKIAIOSFODNN7EXAMPLE/20130524/us-east-1/s3/aws4_request, \
+	// SignedHeaders=host;range;x-amz-date, \
+	// Signature=fe5f80f77d5fa3beca038a248ff027d0445342fe2855ddc963176630326f1024
+	//
+	// See https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-auth-using-authorization-header.html
+
+	// Validate form of entire authorization header
+	tokens := strings.Split(authorizationStr, ", ")
+	if len(tokens) != 3 || tokens[0] == "" || tokens[1] == "" || tokens[2] == "" {
+		return nil, fmt.Errorf("malformed Authorization header")
 	}
-	// validate credential component of authorization header, then extract region
-	// and service name
-	matches := reForCredentialComponent.FindStringSubmatch(credentialsComponent)
-	if len(matches) != 3 {
+
+	// Extract region and service name from credential component
+	credentialParts := strings.SplitN(tokens[0], "/", 5)
+	if len(credentialParts) != 5 {
 		return nil, fmt.Errorf("malformed credential component of Authorization header")
 	}
 
+	region := credentialParts[2]
+	serviceName := credentialParts[3]
+
+	// Extract signed headers from signed headers component
+	signedHeaders := strings.Split(
+		strings.TrimPrefix(tokens[1], "SignedHeaders="),
+		";",
+	)
+
 	return &requestMetadata{
-		region:      matches[1],
-		serviceName: matches[2],
+		region:        region,
+		serviceName:   serviceName,
+		signedHeaders: signedHeaders,
 	}, nil
 }
 
-// requestMetadata captures the metadata of a signed AWS request: date, region
-// and service name
+// requestMetadata captures the metadata of a signed AWS request: date, region, service
+// name and signed headers
 type requestMetadata struct {
-	date        time.Time
-	region      string
-	serviceName string
-}
-
-// extractCredentialsComponent extracts the credentials component from an
-// authorization header string
-func extractCredentialsComponent(authorizationStr string) (string, error) {
-	// Parse the following (line breaks added):
-	// AWS4-HMAC-SHA256
-	// Credential=AKIAJC5FABNOFVBKRWHA/20171103/us-east-1/ec2/aws4_request, \
-	// SignedHeaders=content-type;host;x-amz-date, \
-	// Signature=c4a8ade09a5e0c644cc282311c36aae6c834596076ffde7db7d1195c7b454ed0
-
-	// validate form of entire authorization header
-	tokens := strings.Split(authorizationStr, ", ")
-	if len(tokens) != 3 || tokens[0] == "" || tokens[1] == "" || tokens[2] == "" {
-		return "", fmt.Errorf("malformed Authorization header")
-	}
-
-	// trim prefix and return credential component
-	return strings.TrimPrefix(tokens[0], "AWS4-HMAC-SHA256 "), nil
+	date          time.Time
+	region        string
+	serviceName   string
+	signedHeaders []string
 }
 
 // newRequestMetadata parses the request headers to extract the metadata
@@ -99,21 +89,21 @@ func newRequestMetadata(r *gohttp.Request) (*requestMetadata, error) {
 		return nil, nil
 	}
 
-	// parse date string
+	// Parse date string
 	//
 	date, err := newAmzDate(amzDateStr)
 	if err != nil {
 		return nil, err
 	}
 
-	// create request metadata by extracting service name and region from
+	// Create request metadata by extracting service name and region from
 	// Authorization header
 	reqMeta, err := requestMetadataFromAuthz(authorizationStr)
 	if err != nil {
 		return nil, err
 	}
 
-	// populate request metadata with date
+	// Populate request metadata with date
 	reqMeta.date = date
 
 	return reqMeta, nil
@@ -169,14 +159,10 @@ func signRequest(
 		reqMeta.region,
 		reqMeta.date,
 	)
-	if err != nil {
-		return err
-	}
-
-	return nil
+	return err
 }
 
-// setAmzEndpoint, when the request URL is http://secretless.empty, sets the
+// maybeSetAmzEndpoint, when the request URL is http://secretless.empty, sets the
 // request endpoint using the default AWS endpoint resolver. The resolver allows
 // the connector to mimic a typical AWS client and provides a TLS endpoint where
 // possible.
@@ -188,12 +174,12 @@ func signRequest(
 // endpoint and then this connector can use the AWS SDK to resolve the endpoint
 // in the same way the client might via a direct call to Amazon over HTTPS.
 //
-// Note that if the client to specifies an HTTP (not HTTPS) endpoint that is not
-// http://secretless.empty it will be respected.
+// Note that if the client specifies an HTTP (not HTTPS, because Secretless does not proxy
+// HTTPS requests) endpoint that is not http://secretless.empty it will be respected.
 //
 // Note: There is a plan to add a configuration option to instruct Secretless to
-// upgrade the connect between Secretless and the target endpoint to TLS.
-func setAmzEndpoint(req *gohttp.Request, reqMeta *requestMetadata) error {
+// upgrade the connect between Secretless and the target endpoint to TLS
+func maybeSetAmzEndpoint(req *gohttp.Request, reqMeta *requestMetadata) error {
 	shouldSetEndpoint := req.URL.Scheme == "http" &&
 		req.URL.Host == "secretless.empty"
 

internal/plugin/connectors/http/aws/connector.go

@@ -2,6 +2,7 @@ package aws
 
 import (
 	gohttp "net/http"
+	"strings"
 
 	"github.com/cyberark/secretless-broker/pkg/secretless/log"
 	"github.com/cyberark/secretless-broker/pkg/secretless/plugin/connector"
@@ -25,7 +26,7 @@ func (c *Connector) Connect(
 ) error {
 	var err error
 
-	// Extract metadata of a signed AWS request: date, region and service name
+	// Extract metadata of a signed AWS request: date, region and service name.
 	reqMeta, err := newRequestMetadata(req)
 	if err != nil {
 		return err
@@ -40,16 +41,70 @@ func (c *Connector) Connect(
 	// Set AWS endpoint
 	// NOTE: this must be done before signing the request, otherwise the modified request
 	// will fail the integrity check.
-	err = setAmzEndpoint(req, reqMeta)
+	err = maybeSetAmzEndpoint(req, reqMeta)
 	if err != nil {
 		return err
 	}
 
 	// Use metadata and credentials to sign request
 	c.logger.Debugf(
-		"Signing for service=%s region=%s",
+		"Signing for service=%s region=%s signedHeaders=%s",
 		reqMeta.serviceName,
 		reqMeta.region,
+		strings.Join(reqMeta.signedHeaders, ","),
 	)
-	return signRequest(req, reqMeta, credentialsByID)
+
+	// Temporarily remove any headers that were not signed in the original request.
+	unsignedHeaders := removeUnsignedHeaders(req, reqMeta)
+
+	// Sign the request.
+	err = signRequest(req, reqMeta, credentialsByID)
+	if err != nil {
+		return err
+	}
+
+	// Reinstate unsigned headers without clobbering the effects of signing.
+	reinstateUnsignedHeaders(req, unsignedHeaders)
+
+	return nil
+}
+
+func removeUnsignedHeaders(req *gohttp.Request, reqMeta *requestMetadata) map[string][]string {
+	var signedHeadersMap = map[string]struct{}{}
+	for _, key := range reqMeta.signedHeaders {
+		signedHeadersMap[key] = struct{}{}
+	}
+
+	var unsignedHeaders = map[string][]string{}
+	for key, value := range req.Header {
+		if _, isSignedHeader := signedHeadersMap[key]; isSignedHeader {
+			continue
+		}
+
+		unsignedHeaders[key] = value
+		req.Header.Del(key)
+	}
+
+	return unsignedHeaders
+}
+
+func reinstateUnsignedHeaders(req *gohttp.Request, unsignedHeaders map[string][]string) {
+	// Reserved meaning the headers already on the request. We shouldn't touch those because
+	// they should only be the signed headers and those generated by signing
+	var reservedHeaders = map[string]struct{}{}
+	for key := range req.Header {
+		reservedHeaders[key] = struct{}{}
+	}
+
+	for key, values := range unsignedHeaders {
+		// Ignore reserved headers, we don't want to mess with those!
+		if _, isReservedHeader := reservedHeaders[key]; isReservedHeader {
+			continue
+		}
+
+		// Add back all the values for each non-reserved unsigned header
+		for _, value := range values {
+			req.Header.Add(key, value)
+		}
+	}
 }