diff --git a/.gitleaks.toml b/.gitleaks.toml index a03399d9c..783228307 100644 --- a/.gitleaks.toml +++ b/.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 ] diff --git a/CHANGELOG.md b/CHANGELOG.md index ff3f87853..25696e686 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/internal/plugin/connectors/http/aws/aws.go b/internal/plugin/connectors/http/aws/aws.go index 43fc8a59d..b95d538e6 100644 --- a/internal/plugin/connectors/http/aws/aws.go +++ b/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" diff --git a/internal/plugin/connectors/http/aws/connector.go b/internal/plugin/connectors/http/aws/connector.go index 40c756b5c..7064a9979 100644 --- a/internal/plugin/connectors/http/aws/connector.go +++ b/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) + } + } } diff --git a/internal/plugin/connectors/http/aws/connector_test.go b/internal/plugin/connectors/http/aws/connector_test.go new file mode 100644 index 000000000..8ab01e4f2 --- /dev/null +++ b/internal/plugin/connectors/http/aws/connector_test.go @@ -0,0 +1,203 @@ +package aws + +import ( + "bytes" + "context" + "io/ioutil" + "net/http" + "net/http/httputil" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/cyberark/secretless-broker/internal/log" +) + +const authzHeader = "AWS4-HMAC-SHA256 Credential=AKIAIOSFODNN7EXAMPLE/20130524/us-east-1/s3/aws4_request, SignedHeaders=host;range;x-amz-date, Signature=fe5f80f77d5fa3beca038a248ff027d0445342fe2855ddc963176630326f1024" + +type connectTestCase struct { + description string + url string + headers map[string][]string + credentialsByID map[string][]byte + assert func(t *testing.T, beforeR *http.Request, afterR *http.Request, err error) +} + +const connectTestCaseBodyContents = "xyz" +const connectTestCaseBodySHA256 = "3608bca1e44ea6c4d268eb6db02260269892c0b42b86bbf1e77a6fa16c3c9282" + +func (c connectTestCase) Run(t *testing.T) { + t.Run(c.description, func(t *testing.T) { + // Create original request before connect is called + + // Use a request body with standard contents for ease of calculating + // x-amz-content-sha256 + var buf bytes.Buffer + buf.Write([]byte(connectTestCaseBodyContents)) + + beforeR, _ := http.NewRequest("PUT", c.url, &buf) + beforeR.Header.Set( + "x-amz-content-sha256", + "this-will-be-generated-at-signing", + ) + + // Unsigned headers + beforeR.Header.Set("Unsigned-Header-1", "Unsigned-Header-1-Value") + beforeR.Header.Set("Unsigned-Header-2", "Unsigned-Header-2-Value") + + for key, values := range c.headers { + for _, value := range values { + beforeR.Header.Add(key, value) + } + } + + // Create a clone of the original request. We need the original for comparisons + // during assertion. + afterR := beforeR.Clone(context.Background()) + // This is needed because the cloning mechanism for the body isn't deep. + // See https://github.com/golang/go/issues/36095 + afterR.Body, _ = beforeR.GetBody() + + // Call Connect method using the clone of the original request. Some of our assertions + // will be based on the comparison of the original and the clone, since Connect will + // potentially mutate the request passed to it. + connector := Connector{logger: log.NewWithOptions(ioutil.Discard, "", false)} + err := connector.Connect(afterR, c.credentialsByID) + + // Make assertions + c.assert(t, beforeR, afterR, err) + }) +} + +var testCases = []connectTestCase{ + { + description: "no signing", + url: "http://meow.moo", + headers: nil, + credentialsByID: nil, + assert: func(t *testing.T, beforeR *http.Request, afterR *http.Request, err error) { + assert.NoError(t, err) + + beforeRDump, err := httputil.DumpRequest(beforeR, true) + assert.NoError(t, err) + afterRDump, err := httputil.DumpRequest(afterR, true) + assert.NoError(t, err) + + // The request should remain the same before and after because there is no + // initial-signing to override. + assert.Equal( + t, + string(beforeRDump), + string(afterRDump), + ) + }, + }, + { + description: "signing without endpoint discovery", + url: "http://meow.moo", + headers: map[string][]string{ + "Authorization": {authzHeader}, + "X-Amz-Date": {"20210102T150405Z"}, + }, + credentialsByID: map[string][]byte{ + "accessKeyId": []byte("accessKeyIdValue"), + "secretAccessKey": []byte("secretAccessKeyValue"), + }, + assert: func(t *testing.T, beforeR *http.Request, afterR *http.Request, err error) { + assert.NoError(t, err) + + // The request URL should remain the same because endpoint discovery is not + // being used + assert.Equal( + t, + beforeR.URL, + afterR.URL, + ) + // The Authorization should be modified and should use the injected credentials + assert.NotEqual( + t, + beforeR.Header.Get("Authorization"), + afterR.Header.Get("Authorization"), + ) + assert.Equal( + t, + // The expected Authorization header value has been manually calculated + "AWS4-HMAC-SHA256 Credential=accessKeyIdValue/20210102/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=44890edc510facaf7f55bec0cb8eb04a1444690d265858fdab15123b489d5d7b", + afterR.Header.Get("Authorization"), + ) + + assertOnHeadersAfterSigning(t, afterR) + }, + }, + { + description: "signing with endpoint discovery", + url: "http://secretless.empty", + headers: map[string][]string{ + "Authorization": {authzHeader}, + "X-Amz-Date": {"20210102T150405Z"}, + }, + credentialsByID: map[string][]byte{ + "accessKeyId": []byte("accessKeyIdValue"), + "secretAccessKey": []byte("secretAccessKeyValue"), + }, + assert: func(t *testing.T, beforeR *http.Request, afterR *http.Request, err error) { + assert.NoError(t, err) + + // The request URL is changed to one determined by endpoint discovery + assert.Equal( + t, + "https://s3.amazonaws.com", + afterR.URL.String(), + ) + // The Authorization should be modified and should use the injected credentials + assert.Equal( + t, + // The expected Authorization header value has been manually calculated + "AWS4-HMAC-SHA256 Credential=accessKeyIdValue/20210102/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=d75dd6809bc23c1045d7ea19f80bf18fc976b0943eed8aac5aa809810a6b2368", + afterR.Header.Get("Authorization"), + ) + + assertOnHeadersAfterSigning(t, afterR) + }, + }, + { + description: "missing credentials for signing", + url: "http://meow.moo", + headers: map[string][]string{ + "Authorization": {authzHeader}, + "X-Amz-Date": {"20060102T150405Z"}, + }, + assert: func(t *testing.T, beforeR *http.Request, afterR *http.Request, err error) { + assert.Error(t, err) + assert.Contains(t, err.Error(), "AWS connection parameter") + assert.Contains(t, err.Error(), "is not available") + }, + }, +} + +func assertOnHeadersAfterSigning(t *testing.T, afterR *http.Request) { + // X-Amz-Content-Sha256 is always recalculated + assert.Equal( + t, + connectTestCaseBodySHA256, + afterR.Header.Get("X-Amz-Content-Sha256"), + ) + + // Unsigned headers remain unchanged + assert.Equal( + t, + "Unsigned-Header-1-Value", + afterR.Header.Get("Unsigned-Header-1"), + ) + assert.Equal( + t, + "Unsigned-Header-2-Value", + afterR.Header.Get("Unsigned-Header-2"), + ) +} + +func TestConnector_Connect(t *testing.T) { + for _, testCase := range testCases { + testCase.Run(t) + } +}