diff --git a/.secrets.baseline b/.secrets.baseline index c4c8580..07cd2b0 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -3,7 +3,7 @@ "files": "go.sum|package-lock.json|^.secrets.baseline$", "lines": null }, - "generated_at": "2023-11-06T17:25:56Z", + "generated_at": "2023-11-17T20:18:24Z", "plugins_used": [ { "name": "AWSKeyDetector" @@ -82,7 +82,7 @@ "hashed_secret": "91dfd9ddb4198affc5c194cd8ce6d338fde470e2", "is_secret": false, "is_verified": false, - "line_number": 81, + "line_number": 82, "type": "Secret Keyword", "verified_result": null }, @@ -90,7 +90,7 @@ "hashed_secret": "e0d246cf37df7d1a561ed649d108dd14f36f28bf", "is_secret": false, "is_verified": false, - "line_number": 241, + "line_number": 242, "type": "Secret Keyword", "verified_result": null }, @@ -98,7 +98,7 @@ "hashed_secret": "98635b2eaa2379f28cd6d72a38299f286b81b459", "is_secret": false, "is_verified": false, - "line_number": 548, + "line_number": 549, "type": "Secret Keyword", "verified_result": null }, @@ -106,7 +106,7 @@ "hashed_secret": "47fcf185ee7e15fe05cae31fbe9e4ebe4a06a40d", "is_secret": false, "is_verified": false, - "line_number": 597, + "line_number": 692, "type": "Secret Keyword", "verified_result": null } @@ -116,7 +116,7 @@ "hashed_secret": "bc2f74c22f98f7b6ffbc2f67453dbfa99bce9a32", "is_secret": false, "is_verified": false, - "line_number": 744, + "line_number": 751, "type": "Secret Keyword", "verified_result": null } diff --git a/.travis.yml b/.travis.yml index 095c9db..4568b06 100644 --- a/.travis.yml +++ b/.travis.yml @@ -10,6 +10,13 @@ go: notifications: email: false +addons: + hosts: + - region1.cloud.ibm.com + - region1.notcloud.ibm.com + - region2.cloud.ibm.com + - region2.notcloud.ibm.com + env: global: - GO111MODULE=on diff --git a/Makefile b/Makefile index 9914aca..417b593 100644 --- a/Makefile +++ b/Makefile @@ -16,7 +16,7 @@ test: lint: ${LINT} run --build-tags=all - DIFF=$$(${FORMATTER} -d core); if [[ -n "$$DIFF" ]]; then printf "\n$$DIFF" && exit 1; fi + DIFF=$$(${FORMATTER} -d core); if [ -n "$$DIFF" ]; then printf "\n$$DIFF" && exit 1; fi scan-gosec: ${GOSEC} ./... diff --git a/core/base_service.go b/core/base_service.go index defeef2..1cf3631 100644 --- a/core/base_service.go +++ b/core/base_service.go @@ -38,6 +38,7 @@ import ( const ( headerNameUserAgent = "User-Agent" sdkName = "ibm-go-sdk-core" + maxRedirects = 10 ) // ServiceOptions is a struct of configuration values for a service. @@ -117,7 +118,7 @@ func (service *BaseService) Clone() *BaseService { // First, copy the service options struct. serviceOptions := *service.Options - // Next, make a copy the service struct, then use the copy of the service options. + // Next, make a copy of the service struct, then use the copy of the service options. // Note, we'll re-use the "Client" instance from the original BaseService instance. clone := *service clone.Options = &serviceOptions @@ -234,7 +235,7 @@ func (service *BaseService) SetDefaultHeaders(headers http.Header) { // the retryable client; otherwise "client" will be stored // directly on "service". func (service *BaseService) SetHTTPClient(client *http.Client) { - setMinimumTLSVersion(client) + setupHTTPClient(client) if isRetryableClient(service.Client) { // If "service" is currently holding a retryable client, @@ -298,15 +299,76 @@ func (service *BaseService) IsSSLDisabled() bool { return false } -// setMinimumTLSVersion sets the minimum TLS version required by the client to TLS v1.2 -func setMinimumTLSVersion(client *http.Client) { - if tr, ok := client.Transport.(*http.Transport); tr != nil && ok { - if tr.TLSClientConfig == nil { - tr.TLSClientConfig = &tls.Config{} // #nosec G402 +// setupHTTPClient will configure "client" for use with the BaseService object. +func setupHTTPClient(client *http.Client) { + // Set our "CheckRedirect" function to allow safe headers to be included + // in redirected requests under certain conditions. + if client.CheckRedirect == nil { + client.CheckRedirect = checkRedirect + } +} + +// checkRedirect is used as an override for the default "CheckRedirect" function supplied +// by the net/http package and implements some additional logic required by IBM SDKs. +func checkRedirect(req *http.Request, via []*http.Request) error { + + // The net/http module is implemented such that it will only include "safe" headers + // ("Authorization", "WWW-Authenticate", "Cookie", "Cookie2") when redirecting a request + // if the redirected host is the same host or a sub-domain of the original request's host. + // Example: foo.com redirected to foo.com or bar.foo.com would work, but bar.com would not. + // This "CheckRedirect" implementation will propagate "safe" headers in a redirected request + // only in situations where the hosts associated with the original and redirected request URLs + // are both located within the ".cloud.ibm.com" domain. + + // First, perform the check that is done by the default CheckRedirect function + // to ensure we don't exhaust our max redirect limit. + if len(via) >= maxRedirects { + GetLogger().Debug("Exceeded max redirects: %d", maxRedirects) + return fmt.Errorf("stopped after %d redirects", maxRedirects) + } + + if len(via) > 0 { + GetLogger().Debug("Detected %d prior request(s)", len(via)) + originalReq := via[0] + redirectedReq := req + GetLogger().Debug("Redirecting request from %s to %s", originalReq.URL.String(), redirectedReq.URL.String()) + redirectedHeader := req.Header + originalHeader := via[0].Header + + originalHost := originalReq.URL.Hostname() + redirectedHost := redirectedReq.URL.Hostname() + + if shouldCopySafeHeadersOnRedirect(originalHost, redirectedHost) { + + // We're only concerned with "safe" headers since these are the ones that are not + // propagated automatically by net/http for a "cross-site" redirect. + for _, headerKey := range []string{"Authorization", "WWW-Authenticate", "Cookie", "Cookie2"} { + // If the original request contains a value for "headerKey" + // *and* this header is not already present in the redirected request, + // then copy the value from the original request to the redirected request. + if v, inOriginalRequest := originalHeader[headerKey]; inOriginalRequest { + if _, inRedirectedRequest := redirectedHeader[headerKey]; !inRedirectedRequest { + redirectedHeader[headerKey] = v + GetLogger().Debug("Propagating header '%s' in redirected request", headerKey) + } + } + } + } else { + GetLogger().Debug("Redirected request is not within the trusted domain.") } - - tr.TLSClientConfig.MinVersion = tls.VersionTLS12 + } else { + GetLogger().Debug("Detected no prior requests!") } + return nil +} + +// shouldCopySafeHeadersOnRedirect returns true iff safe headers should be copied +// to a redirected request. +func shouldCopySafeHeadersOnRedirect(fromHost, toHost string) bool { + GetLogger().Debug("hosts: %s %s", fromHost, toHost) + sameHost := fromHost == toHost + safeDomain := strings.HasSuffix(fromHost, ".cloud.ibm.com") && strings.HasSuffix(toHost, ".cloud.ibm.com") + return sameHost || safeDomain } // SetEnableGzipCompression sets the service's EnableGzipCompression field @@ -693,7 +755,7 @@ func (service *BaseService) DisableRetries() { // DefaultHTTPClient returns a non-retryable http client with default configuration. func DefaultHTTPClient() *http.Client { client := cleanhttp.DefaultPooledClient() - setMinimumTLSVersion(client) + setupHTTPClient(client) return client } @@ -731,7 +793,7 @@ func NewRetryableClientWithHTTPClient(httpClient *http.Client) *retryablehttp.Cl // as our embedded client used to invoke individual requests. client.HTTPClient = httpClient } else { - // Otherwise, we'll use construct a default HTTP client and use that + // Otherwise, we'll construct a default HTTP client and use that client.HTTPClient = DefaultHTTPClient() } diff --git a/core/base_service_redirect_test.go b/core/base_service_redirect_test.go new file mode 100644 index 0000000..fd2ff65 --- /dev/null +++ b/core/base_service_redirect_test.go @@ -0,0 +1,155 @@ +//go:build all || slow || auth +// +build all slow auth + +package core + +// (C) Copyright IBM Corp. 2023. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +// Note: this unit test depends on some bogus hostnames being defined in /etc/hosts. +// Append this to your /etc/hosts file: +// # for testing +// 127.0.0.1 region1.cloud.ibm.com region2.cloud.ibm.com region1.notcloud.ibm.com region2.notcloud.ibm.com + +var ( + operationPath string = "/api/redirector" + + // To enable debug mode while running these tests, set this to LevelDebug. + redirectTestLogLevel LogLevel = LevelError +) + +// Start a mock server that will redirect requests to the second mock server +// located at "redirectServerURL" +func startMockServer1(t *testing.T, redirectServerURL string) *httptest.Server { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + t.Logf(`server1 received request: %s %s`, req.Method, req.URL.String()) + + // Make sure the Authorization header was sent. + assert.NotEmpty(t, req.Header.Get("Authorization")) + + path := req.URL.Path + location := redirectServerURL + path + + // Create the response (a 302 redirect). + w.Header().Add("Location", location) + w.WriteHeader(http.StatusFound) + t.Logf(`Sent redirect request to: %s`, location) + })) + return server +} + +// Start a second mock server to which redirected requests will be sent. +func startMockServer2(t *testing.T) *httptest.Server { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + t.Logf(`server2 received request: %s %s`, req.Method, req.URL.String()) + + // Create the response. + if req.Header.Get("Authorization") != "" { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprintf(w, `{"name":"Jason Bourne"}`) + } else { + w.WriteHeader(http.StatusUnauthorized) + } + })) + return server +} + +func startServers(t *testing.T, host1 string, host2 string) (server1 *httptest.Server, server1URL string, + server2 *httptest.Server, server2URL string) { + server2 = startMockServer2(t) + server2URL = strings.ReplaceAll(server2.URL, "127.0.0.1", host2) + t.Logf(`Server 2 listening on endpoint: %s (%s)`, server2URL, server2.URL) + + server1 = startMockServer1(t, server2URL) + server1URL = strings.ReplaceAll(server1.URL, "127.0.0.1", host1) + t.Logf(`Server 1 listening on endpoint: %s (%s)`, server1URL, server1.URL) + + return +} + +func testRedirection(t *testing.T, host1 string, host2 string, expectedStatusCode int) { + GetLogger().SetLogLevel(redirectTestLogLevel) + + // Both servers within trusted domain. + server1, server1URL, server2, _ := startServers(t, host1, host2) + defer server1.Close() + defer server2.Close() + + builder := NewRequestBuilder("GET") + _, err := builder.ResolveRequestURL(server1URL, operationPath, nil) + assert.Nil(t, err) + req, _ := builder.Build() + + authenticator, err := NewBearerTokenAuthenticator("this is not a secret") + assert.Nil(t, err) + assert.NotNil(t, authenticator) + + options := &ServiceOptions{ + URL: server1.URL, + Authenticator: authenticator, + } + service, err := NewBaseService(options) + assert.Nil(t, err) + + var foo *Foo + detailedResponse, err := service.Request(req, &foo) + assert.NotNil(t, detailedResponse) + assert.Equal(t, expectedStatusCode, detailedResponse.StatusCode) + if expectedStatusCode >= 200 && expectedStatusCode <= 299 { + assert.Nil(t, err) + + result, ok := detailedResponse.Result.(*Foo) + assert.Equal(t, true, ok) + assert.NotNil(t, result) + assert.NotNil(t, foo) + assert.Equal(t, "Jason Bourne", *result.Name) + } else { + assert.NotNil(t, err) + } +} + +func TestRedirectAuthSuccess1(t *testing.T) { + testRedirection(t, "region1.cloud.ibm.com", "region2.cloud.ibm.com", http.StatusOK) +} + +func TestRedirectAuthSuccess2(t *testing.T) { + testRedirection(t, "region1.cloud.ibm.com", "region1.cloud.ibm.com", http.StatusOK) +} + +func TestRedirectAuthSuccess3(t *testing.T) { + testRedirection(t, "region1.notcloud.ibm.com", "region1.notcloud.ibm.com", http.StatusOK) +} + +func TestRedirectAuthFail1(t *testing.T) { + testRedirection(t, "region1.notcloud.ibm.com", "region2.cloud.ibm.com", http.StatusUnauthorized) +} + +func TestRedirectAuthFail2(t *testing.T) { + testRedirection(t, "region1.cloud.ibm.com", "region2.notcloud.ibm.com", http.StatusUnauthorized) +} + +func TestRedirectAuthFail3(t *testing.T) { + testRedirection(t, "region1.notcloud.ibm.com", "region2.notcloud.ibm.com", http.StatusUnauthorized) +} diff --git a/core/base_service_test.go b/core/base_service_test.go index 9ee052c..65fa2dc 100644 --- a/core/base_service_test.go +++ b/core/base_service_test.go @@ -1769,7 +1769,6 @@ func TestClientWithRetries(t *testing.T) { service.SetHTTPClient(client) actualClient := service.GetHTTPClient() assert.Equal(t, client, actualClient) - assert.Equal(t, *client, *actualClient) assert.Equal(t, client, service.Client) // Next, enable retries and make sure the client survived. @@ -1777,7 +1776,6 @@ func TestClientWithRetries(t *testing.T) { assert.True(t, isRetryableClient(service.Client)) actualClient = service.GetHTTPClient() assert.Equal(t, client, actualClient) - assert.Equal(t, *client, *actualClient) // Finally, disable retries and make sure // we're left with the same client instance. @@ -1785,7 +1783,6 @@ func TestClientWithRetries(t *testing.T) { assert.False(t, isRetryableClient(service.Client)) actualClient = service.GetHTTPClient() assert.Equal(t, client, actualClient) - assert.Equal(t, *client, *actualClient) assert.Equal(t, client, service.Client) // Create a new service and perform the steps in a different order. @@ -1804,7 +1801,6 @@ func TestClientWithRetries(t *testing.T) { assert.True(t, isRetryableClient(service.Client)) actualClient = service.GetHTTPClient() assert.Equal(t, client, actualClient) - assert.Equal(t, *client, *actualClient) } func TestSetEnableGzipCompression(t *testing.T) { @@ -2067,8 +2063,11 @@ func getTLSVersion(service *BaseService) int { var tlsVersion int = -1 client := service.GetHTTPClient() if client != nil { - tr := client.Transport.(*http.Transport) - tlsVersion = int(tr.TLSClientConfig.MinVersion) + if tr, ok := client.Transport.(*http.Transport); ok { + if tr.TLSClientConfig != nil { + tlsVersion = int(tr.TLSClientConfig.MinVersion) + } + } } return tlsVersion } @@ -2083,7 +2082,7 @@ func TestMinSSLVersion(t *testing.T) { assert.NotNil(t, service.Client) // Check the default config. - assert.Equal(t, getTLSVersion(service), tls.VersionTLS12) + assert.Equal(t, getTLSVersion(service), -1) // Set a insecureClient with different value. insecureClient := &http.Client{} @@ -2093,9 +2092,9 @@ func TestMinSSLVersion(t *testing.T) { }, } service.SetHTTPClient(insecureClient) - assert.Equal(t, getTLSVersion(service), tls.VersionTLS12) + assert.Equal(t, tls.VersionTLS10, getTLSVersion(service)) // Check retryable client config. service.EnableRetries(3, 30*time.Second) - assert.Equal(t, getTLSVersion(service), tls.VersionTLS12) + assert.Equal(t, tls.VersionTLS10, getTLSVersion(service)) }