Skip to content

feat(s3): make Accept-Encoding signing configurable - #316

Open
kinorai wants to merge 1 commit into
longhorn:masterfrom
kinorai:feat/configurable-sign-accept-encoding
Open

feat(s3): make Accept-Encoding signing configurable#316
kinorai wants to merge 1 commit into
longhorn:masterfrom
kinorai:feat/configurable-sign-accept-encoding

Conversation

@kinorai

@kinorai kinorai commented Aug 16, 2026

Copy link
Copy Markdown

Which issue(s) this PR fixes:

Issue longhorn/longhorn#13756

What this PR does / why we need it:

ignoreSigningHeaders already exists and already does the right thing, but it is
reachable only when the endpoint hostname contains storage.googleapis.com.

aws-sdk-go-v2 sends Accept-Encoding: identity and, unlike v1, includes
accept-encoding in the SigV4 SignedHeaders set. Any hop that alters the header
between the client and the endpoint therefore breaks signature verification with
SignatureDoesNotMatch. GCS is not the only such hop: a reverse proxy or a CDN in
front of an S3-compatible endpoint does the same thing, and Cloudflare rewrites the
header unconditionally by design
(docs). That population
cannot be detected from the endpoint string, so today it has no way to reach the
workaround and every backup fails.

This replaces the hostname match with AWS_SIGN_ACCEPT_ENCODING, a new backup target
secret key, following the VIRTUAL_HOSTED_STYLE precedent:

  • Default (unset) keeps today's behaviour exactly: the header is signed.
  • AWS_SIGN_ACCEPT_ENCODING=false excludes it from SignedHeaders.
  • The GCS hostname check is kept and takes priority, so GCS cannot regress even if a
    user sets the key to true.

ignoreSigningHeaders restores the header after signing, so the request on the wire
is unchanged either way.

The value is parsed with strconv.ParseBool rather than the exact string compare
VIRTUAL_HOSTED_STYLE uses fifty lines below. The asymmetry is deliberate: a value
this code rejects reproduces the total backup failure the key exists to fix, so 0,
False and a trailing newline from the Secret are all accepted, and a genuinely
malformed value is logged once rather than dropped silently. Happy to match the
neighbour instead if you prefer consistency there.

Special notes for your reviewer:

This is the first of three PRs, and the merge order matters.

  1. This PR.
  2. feat(proxy): allowlist AWS_SIGN_ACCEPT_ENCODING longhorn-instance-manager#1615 — adds the key to backupEnvAllowlist.
  3. feat(backuptarget): plumb AWS_SIGN_ACCEPT_ENCODING through longhorn-manager#5100 — plumbs the key out of the backup target secret.

Order 3 before 2 breaks every S3 backup and restore, for all users, not only those
who set the key. getBackupCredentialEnv appends the key unconditionally, and
validateBackupEnv in the instance manager rejects the whole request on one key
outside its allowlist. Each of the two dependent PRs also needs a vendor bump of this
module. The docs change for longhorn/website is separate.

On the design choice. The issue proposed either a secret key or "apply the
workaround whenever AWS_ENDPOINTS is set". I took the secret key, because the
second option silently changes signing for every existing non-AWS endpoint. Happy to
switch if you prefer the other shape.

Testing, unit. s3/s3_sign_accept_encoding_test.go adds four tests with no
external dependency:

  • TestIgnoreAcceptEncodingSigning covers the decision across empty / true /
    false / False / 0 / trailing newline / trailing CRLF / malformed, for a custom
    endpoint, no endpoint, and the GCS endpoint with all three values.
  • TestIgnoreAcceptEncodingSigningWhenUnset covers the genuinely unset variable,
    which t.Setenv cannot express.
  • TestAcceptEncodingIsSignedByDefault pins the default.
  • TestSignAcceptEncodingFalseExcludesAcceptEncodingFromSignature is the regression
    test: accept-encoding is absent from SignedHeaders, and Accept-Encoding: identity is still on the wire.

The last two read the real Authorization header off the existing fakeS3Server in
s3_service_unit_test.go, which I extended to record it.

I mutation-tested all three assertions rather than trusting them. Reverting the call
site to the old GCS-only gate fails the exclusion test. Breaking restoreIgnored
fails the wire test, because Go's http.Transport substitutes gzip when the header
is absent. Dropping the trim fails the two Secret-newline cases.

