feat(s3): make Accept-Encoding signing configurable

s3/s3_service.go

@@ -7,7 +7,9 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	"strconv"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
@@ -33,6 +35,11 @@ type service struct {
 const (
 	VirtualHostedStyle = "VIRTUAL_HOSTED_STYLE"
 
+	// AWSSignAcceptEncoding controls whether `Accept-Encoding` is included in the
+	// SigV4 SignedHeaders set. Set it to "false" for an endpoint that is reached
+	// through a proxy which alters the header in transit.
+	AWSSignAcceptEncoding = "AWS_SIGN_ACCEPT_ENCODING"
+
 	// AWSRetryMaxAttempts is the default maximum number of retry attempts for a single API operation that fails with a retryable error.
 	AWSRetryMaxAttempts = 5
 	// AWSRetryMaximumAttempts maximum number attempts that should be made.
@@ -52,6 +59,51 @@ const (
 	maxSinglePutObjectSize int64 = 5 * 1024 * 1024 * 1024
 )
 
+// warnInvalidSignAcceptEncoding keeps the warning for a malformed
+// AWS_SIGN_ACCEPT_ENCODING value out of the per-request path.
+var warnInvalidSignAcceptEncoding sync.Once
+
+// ignoreAcceptEncodingSigning reports whether `Accept-Encoding` must be excluded
+// from the SigV4 SignedHeaders set for the given endpoint.
+//
+// aws-sdk-go-v2 sends `Accept-Encoding: identity` and, unlike v1, includes
+// `accept-encoding` in SignedHeaders. Anything that alters the header between
+// the client and the endpoint therefore breaks signature verification at the
+// endpoint with SignatureDoesNotMatch.
+// (https://github.com/aws/aws-sdk-go-v2/issues/1816 and https://github.com/rclone/rclone/issues/6670)
+//
+// Google Cloud Storage always alters the header (it appends gzip(gfe)), so it is
+// detected by endpoint and never depends on the user setting. A reverse proxy or
+// a CDN in front of an S3-compatible endpoint does the same thing (Cloudflare
+// replaces the value with "gzip, br" by design) but cannot be detected from the
+// endpoint. AWS_SIGN_ACCEPT_ENCODING=false lets the user exclude the header for
+// those endpoints. It defaults to true, which keeps the existing behavior.
+func ignoreAcceptEncodingSigning(endpoints string) bool {
+	if strings.Contains(endpoints, "storage.googleapis.com") {
+		return true
+	}
+
+	// The value comes from a Secret, which commonly carries a trailing newline.
+	value := strings.TrimSpace(os.Getenv(AWSSignAcceptEncoding))
+	if value == "" {
+		return false
+	}
+
+	sign, err := strconv.ParseBool(value)
+	if err != nil {
+		// Warn once rather than per request, because a new client is built for
+		// every S3 operation. Without this the user sees the same
+		// SignatureDoesNotMatch failure the setting is meant to fix, with no
+		// indication that the value was rejected.
+		warnInvalidSignAcceptEncoding.Do(func() {
+			log.Warnf("Invalid %v value %q, expecting a boolean. Keeping Accept-Encoding in the request signature.",
+				AWSSignAcceptEncoding, value)
+		})
+		return false
+	}
+	return !sign
+}
+
 func newService(u *url.URL) (*service, error) {
 	s := service{}
 	if u.User != nil {
@@ -118,12 +170,10 @@ func (s *service) newInstance(ctx context.Context, retryBackoff bool) (*s3.Clien
 				so.MaxBackoff = AWSRetryMaximumBackoff
 			})
 		}
-		// Google Cloud Storage alters the `Accept-Encoding` header (GCS might changes the header on its way to GCS by appending gzip(gfe) as accepted encoding),
-		// which causing signature mismatches and breaks the v2 request signature verification.
-		// (https://github.com/aws/aws-sdk-go-v2/issues/1816 and https://github.com/rclone/rclone/issues/6670)
-		// `Accept-Encoding` is added as one of the SignedHeaders in v2 but it is not used in v1.
-		// Remove `Accept-Encoding` from SignedHeaders is added as a workaround to make the v2 signature compatible with GCS.
-		if strings.Contains(endpoints, "storage.googleapis.com") {
+		// Remove `Accept-Encoding` from SignedHeaders for endpoints that alter it in
+		// transit. ignoreSigningHeaders restores the header after signing, so the
+		// request on the wire is unchanged.
+		if ignoreAcceptEncodingSigning(endpoints) {
 			ignoreSigningHeaders(o, []string{"Accept-Encoding"})
 		}
 	}), nil

s3/s3_service_unit_test.go

@@ -19,6 +19,23 @@ type recordedRequest struct {
 	method string
 	path   string
 	query  string
+	// authorization is the raw SigV4 Authorization header, which carries the
+	// SignedHeaders list the request was signed over.
+	authorization string
+	// acceptEncoding is the `Accept-Encoding` value as it arrived on the wire.
+	acceptEncoding string
+}
+
+// signedHeaders returns the SignedHeaders list from the SigV4 Authorization
+// header, e.g. "host;x-amz-content-sha256;x-amz-date".
+func (r recordedRequest) signedHeaders() string {
+	for _, part := range strings.Split(r.authorization, " ") {
+		part = strings.TrimSuffix(strings.TrimSpace(part), ",")
+		if after, ok := strings.CutPrefix(part, "SignedHeaders="); ok {
+			return after
+		}
+	}
+	return ""
 }
 
 // fakeS3Server fakes just enough of the S3 API (PutObject,
@@ -49,9 +66,11 @@ func (f *fakeS3Server) handle(w http.ResponseWriter, r *http.Request) {
 
 	f.mu.Lock()
 	f.requests = append(f.requests, recordedRequest{
-		method: r.Method,
-		path:   r.URL.Path,
-		query:  r.URL.RawQuery,
+		method:         r.Method,
+		path:           r.URL.Path,
+		query:          r.URL.RawQuery,
+		authorization:  r.Header.Get("Authorization"),
+		acceptEncoding: r.Header.Get("Accept-Encoding"),
 	})
 	f.mu.Unlock()
 

s3/s3_sign_accept_encoding_test.go

@@ -0,0 +1,124 @@
+package s3
+
+import (
+	"bytes"
+	"context"
+	"os"
+	"strings"
+	"testing"
+)
+
+// TestIgnoreAcceptEncodingSigning covers the decision itself, including the
+// Google Cloud Storage endpoint, which cannot be exercised through the fake
+// server below because that server is reached by its own address.
+func TestIgnoreAcceptEncodingSigning(t *testing.T) {
+	cases := []struct {
+		name      string
+		endpoints string
+		env       string
+		want      bool
+	}{
+		{"empty, custom endpoint", "https://s3.example.com", "", false},
+		{"true, custom endpoint", "https://s3.example.com", "true", false},
+		{"false, custom endpoint", "https://s3.example.com", "false", true},
+		{"False, custom endpoint", "https://s3.example.com", "False", true},
+		{"0, custom endpoint", "https://s3.example.com", "0", true},
+		// A Secret value commonly arrives with a trailing newline.
+		{"false with trailing newline", "https://s3.example.com", "false\n", true},
+		{"false with trailing CR", "https://s3.example.com", "false\r\n", true},
+		{"garbage falls back to signing", "https://s3.example.com", "yes-please", false},
+		{"empty, no endpoint", "", "", false},
+		{"false, no endpoint", "", "false", true},
+		{"empty, GCS endpoint", "https://storage.googleapis.com", "", true},
+		{"true, GCS endpoint", "https://storage.googleapis.com", "true", true},
+		{"false, GCS endpoint", "https://storage.googleapis.com", "false", true},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Setenv(AWSSignAcceptEncoding, tc.env)
+			if got := ignoreAcceptEncodingSigning(tc.endpoints); got != tc.want {
+				t.Fatalf("ignoreAcceptEncodingSigning(%q) with %s=%q = %v, want %v",
+					tc.endpoints, AWSSignAcceptEncoding, tc.env, got, tc.want)
+			}
+		})
+	}
+}
+
+// TestAcceptEncodingIsSignedByDefault pins the default behavior: aws-sdk-go-v2
+// signs `Accept-Encoding`, so an endpoint that does not alter the header keeps
+// working exactly as before.
+func TestAcceptEncodingIsSignedByDefault(t *testing.T) {
+	server := newFakeS3Server()
+	defer server.Close()
+
+	svc := newTestService(t, server.URL)
+	t.Setenv(AWSSignAcceptEncoding, "")
+
+	if err := svc.PutObjectAsSinglePart(context.Background(), "backups/volume.cfg", bytes.NewReader([]byte("test"))); err != nil {
+		t.Fatalf("PutObjectAsSinglePart failed: %v", err)
+	}
+
+	requests := server.recordedRequests()
+	if len(requests) != 1 {
+		t.Fatalf("expected exactly 1 request, got %d: %+v", len(requests), requests)
+	}
+
+	if signed := requests[0].signedHeaders(); !strings.Contains(signed, "accept-encoding") {
+		t.Fatalf("expected accept-encoding in SignedHeaders, got %q", signed)
+	}
+}
+
+// TestSignAcceptEncodingFalseExcludesAcceptEncodingFromSignature is the
+// regression test for backup targets reached through a proxy that rewrites
+// `Accept-Encoding` in transit (e.g. a Cloudflare Tunnel, which replaces the
+// value with "gzip, br"). With AWS_SIGN_ACCEPT_ENCODING=false the header must
+// be absent from SignedHeaders, so the endpoint verifies the signature without
+// it, while the header itself is still sent on the wire.
+func TestSignAcceptEncodingFalseExcludesAcceptEncodingFromSignature(t *testing.T) {
+	server := newFakeS3Server()
+	defer server.Close()
+
+	svc := newTestService(t, server.URL)
+	t.Setenv(AWSSignAcceptEncoding, "false")
+
+	if err := svc.PutObjectAsSinglePart(context.Background(), "backups/volume.cfg", bytes.NewReader([]byte("test"))); err != nil {
+		t.Fatalf("PutObjectAsSinglePart failed: %v", err)
+	}
+
+	requests := server.recordedRequests()
+	if len(requests) != 1 {
+		t.Fatalf("expected exactly 1 request, got %d: %+v", len(requests), requests)
+	}
+
+	req := requests[0]
+	signed := req.signedHeaders()
+	// Guard first: signedHeaders() also returns "" for an unsigned request, which
+	// would make the exclusion check below pass without asserting anything.
+	if !strings.Contains(signed, "host") {
+		t.Fatalf("expected a signed request, got SignedHeaders %q", signed)
+	}
+	if strings.Contains(signed, "accept-encoding") {
+		t.Fatalf("expected accept-encoding to be excluded from SignedHeaders, got %q", signed)
+	}
+	// Assert the exact value, not merely that the header is present. Go's
+	// http.Transport substitutes "gzip" whenever the header is absent, so a
+	// non-empty check would still pass if restoreIgnored stopped working.
+	if req.acceptEncoding != "identity" {
+		t.Fatalf("expected Accept-Encoding %q on the wire after signing, got %q", "identity", req.acceptEncoding)
+	}
+}
+
+// TestIgnoreAcceptEncodingSigningWhenUnset covers the genuinely unset variable.
+// t.Setenv can only set an empty value, and the table above relies on that.
+func TestIgnoreAcceptEncodingSigningWhenUnset(t *testing.T) {
+	// t.Setenv registers the cleanup that restores the original value.
+	t.Setenv(AWSSignAcceptEncoding, "")
+	if err := os.Unsetenv(AWSSignAcceptEncoding); err != nil {
+		t.Fatalf("failed to unset %v: %v", AWSSignAcceptEncoding, err)
+	}
+
+	if ignoreAcceptEncodingSigning("https://s3.example.com") {
+		t.Fatal("expected Accept-Encoding to stay signed when the key is unset")
+	}
+}

