From dbcbc7323952e005c9cd2a7569c1a0ac5038a853 Mon Sep 17 00:00:00 2001 From: Kumbirai Tanekha Date: Tue, 19 Oct 2021 22:01:55 +0100 Subject: [PATCH] Fix(aws-connector): Limit signing to signed headers from original request Prior to this change the AWS connector was signing requests using all the headers present on the original request. This was resulting signature mismatches and failed auth, particularly visible when creating a new s3 bucket. With this change the aws connector will sign only the headers on the original request, it achieves this by temporarily hiding the rest of the headers before signing, then them after signing. --- CHANGELOG.md | 8 +++ internal/plugin/connectors/http/aws/aws.go | 70 +++++++++---------- .../plugin/connectors/http/aws/connector.go | 59 +++++++++++++++- 3 files changed, 98 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f2694375..e81c1a32e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,14 @@ 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) + ## [1.7.6] - 2021-09-10 ### Added diff --git a/internal/plugin/connectors/http/aws/aws.go b/internal/plugin/connectors/http/aws/aws.go index 43fc8a59d..29f11e266 100644 --- a/internal/plugin/connectors/http/aws/aws.go +++ b/internal/plugin/connectors/http/aws/aws.go @@ -39,38 +39,9 @@ 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 - } - // validate credential component of authorization header, then extract region - // and service name - matches := reForCredentialComponent.FindStringSubmatch(credentialsComponent) - if len(matches) != 3 { - return nil, fmt.Errorf("malformed credential component of Authorization header") - } - - return &requestMetadata{ - region: matches[1], - serviceName: matches[2], - }, nil -} - -// requestMetadata captures the metadata of a signed AWS request: date, region -// and service name -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, \ @@ -80,11 +51,38 @@ func extractCredentialsComponent(authorizationStr string) (string, error) { // 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") + return nil, fmt.Errorf("malformed Authorization header") + } + + // 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") } - // trim prefix and return credential component - return strings.TrimPrefix(tokens[0], "AWS4-HMAC-SHA256 "), nil + region := credentialParts[2] + serviceName := credentialParts[3] + + // extract signed headers from signed headers component + signedHeaders := strings.Split( + strings.TrimPrefix(tokens[1], "SignedHeaders="), + ";", + ) + + return &requestMetadata{ + region: region, + serviceName: serviceName, + signedHeaders: signedHeaders, + }, nil +} + +// 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 + signedHeaders []string } // newRequestMetadata parses the request headers to extract the metadata @@ -176,7 +174,7 @@ func signRequest( return nil } -// 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. @@ -193,7 +191,7 @@ func signRequest( // // 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 { +func maybeSetAmzEndpoint(req *gohttp.Request, reqMeta *requestMetadata) error { shouldSetEndpoint := req.URL.Scheme == "http" && req.URL.Host == "secretless.empty" diff --git a/internal/plugin/connectors/http/aws/connector.go b/internal/plugin/connectors/http/aws/connector.go index 40c756b5c..98ac8debb 100644 --- a/internal/plugin/connectors/http/aws/connector.go +++ b/internal/plugin/connectors/http/aws/connector.go @@ -25,7 +25,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,7 +40,7 @@ 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 } @@ -51,5 +51,58 @@ func (c *Connector) Connect( reqMeta.serviceName, reqMeta.region, ) - 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 _, ok := signedHeadersMap[key]; ok { + unsignedHeaders[key] = value + continue + } + + 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 _, ok := reservedHeaders[key]; ok { + continue + } + + // Add back all the values for each non-reserved unsigned header + for _, value := range values { + req.Header.Add(key, value) + } + } }