Skip to content

Commit

Permalink
feat: improve behavior of http redirects
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
padamstx committed Nov 21, 2023
1 parent 0b8c2b2 commit 3c67ebd
Show file tree
Hide file tree
Showing 6 changed files with 240 additions and 27 deletions.
12 changes: 6 additions & 6 deletions .secrets.baseline
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -82,31 +82,31 @@
"hashed_secret": "91dfd9ddb4198affc5c194cd8ce6d338fde470e2",
"is_secret": false,
"is_verified": false,
"line_number": 81,
"line_number": 82,
"type": "Secret Keyword",
"verified_result": null
},
{
"hashed_secret": "e0d246cf37df7d1a561ed649d108dd14f36f28bf",
"is_secret": false,
"is_verified": false,
"line_number": 241,
"line_number": 242,
"type": "Secret Keyword",
"verified_result": null
},
{
"hashed_secret": "98635b2eaa2379f28cd6d72a38299f286b81b459",
"is_secret": false,
"is_verified": false,
"line_number": 548,
"line_number": 549,
"type": "Secret Keyword",
"verified_result": null
},
{
"hashed_secret": "47fcf185ee7e15fe05cae31fbe9e4ebe4a06a40d",
"is_secret": false,
"is_verified": false,
"line_number": 597,
"line_number": 692,
"type": "Secret Keyword",
"verified_result": null
}
Expand All @@ -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
}
Expand Down
7 changes: 7 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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} ./...
Expand Down
82 changes: 71 additions & 11 deletions core/base_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -298,15 +299,74 @@ 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)
return strings.HasSuffix(fromHost, ".cloud.ibm.com") && strings.HasSuffix(toHost, ".cloud.ibm.com")
}

// SetEnableGzipCompression sets the service's EnableGzipCompression field
Expand Down Expand Up @@ -693,7 +753,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
}

Expand Down Expand Up @@ -731,7 +791,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()
}

Expand Down
147 changes: 147 additions & 0 deletions core/base_service_redirect_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
Loading

0 comments on commit 3c67ebd

Please sign in to comment.