types/types.go

@@ -29,6 +29,8 @@ const (
 	NOProxy    = "NO_PROXY"
 
 	VirtualHostedStyle = "VIRTUAL_HOSTED_STYLE"
+
+	AWSSignAcceptEncoding = "AWS_SIGN_ACCEPT_ENCODING"
 )
 
 type Mapping struct {

util/credential.go

@@ -41,6 +41,7 @@ func setupS3Credential(credential map[string]string) error {
 	_ = os.Setenv(types.HTTPProxy, credential[types.HTTPProxy])
 	_ = os.Setenv(types.NOProxy, credential[types.NOProxy])
 	_ = os.Setenv(types.VirtualHostedStyle, credential[types.VirtualHostedStyle])
+	_ = os.Setenv(types.AWSSignAcceptEncoding, credential[types.AWSSignAcceptEncoding])
 
 	// set a custom ca cert if available
 	if credential[types.AWSCert] != "" {
@@ -145,6 +146,7 @@ func getS3CredentialFromEnvVars() (map[string]string, error) {
 	credential[types.HTTPProxy] = os.Getenv(types.HTTPProxy)
 	credential[types.NOProxy] = os.Getenv(types.NOProxy)
 	credential[types.VirtualHostedStyle] = os.Getenv(types.VirtualHostedStyle)
+	credential[types.AWSSignAcceptEncoding] = os.Getenv(types.AWSSignAcceptEncoding)
 
 	return credential, nil
 }