feat: improve behavior of http redirects

This commit modifies the Go core so that it will
include "safe" headers when performing a
cross-site redirect where both the original
and redirected hosts are within IBM's
"cloud.ibm.com" domain.

Signed-off-by: Phil Adams <[email protected]>

Author: padamstx

.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

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} ./...

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,83 @@ 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) {
+// setupHTTPClient will configure "client" for use with the BaseService object.
+func setupHTTPClient(client *http.Client) {
+	// Set the minimum TLS version to be 1.2
 	if tr, ok := client.Transport.(*http.Transport); tr != nil && ok {
 		if tr.TLSClientConfig == nil {
 			tr.TLSClientConfig = &tls.Config{} // #nosec G402
 		}
 
 		tr.TLSClientConfig.MinVersion = tls.VersionTLS12
 	}
+
+	// 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.")
+		}
+	} 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)
+	return strings.HasSuffix(fromHost, ".cloud.ibm.com") && strings.HasSuffix(toHost, ".cloud.ibm.com")
 }
 
 // SetEnableGzipCompression sets the service's EnableGzipCompression field
@@ -693,7 +762,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 +800,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()
 	}
 

core/base_service_redirect_test.go

@@ -0,0 +1,147 @@
+//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 := NewBasicAuthenticator("xxx", "yyy")
+	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 TestRedirectAuthSuccess(t *testing.T) {
+	testRedirection(t, "region1.cloud.ibm.com", "region2.cloud.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)
+}

core/base_service_test.go

@@ -1769,23 +1769,20 @@ 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.
 	service.EnableRetries(4, 90*time.Second)
 	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.
 	service.DisableRetries()
 	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) {