Repository navigation
fix(auth): compare the credential scope date before the key lookup - #512
Open
LeeroyHannigan wants to merge 16 commits into
Open
LeeroyHannigan wants to merge 16 commits into
LeeroyHannigan wants to merge 16 commits into
Conversation
…eaders, and time out stalled requests Each behavior below was measured against the service on 2026-09-16 unless marked as hardening. Credential scope. verify_signature() checked the scope's service and date and derived the signing key from whatever region the client named, so a request signed for one deployment verified against another that shared its credentials. check_scope() now compares the region with the configured [server] region and the service with dynamodb, and reports every failing part in one message in the service's order: "Credential should be scoped to a valid region. " then "Credential should be scoped to correct service: 'dynamodb'. " (the region is not named; the trailing spaces are the service's). A malformed scope reports the service's IncompleteSignatureException wire name and its five-element message. Session tokens. A token presented with a long-term key was ignored. The service validates any presented token and a long-term key has none, so the request now fails after signature verification with UnrecognizedClientException. The message for a presented token that does not resolve has no trailing period; the messages for an unknown key and for a missing token keep theirs, as measured. Repeated headers. build_canonical_headers() read only the first value of a header, so a client that sent a signed header twice could never authenticate. Values are now joined with commas in the order sent, which is how the service and botocore canonicalize them; a value that is not visible ASCII contributes an empty value rather than vanishing. A request with more than one host or authorization header is refused with a plain 400 before anything else, as the service's front end does. UNSIGNED-PAYLOAD. The special rejection message is removed. The service answers a client that signs that literal with the plain signature mismatch; since the payload line is always the hash of the received body, the mismatch now falls out of verification with the measured text. Request timeouts (hardening). Nothing bounded how long a connection could take to deliver headers or a body; a Content-Length the client never completed held the connection until the client gave up. [server] request_timeout_secs (default 30) now sets the HTTP/1 header read timeout on the TLS acceptor, the HTTP/2 keep-alive PING interval and timeout, and a body inactivity timeout on every route. A stalled body is answered with 408 and Connection: close; a stalled HTTP/1 header phase is closed at the timeout and a stalled HTTP/2 connection within twice it. The value also bounds the idle wait on a keep-alive connection, which is hyper's own 30 second default that never applied here for lack of a timer. Zero is refused at startup. The security manual's verification steps now list every check in the order the code runs them, and its credential storage section describes the in-process cache that is on by default in place of the sentence that denied it. Signed-off-by: Lee Hannigan <lhnng@amazon.com>
Conflicted file and resolution: - docs/manuals/05-admin-guide.md: both sides edited the [server] region row. Kept the PR's sentence on the credential scope region check and appended main's sentences on the backup ARN character class and cross-region resolution; kept the PR's request_timeout_secs row below it. Auto-merged files that both sides changed, checked by hand: - crates/server/src/handler.rs: main added the ConnectInfo peer address for aws:SourceIp; the PR's repeated-header refusal and body timeout arm sit before it unchanged. - crates/server/src/lib.rs: main added TCP_NODELAY on both listeners; the PR's request timeouts apply to the same axum_server builder. - crates/server/src/serve.rs, crates/config/src/lib.rs, crates/core/src/error/mod.rs, Cargo.toml, Cargo.lock: additive on both sides.
start_server keeps building the router. Serving it (the TLS and plaintext arms, the request timeouts, the HTTPS redirect acceptor, TCP_NODELAY on accepted sockets, shutdown and PID file cleanup) moves unchanged into listener::serve_app, together with its tests. No behaviour changes.
The header read timeout and the HTTP/2 keep-alive settings were installed on the axum_server builder of the TLS arm only. The plaintext arm (dev-mode builds, and any caller of start_server without TLS) went through axum::serve, which installs no timer, so a connection that sent part of its request headers and stalled stayed open there; only the body timeout layer covered it. Both arms now run on axum_server with one helper, with_request_timeouts, that sets the HTTP/1 header read timeout and the HTTP/2 keep-alive interval and timeout. The plaintext arm uses a NodelayAcceptor that sets TCP_NODELAY on each accepted socket, replacing the tap_io listener, and shares the shutdown handle with the TLS arm, so a shutdown signal drains plaintext connections with the same 5 second bound. Test: listener::tests::plaintext_listener_closes_a_stalled_header_phase serves a router on a loopback plaintext listener with a 300 ms timeout, sends a request line and one header with no terminating blank line, and requires the server to close the connection within 3 s. On the commit before this one the test fails with "server held a stalled header phase open for 3s on the plaintext listener".
…route The refusal lived in handle_request, so only the DynamoDB route saw it; /console, /management, /health and /metrics read the first or the joined value of a repeated header as before. The header-confusion case applies to the session-authed console and the Basic-authed management API as much as to the signed route. The check moves to a middleware, repeated_headers::refuse_repeated_headers, layered on the whole router after nesting and inside the security header layers, so every route and the fallback answer with the same 400, Connection: close and plain "400 Bad Request" body the data plane already returned. Measured against the service on 2026-10-23: two Authorization headers or two Host headers get the front end's 400 Bad Request with the connection closed before authentication; a repeated X-Amz-Date is not refused by the front end, so it stays out of this check. Tests: repeated_headers::tests cover the layer on a router shaped like the server's (data plane, /health, nested /console and /management, an unmatched path). tests/test_auth_error_fidelity.py adds test_duplicate_host_header_is_refused_on_every_route and test_duplicate_authorization_header_is_refused_on_every_route over /console/login, /management/accounts and /health, and test_duplicate_authorization_header_is_refused_on_the_data_plane. On the commit before this one the console, management and health cases answer 200 or 401 instead of 400.
Measured against the service on 2026-10-23: a ListTables request signed over one X-Amz-Date or X-Amz-Target value and sent with that header twice gets InvalidSignatureException with a request id. The front end does not refuse these headers the way it refuses a repeated host or authorization; the canonical headers join both values and the signature no longer matches. The server already behaves this way; the test pins it on both targets.
…y lookup The service validates the request timestamp, then the credential scope, then looks the access key up: measured 2026-10-23 with signed ListTables requests, an unknown key with a stale date answers "Signature expired", an unknown key scoped to another region answers "Credential should be scoped to a valid region. ", and a stale date with a wrong region answers "Signature expired". BuiltinAuthProvider::authenticate looked the key up first and answered UnrecognizedClientException for the first two. Run validate_timestamp and check_scope before lookup_credential, in that order. Neither reads the stored credential, so the constant-time handling of the signature and token comparisons after the lookup is unchanged. The comment that said the service returns the invalid-key error for an expired timestamp is removed; the measurement contradicts it. Add unit tests for the three orderings and move the authenticate tests from lib.rs into crates/auth/src/tests.rs, keeping lib.rs under 600 lines. Add the wire case for an unknown key scoped to another region to tests/test_auth_error_fidelity.py; it runs against extenddb and the service.
…listeners axum_server serves every accepted stream through hyper_util's auto builder, which reads the stream to tell HTTP/2 from HTTP/1 apart before either protocol's timer starts, and that read has no bound of its own. A client that connected, or that completed the TLS handshake, and then sent no byte, or sent only a prefix of the 24-byte HTTP/2 preface, held the connection and its task for as long as it kept the socket open. hyper's header read timeout starts after the sniff and the HTTP/2 keep-alive PING starts after the h2 handshake, so neither bound covered that shape. serve_app now wraps both listeners' acceptors in FirstByteAcceptor, which hands every accepted stream on inside FirstByteDeadline. The adapter arms a request_timeout sleep when the stream is handed over and drops it where the auto builder stops sniffing: at a byte that differs from the preface, or at the whole preface. While the deadline is armed, a read that would block also polls it, so the connection task wakes when it fires and the read fails with TimedOut; hyper_util then drops the connection. The redirect acceptor on the TLS port bounds its own first-byte peek with the same value, which closes a connection that sends no byte before the handshake as well. TCP_NODELAY and the plain HTTP redirect behave as before. Tests in listener::tests cover both listener modes: zero bytes after accept and after the TLS handshake, and 16 of the 24 preface bytes, each closed within the timeout. A request that arrives late but before the deadline is still served, and the sniff accounting is pinned directly. The TLS tests build a certificate, so rcgen, already a workspace dependency, becomes a dev-dependency of the crate. The admin guide row for request_timeout_secs states the new bound.
request_timeout_secs was checked for 0 only. The per-connection timers add the value to the current instant, and that addition panics on overflow, so a value near u64::MAX started the server and then panicked the task of every connection it accepted while the process itself stayed up. The startup check now refuses anything above 86400, a day, which is longer than any pause a client takes before sending its request and leaves the addition far from the overflow point. The check moved into check_request_timeout_secs so both bounds carry unit tests, and the admin guide row and the generated config template state the range.
The presented session token was read through HeaderValue::to_str, which fails for a value carrying a byte above 0x7f, so such a header looked absent. A long-term key passes the token check only when no token is presented, so a request with a non-ASCII x-amz-security-token header was served where any visible-ASCII value is refused. Presence now comes from the header map and the value from the header's text, so the header is refused whatever its bytes. A session credential is unaffected: a value that cannot be read as text is compared as empty and does not match the stored token. Whether the service's front end accepts a header value with a byte above 0x7f at all is not measured here, so this keeps the server's own rule, that only an absent header passes for a long-term key.
test_duplicate_host_header_is_refused sent no Authorization header and asserted the status only, so it passed against a server with no repeated-header check at all: the handler answers MissingAuthenticationToken with a 400 as well. It now goes through _raw_request and asserts the connection is closed, and against extenddb the "400 Bad Request" body that tells the listener's refusal apart from the handler's error. The service answers this shape with 400, Connection: close and an HTML error page, so the body assertion stays scoped to extenddb, as it is in the Authorization case next to it.
The scope date was compared inside verify_signature, which runs after lookup_credential, so a request whose scope date does not match X-Amz-Date answered InvalidSignatureException for an access key that exists and UnrecognizedClientException for one that does not. The service compares it before the lookup: measured 2026-10-23 with signed ListTables requests, a scope date of the previous day answers InvalidSignatureException "Date in Credential scope does not match YYYYMMDD from ISO-8601 version of date from HTTP. " for an unknown key and for a known key, the same shape as the region and service complaints, which this branch's base already checks before the lookup. Compare the date in check_scope, next to the region and the service, from the X-Amz-Date value validate_timestamp now returns. Keep the comparison in verify_signature so a caller that verifies a signature on its own keeps the same refusal. The message gains the trailing period and space the service sends, so the complaints read as sentences when more than one is wrong. Tests: an unknown key with a stale scope date reports the scope date; every pre-lookup fault answers with the same status, error type and message for a key that exists and one that does not; the date comparison is on bytes, so another length, letters and a day that does not exist are each a mismatch.
tests/test_sigv4_scope_date.py sends a hand-signed ListTables request with an access key that exists and with one that does not, for each fault the service answers before it looks the key up, and compares the status, the error type, the message and the response header names. It creates the key through the management API, so it runs against extenddb and skips when EXTENDDB_TEST_ENDPOINT is unset. Cases: a correct request, which must still report the unknown key as unknown; the scope region; the scope service; a scope date of the previous day, of the next day, of eight letters and empty; and an X-Amz-Date an hour old. The expected messages for the scope faults are the service's, measured 2026-10-23.
Measured against the service on 2026-10-24: a credential scope wrong in the region, the service and the date reports all three complaints in one message, in that order, with the date last. A scope wrong in the region and the date reports the region complaint then the date complaint. Add the three combinations as a unit test and as wire cases, so the order the complaints accumulate in is pinned.
scope_date_matches compared against ×tamp[..8], which panics if byte 8 falls inside a multi-byte character. Its one caller passes a header value already restricted to visible ASCII, so no request reaches that; get(..8) keeps the comparison safe for any caller.
LeeroyHannigan
requested review from
amrith,
c33howard,
jcshepherd,
pdf-amzn,
robinnsc and
yesyayen
as code owners
October 24, 2026 16:34
LeeroyHannigan
force-pushed
the
fix/sigv4-scope-and-headers
branch
from
October 30, 2026 09:07
4461c26 to
49fb3e2
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #355 (
fix/sigv4-scope-and-headers), which is open. Review the fivecommits on top of 4461c26.
What changes
The SigV4 provider compared the credential scope date with
X-Amz-Dateinsideverify_signature, which runs afterlookup_credential. A request whose scopedate did not match therefore answered
InvalidSignatureExceptionfor an accesskey that exists and
UnrecognizedClientExceptionfor one that does not.DynamoDB compares the scope date before it reads the key, as it does the
timestamp, the region and the service. This branch moves the comparison into
check_scope, which #355 already runs before the lookup, and keeps it inverify_signatureso a caller that verifies a signature on its own keeps thesame refusal.
Measurements
Signed
ListTablesrequests againstdynamodb.us-east-1.amazonaws.com, retriesoff, on 2026-10-23 at 19:33 UTC and 2026-10-24 at 15:10 and 16:18 UTC. The unknown key is
AKIAEXAMPLEEXAMPLE01; the known key is a session credential of the account theprobe ran in.
InvalidSignatureException, "Date in Credential scope does not match YYYYMMDD from ISO-8601 version of date from HTTP. "InvalidSignatureException, "The request signature we calculated does not match the signature you provided. ..."X-Amz-Datean hour oldInvalidSignatureException, "Signature expired: ..."The response header set is the same in every row:
connection,content-length,content-type,date,server,x-amz-crc32,x-amzn-requestid.So the order of the checks the service runs before it reads the key is the
timestamp, then the credential scope, and within the scope the region, the
service, the terminator and the date. A difference between the two keys appears
at the signature comparison, where a known key gets the mismatch message and an
unknown key gets
UnrecognizedClientException.A scope wrong in more than one part reports every complaint in one message, with
the date last:
The repository's message for the date carried no trailing period or space, which
ran two sentences together in the accumulated form. The message now ends with
the period and space the service sends.
Code
crates/auth/src/sigv4/verify.rs:check_scopetakes theX-Amz-Datevalue and compares the scope date against itsYYYYMMDDprefix, appending the complaint after the region and the service. The comparison is on bytes, so another length, letters and a day that does not exist are each a mismatch rather than a parse error.validate_timestampreturns theX-Amz-Datevalue it already extracted.crates/auth/src/lib.rs:authenticatepasses that value tocheck_scope. The lookup, the session token comparison and the signature comparison are unchanged, so the constant-time handling after the lookup still holds.Tests
crates/auth/src/tests.rsand theverify.rsunit tests cover an unknown keywith a stale scope date, the same status, error type and message for a key that
exists and one that does not across every pre-lookup fault, the byte comparison
over six malformed dates, a scope wrong in more than one part, and a matching
scope date that still authenticates.
tests/test_sigv4_scope_date.pyruns the same comparison on the wire. It createsan account, a user and an access key through the management API, then sends a
hand-signed
ListTablesrequest with that key and withAKIAEXAMPLEEXAMPLE01,comparing the status, the error type, the message and the response header names.
Its control case asserts that a correct request still reports the unknown key as
unknown.
At 4461c26 four of the new tests in
crates/auth/src/tests.rsfail, the threenew
verify.rsunit tests do not compile (check_scopetakes no timestampthere), and seven of the eleven cases of the wire file fail; at the head of this
branch all of them pass. The wire file is
collected by
.github/workflows/integration.ymlin therun-integrationandrun-integration-sqlitejobs, step "Run integration tests", which runsdevtools/run-tests --extenddb --pytest --comprehensive --parallel --filter "not import_export".Gates:
cargo +1.97.0 fmt --allwith--checkcargo +1.97.0 clippy --all-targetswith-D warningscargo test --workspace --exclude extenddb-storage-cassandra, 1,803 passedcargo test -p extenddb-storage-cassandra --lib, 61 passedcargo +1.88.0 testover the same selection, the MSRV gateThe Cassandra crate runs as a library test because another process on this host
holds 9042. Wire suites on SQLite and PostgreSQL: the eleven cases of the new
file pass on both, and the authentication files show no failure that does not
also happen at the base.
Not changed here
These three differences are present on main and on the base branch, so none is
fixed on this branch.
InvalidSignatureExceptionwith "Signature expired:<request time>is now earlier than<window>" for a stale date and "Signature not yet current: ..." for one ahead of the window, where the server answersUnrecognizedClientExceptionwith one message for both directions.SignedHeadershost rule before the credential scope, and its message reads "'Host' or ':authority' must be a 'SignedHeader' in the AWS Authorization." The server checks that rule after the scope and words it differently.aws4_requestis refused by the header parser withIncompleteSignature, where the service accumulates "Credential should be scoped with a valid terminator: 'aws4_request'. " with the other scope complaints.