Testing, end to end. Docker only, no cluster. Two real S3 servers, each behind an
nginx that reproduces the Cloudflare edge (proxy_set_header Accept-Encoding "gzip"
plus gunzip on, since the edge decompresses the origin response before returning
it), plus a control arm with no rewrite. Driven by the existing s3test suite:

go test -tags=s3test -run 'Test$' ./s3/
origin proxy AWS_SIGN_ACCEPT_ENCODING result
MinIO 2025-04-22 rewrites unset FAIL SignatureDoesNotMatch
MinIO 2025-04-22 rewrites true FAIL SignatureDoesNotMatch
MinIO 2025-04-22 rewrites false ok
MinIO 2025-04-22 rewrites "false\n" ok
MinIO 2025-04-22 rewrites malformed FAIL, with the warning logged
MinIO 2025-04-22 none unset ok
MinIO 2025-04-22 none false ok
SeaweedFS 3.97 rewrites unset FAIL SignatureDoesNotMatch
SeaweedFS 3.97 rewrites false ok
SeaweedFS 3.97 none unset ok
SeaweedFS 3.97 none false ok

The control rows isolate the proxy as the cause. The false-without-proxy rows show
the key is safe to set on an endpoint that does not need it. SeaweedFS is the server
from the issue report.

golangci-lint run reports 0 issues, go test -race passes, and go vet and
gofmt are clean. The failures in ./test and TestInspectBackup need the NFS
server that scripts/test starts, and fail identically on master in my environment.

Additional documentation or context

rclone made exactly this change for the same reason. It began as a GCS-only quirk and
became the user-settable --s3-sign-accept-encoding in commit e175c863
("Splitting them like this makes them applicable for other providers such as
ActiveScale"). Users confirmed it fixes the Cloudflare case in rclone/rclone#8206,
and RustFS now documents the flag as required in its Cloudflare Tunnel guide.

apache/iceberg-go solved it with an opt-in property rather than a hostname match
(s3.compat-mode, PRs #1423 and #1559). litestream took the GCS-gated route and
immediately hit the follow-on report for a different provider behind a proxy
(benbjohnson/litestream#1424, still open).

The upstream SDK will not fix this: aws/aws-sdk-go-v2#1816 is closed with "the SDKs
don't guarantee compatibility with 3rd party platforms".


Prepared with AI assistance (Claude Code). Every test and command reported above was run, not estimated.

aws-sdk-go-v2 sends `Accept-Encoding: identity` and, unlike v1, includes
`accept-encoding` in the SigV4 SignedHeaders set. Any hop that alters the
header between the client and the endpoint therefore breaks signature
verification at the endpoint with:
"AWS Error: SignatureDoesNotMatch The request signature we calculated does
not match the signature you provided."

ignoreSigningHeaders already works around this, but it is reachable only
when the endpoint hostname contains storage.googleapis.com. GCS is not the
only hop that alters the header: a reverse proxy or a CDN in front of an
S3-compatible endpoint does the same thing, and Cloudflare rewrites the
header unconditionally by design. That population cannot be detected from
the endpoint string, so today it has no way to reach the workaround and
every backup fails.

Add AWS_SIGN_ACCEPT_ENCODING, a backup target secret key following the
VIRTUAL_HOSTED_STYLE precedent. Unset keeps the existing behavior, so the
header is still signed. Setting it to false excludes the header from
SignedHeaders. The storage.googleapis.com check is kept and takes priority,
so GCS cannot regress even if a user sets the key to true.

ignoreSigningHeaders restores the header after signing, so the request on
the wire is unchanged either way.

The value is parsed with strconv.ParseBool rather than the exact string
compare VIRTUAL_HOSTED_STYLE uses, because a rejected value here reproduces
the total backup failure this key exists to fix. For the same reason the
value is trimmed, since a Secret commonly carries a trailing newline, and a
malformed value is logged once instead of failing silently.

rclone made the same change for the same reason: a GCS-only quirk became
the user-settable --s3-sign-accept-encoding in commit e175c863.

Longhorn 13756

Signed-off-by: kinorai <kinorai@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@derekbit

Copy link
Copy Markdown
Member

@mantissahz Please review the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants