From 59ea591bcb34f443d248d68b8991a7a96dfd29c4 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Wed, 9 Sep 2026 11:25:16 +0100 Subject: [PATCH] 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 cae10713e8e400ce6131a4970445ff8573a23bdf) --- backend/http/http.go | 38 ++------ backend/s3/s3.go | 42 ++------- backend/s3/s3_test.go | 48 ---------- lib/rest/rest.go | 54 +++++++++++ lib/rest/rest_test.go | 204 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 275 insertions(+), 111 deletions(-) diff --git a/backend/http/http.go b/backend/http/http.go index f3ec78105..05dae780d 100644 --- a/backend/http/http.go +++ b/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) diff --git a/backend/s3/s3.go b/backend/s3/s3.go index 389ff7274..e4734c63a 100644 --- a/backend/s3/s3.go +++ b/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. // diff --git a/backend/s3/s3_test.go b/backend/s3/s3_test.go index 9c6f83509..37b905988 100644 --- a/backend/s3/s3_test.go +++ b/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) diff --git a/lib/rest/rest.go b/lib/rest/rest.go index da7b2b534..ecb35047f 100644 --- a/lib/rest/rest.go +++ b/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) diff --git a/lib/rest/rest_test.go b/lib/rest/rest_test.go index f95016d81..cb689388b 100644 --- a/lib/rest/rest_test.go +++ b/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) + }) +}