Skip to content

Commit 6368be6

Browse files
committed
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]>
1 parent 0b8c2b2 commit 6368be6

6 files changed

+250
-27
lines changed

.secrets.baseline

+6-6
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
"files": "go.sum|package-lock.json|^.secrets.baseline$",
44
"lines": null
55
},
6-
"generated_at": "2023-11-06T17:25:56Z",
6+
"generated_at": "2023-11-17T20:18:24Z",
77
"plugins_used": [
88
{
99
"name": "AWSKeyDetector"
@@ -82,31 +82,31 @@
8282
"hashed_secret": "91dfd9ddb4198affc5c194cd8ce6d338fde470e2",
8383
"is_secret": false,
8484
"is_verified": false,
85-
"line_number": 81,
85+
"line_number": 82,
8686
"type": "Secret Keyword",
8787
"verified_result": null
8888
},
8989
{
9090
"hashed_secret": "e0d246cf37df7d1a561ed649d108dd14f36f28bf",
9191
"is_secret": false,
9292
"is_verified": false,
93-
"line_number": 241,
93+
"line_number": 242,
9494
"type": "Secret Keyword",
9595
"verified_result": null
9696
},
9797
{
9898
"hashed_secret": "98635b2eaa2379f28cd6d72a38299f286b81b459",
9999
"is_secret": false,
100100
"is_verified": false,
101-
"line_number": 548,
101+
"line_number": 549,
102102
"type": "Secret Keyword",
103103
"verified_result": null
104104
},
105105
{
106106
"hashed_secret": "47fcf185ee7e15fe05cae31fbe9e4ebe4a06a40d",
107107
"is_secret": false,
108108
"is_verified": false,
109-
"line_number": 597,
109+
"line_number": 692,
110110
"type": "Secret Keyword",
111111
"verified_result": null
112112
}
@@ -116,7 +116,7 @@
116116
"hashed_secret": "bc2f74c22f98f7b6ffbc2f67453dbfa99bce9a32",
117117
"is_secret": false,
118118
"is_verified": false,
119-
"line_number": 744,
119+
"line_number": 751,
120120
"type": "Secret Keyword",
121121
"verified_result": null
122122
}

.travis.yml

+7
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,13 @@ go:
1010
notifications:
1111
email: false
1212

13+
addons:
14+
hosts:
15+
- region1.cloud.ibm.com
16+
- region1.notcloud.ibm.com
17+
- region2.cloud.ibm.com
18+
- region2.notcloud.ibm.com
19+
1320
env:
1421
global:
1522
- GO111MODULE=on

Makefile

+1-1
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ test:
1616

1717
lint:
1818
${LINT} run --build-tags=all
19-
DIFF=$$(${FORMATTER} -d core); if [[ -n "$$DIFF" ]]; then printf "\n$$DIFF" && exit 1; fi
19+
DIFF=$$(${FORMATTER} -d core); if [ -n "$$DIFF" ]; then printf "\n$$DIFF" && exit 1; fi
2020

2121
scan-gosec:
2222
${GOSEC} ./...

core/base_service.go

