rest: add StripHeadersOnCrossHostRedirectFn and use it in the s3 and http backends

The s3 and http backends each carried their own copy of the same
CheckRedirect policy. This moves it into lib/rest as
StripHeadersOnCrossHostRedirectFn, so that any backend which sends
credentials in headers to a host it does not control can use the same
policy. It logs at debug level when it strips headers so that a
resulting 401 from the redirect target can be diagnosed. A header
starting with "*" is matched without canonicalising, as that is how
SetHeader and Opts.ExtraHeaders send it.

This changes the s3 policy in two edge cases:

- It compared the raw URL host string, so a redirect to the same host
  in different case or with an explicit default port stripped the
  credentials. It now uses SameHost like the http backend, which treats
  those as the same host.
- The downgrade check compared each hop with the previous one. It now
  compares with the original request like the other backends, so a
  chain which starts on plaintext HTTP may return to it.

The behaviour tests move to lib/rest. The backend tests which check the
policy is wired into each client are kept.

(cherry picked from commit cae10713e8)

backend/http/http.go

@@ -526,40 +526,20 @@ func addHeaders(req *http.Request, opt *Options) {
 // rest.ErrHTTPSDowngrade and strips the configured headers when the
 // redirect chain has left the originally requested host at any point.
 func checkRedirect(opt *Options) func(req *http.Request, via []*http.Request) error {
+	headers := make([]string, 0, len(opt.Headers)/2)
+	for i := 0; i < len(opt.Headers); i += 2 {
+		headers = append(headers, opt.Headers[i])
+	}
+	stripHeaders := rest.StripHeadersOnCrossHostRedirectFn(headers...)
 	return func(req *http.Request, via []*http.Request) error {
-		if err := rest.RefuseHTTPSDowngradeRedirectFn(req, via); err != nil {
-			if errors.Is(err, rest.ErrHTTPSDowngrade) {
-				err = fmt.Errorf("%w (the configured headers would be sent to the plaintext target)", err)
-			}
-			return err
+		err := stripHeaders(req, via)
+		if errors.Is(err, rest.ErrHTTPSDowngrade) {
+			err = fmt.Errorf("%w (the configured headers would be sent to the plaintext target)", err)
 		}
-		if redirectLeavesHost(req, via) {
-			for i := 0; i < len(opt.Headers); i += 2 {
-				req.Header.Del(opt.Headers[i])
-			}
-		}
-		return nil
+		return err
 	}
 }
 
-// redirectLeavesHost reports whether any hop in the redirect chain
-// via plus the pending request req is to a different host from the
-// original request via[0].
-//
-// net/http copies the headers afresh from the original request for
-// every hop, so once the chain has visited another host the headers
-// must be stripped from every subsequent hop, even one back to the
-// original host, as the other host chose the URL.
-func redirectLeavesHost(req *http.Request, via []*http.Request) bool {
-	origin := via[0].URL
-	for _, hop := range via {
-		if !rest.SameHost(hop.URL, origin) {
-			return true
-		}
-	}
-	return !rest.SameHost(req.URL, origin)
-}
-
 // Adds the configured headers to the request if any
 func (f *Fs) addHeaders(req *http.Request) {
 	addHeaders(req, &f.opt)

backend/s3/s3.go

@@ -1357,10 +1357,10 @@ func getClient(ctx context.Context, opt *Options) *http.Client {
 }
 
 // s3RedirectSecretHeaders are the request headers carrying origin-bound
-// secrets that must not be forwarded when a redirect crosses a host or
-// downgrades the scheme. Go strips Authorization on a hostname change but not
-// on a scheme downgrade, and has no knowledge that the SSE-C headers hold raw
-// encryption keys, so we strip them all ourselves.
+// secrets that must not be forwarded when a redirect leaves the original
+// host. Go strips Authorization on a hostname change but keeps it for a
+// subdomain or a different port, and has no knowledge that the SSE-C
+// headers hold raw encryption keys, so we strip them all ourselves.
 var s3RedirectSecretHeaders = []string{
 	"X-Amz-Security-Token",  // AWS STS session token
 	"X-Amz-S3session-Token", // S3 Express (directory bucket) session token
@@ -1375,36 +1375,10 @@ var s3RedirectSecretHeaders = []string{
 	"Referer", // may be a presigned request
 }
 
-func s3CheckRedirect(req *http.Request, via []*http.Request) error {
-	if len(via) >= 10 {
-		return errors.New("stopped after 10 redirects")
-	}
-	// Never follow a redirect that downgrades the transport from HTTPS to
-	// HTTP. An S3 endpoint has no legitimate reason to do this, and replaying
-	// the request over plaintext would expose whatever credential it carries.
-	if via[len(via)-1].URL.Scheme == "https" && req.URL.Scheme == "http" {
-		return fmt.Errorf("refusing to follow insecure redirect from HTTPS to HTTP: %s", req.URL.Redacted())
-	}
-	if s3RedirectCrossesHost(req, via) {
-		for _, header := range s3RedirectSecretHeaders {
-			req.Header.Del(header)
-		}
-	}
-	return nil
-}
-
-func s3RedirectCrossesHost(req *http.Request, via []*http.Request) bool {
-	if len(via) == 0 {
-		return false
-	}
-	scheme, host := via[0].URL.Scheme, via[0].URL.Host
-	for _, redirect := range via[1:] {
-		if redirect.URL.Host != host || redirect.URL.Scheme != scheme {
-			return true
-		}
-	}
-	return host != req.URL.Host || scheme != req.URL.Scheme
-}
+// s3CheckRedirect is the http.Client CheckRedirect policy for S3. It
+// refuses HTTPS to HTTP downgrades and strips s3RedirectSecretHeaders
+// once a redirect has left the original host.
+var s3CheckRedirect = rest.StripHeadersOnCrossHostRedirectFn(s3RedirectSecretHeaders...)
 
 // Fixup the request if needed.
 //

backend/s3/s3_test.go

@@ -228,54 +228,6 @@ func TestS3CheckRedirectRejectsSchemeDowngrade(t *testing.T) {
 	}
 }
 
-func TestRedirectCrossesHost(t *testing.T) {
-	mustReq := func(method, url string) *http.Request {
-		req, err := http.NewRequest(method, url, nil)
-		require.NoError(t, err)
-		return req
-	}
-	for _, test := range []struct {
-		name string
-		via  []string
-		req  string
-		want bool
-	}{
-		{
-			name: "SameHost",
-			via:  []string{"https://bucket.example.com/"},
-			req:  "https://bucket.example.com/redirected",
-			want: false,
-		},
-		{
-			name: "DifferentHost",
-			via:  []string{"https://bucket.example.com/"},
-			req:  "https://evil.example.com/redirected",
-			want: true,
-		},
-		{
-			name: "SchemeDowngradeSameHost",
-			via:  []string{"https://bucket.example.com/"},
-			req:  "http://bucket.example.com/redirected",
-			want: true,
-		},
-		{
-			name: "SchemeDowngradeMidChain",
-			via:  []string{"https://bucket.example.com/", "http://bucket.example.com/middle"},
-			req:  "http://bucket.example.com/final",
-			want: true,
-		},
-	} {
-		t.Run(test.name, func(t *testing.T) {
-			via := make([]*http.Request, len(test.via))
-			for i, url := range test.via {
-				via[i] = mustReq(http.MethodGet, url)
-			}
-			got := s3RedirectCrossesHost(mustReq(http.MethodGet, test.req), via)
-			assert.Equal(t, test.want, got)
-		})
-	}
-}
-
 func TestClientStopsAfterTenRedirects(t *testing.T) {
 	_, _, client := SetupS3Test(t)
 

lib/rest/rest.go

@@ -307,6 +307,60 @@ func RefuseHTTPSDowngradeRedirectFn(req *http.Request, via []*http.Request) erro
 	return nil
 }
 
+// StripHeadersOnCrossHostRedirectFn returns a CheckRedirect function
+// which follows redirects like the default net/http client but refuses
+// an HTTPS to HTTP downgrade with ErrHTTPSDowngrade and removes the
+// named headers from the request once the redirect chain has left the
+// host of the original request.
+//
+// Start a header with "*" to match it without canonicalising, as
+// SetHeader and Opts.ExtraHeaders do.
+//
+// Go strips Authorization on a hostname change but keeps it for a
+// subdomain or a different port, and has no idea which custom headers
+// carry secrets, so a backend which sends credentials in headers to a
+// host it does not control uses this to keep them at home.
+func StripHeadersOnCrossHostRedirectFn(headers ...string) func(req *http.Request, via []*http.Request) error {
+	return func(req *http.Request, via []*http.Request) error {
+		err := RefuseHTTPSDowngradeRedirectFn(req, via)
+		if err != nil {
+			return err
+		}
+		if redirectLeavesHost(req, via) {
+			fs.Debugf(nil, "redirect to %s leaves %s: not sending %s", req.URL.Redacted(), via[0].URL.Redacted(), strings.Join(headers, ", "))
+			for _, header := range headers {
+				if strings.HasPrefix(header, "*") {
+					delete(req.Header, header[1:])
+				} else {
+					req.Header.Del(header)
+				}
+			}
+		}
+		return nil
+	}
+}
+
+// redirectLeavesHost reports whether any hop in the redirect chain
+// via plus the pending request req is to a different host from the
+// original request via[0].
+//
+// net/http copies the headers afresh from the original request for
+// every hop, so once the chain has visited another host the headers
+// must be stripped from every subsequent hop, even one back to the
+// original host, as the other host chose the URL.
+func redirectLeavesHost(req *http.Request, via []*http.Request) bool {
+	if len(via) == 0 {
+		return false
+	}
+	origin := via[0].URL
+	for _, hop := range via[1:] {
+		if !SameHost(hop.URL, origin) {
+			return true
+		}
+	}
+	return !SameHost(req.URL, origin)
+}
+
 // Do calls the internal http.Client.Do method
 func (api *Client) Do(req *http.Request) (*http.Response, error) {
 	return api.c.Do(req)

lib/rest/rest_test.go

@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"strings"
 	"sync/atomic"
 	"testing"
 
@@ -230,3 +231,206 @@ func TestReadBodyLimit(t *testing.T) {
 		assert.Less(t, written.Load(), int64(maxChunks*chunk), "client should stop reading before the server stops sending")
 	})
 }
+
+func TestRedirectLeavesHost(t *testing.T) {
+	for _, test := range []struct {
+		name string
+		via  []string
+		req  string
+		want bool
+	}{
+		{"NoVia", nil, "https://example.com/a", false},
+		{"SameHost", []string{"https://example.com/"}, "https://example.com/b", false},
+		{"SameHostDifferentCase", []string{"https://example.com/"}, "https://EXAMPLE.com/b", false},
+		{"SameHostDefaultPort", []string{"https://example.com/"}, "https://example.com:443/b", false},
+		// An upgrade changes the default port so counts as a new host
+		{"SameHostUpgrade", []string{"http://example.com/"}, "https://example.com/b", true},
+		{"DifferentHost", []string{"https://example.com/"}, "https://other.example.com/b", true},
+		{"DifferentPort", []string{"https://example.com/"}, "https://example.com:8443/b", true},
+		{"BackToOrigin", []string{"https://example.com/", "https://other.example.com/b"}, "https://example.com/c", true},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			via := make([]*http.Request, len(test.via))
+			for i, rawURL := range test.via {
+				via[i] = mkRedirectReq(t, rawURL, "GET")
+			}
+			assert.Equal(t, test.want, redirectLeavesHost(mkRedirectReq(t, test.req, "GET"), via))
+		})
+	}
+}
+
+// stripTestHeaders are the headers StripHeadersOnCrossHostRedirectFn
+// is asked to strip in the tests, with the value each is sent with.
+//
+// The one starting with "*" is sent without canonicalising.
+var stripTestHeaders = map[string]string{
+	"Authorization":   "secret-token",
+	"X-Custom-Secret": "secret-custom",
+	"*x-raw-secret":   "secret-raw",
+}
+
+// stripTestRequest makes a GET request to rawURL carrying the headers
+// to be stripped plus a harmless marker header.
+func stripTestRequest(t *testing.T, rawURL string) *http.Request {
+	req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, rawURL, nil)
+	require.NoError(t, err)
+	for header, value := range stripTestHeaders {
+		if raw, ok := strings.CutPrefix(header, "*"); ok {
+			req.Header[raw] = []string{value}
+		} else {
+			req.Header.Set(header, value)
+		}
+	}
+	req.Header.Set("X-Marker", "marker")
+	return req
+}
+
+// stripTestClient makes an http.Client using StripHeadersOnCrossHostRedirectFn
+func stripTestClient() *http.Client {
+	headers := make([]string, 0, len(stripTestHeaders))
+	for header := range stripTestHeaders {
+		headers = append(headers, header)
+	}
+	return &http.Client{CheckRedirect: StripHeadersOnCrossHostRedirectFn(headers...)}
+}
+
+func TestStripHeadersOnCrossHostRedirectFn(t *testing.T) {
+	assertStripped := func(r *http.Request) {
+		for header := range stripTestHeaders {
+			assert.Empty(t, r.Header.Get(strings.TrimPrefix(header, "*")), "%s should have been stripped", header)
+		}
+		assert.Equal(t, "marker", r.Header.Get("X-Marker"))
+	}
+	assertKept := func(r *http.Request) {
+		for header, value := range stripTestHeaders {
+			assert.Equal(t, value, r.Header.Get(strings.TrimPrefix(header, "*")), "%s should have been kept", header)
+		}
+		assert.Equal(t, "marker", r.Header.Get("X-Marker"))
+	}
+
+	t.Run("CrossHost", func(t *testing.T) {
+		// httptest servers listen on 127.0.0.1:port so two servers
+		// count as different hosts.
+		target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			assertStripped(r)
+			w.WriteHeader(http.StatusOK)
+		}))
+		defer target.Close()
+		redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			http.Redirect(w, r, target.URL, http.StatusTemporaryRedirect)
+		}))
+		defer redirector.Close()
+
+		resp, err := stripTestClient().Do(stripTestRequest(t, redirector.URL))
+		require.NoError(t, err)
+		assert.Equal(t, http.StatusOK, resp.StatusCode)
+		assert.NoError(t, resp.Body.Close())
+	})
+
+	t.Run("BackToOrigin", func(t *testing.T) {
+		// The origin redirects to another host which redirects back
+		// to the origin. The headers must stay stripped on the way
+		// back as the other host chose the URL.
+		var origin *httptest.Server
+		bouncer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			assertStripped(r)
+			http.Redirect(w, r, origin.URL+"/final", http.StatusTemporaryRedirect)
+		}))
+		defer bouncer.Close()
+		origin = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			switch r.URL.Path {
+			case "/":
+				assertKept(r)
+				http.Redirect(w, r, bouncer.URL, http.StatusTemporaryRedirect)
+			case "/final":
+				assertStripped(r)
+				w.WriteHeader(http.StatusOK)
+			default:
+				http.NotFound(w, r)
+			}
+		}))
+		defer origin.Close()
+
+		resp, err := stripTestClient().Do(stripTestRequest(t, origin.URL))
+		require.NoError(t, err)
+		assert.Equal(t, http.StatusOK, resp.StatusCode)
+		assert.NoError(t, resp.Body.Close())
+	})
+
+	t.Run("SameHost", func(t *testing.T) {
+		server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			switch r.URL.Path {
+			case "/":
+				http.Redirect(w, r, "/redirected", http.StatusTemporaryRedirect)
+			case "/redirected":
+				assertKept(r)
+				w.WriteHeader(http.StatusOK)
+			default:
+				http.NotFound(w, r)
+			}
+		}))
+		defer server.Close()
+
+		resp, err := stripTestClient().Do(stripTestRequest(t, server.URL))
+		require.NoError(t, err)
+		assert.Equal(t, http.StatusOK, resp.StatusCode)
+		assert.NoError(t, resp.Body.Close())
+	})
+
+	t.Run("SameHostDifferentCase", func(t *testing.T) {
+		var server *httptest.Server
+		server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			switch r.URL.Path {
+			case "/":
+				u, err := url.Parse(server.URL)
+				require.NoError(t, err)
+				http.Redirect(w, r, "http://LOCALHOST:"+u.Port()+"/redirected", http.StatusTemporaryRedirect)
+			case "/redirected":
+				assertKept(r)
+				w.WriteHeader(http.StatusOK)
+			default:
+				http.NotFound(w, r)
+			}
+		}))
+		defer server.Close()
+
+		u, err := url.Parse(server.URL)
+		require.NoError(t, err)
+		resp, err := stripTestClient().Do(stripTestRequest(t, "http://localhost:"+u.Port()+"/"))
+		require.NoError(t, err)
+		assert.Equal(t, http.StatusOK, resp.StatusCode)
+		assert.NoError(t, resp.Body.Close())
+	})
+
+	t.Run("RefusesDowngrade", func(t *testing.T) {
+		plain := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			assert.Fail(t, "plaintext server should not have been contacted")
+		}))
+		defer plain.Close()
+		tlsServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			http.Redirect(w, r, plain.URL, http.StatusTemporaryRedirect)
+		}))
+		defer tlsServer.Close()
+
+		client := stripTestClient()
+		client.Transport = tlsServer.Client().Transport
+		// On a CheckRedirect error the client returns the redirect
+		// response, with its body already closed, alongside the error.
+		resp, err := client.Do(stripTestRequest(t, tlsServer.URL)) //nolint:bodyclose // closed by the client
+		require.ErrorIs(t, err, ErrHTTPSDowngrade)
+		require.NotNil(t, resp)
+		assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
+	})
+
+	t.Run("StopsAfterTenRedirects", func(t *testing.T) {
+		server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			http.Redirect(w, r, r.URL.String(), http.StatusTemporaryRedirect)
+		}))
+		defer server.Close()
+
+		resp, err := stripTestClient().Do(stripTestRequest(t, server.URL)) //nolint:bodyclose // closed by the client
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "stopped after 10 redirects")
+		require.NotNil(t, resp)
+	})
+}