+73-11
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import (
3838
const (
3939
headerNameUserAgent = "User-Agent"
4040
sdkName = "ibm-go-sdk-core"
41+
maxRedirects = 10
4142
)
4243

4344
// ServiceOptions is a struct of configuration values for a service.
@@ -117,7 +118,7 @@ func (service *BaseService) Clone() *BaseService {
117118
// First, copy the service options struct.
118119
serviceOptions := *service.Options
119120

120-
// Next, make a copy the service struct, then use the copy of the service options.
121+
// Next, make a copy of the service struct, then use the copy of the service options.
121122
// Note, we'll re-use the "Client" instance from the original BaseService instance.
122123
clone := *service
123124
clone.Options = &serviceOptions
@@ -234,7 +235,7 @@ func (service *BaseService) SetDefaultHeaders(headers http.Header) {
234235
// the retryable client; otherwise "client" will be stored
235236
// directly on "service".
236237
func (service *BaseService) SetHTTPClient(client *http.Client) {
237-
setMinimumTLSVersion(client)
238+
setupHTTPClient(client)
238239

239240
if isRetryableClient(service.Client) {
240241
// If "service" is currently holding a retryable client,
@@ -298,15 +299,76 @@ func (service *BaseService) IsSSLDisabled() bool {
298299
return false
299300
}
300301

301-
// setMinimumTLSVersion sets the minimum TLS version required by the client to TLS v1.2
302-
func setMinimumTLSVersion(client *http.Client) {
303-
if tr, ok := client.Transport.(*http.Transport); tr != nil && ok {
304-
if tr.TLSClientConfig == nil {
305-
tr.TLSClientConfig = &tls.Config{} // #nosec G402
302+
// setupHTTPClient will configure "client" for use with the BaseService object.
303+
func setupHTTPClient(client *http.Client) {
304+
// Set our "CheckRedirect" function to allow safe headers to be included
305+
// in redirected requests under certain conditions.
306+
if client.CheckRedirect == nil {
307+
client.CheckRedirect = checkRedirect
308+
}
309+
}
310+
311+
// checkRedirect is used as an override for the default "CheckRedirect" function supplied
312+
// by the net/http package and implements some additional logic required by IBM SDKs.
313+
func checkRedirect(req *http.Request, via []*http.Request) error {
314+
315+
// The net/http module is implemented such that it will only include "safe" headers
316+
// ("Authorization", "WWW-Authenticate", "Cookie", "Cookie2") when redirecting a request
317+
// if the redirected host is the same host or a sub-domain of the original request's host.
318+
// Example: foo.com redirected to foo.com or bar.foo.com would work, but bar.com would not.
319+
// This "CheckRedirect" implementation will propagate "safe" headers in a redirected request
320+
// only in situations where the hosts associated with the original and redirected request URLs
321+
// are both located within the ".cloud.ibm.com" domain.
322+
323+
// First, perform the check that is done by the default CheckRedirect function
324+
// to ensure we don't exhaust our max redirect limit.
325+
if len(via) >= maxRedirects {
326+
GetLogger().Debug("Exceeded max redirects: %d", maxRedirects)
327+
return fmt.Errorf("stopped after %d redirects", maxRedirects)
328+
}
329+
330+
if len(via) > 0 {
331+
GetLogger().Debug("Detected %d prior request(s)", len(via))
332+
originalReq := via[0]
333+
redirectedReq := req
334+
GetLogger().Debug("Redirecting request from %s to %s", originalReq.URL.String(), redirectedReq.URL.String())
335+
redirectedHeader := req.Header
336+
originalHeader := via[0].Header
337+
338+
originalHost := originalReq.URL.Hostname()
339+
redirectedHost := redirectedReq.URL.Hostname()
340+
341+
if shouldCopySafeHeadersOnRedirect(originalHost, redirectedHost) {
342+
343+
// We're only concerned with "safe" headers since these are the ones that are not
344+
// propagated automatically by net/http for a "cross-site" redirect.
345+
for _, headerKey := range []string{"Authorization", "WWW-Authenticate", "Cookie", "Cookie2"} {
346+
// If the original request contains a value for "headerKey"
347+
// *and* this header is not already present in the redirected request,
348+
// then copy the value from the original request to the redirected request.
349+
if v, inOriginalRequest := originalHeader[headerKey]; inOriginalRequest {
350+
if _, inRedirectedRequest := redirectedHeader[headerKey]; !inRedirectedRequest {
351+
redirectedHeader[headerKey] = v
352+
GetLogger().Debug("Propagating header '%s' in redirected request", headerKey)
353+
}
354+
}
355+
}
356+
} else {
357+
GetLogger().Debug("Redirected request is not within the trusted domain.")
306358
}
307-
308-
tr.TLSClientConfig.MinVersion = tls.VersionTLS12
359+
} else {
360+
GetLogger().Debug("Detected no prior requests!")
309361
}
362+
return nil
363+
}
364+
365+
// shouldCopySafeHeadersOnRedirect returns true iff safe headers should be copied
366+
// to a redirected request.
367+
func shouldCopySafeHeadersOnRedirect(fromHost, toHost string) bool {
368+
GetLogger().Debug("hosts: %s %s", fromHost, toHost)
369+
sameHost := fromHost == toHost
370+
safeDomain := strings.HasSuffix(fromHost, ".cloud.ibm.com") && strings.HasSuffix(toHost, ".cloud.ibm.com")
371+
return sameHost || safeDomain
310372
}
311373

312374
// SetEnableGzipCompression sets the service's EnableGzipCompression field
@@ -693,7 +755,7 @@ func (service *BaseService) DisableRetries() {
693755
// DefaultHTTPClient returns a non-retryable http client with default configuration.
694756
func DefaultHTTPClient() *http.Client {
695757
client := cleanhttp.DefaultPooledClient()
696-
setMinimumTLSVersion(client)
758+
setupHTTPClient(client)
697759
return client
698760
}
699761

@@ -731,7 +793,7 @@ func NewRetryableClientWithHTTPClient(httpClient *http.Client) *retryablehttp.Cl
731793
// as our embedded client used to invoke individual requests.
732794
client.HTTPClient = httpClient
733795
} else {
734-
// Otherwise, we'll use construct a default HTTP client and use that
796+
// Otherwise, we'll construct a default HTTP client and use that
735797
client.HTTPClient = DefaultHTTPClient()
736798
}
737799

core/base_service_redirect_test.go

+155
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
//go:build all || slow || auth
2+
// +build all slow auth
3+
4+
package core
5+
6+
// (C) Copyright IBM Corp. 2023.
7+
//
8+
// Licensed under the Apache License, Version 2.0 (the "License");
9+
// you may not use this file except in compliance with the License.
10+
// You may obtain a copy of the License at
11+
//
12+
// http://www.apache.org/licenses/LICENSE-2.0
13+
//
14+
// Unless required by applicable law or agreed to in writing, software
15+
// distributed under the License is distributed on an "AS IS" BASIS,
16+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
17+
// See the License for the specific language governing permissions and
18+
// limitations under the License.
19+
20+
import (
21+
"fmt"
22+
"net/http"
23+
"net/http/httptest"
24+
"strings"
25+
"testing"
26+
27+
"github.com/stretchr/testify/assert"
28+
)
29+
30+
// Note: this unit test depends on some bogus hostnames being defined in /etc/hosts.
31+
// Append this to your /etc/hosts file:
32+
// # for testing
33+
// 127.0.0.1 region1.cloud.ibm.com region2.cloud.ibm.com region1.notcloud.ibm.com region2.notcloud.ibm.com
34+
35+
var (
36+
operationPath string = "/api/redirector"
37+
38+
// To enable debug mode while running these tests, set this to LevelDebug.
39+
redirectTestLogLevel LogLevel = LevelError
40+
)
41+
42+
// Start a mock server that will redirect requests to the second mock server
43+
// located at "redirectServerURL"
44+
func startMockServer1(t *testing.T, redirectServerURL string) *httptest.Server {
45+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
46+
t.Logf(`server1 received request: %s %s`, req.Method, req.URL.String())
47+
48+
// Make sure the Authorization header was sent.
49+
assert.NotEmpty(t, req.Header.Get("Authorization"))
50+
51+
path := req.URL.Path
52+
location := redirectServerURL + path
53+
54+
// Create the response (a 302 redirect).
55+
w.Header().Add("Location", location)
56+
w.WriteHeader(http.StatusFound)
57+
t.Logf(`Sent redirect request to: %s`, location)
58+
}))
59+
return server
60+
}
61+
62+
// Start a second mock server to which redirected requests will be sent.
63+
func startMockServer2(t *testing.T) *httptest.Server {
64+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
65+
t.Logf(`server2 received request: %s %s`, req.Method, req.URL.String())
66+
67+
// Create the response.
68+
if req.Header.Get("Authorization") != "" {
69+
w.Header().Set("Content-Type", "application/json")
70+
w.WriteHeader(http.StatusOK)
71+
fmt.Fprintf(w, `{"name":"Jason Bourne"}`)
72+
} else {
73+
w.WriteHeader(http.StatusUnauthorized)
74+
}
75+
}))
76+
return server
77+
}
78+
79+
func startServers(t *testing.T, host1 string, host2 string) (server1 *httptest.Server, server1URL string,
80+
server2 *httptest.Server, server2URL string) {
81+
server2 = startMockServer2(t)
82+
server2URL = strings.ReplaceAll(server2.URL, "127.0.0.1", host2)
83+
t.Logf(`Server 2 listening on endpoint: %s (%s)`, server2URL, server2.URL)
84+
85+
server1 = startMockServer1(t, server2URL)
86+
server1URL = strings.ReplaceAll(server1.URL, "127.0.0.1", host1)
87+
t.Logf(`Server 1 listening on endpoint: %s (%s)`, server1URL, server1.URL)
88+
89+
return
90+
}
91+
92+
func testRedirection(t *testing.T, host1 string, host2 string, expectedStatusCode int) {
93+
GetLogger().SetLogLevel(redirectTestLogLevel)
94+
95+
// Both servers within trusted domain.
96+
server1, server1URL, server2, _ := startServers(t, host1, host2)
97+
defer server1.Close()
98+
defer server2.Close()
99+
100+
builder := NewRequestBuilder("GET")
101+
_, err := builder.ResolveRequestURL(server1URL, operationPath, nil)
102+
assert.Nil(t, err)
103+
req, _ := builder.Build()
104+
105+
authenticator, err := NewBearerTokenAuthenticator("this is not a secret")
106+
assert.Nil(t, err)
107+
assert.NotNil(t, authenticator)
108+
109+
options := &ServiceOptions{
110+
URL: server1.URL,
111+
Authenticator: authenticator,
112+
}
113+
service, err := NewBaseService(options)
114+
assert.Nil(t, err)
115+
116+
var foo *Foo
117+
detailedResponse, err := service.Request(req, &foo)
118+
assert.NotNil(t, detailedResponse)
119+
assert.Equal(t, expectedStatusCode, detailedResponse.StatusCode)
120+
if expectedStatusCode >= 200 && expectedStatusCode <= 299 {
121+
assert.Nil(t, err)
122+
123+
result, ok := detailedResponse.Result.(*Foo)
124+
assert.Equal(t, true, ok)
125+
assert.NotNil(t, result)
126+
assert.NotNil(t, foo)
127+
assert.Equal(t, "Jason Bourne", *result.Name)
128+
} else {
129+
assert.NotNil(t, err)
130+
}
131+
}
132+
133+
func TestRedirectAuthSuccess1(t *testing.T) {
134+
testRedirection(t, "region1.cloud.ibm.com", "region2.cloud.ibm.com", http.StatusOK)
135+
}
136+
137+
func TestRedirectAuthSuccess2(t *testing.T) {
138+
testRedirection(t, "region1.cloud.ibm.com", "region1.cloud.ibm.com", http.StatusOK)
139+
}
140+
141+
func TestRedirectAuthSuccess3(t *testing.T) {
142+
testRedirection(t, "region1.notcloud.ibm.com", "region1.notcloud.ibm.com", http.StatusOK)
143+
}
144+
145+
func TestRedirectAuthFail1(t *testing.T) {
146+
testRedirection(t, "region1.notcloud.ibm.com", "region2.cloud.ibm.com", http.StatusUnauthorized)
147+
}
148+
149+
func TestRedirectAuthFail2(t *testing.T) {
150+
testRedirection(t, "region1.cloud.ibm.com", "region2.notcloud.ibm.com", http.StatusUnauthorized)
151+
}
152+
153+
func TestRedirectAuthFail3(t *testing.T) {
154+
testRedirection(t, "region1.notcloud.ibm.com", "region2.notcloud.ibm.com", http.StatusUnauthorized)
155+
}

0 commit comments

Comments
 (0)