From 2a66963dc010abe03a2901191c27b2e89146394c Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:50:13 +0000 Subject: [PATCH 01/10] fix(auth): check the scope and session token, canonicalize repeated headers, 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 --- Cargo.lock | 1 + Cargo.toml | 3 +- crates/app/src/init_helpers.rs | 3 +- crates/auth/src/lib.rs | 237 +++++++++++++++++++++++++++-- crates/auth/src/sigv4/canonical.rs | 87 ++++++++--- crates/auth/src/sigv4/parse.rs | 7 +- crates/auth/src/sigv4/verify.rs | 74 ++++++--- crates/config/src/lib.rs | 11 ++ crates/core/src/error/mod.rs | 2 +- crates/server/Cargo.toml | 1 + crates/server/src/handler.rs | 47 ++++-- crates/server/src/lib.rs | 33 +++- crates/server/src/serve.rs | 15 +- docs/manuals/05-admin-guide.md | 3 +- docs/manuals/10-security-model.md | 24 +-- tests/test_auth_error_fidelity.py | 226 +++++++++++++++++++++++++++ 16 files changed, 689 insertions(+), 85 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f978c8011..d19d5ed7f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1844,6 +1844,7 @@ dependencies = [ "extenddb-storage", "futures", "hyper", + "hyper-util", "libc", "metrics", "rand 0.9.4", diff --git a/Cargo.toml b/Cargo.toml index b8bc187ee..e8ee84f51 100755 --- a/Cargo.toml +++ b/Cargo.toml @@ -59,8 +59,9 @@ futures = "0.3" # HTTP axum = { version = "0.8", features = ["macros"] } tower = { version = "0.5", features = ["util"] } -tower-http = { version = "0.6", features = ["compression-gzip", "cors", "set-header"] } +tower-http = { version = "0.6", features = ["compression-gzip", "cors", "set-header", "timeout"] } hyper = { version = "1" } +hyper-util = { version = "0.1", features = ["tokio"] } # URI parsing for the backup store's endpoint check; the same major hyper and # the AWS SDK already use. http = { version = "1" } diff --git a/crates/app/src/init_helpers.rs b/crates/app/src/init_helpers.rs index 406db8b0e..874a4ea69 100755 --- a/crates/app/src/init_helpers.rs +++ b/crates/app/src/init_helpers.rs @@ -247,9 +247,10 @@ backend = "{backend}" [server] bind_addr = "{bind_addr}" # port = 18443 # HTTPS port -# region = "us-east-1" # AWS region for ARN generation +# region = "us-east-1" # AWS region for ARN generation; requests must be signed for this region # run_dir = "{run_dir}" # Directory for PID file # throttling_enabled = false # Enable provisioned throughput throttling +# request_timeout_secs = 30 # Close a connection whose headers or body stall this long [server.tls] # TLS is mandatory. The server refuses to start with enabled = false. diff --git a/crates/auth/src/lib.rs b/crates/auth/src/lib.rs index 7d6a0bb63..e64be6766 100755 --- a/crates/auth/src/lib.rs +++ b/crates/auth/src/lib.rs @@ -107,15 +107,22 @@ pub trait CredentialStore: Send + Sync { /// Parses the `Authorization` header, looks up the access key, decrypts the /// secret, verifies the `SigV4` signature, and validates the request timestamp. /// Handles both long-lived (AKIA*) and temporary (ASIA* + X-Amz-Security-Token) -/// credentials. +/// credentials. The credential scope must name this server's region. pub struct BuiltinAuthProvider { credential_store: C, + region: String, } impl BuiltinAuthProvider { - /// Create a new `BuiltinAuthProvider` with the given credential store. - pub fn new(credential_store: C) -> Self { - Self { credential_store } + /// Create a new `BuiltinAuthProvider` over `credential_store` for a server + /// deployed as `region`. A request whose credential scope names another + /// region is rejected, as the service rejects a scope for a region other + /// than the endpoint's. + pub fn new(credential_store: C, region: impl Into) -> Self { + Self { + credential_store, + region: region.into(), + } } } @@ -158,7 +165,15 @@ impl AuthProvider for BuiltinAuthProvider { // Validate timestamp (±15 minute window) after credential lookup. sigv4::verify::validate_timestamp(headers)?; - // For session credentials, verify X-Amz-Security-Token matches the stored token. + // The credential scope must name this server's region and the + // dynamodb service; the service reports both complaints in one + // message when both are wrong. + sigv4::verify::check_scope(&parsed, &self.region)?; + + // Session tokens. For a session credential the presented token must + // match the stored one. For a long-term key no token may be presented: + // the service validates any presented token and a long-term key has + // none, so the request fails as an invalid token. // CB-12: Session expiration is enforced at the credential store layer (fail-closed). // Expired sessions are never returned — the storage layer returns // ExpiredTokenException on the cache-miss path, and CachedCredentialStore @@ -167,15 +182,15 @@ impl AuthProvider for BuiltinAuthProvider { // S-5: Compare the session token in constant time and defer the // rejection until after signature verification, so neither the token's // byte contents nor its presence changes the failure-path timing. + let presented_token = headers + .get("x-amz-security-token") + .and_then(|v| v.to_str().ok()); let session_token_ok = if credential.is_session { - let token = headers - .get("x-amz-security-token") - .and_then(|v| v.to_str().ok()) - .unwrap_or(""); + let token = presented_token.unwrap_or(""); let expected = credential.session_token.as_deref().unwrap_or(""); sigv4::verify::constant_time_eq(token.as_bytes(), expected.as_bytes()) } else { - true + presented_token.is_none() }; // Verify SigV4 signature — always, even for inactive keys (S-5). @@ -191,11 +206,19 @@ impl AuthProvider for BuiltinAuthProvider { // S-5: Reject inactive keys or a mismatched session token only after // full signature verification, to prevent timing side-channels. - if is_inactive || !session_token_ok { + // The service's message for a presented token that does not resolve + // has no trailing period; its message for an unknown key or a missing + // token does (both measured 2026-09-16). + if is_inactive || (credential.is_session && presented_token.is_none()) { return Err(DynamoDbError::UnrecognizedClientException( "The security token included in the request is invalid.".to_owned(), )); } + if !session_token_ok { + return Err(DynamoDbError::UnrecognizedClientException( + "The security token included in the request is invalid".to_owned(), + )); + } // Build identity from credential (clone fields because ZeroizeOnDrop // prevents moving out of the struct). @@ -288,7 +311,7 @@ mod tests { credential: None, error: Some("The security token included in the request is expired"), }; - let provider = BuiltinAuthProvider::new(store); + let provider = BuiltinAuthProvider::new(store, "us-east-1"); let headers = make_headers_with_auth("ASIAEXTENDDB00000000", Some("test-token-value")); let result = provider.authenticate(&headers, b"{}").await; @@ -309,7 +332,7 @@ mod tests { credential: None, error: None, }; - let provider = BuiltinAuthProvider::new(store); + let provider = BuiltinAuthProvider::new(store, "us-east-1"); let headers = make_headers_with_auth("AKIAXXXXXXXXXXXXXXXX", None); let result = provider.authenticate(&headers, b"{}").await; @@ -323,4 +346,192 @@ mod tests { other => panic!("Expected UnrecognizedClientException, got: {other:?}"), } } + + const SECRET: &str = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"; + + fn credential(is_session: bool) -> StoredCredential { + StoredCredential { + secret_key: SECRET.to_owned(), + account_id: "123456789012".to_owned(), + principal_name: "alice".to_owned(), + session_name: is_session.then(|| "sess".to_owned()), + is_session, + session_token: is_session.then(|| "stored-token".to_owned()), + is_active: true, + expires_at: None, + } + } + + /// Headers for a request correctly signed by a client whose credential + /// scope names `region`. `token`, when present, is sent and signed. + fn signed_headers( + access_key: &str, + region: &str, + token: Option<&str>, + body: &[u8], + ) -> HeaderMap { + let mut headers = make_headers_with_auth(access_key, token); + headers.remove("authorization"); + let date_str = headers["x-amz-date"].to_str().unwrap().to_owned(); + let date_short = date_str[..8].to_owned(); + let mut names = vec!["content-type", "host", "x-amz-date", "x-amz-target"]; + if token.is_some() { + names.push("x-amz-security-token"); + } + let signed = names.join(";"); + let creq = sigv4::canonical::canonical_request("POST", "/", "", &headers, &signed, body); + let scope = format!("{date_short}/{region}/dynamodb/aws4_request"); + let sts = sigv4::canonical::string_to_sign(&date_str, &scope, &creq); + let key = sigv4::signing_key::derive_signing_key(SECRET, &date_short, region, "dynamodb"); + let sig = sigv4::signing_key::compute_signature(&key, &sts); + let auth = format!( + "AWS4-HMAC-SHA256 Credential={access_key}/{scope}, SignedHeaders={signed}, Signature={sig}" + ); + headers.insert("authorization", HeaderValue::from_str(&auth).unwrap()); + headers + } + + fn provider(is_session: bool) -> BuiltinAuthProvider { + BuiltinAuthProvider::new( + MockCredentialStore { + credential: Some(credential(is_session)), + error: None, + }, + "us-east-1", + ) + } + + #[tokio::test] + async fn correctly_signed_long_term_key_authenticates() { + let headers = signed_headers("AKIAEXTENDDB00000000", "us-east-1", None, b"{}"); + let id = provider(false).authenticate(&headers, b"{}").await.unwrap(); + assert!(matches!(id, AuthIdentity::User { .. })); + } + + /// Measured 2026-09-16: a scope for another region is refused with this + /// exact message; the region is not named and the message ends with a space. + #[tokio::test] + async fn scope_for_another_region_is_rejected() { + let headers = signed_headers("AKIAEXTENDDB00000000", "us-west-2", None, b"{}"); + match provider(false).authenticate(&headers, b"{}").await { + Err(DynamoDbError::InvalidSignatureException(msg)) => { + assert_eq!(msg, "Credential should be scoped to a valid region. "); + } + other => panic!("Expected InvalidSignatureException, got: {other:?}"), + } + } + + /// A long-term key has no session token; presenting one is an invalid token. + #[tokio::test] + async fn session_token_with_a_long_term_key_is_rejected() { + let headers = signed_headers( + "AKIAEXTENDDB00000000", + "us-east-1", + Some("any-token"), + b"{}", + ); + match provider(false).authenticate(&headers, b"{}").await { + Err(DynamoDbError::UnrecognizedClientException(msg)) => { + assert_eq!(msg, "The security token included in the request is invalid"); + } + other => panic!("Expected UnrecognizedClientException, got: {other:?}"), + } + } + + #[tokio::test] + async fn session_key_with_matching_token_authenticates() { + let headers = signed_headers( + "ASIAEXTENDDB00000000", + "us-east-1", + Some("stored-token"), + b"{}", + ); + let id = provider(true).authenticate(&headers, b"{}").await.unwrap(); + assert!(matches!(id, AuthIdentity::RoleSession { .. })); + } + + /// Measured 2026-09-16: a presented token that does not resolve has no + /// trailing period; a missing token does. + #[tokio::test] + async fn session_key_token_messages_match_the_service() { + let wrong = signed_headers("ASIAEXTENDDB00000000", "us-east-1", Some("bogus"), b"{}"); + match provider(true).authenticate(&wrong, b"{}").await { + Err(DynamoDbError::UnrecognizedClientException(msg)) => { + assert_eq!(msg, "The security token included in the request is invalid"); + } + other => panic!("Expected UnrecognizedClientException, got: {other:?}"), + } + let missing = signed_headers("ASIAEXTENDDB00000000", "us-east-1", None, b"{}"); + match provider(true).authenticate(&missing, b"{}").await { + Err(DynamoDbError::UnrecognizedClientException(msg)) => { + assert_eq!( + msg, + "The security token included in the request is invalid." + ); + } + other => panic!("Expected UnrecognizedClientException, got: {other:?}"), + } + } + + /// The scope region is compared byte for byte: region names are lowercase + /// and the service does not fold case either. + #[tokio::test] + async fn scope_region_comparison_is_exact() { + for other in ["US-EAST-1", "us-east-1 ", ""] { + let headers = signed_headers("AKIAEXTENDDB00000000", other, None, b"{}"); + match provider(false).authenticate(&headers, b"{}").await { + Err(DynamoDbError::InvalidSignatureException(msg)) => { + assert_eq!( + msg, "Credential should be scoped to a valid region. ", + "{other:?}" + ); + } + other_result => { + panic!("{other:?}: expected InvalidSignatureException, got {other_result:?}") + } + } + } + } + + /// An empty token header on a long-term key is a presented token, and is + /// rejected like any other; only an absent header passes. + #[tokio::test] + async fn empty_session_token_header_with_a_long_term_key_is_rejected() { + let headers = signed_headers("AKIAEXTENDDB00000000", "us-east-1", Some(""), b"{}"); + match provider(false).authenticate(&headers, b"{}").await { + Err(DynamoDbError::UnrecognizedClientException(msg)) => { + assert_eq!(msg, "The security token included in the request is invalid"); + } + other => panic!("Expected UnrecognizedClientException, got: {other:?}"), + } + } + + /// Measured 2026-09-16: a scope wrong in both region and service reports + /// both sentences in one message, region first. + #[tokio::test] + async fn scope_wrong_in_region_and_service_reports_both() { + let mut headers = make_headers_with_auth("AKIAEXTENDDB00000000", None); + headers.remove("authorization"); + let date_str = headers["x-amz-date"].to_str().unwrap().to_owned(); + let date_short = date_str[..8].to_owned(); + let signed = "content-type;host;x-amz-date;x-amz-target"; + let creq = sigv4::canonical::canonical_request("POST", "/", "", &headers, signed, b"{}"); + let scope = format!("{date_short}/us-west-2/s3/aws4_request"); + let sts = sigv4::canonical::string_to_sign(&date_str, &scope, &creq); + let key = sigv4::signing_key::derive_signing_key(SECRET, &date_short, "us-west-2", "s3"); + let sig = sigv4::signing_key::compute_signature(&key, &sts); + let auth = format!( + "AWS4-HMAC-SHA256 Credential=AKIAEXTENDDB00000000/{scope}, SignedHeaders={signed}, Signature={sig}" + ); + headers.insert("authorization", HeaderValue::from_str(&auth).unwrap()); + match provider(false).authenticate(&headers, b"{}").await { + Err(DynamoDbError::InvalidSignatureException(msg)) => { + assert_eq!( + msg, + "Credential should be scoped to a valid region. Credential should be scoped to correct service: 'dynamodb'. " + ); + } + other => panic!("Expected InvalidSignatureException, got: {other:?}"), + } + } } diff --git a/crates/auth/src/sigv4/canonical.rs b/crates/auth/src/sigv4/canonical.rs index 05b9e6183..949c37eb1 100755 --- a/crates/auth/src/sigv4/canonical.rs +++ b/crates/auth/src/sigv4/canonical.rs @@ -39,8 +39,9 @@ pub fn canonical_request( // trusting the header would let the sender choose the hash that the // signature is checked against. When the client includes the header in // SignedHeaders it is covered as an ordinary header by - // build_canonical_headers. UNSIGNED-PAYLOAD is rejected by - // verify_signature() before this function is called. + // build_canonical_headers. A client that signed a literal such as + // UNSIGNED-PAYLOAD in place of the hash fails as a plain signature + // mismatch, which is what the service returns. let payload_hash = sha256_hex(body); format!( @@ -65,10 +66,11 @@ pub fn string_to_sign(timestamp: &str, scope: &str, canonical_request: &str) -> /// Build canonical headers from the signed header list. /// -/// Per the `SigV4` spec, header names are lowercased, values are trimmed, -/// and headers are sorted alphabetically. Each line ends with `\n`. -/// CB-7: Header names are explicitly lowercased to handle clients that -/// send mixed-case `SignedHeaders` values. +/// Per the `SigV4` spec, header names are lowercased, values are trimmed with +/// internal whitespace collapsed, and a header that appears more than once +/// contributes its values joined by commas in the order they were sent. +/// Each line ends with `\n`. CB-7: Header names are explicitly lowercased to +/// handle clients that send mixed-case `SignedHeaders` values. fn build_canonical_headers(headers: &HeaderMap, signed_headers: &str) -> String { let mut result = String::new(); // signed_headers is already sorted and semicolon-delimited. @@ -77,27 +79,32 @@ fn build_canonical_headers(headers: &HeaderMap, signed_headers: &str) -> String // does not require pre-lowercased input. for name in signed_headers.split(';') { let lower = name.to_ascii_lowercase(); - let value = headers - .get(lower.as_str()) - .and_then(|v| v.to_str().ok()) - .unwrap_or(""); + let mut value = headers + .get_all(lower.as_str()) + .iter() + // A value that is not visible ASCII cannot have been signed as + // anything; it contributes an empty value rather than disappearing, + // so a request carrying such a header never canonicalizes the same + // as one without it. + .map(|v| v.to_str().unwrap_or("")) + // Trim leading/trailing whitespace, collapse internal whitespace + .map(|v| v.split_whitespace().collect::>().join(" ")) + .collect::>() + .join(","); // HTTP/2 uses :authority instead of Host. If the host header is empty // or missing, fall back to the :authority pseudo-header value. // Defense-in-depth: handler.rs injects Host from URI authority for // HTTP/2 requests, so this fallback should rarely activate. - let value = if lower == "host" && value.is_empty() { - headers + if lower == "host" && value.is_empty() { + value = headers .get(":authority") .and_then(|v| v.to_str().ok()) - .unwrap_or(value) - } else { - value - }; - // Trim leading/trailing whitespace, collapse internal whitespace - let trimmed = value.split_whitespace().collect::>().join(" "); + .map(|v| v.split_whitespace().collect::>().join(" ")) + .unwrap_or_default(); + } result.push_str(&lower); result.push(':'); - result.push_str(&trimmed); + result.push_str(&value); result.push('\n'); } result @@ -204,4 +211,46 @@ mod tests { assert_eq!(lines[8], sha256_hex(received_body)); assert_ne!(lines[8], sha256_hex(signed_body)); } + + /// A header sent more than once contributes its values joined by commas, + /// which is how the service canonicalizes it. + #[test] + fn repeated_header_values_are_comma_joined_in_order() { + let mut headers = HeaderMap::new(); + headers.insert("host", "localhost:18443".parse().unwrap()); + headers.insert("x-amz-date", "20260415T120000Z".parse().unwrap()); + headers.append("x-amz-meta-dup", " a ".parse().unwrap()); + headers.append("x-amz-meta-dup", "b c".parse().unwrap()); + + let creq = canonical_request( + "POST", + "/", + "", + &headers, + "host;x-amz-date;x-amz-meta-dup", + b"{}", + ); + + let lines: Vec<&str> = creq.split('\n').collect(); + assert_eq!(lines[5], "x-amz-meta-dup:a,b c"); + } + + /// A duplicate value that is not visible ASCII cannot be signed as anything. + /// It must still change the canonical form, so a request carrying it does not + /// verify under a signature made without it. + #[test] + fn unsignable_duplicate_value_changes_the_canonical_form() { + let mut plain = HeaderMap::new(); + plain.insert("host", "localhost:18443".parse().unwrap()); + plain.insert("x-amz-meta-dup", "a".parse().unwrap()); + let mut with_extra = plain.clone(); + with_extra.append( + "x-amz-meta-dup", + axum::http::HeaderValue::from_bytes(b"\xff\xfe").unwrap(), + ); + let a = canonical_request("POST", "/", "", &plain, "host;x-amz-meta-dup", b"{}"); + let b = canonical_request("POST", "/", "", &with_extra, "host;x-amz-meta-dup", b"{}"); + assert_ne!(a, b); + assert!(b.contains("x-amz-meta-dup:a,\n"), "{b}"); + } } diff --git a/crates/auth/src/sigv4/parse.rs b/crates/auth/src/sigv4/parse.rs index 7dc969046..05152667f 100755 --- a/crates/auth/src/sigv4/parse.rs +++ b/crates/auth/src/sigv4/parse.rs @@ -73,9 +73,10 @@ pub fn parse_authorization(header: &str) -> Result = credential.splitn(5, '/').collect(); if parts.len() != 5 || parts[4] != "aws4_request" { - return Err(incomplete( - "Credential must follow the format: AKID/date/region/service/aws4_request", - )); + // Measured 2026-09-16: the service names the rule and echoes the value. + return Err(incomplete(&format!( + "Credential must have exactly 5 slash-delimited elements, e.g. keyid/date/region/service/term, got '{credential}'" + ))); } Ok(ParsedAuthorization { diff --git a/crates/auth/src/sigv4/verify.rs b/crates/auth/src/sigv4/verify.rs index ddccd0cea..0adcc257f 100755 --- a/crates/auth/src/sigv4/verify.rs +++ b/crates/auth/src/sigv4/verify.rs @@ -17,6 +17,36 @@ use super::signing_key; /// Maximum allowed clock skew between client and server (±15 minutes). const MAX_CLOCK_SKEW_SECS: i64 = 15 * 60; +/// Check the credential scope's region and service against this server. +/// +/// The service reports every scope complaint in one message, region first, +/// each sentence ending with a space (measured 2026-09-16: a scope wrong in +/// both gives "Credential should be scoped to a valid region. Credential +/// should be scoped to correct service: 'dynamodb'. "). The region is not +/// named in the message. +/// +/// # Errors +/// +/// Returns `InvalidSignatureException` carrying the accumulated complaints +/// when the region is not `expected_region` or the service is not `dynamodb`. +pub fn check_scope( + parsed: &ParsedAuthorization, + expected_region: &str, +) -> Result<(), DynamoDbError> { + let mut complaints = String::new(); + if parsed.region != expected_region { + complaints.push_str("Credential should be scoped to a valid region. "); + } + if parsed.service != "dynamodb" { + complaints.push_str("Credential should be scoped to correct service: 'dynamodb'. "); + } + if complaints.is_empty() { + Ok(()) + } else { + Err(DynamoDbError::InvalidSignatureException(complaints)) + } +} + /// Verify a `SigV4` signature against the request. /// /// # Arguments @@ -62,16 +92,6 @@ pub fn verify_signature( )); } - // Reject UNSIGNED-PAYLOAD: real DynamoDB requires a computed body hash - // for all API calls. UNSIGNED-PAYLOAD is only valid for S3. - if let Some(content_sha) = headers.get("x-amz-content-sha256") - && content_sha.as_bytes() == b"UNSIGNED-PAYLOAD" - { - return Err(DynamoDbError::InvalidSignatureException( - "UNSIGNED-PAYLOAD is not supported for DynamoDB operations.".to_owned(), - )); - } - // Build canonical request let creq = canonical::canonical_request( method, @@ -300,23 +320,35 @@ mod tests { } } - /// Reject UNSIGNED-PAYLOAD for DynamoDB API calls. + /// A client that signs the payload line as the `UNSIGNED-PAYLOAD` literal + /// (an S3 convention) fails with the plain mismatch, which is what the + /// service returns (measured 2026-09-16): the payload line is always the + /// hash of the received body. #[test] - fn reject_unsigned_payload() { - let parsed = make_parsed( - "dynamodb", - "20260415", - "content-type;host;x-amz-content-sha256;x-amz-date", - ); + fn unsigned_payload_literal_is_a_plain_signature_mismatch() { let mut headers = make_headers("20260415T120000Z"); headers.insert("x-amz-content-sha256", "UNSIGNED-PAYLOAD".parse().unwrap()); - let err = verify_signature(&parsed, "secret", "POST", "/", "", &headers, b"{}"); - match err { + let signed = "content-type;host;x-amz-content-sha256;x-amz-date"; + // Sign the way such a client does: the literal in place of the body hash. + let creq = canonical::canonical_request("POST", "/", "", &headers, signed, SIGNED_BODY) + .rsplit_once('\n') + .map(|(head, _)| format!("{head}\nUNSIGNED-PAYLOAD")) + .unwrap(); + let sts = canonical::string_to_sign( + "20260415T120000Z", + "20260415/us-east-1/dynamodb/aws4_request", + &creq, + ); + let key = signing_key::derive_signing_key(SECRET, "20260415", "us-east-1", "dynamodb"); + let mut parsed = make_parsed("dynamodb", "20260415", signed); + parsed.signature = signing_key::compute_signature(&key, &sts); + match verify_signature(&parsed, SECRET, "POST", "/", "", &headers, SIGNED_BODY) { Err(DynamoDbError::InvalidSignatureException(msg)) => { assert!( - msg.contains("UNSIGNED-PAYLOAD"), - "Expected 'UNSIGNED-PAYLOAD' in: {msg}" + msg.starts_with("The request signature we calculated does not match"), + "{msg}" ); + assert!(!msg.contains("UNSIGNED-PAYLOAD"), "{msg}"); } other => panic!("Expected InvalidSignatureException, got: {other:?}"), } diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index 4cc5cf123..33f9aff23 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -75,6 +75,16 @@ pub struct ServerConfig { /// Enable provisioned throughput throttling via token buckets. /// When `None` or `false`, all requests are allowed regardless of capacity. pub throttling_enabled: Option, + /// Seconds a connection may take to deliver its request headers, and + /// seconds a request body may go without delivering data, before the + /// server closes the connection. Bounds how long a stalled or malicious + /// client can hold a connection open. Default 30. + #[serde(default = "default_request_timeout_secs")] + pub request_timeout_secs: u64, +} + +fn default_request_timeout_secs() -> u64 { + 30 } impl Default for ServerConfig { @@ -86,6 +96,7 @@ impl Default for ServerConfig { run_dir: default_run_dir(), tls: TlsConfig::default(), throttling_enabled: None, + request_timeout_secs: default_request_timeout_secs(), } } } diff --git a/crates/core/src/error/mod.rs b/crates/core/src/error/mod.rs index 845988b37..b73b67862 100755 --- a/crates/core/src/error/mod.rs +++ b/crates/core/src/error/mod.rs @@ -192,7 +192,7 @@ impl DynamoDbError { Self::InternalServerError(_) => "InternalServerError", Self::AccessDeniedException(_) => "AccessDeniedException", Self::MissingAuthenticationToken(_) => "MissingAuthenticationToken", - Self::IncompleteSignature(_) => "IncompleteSignature", + Self::IncompleteSignature(_) => "IncompleteSignatureException", Self::UnrecognizedClientException(_) => "UnrecognizedClientException", Self::ExpiredTokenException(_) => "ExpiredTokenException", Self::ExpiredIteratorException(_) => "ExpiredIteratorException", diff --git a/crates/server/Cargo.toml b/crates/server/Cargo.toml index da5da4f74..10f50783f 100755 --- a/crates/server/Cargo.toml +++ b/crates/server/Cargo.toml @@ -23,6 +23,7 @@ async-trait = { workspace = true } tower = { workspace = true } tower-http = { workspace = true } hyper = { workspace = true } +hyper-util = { workspace = true } tokio = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } diff --git a/crates/server/src/handler.rs b/crates/server/src/handler.rs index ef3765bda..1e08c2402 100755 --- a/crates/server/src/handler.rs +++ b/crates/server/src/handler.rs @@ -10,7 +10,7 @@ use std::sync::Arc; use axum::body::Bytes; use axum::extract::{ConnectInfo, State}; use axum::http::HeaderMap; -use axum::response::Response; +use axum::response::{IntoResponse, Response}; use extenddb_core::error::DynamoDbError; use extenddb_engine::OperationContext; use serde_json::Value; @@ -48,14 +48,43 @@ pub(crate) async fn handle_request( } let request_id = uuid::Uuid::new_v4().to_string(); - // SP-WIRE-007: body limit exceeded → 413 - let Ok(body) = body else { - return error_response( - &DynamoDbError::RequestEntityTooLargeException( - "Request size has exceeded the maximum allowed size".to_owned(), - ), - &request_id, - ); + // A request carrying more than one `host` or `authorization` header is + // refused outright, as the service's front end does (400, connection + // closed). Either duplicate is a header-confusion shape a proxy in front + // of this server could resolve differently from the signature check. + if headers.get_all("host").iter().count() > 1 + || headers.get_all("authorization").iter().count() > 1 + { + return ( + axum::http::StatusCode::BAD_REQUEST, + [(axum::http::header::CONNECTION, "close")], + "400 Bad Request", + ) + .into_response(); + } + + // SP-WIRE-007: body limit exceeded → 413. Any other failure to read the + // body is the body timeout (the client stopped sending) → 408. + let body = match body { + Ok(body) => body, + Err(axum::extract::rejection::BytesRejection::FailedToBufferBody( + axum::extract::rejection::FailedToBufferBody::LengthLimitError(_), + )) => { + return error_response( + &DynamoDbError::RequestEntityTooLargeException( + "Request size has exceeded the maximum allowed size".to_owned(), + ), + &request_id, + ); + } + Err(_) => { + return ( + axum::http::StatusCode::REQUEST_TIMEOUT, + [(axum::http::header::CONNECTION, "close")], + "request body was not received within the server's request timeout", + ) + .into_response(); + } }; // Extract operation from X-Amz-Target diff --git a/crates/server/src/lib.rs b/crates/server/src/lib.rs index 3b1b30309..d4786eaac 100755 --- a/crates/server/src/lib.rs +++ b/crates/server/src/lib.rs @@ -48,6 +48,7 @@ pub use key_info_cache::CachedTableKeyInfoStore; use serde_json::json; use tower::ServiceBuilder; use tower_http::set_header::SetResponseHeaderLayer; +use tower_http::timeout::RequestBodyTimeoutLayer; /// Application state shared across all handlers. pub struct AppState { @@ -65,6 +66,8 @@ pub struct AppState { pub metrics: Arc, /// Whether TLS is enabled (affects cookie Secure flag). pub tls_enabled: bool, + /// Header read timeout and body inactivity timeout for every connection. + pub request_timeout: Duration, /// Developer mode: authorization is opened for authenticated callers (SigV4 /// is still verified). Only ever true in a `dev-mode` build bound to /// loopback; the bin's serve path enforces those preconditions. @@ -123,6 +126,7 @@ pub async fn start_server( const DYNAMODB_BODY_LIMIT: usize = 16 * 1024 * 1024; let tls_enabled = state.tls_enabled; + let request_timeout = state.request_timeout; let catalog_store = state.catalog_store.clone(); let auth_cache_for_mgmt = state.auth_cache.clone(); let auth_cache_for_console = state.auth_cache.clone(); @@ -205,7 +209,12 @@ pub async fn start_server( } // Apply security headers to all routes. - let app = app.layer(security_layers); + // A body that stops delivering data for `request_timeout` errors out, so a + // client that announces a Content-Length it never sends cannot hold the + // connection open. The handler answers that error with 408. + let app = app + .layer(security_layers) + .layer(RequestBodyTimeoutLayer::new(request_timeout)); // Add HSTS header only when TLS is enabled. let app = if tls_enabled { @@ -257,9 +266,27 @@ pub async fn start_server( // confusing TLS handshake failure. The same acceptor sets // TCP_NODELAY on every accepted socket before the handshake. let redirect_acceptor = HttpsRedirectAcceptor { addr: local_addr }; - axum_server::from_tcp_rustls(std_listener, rustls_config)? + let mut server = axum_server::from_tcp_rustls(std_listener, rustls_config)? .map(|tls| tls.acceptor(redirect_acceptor)) - .handle(handle) + .handle(handle); + // HTTP/1: a connection that has not finished sending its request + // headers within `request_timeout` is closed; the same bound applies + // to the idle wait for the next request on a keep-alive connection. + server + .http_builder() + .http1() + .timer(hyper_util::rt::TokioTimer::new()) + .header_read_timeout(request_timeout); + // HTTP/2: a connection that stops answering PING frames is closed. + // A client that sends the preface and then stalls holds the + // connection for at most interval plus timeout. + server + .http_builder() + .http2() + .timer(hyper_util::rt::TokioTimer::new()) + .keep_alive_interval(Some(request_timeout)) + .keep_alive_timeout(request_timeout); + server .serve(app.into_make_service_with_connect_info::()) .await?; } else { diff --git a/crates/server/src/serve.rs b/crates/server/src/serve.rs index 9ebbaccd4..7c84fc8ff 100644 --- a/crates/server/src/serve.rs +++ b/crates/server/src/serve.rs @@ -375,6 +375,12 @@ async fn serve_inner(params: ServeParams, port: u16) -> anyhow::Result<()> { soft_ttl_seconds, negative_ttl_seconds, max_entries." ); } + if app_config.server.request_timeout_secs == 0 { + anyhow::bail!( + "Invalid [server] configuration: request_timeout_secs must be at least 1 \ + (0 would close every connection before it could send a request)." + ); + } if !cache_enabled { tracing::warn!( "auth.cache.enabled = false — auth/authz caches are in pass-through mode \ @@ -393,9 +399,11 @@ async fn serve_inner(params: ServeParams, port: u16) -> anyhow::Result<()> { make_cache_cfg("credential"), ) }); - let auth: Arc = Arc::new( - extenddb_auth::BuiltinAuthProvider::new((*cached_cred_store).clone()), - ); + let auth: Arc = + Arc::new(extenddb_auth::BuiltinAuthProvider::new( + (*cached_cred_store).clone(), + app_config.server.region.as_str(), + )); // Phase 3: Build the authorization cache. let authz_cache: Arc = { @@ -587,6 +595,7 @@ async fn serve_inner(params: ServeParams, port: u16) -> anyhow::Result<()> { ), metrics: metrics.clone(), tls_enabled, + request_timeout: Duration::from_secs(app_config.server.request_timeout_secs), dev_mode, import_paths, export_paths, diff --git a/docs/manuals/05-admin-guide.md b/docs/manuals/05-admin-guide.md index 4f5e07829..ac8215e97 100755 --- a/docs/manuals/05-admin-guide.md +++ b/docs/manuals/05-admin-guide.md @@ -90,7 +90,8 @@ These settings require a server restart to take effect. |-----|---------|-------------| | `bind_addr` | `127.0.0.1` | Network interface to bind | | `port` | `18443` | HTTP port | -| `region` | `us-east-1` | AWS region for ARN generation. Baked into every backup ARN the server issues, so it must stay within `[a-z0-9-]` (a value outside that class makes the server's own backup ARNs "not valid"; the server logs a warning at startup). A backup ARN of the caller's account in another region is resolved in the server's region first, then as written; see the upgrade manual before changing this on a deployment with backups | +| `region` | `us-east-1` | AWS region for ARN generation. Requests must carry a credential scope for this region; any other region is rejected with `InvalidSignatureException`, as the service rejects a scope for a region other than the endpoint's. Baked into every backup ARN the server issues, so it must stay within `[a-z0-9-]` (a value outside that class makes the server's own backup ARNs "not valid"; the server logs a warning at startup). A backup ARN of the caller's account in another region is resolved in the server's region first, then as written; see the upgrade manual before changing this on a deployment with backups | +| `request_timeout_secs` | `30` | Seconds a connection may take to deliver its request headers, and seconds a request body may stall without delivering data, before the server closes it (a stalled body gets HTTP 408). The same value bounds how long an idle keep-alive connection waits for its next request, so SDK connection pools see idle connections closed after it (they reopen transparently), and it is the HTTP/2 keep-alive PING interval and timeout, so a stalled HTTP/2 connection is closed within twice the value. Must be at least 1 | #### [storage] diff --git a/docs/manuals/10-security-model.md b/docs/manuals/10-security-model.md index bb7a1c75d..a9a36f36e 100755 --- a/docs/manuals/10-security-model.md +++ b/docs/manuals/10-security-model.md @@ -34,15 +34,19 @@ extenddb uses SigV4 signature verification with a local credential store and IAM #### SigV4 Verification -1. Extract the `Authorization` header (AWS4-HMAC-SHA256 scheme) -2. Parse credential scope, signed headers, and signature -3. Look up the access key in the credential store -4. Decrypt the secret key (AES-256-GCM) -5. Compute the signing key: HMAC-SHA256 chain over date, region, service, "aws4_request" -6. Compute the string-to-sign from the canonical request. The hashed-payload line is the SHA-256 of the body the server received; a client-supplied `x-amz-content-sha256` header is not used for it, so a request whose body was altered after signing fails at step 7 -7. Compare computed signature against the provided signature (constant-time comparison) -8. On mismatch: return `InvalidSignatureException` (HTTP 400) -9. On unknown key: return `UnrecognizedClientException` (HTTP 400) +The steps run in this order; every rejection is HTTP 400. + +1. Extract the `Authorization` header (AWS4-HMAC-SHA256 scheme); absent: `MissingAuthenticationToken` +2. Parse credential scope, signed headers, and signature; malformed: `IncompleteSignatureException`. A request with more than one `host` or `authorization` header is refused before this point with a plain 400, as the service's front end does +3. Look up the access key in the credential store (through the credential cache, see below); unknown: `UnrecognizedClientException` +4. Check `X-Amz-Date` against the server clock: more than 15 minutes of skew is `UnrecognizedClientException` (signature expired) +5. Check the credential scope: the region must be this server's `[server] region` and the service must be `dynamodb`; otherwise `InvalidSignatureException` carrying one sentence per failing part, region first, as the service reports it +6. Compare the session token: a temporary key must present its stored token, and a long-term key must present none; the decision is applied at step 9 +7. Reconstruct the canonical request and the string-to-sign. `host` must be among the signed headers. Repeated headers contribute their values comma-joined. The hashed-payload line is the SHA-256 of the body the server received; a client-supplied `x-amz-content-sha256` header is not used for it, so a request whose body was altered after signing, or signed with a literal such as `UNSIGNED-PAYLOAD`, fails at step 8 +8. Derive the signing key (HMAC-SHA256 chain over date, region, service, "aws4_request") and compare the computed signature with the provided one in constant time; mismatch: `InvalidSignatureException` +9. Apply the deferred decisions from steps 3 and 6, after the signature check so that failure paths take the same time: an inactive key or a wrong or missing token is `UnrecognizedClientException` + +Request bodies are parsed before authentication (the service does the same), so a malformed body returns `SerializationException` whatever the signature. Headers not named in `SignedHeaders` do not take part in verification; the service behaves the same way, and clients sign every `x-amz-*` header they send. #### Credential Types @@ -56,7 +60,7 @@ extenddb uses SigV4 signature verification with a local credential store and IAM - Secret keys encrypted with AES-256-GCM using a per-catalog encryption key - Encryption key generated during `extenddb init` and stored in the catalog database - Console passwords hashed with bcrypt (cost factor 12) -- No in-process credential cache — every request reads directly from PostgreSQL +- Credentials, policies, tags, and table key information are cached in process (`[auth.cache]`, enabled by default, 60 second hard TTL, 30 second stale-while-revalidate). A change made through this instance's admin API or console invalidates its own cache at once; a change made on another instance sharing the catalog is visible here within the TTL, so a key revoked elsewhere can be accepted by this instance for up to 60 seconds. See the admin guide for the settings and the kill switch ## Authorization diff --git a/tests/test_auth_error_fidelity.py b/tests/test_auth_error_fidelity.py index 6b6602dd6..bb3de7126 100755 --- a/tests/test_auth_error_fidelity.py +++ b/tests/test_auth_error_fidelity.py @@ -241,3 +241,229 @@ def test_unchanged_body_is_accepted(self, dynamodb_client, table): TableName=table, Key={"pk": {"S": "intact"}}, ConsistentRead=True ) assert got["Item"]["pk"]["S"] == "intact" + + +def _env_credentials() -> tuple[str, str, str | None]: + """Credentials to sign with: the environment first, then the SDK's chain.""" + access_key = os.environ.get("AWS_ACCESS_KEY_ID", "") + secret_key = os.environ.get("AWS_SECRET_ACCESS_KEY", "") + if access_key and secret_key: + return access_key, secret_key, os.environ.get("AWS_SESSION_TOKEN") or None + resolved = boto3.Session().get_credentials() + if resolved is None: + pytest.skip("no credentials available to sign the request") + frozen = resolved.get_frozen_credentials() + return frozen.access_key, frozen.secret_key, frozen.token + + +class TestRequestChecks: + """Checks on the signed request other than the signature itself. + + Each expected message was measured against the service on 2026-09-16. + """ + + @pytest.fixture(autouse=True) + def setup(self, endpoint_url): + if endpoint_url and not os.environ.get("EXTENDDB_ADMIN_USER", "").strip(): + pytest.fail( + "MISCONFIGURED: request check tests require auth-enabled extenddb " + "(set EXTENDDB_ADMIN_USER to signal builtin auth mode)." + ) + self.endpoint_url = endpoint_url + self.region = os.environ.get("AWS_DEFAULT_REGION", "us-east-1") + self.url = endpoint_url or f"https://dynamodb.{self.region}.amazonaws.com/" + + def _post(self, body: dict, *, headers: dict | None = None, region: str | None = None, + token: str | None = "env") -> requests.Response: + """Sign and send one ListTables-style request with botocore's signer. + + ``headers`` are added before signing so they are covered. ``region`` + overrides the credential scope. ``token`` "env" uses the environment's + session token, if any; None sends no token; a string sends that token. + """ + access_key, secret_key, env_token = _env_credentials() + if token == "env": + token = env_token + body_bytes = json.dumps(body).encode("utf-8") + hdrs = { + "X-Amz-Target": "DynamoDB_20120810.ListTables", + "Content-Type": "application/x-amz-json-1.0", + } + hdrs.update(headers or {}) + aws_req = AWSRequest(method="POST", url=self.url, data=body_bytes, headers=hdrs) + SigV4Auth(Credentials(access_key, secret_key, token), "dynamodb", region or self.region).add_auth(aws_req) + return requests.post( + self.url, + data=body_bytes, + headers=dict(aws_req.headers), + verify=self.endpoint_url is None, + ) + + def test_scope_for_another_region_is_rejected(self): + other = "us-west-2" if self.region != "us-west-2" else "us-east-1" + resp = self._post({}, region=other) + assert resp.status_code == 400, resp.text + assert resp.json()["__type"].endswith("InvalidSignatureException"), resp.text + assert resp.json()["message"] == "Credential should be scoped to a valid region. " + + def test_scope_wrong_in_region_and_service_reports_both(self): + access_key, secret_key, token = _env_credentials() + other = "us-west-2" if self.region != "us-west-2" else "us-east-1" + body_bytes = json.dumps({}).encode("utf-8") + aws_req = AWSRequest( + method="POST", + url=self.url, + data=body_bytes, + headers={ + "X-Amz-Target": "DynamoDB_20120810.ListTables", + "Content-Type": "application/x-amz-json-1.0", + }, + ) + SigV4Auth(Credentials(access_key, secret_key, token), "s3", other).add_auth(aws_req) + resp = requests.post(self.url, data=body_bytes, headers=dict(aws_req.headers), verify=self.endpoint_url is None) + assert resp.status_code == 400, resp.text + assert resp.json()["__type"].endswith("InvalidSignatureException"), resp.text + assert resp.json()["message"] == ( + "Credential should be scoped to a valid region. " + "Credential should be scoped to correct service: 'dynamodb'. " + ) + + def test_duplicate_host_header_is_refused(self): + # The service's front end refuses a request with two host headers + # before it reaches signing (400, connection closed). + import http.client # noqa: PLC0415 + import ssl # noqa: PLC0415 + from urllib.parse import urlparse # noqa: PLC0415 + + u = urlparse(self.url) + ctx = ssl.create_default_context() + if self.endpoint_url is not None: + ctx.check_hostname = False + ctx.verify_mode = ssl.CERT_NONE + conn = http.client.HTTPSConnection(u.hostname, u.port or 443, context=ctx, timeout=30) + conn.putrequest("POST", "/", skip_host=True, skip_accept_encoding=True) + conn.putheader("Host", u.netloc) + conn.putheader("Host", "evil.example.com") + conn.putheader("X-Amz-Target", "DynamoDB_20120810.ListTables") + conn.putheader("Content-Type", "application/x-amz-json-1.0") + conn.putheader("Content-Length", "2") + conn.endheaders() + conn.send(b"{}") + resp = conn.getresponse() + resp.read() + conn.close() + assert resp.status == 400 + + def test_session_token_with_a_long_term_key_is_rejected(self): + _, _, env_token = _env_credentials() + if env_token: + pytest.skip("the environment's credentials are temporary; this case needs a long-term key") + resp = self._post({}, token="AQoDYXdzEJr-not-a-real-token") + assert resp.status_code == 400, resp.text + assert resp.json()["__type"].endswith("UnrecognizedClientException"), resp.text + assert resp.json()["message"] == "The security token included in the request is invalid" + + def test_repeated_signed_header_is_canonicalized_comma_joined(self): + # botocore joins repeated header values with commas when signing; the + # server must canonicalize the same way, or every such request fails. + access_key, secret_key, token = _env_credentials() + body_bytes = json.dumps({}).encode("utf-8") + aws_req = AWSRequest( + method="POST", + url=self.url, + data=body_bytes, + headers={ + "X-Amz-Target": "DynamoDB_20120810.ListTables", + "Content-Type": "application/x-amz-json-1.0", + }, + ) + aws_req.headers.add_header("X-Amz-Meta-Dup", "first") + aws_req.headers.add_header("X-Amz-Meta-Dup", "second") + SigV4Auth(Credentials(access_key, secret_key, token), "dynamodb", self.region).add_auth(aws_req) + # Send every header as its own line, including both X-Amz-Meta-Dup values. + prepared_headers = list(aws_req.headers.items()) + import http.client # noqa: PLC0415 + import ssl # noqa: PLC0415 + from urllib.parse import urlparse # noqa: PLC0415 + + u = urlparse(self.url) + ctx = ssl.create_default_context() + if self.endpoint_url is not None: + ctx.check_hostname = False + ctx.verify_mode = ssl.CERT_NONE + conn = http.client.HTTPSConnection(u.hostname, u.port or 443, context=ctx, timeout=30) + conn.putrequest("POST", "/", skip_host=True, skip_accept_encoding=True) + conn.putheader("Host", u.netloc) + for k, v in prepared_headers: + conn.putheader(k, v) + conn.putheader("Content-Length", str(len(body_bytes))) + conn.endheaders() + conn.send(body_bytes) + resp = conn.getresponse() + text = resp.read().decode() + conn.close() + assert resp.status == 200, text + + def test_unsigned_payload_literal_is_a_plain_signature_mismatch(self): + # A client that signs the payload line as the S3 literal fails with the + # ordinary mismatch: the service does not special-case the literal. + access_key, secret_key, token = _env_credentials() + body_bytes = json.dumps({}).encode("utf-8") + aws_req = AWSRequest( + method="POST", + url=self.url, + data=body_bytes, + headers={ + "X-Amz-Target": "DynamoDB_20120810.ListTables", + "Content-Type": "application/x-amz-json-1.0", + "X-Amz-Content-SHA256": "UNSIGNED-PAYLOAD", + }, + ) + signer = SigV4Auth(Credentials(access_key, secret_key, token), "dynamodb", self.region) + signer.payload = lambda request: "UNSIGNED-PAYLOAD" # sign the literal, as such a client does + signer.add_auth(aws_req) + resp = requests.post(self.url, data=body_bytes, headers=dict(aws_req.headers), verify=self.endpoint_url is None) + assert resp.status_code == 400, resp.text + assert resp.json()["__type"].endswith("InvalidSignatureException"), resp.text + assert resp.json()["message"].startswith("The request signature we calculated does not match"), resp.text + assert "UNSIGNED-PAYLOAD" not in resp.json()["message"] + + def test_stalled_body_is_closed_by_the_server(self): + """A Content-Length the client never delivers must not hold the connection. + + Runs against extenddb only. The server's `request_timeout_secs` bounds the + wait; the test allows that plus ten seconds, reading the value from + EXTENDDB_REQUEST_TIMEOUT_SECS (default 30). + """ + if self.endpoint_url is None: + pytest.skip("timeout behavior is a server property; not measured against the service") + import http.client # noqa: PLC0415 + import socket # noqa: PLC0415 + import ssl # noqa: PLC0415 + import time # noqa: PLC0415 + from urllib.parse import urlparse # noqa: PLC0415 + + timeout = int(os.environ.get("EXTENDDB_REQUEST_TIMEOUT_SECS", "30")) + u = urlparse(self.url) + ctx = ssl.create_default_context() + ctx.check_hostname = False + ctx.verify_mode = ssl.CERT_NONE + conn = http.client.HTTPSConnection(u.hostname, u.port or 443, context=ctx, timeout=timeout + 10) + conn.putrequest("POST", "/", skip_host=True, skip_accept_encoding=True) + conn.putheader("Host", f"{u.hostname}:{u.port or 443}") + conn.putheader("X-Amz-Target", "DynamoDB_20120810.ListTables") + conn.putheader("Content-Type", "application/x-amz-json-1.0") + conn.putheader("Content-Length", "100") + conn.endheaders() + conn.send(b"{}") # 2 of the announced 100 bytes; nothing more follows + started = time.monotonic() + try: + resp = conn.getresponse() + status = resp.status + resp.read() + except (http.client.RemoteDisconnected, ConnectionResetError, socket.timeout) as e: + status = type(e).__name__ + elapsed = time.monotonic() - started + conn.close() + assert status in (408, "RemoteDisconnected", "ConnectionResetError"), status + assert elapsed < timeout + 10, f"server held the stalled connection for {elapsed:.0f}s" From 8731fbfb7f2108a445dcf39332fa6df2987b34e3 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:50:13 +0000 Subject: [PATCH 02/10] refactor(server): move the listener code into listener.rs 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. --- crates/server/src/lib.rs | 296 +------------------------------ crates/server/src/listener.rs | 320 ++++++++++++++++++++++++++++++++++ 2 files changed, 322 insertions(+), 294 deletions(-) mode change 100755 => 100644 crates/server/src/lib.rs create mode 100644 crates/server/src/listener.rs diff --git a/crates/server/src/lib.rs b/crates/server/src/lib.rs old mode 100755 new mode 100644 index d4786eaac..c57bec22c --- a/crates/server/src/lib.rs +++ b/crates/server/src/lib.rs @@ -17,6 +17,7 @@ mod handler; mod import_export_paths; pub mod key_info_cache; mod leading_keys; +mod listener; pub mod management; mod metrics_endpoint; pub mod rate_limit; @@ -226,303 +227,10 @@ pub async fn start_server( app }; - let local_addr = listener.local_addr()?; - - if let Some(tls_cfg) = tls { - // P57 Bug 2 fix: rustls 0.23 requires an explicit CryptoProvider. - // Install aws-lc-rs as the default before creating any TLS config. - // Ignore the error if a provider was already installed (e.g., by sqlx). - let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); - - tracing::info!("extenddb listening on {local_addr} (HTTPS)"); - let rustls_config = axum_server::tls_rustls::RustlsConfig::from_pem_file( - &tls_cfg.cert_path, - &tls_cfg.key_path, - ) - .await - // AI-3: The error intentionally does not include the file path to avoid - // leaking filesystem structure in logs. See docs/troubleshooting.md for - // the "Failed to load TLS certificates" entry. - .map_err(|e| anyhow::anyhow!("Failed to load TLS certificates: {e}"))?; - - let handle = axum_server::Handle::new(); - let shutdown_handle = handle.clone(); - let shutdown_pid = pid_file.clone(); - tokio::spawn(async move { - shutdown_signal().await; - shutdown_handle.graceful_shutdown(Some(Duration::from_secs(5))); - // P50: Log timeout but don't call std::process::exit — let the - // runtime shut down normally so destructors (including ZeroizeOnDrop) run. - tokio::time::sleep(Duration::from_secs(10)).await; - tracing::warn!("Graceful shutdown timed out, forcing PID file cleanup"); - cleanup_pid_file(shutdown_pid.as_deref()); - }); - - let std_listener = listener.into_std()?; - // AI-2: Use a custom acceptor that peeks the first byte of each - // connection. If it's plain HTTP (not 0x16 TLS ClientHello), write - // a 301 redirect to HTTPS and reject the connection before the TLS - // handshake. This gives users a helpful redirect instead of a - // confusing TLS handshake failure. The same acceptor sets - // TCP_NODELAY on every accepted socket before the handshake. - let redirect_acceptor = HttpsRedirectAcceptor { addr: local_addr }; - let mut server = axum_server::from_tcp_rustls(std_listener, rustls_config)? - .map(|tls| tls.acceptor(redirect_acceptor)) - .handle(handle); - // HTTP/1: a connection that has not finished sending its request - // headers within `request_timeout` is closed; the same bound applies - // to the idle wait for the next request on a keep-alive connection. - server - .http_builder() - .http1() - .timer(hyper_util::rt::TokioTimer::new()) - .header_read_timeout(request_timeout); - // HTTP/2: a connection that stops answering PING frames is closed. - // A client that sends the preface and then stalls holds the - // connection for at most interval plus timeout. - server - .http_builder() - .http2() - .timer(hyper_util::rt::TokioTimer::new()) - .keep_alive_interval(Some(request_timeout)) - .keep_alive_timeout(request_timeout); - server - .serve(app.into_make_service_with_connect_info::()) - .await?; - } else { - tracing::info!("extenddb listening on {local_addr}"); - axum::serve( - nodelay_listener(listener), - app.into_make_service_with_connect_info::(), - ) - .with_graceful_shutdown(graceful_shutdown(pid_file.clone())) - .await?; - } - - // Normal shutdown path — clean up PID file after connections drain. - cleanup_pid_file(pid_file.as_deref()); - - Ok(()) + listener::serve_app(listener, app, request_timeout, tls, pid_file).await } /// GET /health — REQ-OBS-006 async fn health_check() -> impl IntoResponse { (StatusCode::OK, axum::Json(json!({"status": "healthy"}))) } - -async fn graceful_shutdown(pid_file: Option) { - shutdown_signal().await; - // P50: Spawn a timeout task that cleans up the PID file if connections - // don't drain. Does NOT call std::process::exit — the runtime shuts down - // normally so destructors (including ZeroizeOnDrop) run. - tokio::spawn(async move { - tokio::time::sleep(Duration::from_secs(5)).await; - tracing::warn!("Graceful shutdown timed out after 5s, forcing PID file cleanup"); - cleanup_pid_file(pid_file.as_deref()); - }); -} - -/// Remove the PID file if it exists. Best-effort — log but don't fail. -fn cleanup_pid_file(pid_file: Option<&std::path::Path>) { - if let Some(path) = pid_file - && let Err(e) = std::fs::remove_file(path) - && e.kind() != std::io::ErrorKind::NotFound - { - tracing::warn!("Failed to remove PID file {}: {e}", path.display()); - } -} - -/// Set `TCP_NODELAY` on an accepted socket. -/// -/// With Nagle's algorithm on, the kernel holds a small write while an earlier -/// small write on the connection is unacknowledged, and the client's delayed -/// ACK leaves with its next request on that connection or when its timer -/// fires (about 40 ms on Linux). Once one response is written while the -/// previous one is unacknowledged, each response waits for the client's next -/// request, and the reply to that request is held behind it in turn. The -/// response latency then equals the gap between the client's requests on the -/// connection, for every gap under the delayed-ACK timer. `TCP_NODELAY` sends -/// each write as it is made. The option lives on the TCP socket, so under TLS -/// it is set before the handshake and applies below the TLS stream. A failure -/// to set it is logged at debug and the connection is served with the kernel -/// default. -fn set_tcp_nodelay(stream: &tokio::net::TcpStream) { - if let Err(e) = stream.set_nodelay(true) { - tracing::debug!("Failed to set TCP_NODELAY on accepted connection: {e}"); - } -} - -/// The plaintext listener: a `TcpListener` whose accepted sockets carry -/// `TCP_NODELAY`. -type NodelayListener = axum::serve::TapIo; - -/// Wrap the plaintext listener so every accepted socket carries `TCP_NODELAY`. -/// -/// `axum::serve` does not set the option itself; `ListenerExt::tap_io` is its -/// hook for per-connection socket options. -fn nodelay_listener(listener: tokio::net::TcpListener) -> NodelayListener { - use axum::serve::ListenerExt; - listener.tap_io(|stream| set_tcp_nodelay(stream)) -} - -/// AI-2: Acceptor that detects plain HTTP connections on the TLS port. -/// -/// Sets `TCP_NODELAY` on the accepted socket first (see [`set_tcp_nodelay`]). -/// Then peeks the first byte of the connection. If it's `0x16` (TLS -/// `ClientHello`), the connection passes through to the TLS acceptor. If it's -/// a plain HTTP verb, a 301 redirect to `https://` is written and the -/// connection is rejected with an IO error (which `axum_server` handles by -/// dropping it). -/// -/// `axum_server` 0.8 ships a `NoDelayAcceptor`, but it cannot be stacked under -/// this acceptor without a generic wrapper, so this acceptor sets the option -/// itself, and logs instead of rejecting on failure. -#[derive(Clone)] -struct HttpsRedirectAcceptor { - addr: std::net::SocketAddr, -} - -impl axum_server::accept::Accept - for HttpsRedirectAcceptor -{ - type Stream = tokio::net::TcpStream; - type Service = S; - type Future = std::pin::Pin< - Box< - dyn std::future::Future> + Send, - >, - >; - - fn accept(&self, stream: tokio::net::TcpStream, service: S) -> Self::Future { - let addr = self.addr; - Box::pin(async move { - set_tcp_nodelay(&stream); - let mut peek_buf = [0u8; 1]; - match stream.peek(&mut peek_buf).await { - Ok(0) => { - // Connection closed before sending data. - Err(std::io::Error::new( - std::io::ErrorKind::ConnectionReset, - "empty connection", - )) - } - Ok(_) if peek_buf[0] == 0x16 => { - // TLS ClientHello — pass through to TLS acceptor. - Ok((stream, service)) - } - Ok(_) => { - // Plain HTTP — send redirect and reject. - send_https_redirect(&stream, addr); - Err(std::io::Error::new( - std::io::ErrorKind::InvalidData, - "plain HTTP redirected to HTTPS", - )) - } - Err(e) => Err(e), - } - }) - } -} - -/// Write an HTTP 301 redirect response to HTTPS on the raw TCP stream. -fn send_https_redirect(stream: &tokio::net::TcpStream, addr: std::net::SocketAddr) { - let location = format!("https://{addr}/"); - let response = format!( - "HTTP/1.1 301 Moved Permanently\r\n\ - Location: {location}\r\n\ - Content-Length: 0\r\n\ - Connection: close\r\n\r\n" - ); - // Best-effort write — if it fails, the client already got a connection error. - let _ = stream.try_write(response.as_bytes()); -} - -#[cfg(unix)] -async fn shutdown_signal() { - let ctrl_c = tokio::signal::ctrl_c(); - let Ok(mut sigterm) = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate()) - else { - tracing::error!("Failed to register SIGTERM handler, falling back to Ctrl+C only"); - let _ = ctrl_c.await; - tracing::info!("Shutdown signal received, draining connections..."); - return; - }; - tokio::select! { - _ = ctrl_c => {}, - _ = sigterm.recv() => {}, - } - tracing::info!("Shutdown signal received, draining connections..."); -} - -/// Non-unix: there is no SIGTERM; Ctrl+C (which tokio maps to the console -/// control handler on Windows) is the only shutdown signal — and it only -/// fires for an interactive console. Supervisors that terminate the process -/// directly (including the npm launcher's `stop()`, which is -/// `TerminateProcess` on Windows) hard-kill without reaching this path, so -/// the worker drain is skipped. That is acceptable: drain only cancels and -/// joins background workers — it flushes nothing, and storage commits -/// per-transaction. -#[cfg(not(unix))] -async fn shutdown_signal() { - let _ = tokio::signal::ctrl_c().await; - tracing::info!("Shutdown signal received, draining connections..."); -} - -#[cfg(test)] -mod tests { - use super::*; - use tokio::io::AsyncWriteExt; - use tokio::net::{TcpListener, TcpStream}; - - /// Bind a loopback listener and connect one client to it. - async fn bind_and_connect() -> (TcpListener, TcpStream) { - let listener = TcpListener::bind("127.0.0.1:0") - .await - .expect("bind loopback"); - let addr = listener.local_addr().expect("local addr"); - let client = TcpStream::connect(addr).await.expect("connect"); - (listener, client) - } - - /// The TLS path: the acceptor that runs before the TLS handshake sets - /// `TCP_NODELAY` on the accepted socket, read back from the socket the - /// acceptor hands on. - #[tokio::test] - async fn tls_acceptor_sets_tcp_nodelay_before_the_handshake() { - use axum_server::accept::Accept; - - let (listener, mut client) = bind_and_connect().await; - // A TLS ClientHello starts with the handshake record type. - client - .write_all(&[0x16]) - .await - .expect("write ClientHello byte"); - let (accepted, _) = listener.accept().await.expect("accept"); - assert!( - !accepted.nodelay().expect("read TCP_NODELAY"), - "a fresh accepted socket has Nagle on" - ); - - let acceptor = HttpsRedirectAcceptor { - addr: listener.local_addr().expect("local addr"), - }; - let (stream, ()) = acceptor - .accept(accepted, ()) - .await - .expect("a TLS ClientHello passes through to the TLS acceptor"); - assert!(stream.nodelay().expect("read TCP_NODELAY")); - } - - /// The plaintext path: the listener handed to `axum::serve` sets - /// `TCP_NODELAY` on every socket it accepts. - #[tokio::test] - async fn plaintext_listener_sets_tcp_nodelay_on_accepted_sockets() { - use axum::serve::Listener; - - let (listener, client) = bind_and_connect().await; - let mut listener = nodelay_listener(listener); - let (accepted, peer) = listener.accept().await; - assert_eq!(peer, client.local_addr().expect("client addr")); - assert!(accepted.nodelay().expect("read TCP_NODELAY")); - } -} diff --git a/crates/server/src/listener.rs b/crates/server/src/listener.rs new file mode 100644 index 000000000..0e56dcdca --- /dev/null +++ b/crates/server/src/listener.rs @@ -0,0 +1,320 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! The listener: serves the built router over TLS or plaintext, applies the +//! per-connection request timeouts, sets `TCP_NODELAY` on accepted sockets, +//! redirects plain HTTP arriving on the TLS port, and handles shutdown. + +use std::path::PathBuf; +use std::time::Duration; + +use axum::Router; + +use crate::ServerTlsConfig; + +/// Serve `app` on `listener` until a shutdown signal arrives. +/// +/// When `tls` is `Some`, the server serves HTTPS using `axum-server` with +/// rustls. When `tls` is `None`, the server serves plaintext HTTP. +pub(crate) async fn serve_app( + listener: tokio::net::TcpListener, + app: Router, + request_timeout: Duration, + tls: Option, + pid_file: Option, +) -> Result<(), anyhow::Error> { + let local_addr = listener.local_addr()?; + + if let Some(tls_cfg) = tls { + // P57 Bug 2 fix: rustls 0.23 requires an explicit CryptoProvider. + // Install aws-lc-rs as the default before creating any TLS config. + // Ignore the error if a provider was already installed (e.g., by sqlx). + let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); + + tracing::info!("extenddb listening on {local_addr} (HTTPS)"); + let rustls_config = axum_server::tls_rustls::RustlsConfig::from_pem_file( + &tls_cfg.cert_path, + &tls_cfg.key_path, + ) + .await + // AI-3: The error intentionally does not include the file path to avoid + // leaking filesystem structure in logs. See docs/troubleshooting.md for + // the "Failed to load TLS certificates" entry. + .map_err(|e| anyhow::anyhow!("Failed to load TLS certificates: {e}"))?; + + let handle = axum_server::Handle::new(); + let shutdown_handle = handle.clone(); + let shutdown_pid = pid_file.clone(); + tokio::spawn(async move { + shutdown_signal().await; + shutdown_handle.graceful_shutdown(Some(Duration::from_secs(5))); + // P50: Log timeout but don't call std::process::exit — let the + // runtime shut down normally so destructors (including ZeroizeOnDrop) run. + tokio::time::sleep(Duration::from_secs(10)).await; + tracing::warn!("Graceful shutdown timed out, forcing PID file cleanup"); + cleanup_pid_file(shutdown_pid.as_deref()); + }); + + let std_listener = listener.into_std()?; + // AI-2: Use a custom acceptor that peeks the first byte of each + // connection. If it's plain HTTP (not 0x16 TLS ClientHello), write + // a 301 redirect to HTTPS and reject the connection before the TLS + // handshake. This gives users a helpful redirect instead of a + // confusing TLS handshake failure. The same acceptor sets + // TCP_NODELAY on every accepted socket before the handshake. + let redirect_acceptor = HttpsRedirectAcceptor { addr: local_addr }; + let mut server = axum_server::from_tcp_rustls(std_listener, rustls_config)? + .map(|tls| tls.acceptor(redirect_acceptor)) + .handle(handle); + // HTTP/1: a connection that has not finished sending its request + // headers within `request_timeout` is closed; the same bound applies + // to the idle wait for the next request on a keep-alive connection. + server + .http_builder() + .http1() + .timer(hyper_util::rt::TokioTimer::new()) + .header_read_timeout(request_timeout); + // HTTP/2: a connection that stops answering PING frames is closed. + // A client that sends the preface and then stalls holds the + // connection for at most interval plus timeout. + server + .http_builder() + .http2() + .timer(hyper_util::rt::TokioTimer::new()) + .keep_alive_interval(Some(request_timeout)) + .keep_alive_timeout(request_timeout); + server + .serve(app.into_make_service_with_connect_info::()) + .await?; + } else { + tracing::info!("extenddb listening on {local_addr}"); + axum::serve( + nodelay_listener(listener), + app.into_make_service_with_connect_info::(), + ) + .with_graceful_shutdown(graceful_shutdown(pid_file.clone())) + .await?; + } + + // Normal shutdown path — clean up PID file after connections drain. + cleanup_pid_file(pid_file.as_deref()); + + Ok(()) +} + +async fn graceful_shutdown(pid_file: Option) { + shutdown_signal().await; + // P50: Spawn a timeout task that cleans up the PID file if connections + // don't drain. Does NOT call std::process::exit — the runtime shuts down + // normally so destructors (including ZeroizeOnDrop) run. + tokio::spawn(async move { + tokio::time::sleep(Duration::from_secs(5)).await; + tracing::warn!("Graceful shutdown timed out after 5s, forcing PID file cleanup"); + cleanup_pid_file(pid_file.as_deref()); + }); +} + +/// Remove the PID file if it exists. Best-effort — log but don't fail. +fn cleanup_pid_file(pid_file: Option<&std::path::Path>) { + if let Some(path) = pid_file + && let Err(e) = std::fs::remove_file(path) + && e.kind() != std::io::ErrorKind::NotFound + { + tracing::warn!("Failed to remove PID file {}: {e}", path.display()); + } +} + +/// Set `TCP_NODELAY` on an accepted socket. +/// +/// With Nagle's algorithm on, the kernel holds a small write while an earlier +/// small write on the connection is unacknowledged, and the client's delayed +/// ACK leaves with its next request on that connection or when its timer +/// fires (about 40 ms on Linux). Once one response is written while the +/// previous one is unacknowledged, each response waits for the client's next +/// request, and the reply to that request is held behind it in turn. The +/// response latency then equals the gap between the client's requests on the +/// connection, for every gap under the delayed-ACK timer. `TCP_NODELAY` sends +/// each write as it is made. The option lives on the TCP socket, so under TLS +/// it is set before the handshake and applies below the TLS stream. A failure +/// to set it is logged at debug and the connection is served with the kernel +/// default. +fn set_tcp_nodelay(stream: &tokio::net::TcpStream) { + if let Err(e) = stream.set_nodelay(true) { + tracing::debug!("Failed to set TCP_NODELAY on accepted connection: {e}"); + } +} + +/// The plaintext listener: a `TcpListener` whose accepted sockets carry +/// `TCP_NODELAY`. +type NodelayListener = axum::serve::TapIo; + +/// Wrap the plaintext listener so every accepted socket carries `TCP_NODELAY`. +/// +/// `axum::serve` does not set the option itself; `ListenerExt::tap_io` is its +/// hook for per-connection socket options. +fn nodelay_listener(listener: tokio::net::TcpListener) -> NodelayListener { + use axum::serve::ListenerExt; + listener.tap_io(|stream| set_tcp_nodelay(stream)) +} + +/// AI-2: Acceptor that detects plain HTTP connections on the TLS port. +/// +/// Sets `TCP_NODELAY` on the accepted socket first (see [`set_tcp_nodelay`]). +/// Then peeks the first byte of the connection. If it's `0x16` (TLS +/// `ClientHello`), the connection passes through to the TLS acceptor. If it's +/// a plain HTTP verb, a 301 redirect to `https://` is written and the +/// connection is rejected with an IO error (which `axum_server` handles by +/// dropping it). +/// +/// `axum_server` 0.8 ships a `NoDelayAcceptor`, but it cannot be stacked under +/// this acceptor without a generic wrapper, so this acceptor sets the option +/// itself, and logs instead of rejecting on failure. +#[derive(Clone)] +struct HttpsRedirectAcceptor { + addr: std::net::SocketAddr, +} + +impl axum_server::accept::Accept + for HttpsRedirectAcceptor +{ + type Stream = tokio::net::TcpStream; + type Service = S; + type Future = std::pin::Pin< + Box< + dyn std::future::Future> + Send, + >, + >; + + fn accept(&self, stream: tokio::net::TcpStream, service: S) -> Self::Future { + let addr = self.addr; + Box::pin(async move { + set_tcp_nodelay(&stream); + let mut peek_buf = [0u8; 1]; + match stream.peek(&mut peek_buf).await { + Ok(0) => { + // Connection closed before sending data. + Err(std::io::Error::new( + std::io::ErrorKind::ConnectionReset, + "empty connection", + )) + } + Ok(_) if peek_buf[0] == 0x16 => { + // TLS ClientHello — pass through to TLS acceptor. + Ok((stream, service)) + } + Ok(_) => { + // Plain HTTP — send redirect and reject. + send_https_redirect(&stream, addr); + Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "plain HTTP redirected to HTTPS", + )) + } + Err(e) => Err(e), + } + }) + } +} + +/// Write an HTTP 301 redirect response to HTTPS on the raw TCP stream. +fn send_https_redirect(stream: &tokio::net::TcpStream, addr: std::net::SocketAddr) { + let location = format!("https://{addr}/"); + let response = format!( + "HTTP/1.1 301 Moved Permanently\r\n\ + Location: {location}\r\n\ + Content-Length: 0\r\n\ + Connection: close\r\n\r\n" + ); + // Best-effort write — if it fails, the client already got a connection error. + let _ = stream.try_write(response.as_bytes()); +} + +#[cfg(unix)] +async fn shutdown_signal() { + let ctrl_c = tokio::signal::ctrl_c(); + let Ok(mut sigterm) = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate()) + else { + tracing::error!("Failed to register SIGTERM handler, falling back to Ctrl+C only"); + let _ = ctrl_c.await; + tracing::info!("Shutdown signal received, draining connections..."); + return; + }; + tokio::select! { + _ = ctrl_c => {}, + _ = sigterm.recv() => {}, + } + tracing::info!("Shutdown signal received, draining connections..."); +} + +/// Non-unix: there is no SIGTERM; Ctrl+C (which tokio maps to the console +/// control handler on Windows) is the only shutdown signal — and it only +/// fires for an interactive console. Supervisors that terminate the process +/// directly (including the npm launcher's `stop()`, which is +/// `TerminateProcess` on Windows) hard-kill without reaching this path, so +/// the worker drain is skipped. That is acceptable: drain only cancels and +/// joins background workers — it flushes nothing, and storage commits +/// per-transaction. +#[cfg(not(unix))] +async fn shutdown_signal() { + let _ = tokio::signal::ctrl_c().await; + tracing::info!("Shutdown signal received, draining connections..."); +} + +#[cfg(test)] +mod tests { + use super::*; + use tokio::io::AsyncWriteExt; + use tokio::net::{TcpListener, TcpStream}; + + /// Bind a loopback listener and connect one client to it. + async fn bind_and_connect() -> (TcpListener, TcpStream) { + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("bind loopback"); + let addr = listener.local_addr().expect("local addr"); + let client = TcpStream::connect(addr).await.expect("connect"); + (listener, client) + } + + /// The TLS path: the acceptor that runs before the TLS handshake sets + /// `TCP_NODELAY` on the accepted socket, read back from the socket the + /// acceptor hands on. + #[tokio::test] + async fn tls_acceptor_sets_tcp_nodelay_before_the_handshake() { + use axum_server::accept::Accept; + + let (listener, mut client) = bind_and_connect().await; + // A TLS ClientHello starts with the handshake record type. + client + .write_all(&[0x16]) + .await + .expect("write ClientHello byte"); + let (accepted, _) = listener.accept().await.expect("accept"); + assert!( + !accepted.nodelay().expect("read TCP_NODELAY"), + "a fresh accepted socket has Nagle on" + ); + + let acceptor = HttpsRedirectAcceptor { + addr: listener.local_addr().expect("local addr"), + }; + let (stream, ()) = acceptor + .accept(accepted, ()) + .await + .expect("a TLS ClientHello passes through to the TLS acceptor"); + assert!(stream.nodelay().expect("read TCP_NODELAY")); + } + + /// The plaintext path: the listener handed to `axum::serve` sets + /// `TCP_NODELAY` on every socket it accepts. + #[tokio::test] + async fn plaintext_listener_sets_tcp_nodelay_on_accepted_sockets() { + use axum::serve::Listener; + + let (listener, client) = bind_and_connect().await; + let mut listener = nodelay_listener(listener); + let (accepted, peer) = listener.accept().await; + assert_eq!(peer, client.local_addr().expect("client addr")); + assert!(accepted.nodelay().expect("read TCP_NODELAY")); + } +} From c9326f284caf8c7d04ab92d16a016f4f664b33c2 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:50:13 +0000 Subject: [PATCH 03/10] fix(server): apply the request timeouts on the plaintext listener too 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". --- crates/server/src/listener.rs | 183 +++++++++++++++++++++------------- 1 file changed, 116 insertions(+), 67 deletions(-) diff --git a/crates/server/src/listener.rs b/crates/server/src/listener.rs index 0e56dcdca..7b932918e 100644 --- a/crates/server/src/listener.rs +++ b/crates/server/src/listener.rs @@ -15,7 +15,9 @@ use crate::ServerTlsConfig; /// Serve `app` on `listener` until a shutdown signal arrives. /// /// When `tls` is `Some`, the server serves HTTPS using `axum-server` with -/// rustls. When `tls` is `None`, the server serves plaintext HTTP. +/// rustls. When `tls` is `None`, the server serves plaintext HTTP. Both arms +/// run on the same `axum-server` builder, so the request timeouts, the +/// shutdown handle and `TCP_NODELAY` apply to every listener mode. pub(crate) async fn serve_app( listener: tokio::net::TcpListener, app: Router, @@ -25,6 +27,22 @@ pub(crate) async fn serve_app( ) -> Result<(), anyhow::Error> { let local_addr = listener.local_addr()?; + let handle = axum_server::Handle::new(); + let shutdown_handle = handle.clone(); + let shutdown_pid = pid_file.clone(); + tokio::spawn(async move { + shutdown_signal().await; + shutdown_handle.graceful_shutdown(Some(Duration::from_secs(5))); + // P50: Log timeout but don't call std::process::exit — let the + // runtime shut down normally so destructors (including ZeroizeOnDrop) run. + tokio::time::sleep(Duration::from_secs(10)).await; + tracing::warn!("Graceful shutdown timed out, forcing PID file cleanup"); + cleanup_pid_file(shutdown_pid.as_deref()); + }); + + let std_listener = listener.into_std()?; + let make_service = app.into_make_service_with_connect_info::(); + if let Some(tls_cfg) = tls { // P57 Bug 2 fix: rustls 0.23 requires an explicit CryptoProvider. // Install aws-lc-rs as the default before creating any TLS config. @@ -42,20 +60,6 @@ pub(crate) async fn serve_app( // the "Failed to load TLS certificates" entry. .map_err(|e| anyhow::anyhow!("Failed to load TLS certificates: {e}"))?; - let handle = axum_server::Handle::new(); - let shutdown_handle = handle.clone(); - let shutdown_pid = pid_file.clone(); - tokio::spawn(async move { - shutdown_signal().await; - shutdown_handle.graceful_shutdown(Some(Duration::from_secs(5))); - // P50: Log timeout but don't call std::process::exit — let the - // runtime shut down normally so destructors (including ZeroizeOnDrop) run. - tokio::time::sleep(Duration::from_secs(10)).await; - tracing::warn!("Graceful shutdown timed out, forcing PID file cleanup"); - cleanup_pid_file(shutdown_pid.as_deref()); - }); - - let std_listener = listener.into_std()?; // AI-2: Use a custom acceptor that peeks the first byte of each // connection. If it's plain HTTP (not 0x16 TLS ClientHello), write // a 301 redirect to HTTPS and reject the connection before the TLS @@ -63,37 +67,20 @@ pub(crate) async fn serve_app( // confusing TLS handshake failure. The same acceptor sets // TCP_NODELAY on every accepted socket before the handshake. let redirect_acceptor = HttpsRedirectAcceptor { addr: local_addr }; - let mut server = axum_server::from_tcp_rustls(std_listener, rustls_config)? + let server = axum_server::from_tcp_rustls(std_listener, rustls_config)? .map(|tls| tls.acceptor(redirect_acceptor)) .handle(handle); - // HTTP/1: a connection that has not finished sending its request - // headers within `request_timeout` is closed; the same bound applies - // to the idle wait for the next request on a keep-alive connection. - server - .http_builder() - .http1() - .timer(hyper_util::rt::TokioTimer::new()) - .header_read_timeout(request_timeout); - // HTTP/2: a connection that stops answering PING frames is closed. - // A client that sends the preface and then stalls holds the - // connection for at most interval plus timeout. - server - .http_builder() - .http2() - .timer(hyper_util::rt::TokioTimer::new()) - .keep_alive_interval(Some(request_timeout)) - .keep_alive_timeout(request_timeout); - server - .serve(app.into_make_service_with_connect_info::()) + with_request_timeouts(server, request_timeout) + .serve(make_service) .await?; } else { tracing::info!("extenddb listening on {local_addr}"); - axum::serve( - nodelay_listener(listener), - app.into_make_service_with_connect_info::(), - ) - .with_graceful_shutdown(graceful_shutdown(pid_file.clone())) - .await?; + let server = axum_server::from_tcp(std_listener)? + .acceptor(NodelayAcceptor) + .handle(handle); + with_request_timeouts(server, request_timeout) + .serve(make_service) + .await?; } // Normal shutdown path — clean up PID file after connections drain. @@ -102,16 +89,32 @@ pub(crate) async fn serve_app( Ok(()) } -async fn graceful_shutdown(pid_file: Option) { - shutdown_signal().await; - // P50: Spawn a timeout task that cleans up the PID file if connections - // don't drain. Does NOT call std::process::exit — the runtime shuts down - // normally so destructors (including ZeroizeOnDrop) run. - tokio::spawn(async move { - tokio::time::sleep(Duration::from_secs(5)).await; - tracing::warn!("Graceful shutdown timed out after 5s, forcing PID file cleanup"); - cleanup_pid_file(pid_file.as_deref()); - }); +/// Apply the per-connection request timeouts to a server, in either +/// listener mode. +/// +/// HTTP/1: a connection that has not finished sending its request headers +/// within `request_timeout` is closed; the same bound applies to the idle +/// wait for the next request on a keep-alive connection. +/// +/// HTTP/2: a connection that stops answering PING frames is closed. A client +/// that sends the preface and then stalls holds the connection for at most +/// interval plus timeout. +fn with_request_timeouts( + mut server: axum_server::Server, + request_timeout: Duration, +) -> axum_server::Server { + server + .http_builder() + .http1() + .timer(hyper_util::rt::TokioTimer::new()) + .header_read_timeout(request_timeout); + server + .http_builder() + .http2() + .timer(hyper_util::rt::TokioTimer::new()) + .keep_alive_interval(Some(request_timeout)) + .keep_alive_timeout(request_timeout); + server } /// Remove the PID file if it exists. Best-effort — log but don't fail. @@ -144,17 +147,24 @@ fn set_tcp_nodelay(stream: &tokio::net::TcpStream) { } } -/// The plaintext listener: a `TcpListener` whose accepted sockets carry -/// `TCP_NODELAY`. -type NodelayListener = axum::serve::TapIo; - -/// Wrap the plaintext listener so every accepted socket carries `TCP_NODELAY`. +/// The plaintext acceptor: sets `TCP_NODELAY` on every accepted socket (see +/// [`set_tcp_nodelay`]) and hands it on unchanged. /// -/// `axum::serve` does not set the option itself; `ListenerExt::tap_io` is its -/// hook for per-connection socket options. -fn nodelay_listener(listener: tokio::net::TcpListener) -> NodelayListener { - use axum::serve::ListenerExt; - listener.tap_io(|stream| set_tcp_nodelay(stream)) +/// `axum_server` 0.8 ships a `NoDelayAcceptor`, but it rejects the +/// connection when the option cannot be set; this one logs and serves it, +/// as the TLS acceptor does. +#[derive(Clone)] +struct NodelayAcceptor; + +impl axum_server::accept::Accept for NodelayAcceptor { + type Stream = tokio::net::TcpStream; + type Service = S; + type Future = std::future::Ready>; + + fn accept(&self, stream: tokio::net::TcpStream, service: S) -> Self::Future { + set_tcp_nodelay(&stream); + std::future::ready(Ok((stream, service))) + } } /// AI-2: Acceptor that detects plain HTTP connections on the TLS port. @@ -276,6 +286,38 @@ mod tests { (listener, client) } + /// The plaintext path: a connection that sends part of its request + /// headers and then stalls is closed once `request_timeout` passes, + /// as it is on the TLS path. The client reads EOF (or a reset) well + /// before a bound of several times the timeout; without the timer the + /// read blocks until that bound. + #[tokio::test] + async fn plaintext_listener_closes_a_stalled_header_phase() { + use tokio::io::AsyncReadExt; + + let timeout = Duration::from_millis(300); + let (listener, mut client) = bind_and_connect().await; + let app = Router::new().route("/", axum::routing::get(|| async { "ok" })); + let server = tokio::spawn(serve_app(listener, app, timeout, None, None)); + + // A request line and one header, no terminating blank line. + client + .write_all(b"GET / HTTP/1.1\r\nHost: localhost\r\n") + .await + .expect("write partial headers"); + let mut buf = [0u8; 64]; + let outcome = tokio::time::timeout(timeout * 10, client.read(&mut buf)).await; + server.abort(); + match outcome { + Ok(Ok(0)) | Ok(Err(_)) => {} + Ok(Ok(n)) => panic!("server answered {n} bytes instead of closing the connection"), + Err(_) => panic!( + "server held a stalled header phase open for {:?} on the plaintext listener", + timeout * 10 + ), + } + } + /// The TLS path: the acceptor that runs before the TLS handshake sets /// `TCP_NODELAY` on the accepted socket, read back from the socket the /// acceptor hands on. @@ -305,16 +347,23 @@ mod tests { assert!(stream.nodelay().expect("read TCP_NODELAY")); } - /// The plaintext path: the listener handed to `axum::serve` sets - /// `TCP_NODELAY` on every socket it accepts. + /// The plaintext path: the acceptor sets `TCP_NODELAY` on every socket + /// it accepts. #[tokio::test] async fn plaintext_listener_sets_tcp_nodelay_on_accepted_sockets() { - use axum::serve::Listener; + use axum_server::accept::Accept; let (listener, client) = bind_and_connect().await; - let mut listener = nodelay_listener(listener); - let (accepted, peer) = listener.accept().await; + let (accepted, peer) = listener.accept().await.expect("accept"); assert_eq!(peer, client.local_addr().expect("client addr")); - assert!(accepted.nodelay().expect("read TCP_NODELAY")); + assert!( + !accepted.nodelay().expect("read TCP_NODELAY"), + "a fresh accepted socket has Nagle on" + ); + let (stream, ()) = NodelayAcceptor + .accept(accepted, ()) + .await + .expect("the plaintext acceptor hands the socket on"); + assert!(stream.nodelay().expect("read TCP_NODELAY")); } } From b6703e75e46dd54e2a292052c2b289f0a45ab4e9 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:50:13 +0000 Subject: [PATCH 04/10] fix(server): refuse a repeated host or authorization header on every 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. --- crates/server/src/handler.rs | 16 +-- crates/server/src/lib.rs | 7 +- crates/server/src/repeated_headers.rs | 155 ++++++++++++++++++++++++++ tests/test_auth_error_fidelity.py | 91 +++++++++++++++ 4 files changed, 254 insertions(+), 15 deletions(-) create mode 100644 crates/server/src/repeated_headers.rs diff --git a/crates/server/src/handler.rs b/crates/server/src/handler.rs index 1e08c2402..23ee1c788 100755 --- a/crates/server/src/handler.rs +++ b/crates/server/src/handler.rs @@ -48,20 +48,8 @@ pub(crate) async fn handle_request( } let request_id = uuid::Uuid::new_v4().to_string(); - // A request carrying more than one `host` or `authorization` header is - // refused outright, as the service's front end does (400, connection - // closed). Either duplicate is a header-confusion shape a proxy in front - // of this server could resolve differently from the signature check. - if headers.get_all("host").iter().count() > 1 - || headers.get_all("authorization").iter().count() > 1 - { - return ( - axum::http::StatusCode::BAD_REQUEST, - [(axum::http::header::CONNECTION, "close")], - "400 Bad Request", - ) - .into_response(); - } + // A repeated `host` or `authorization` header was refused before routing + // (see `repeated_headers`), so each is read as one value from here on. // SP-WIRE-007: body limit exceeded → 413. Any other failure to read the // body is the body timeout (the client stopped sending) → 408. diff --git a/crates/server/src/lib.rs b/crates/server/src/lib.rs index c57bec22c..800ac07d0 100644 --- a/crates/server/src/lib.rs +++ b/crates/server/src/lib.rs @@ -21,6 +21,7 @@ mod listener; pub mod management; mod metrics_endpoint; pub mod rate_limit; +mod repeated_headers; mod request_helpers; mod request_tags; mod response; @@ -209,11 +210,15 @@ pub async fn start_server( ); } - // Apply security headers to all routes. + // Refuse a repeated host or authorization header on every route, then + // apply security headers to all routes (the refusal carries them too). // A body that stops delivering data for `request_timeout` errors out, so a // client that announces a Content-Length it never sends cannot hold the // connection open. The handler answers that error with 408. let app = app + .layer(axum::middleware::from_fn( + repeated_headers::refuse_repeated_headers, + )) .layer(security_layers) .layer(RequestBodyTimeoutLayer::new(request_timeout)); diff --git a/crates/server/src/repeated_headers.rs b/crates/server/src/repeated_headers.rs new file mode 100644 index 000000000..e803ff32c --- /dev/null +++ b/crates/server/src/repeated_headers.rs @@ -0,0 +1,155 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Refuse a request that repeats a header the server reads as one value. +//! +//! A request carrying more than one `host` or `authorization` header is +//! refused before routing, as the service's front end does (400, connection +//! closed, no request id). Either duplicate is a header-confusion shape a +//! proxy in front of this server could resolve differently from the server: +//! the SigV4 check binds the host it signs, the console binds a session to a +//! host, and the management API reads one Basic credential. Applied as a +//! layer on the whole router so the DynamoDB route, `/console`, +//! `/management`, `/health` and `/metrics` all answer the same way. + +use axum::extract::Request; +use axum::http::{HeaderMap, StatusCode, header}; +use axum::middleware::Next; +use axum::response::{IntoResponse, Response}; + +/// Whether `headers` carries more than one `host` or `authorization` value. +fn has_repeated_host_or_authorization(headers: &HeaderMap) -> bool { + headers.get_all(header::HOST).iter().count() > 1 + || headers.get_all(header::AUTHORIZATION).iter().count() > 1 +} + +/// The refusal: 400 with the connection closed and a plain text body. +fn refusal() -> Response { + ( + StatusCode::BAD_REQUEST, + [(header::CONNECTION, "close")], + "400 Bad Request", + ) + .into_response() +} + +/// Middleware: refuse a request with a repeated `host` or `authorization` +/// header; pass every other request on. +pub(crate) async fn refuse_repeated_headers(request: Request, next: Next) -> Response { + if has_repeated_host_or_authorization(request.headers()) { + return refusal(); + } + next.run(request).await +} + +#[cfg(test)] +mod tests { + use super::*; + use axum::Router; + use axum::body::Body; + use axum::routing::{get, post}; + use tower::ServiceExt; + + /// A router shaped like the server's: a data-plane route, a nested + /// console and a nested management API, with the guard layered on the + /// whole thing after nesting. + fn router() -> Router { + let console = Router::new().route("/login", get(|| async { "login" })); + let management = Router::new().route("/accounts", get(|| async { "accounts" })); + Router::new() + .route("/", post(|| async { "data plane" })) + .route("/health", get(|| async { "healthy" })) + .nest("/console", console) + .nest("/management", management) + .layer(axum::middleware::from_fn(refuse_repeated_headers)) + } + + async fn send( + headers: &[(&str, &str)], + method: &str, + path: &str, + ) -> (StatusCode, Option, String) { + let mut builder = axum::http::Request::builder().method(method).uri(path); + for (name, value) in headers { + builder = builder.header(*name, *value); + } + let response = router() + .oneshot(builder.body(Body::empty()).expect("request")) + .await + .expect("response"); + let status = response.status(); + let connection = response + .headers() + .get(header::CONNECTION) + .map(|v| v.to_str().expect("ascii").to_owned()); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .expect("body"); + ( + status, + connection, + String::from_utf8(body.to_vec()).expect("utf8"), + ) + } + + #[tokio::test] + async fn a_repeated_host_is_refused_on_every_route() { + for (method, path) in [ + ("POST", "/"), + ("GET", "/health"), + ("GET", "/console/login"), + ("GET", "/management/accounts"), + ("GET", "/no-such-route"), + ] { + let (status, connection, body) = send( + &[("host", "a.example"), ("host", "b.example")], + method, + path, + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "{method} {path}"); + assert_eq!(connection.as_deref(), Some("close"), "{method} {path}"); + assert_eq!(body, "400 Bad Request", "{method} {path}"); + } + } + + #[tokio::test] + async fn a_repeated_authorization_is_refused_on_every_route() { + for (method, path) in [ + ("POST", "/"), + ("GET", "/console/login"), + ("GET", "/management/accounts"), + ] { + let (status, connection, body) = send( + &[ + ("host", "a.example"), + ("authorization", "Basic YWRtaW46eA=="), + ("authorization", "Basic YWRtaW46eQ=="), + ], + method, + path, + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "{method} {path}"); + assert_eq!(connection.as_deref(), Some("close"), "{method} {path}"); + assert_eq!(body, "400 Bad Request", "{method} {path}"); + } + } + + #[tokio::test] + async fn single_headers_and_repeated_other_headers_pass() { + let (status, _, body) = send( + &[ + ("host", "a.example"), + ("authorization", "Basic YWRtaW46eA=="), + ("x-amz-date", "20260101T000000Z"), + ("x-amz-date", "20260102T000000Z"), + ], + "GET", + "/console/login", + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(body, "login"); + } +} diff --git a/tests/test_auth_error_fidelity.py b/tests/test_auth_error_fidelity.py index bb3de7126..b122d8b9e 100755 --- a/tests/test_auth_error_fidelity.py +++ b/tests/test_auth_error_fidelity.py @@ -20,6 +20,7 @@ import json import os from typing import Any +from urllib.parse import urlparse import boto3 import pytest @@ -354,6 +355,96 @@ def test_duplicate_host_header_is_refused(self): conn.close() assert resp.status == 400 + def _raw_request(self, method: str, path: str, headers: list[tuple[str, str]], body: bytes = b""): + """Send one request with each header on its own line, repeats included. + + Returns (status, body text, Connection header). http.client sends the + headers in the order given, so a repeated name goes out twice. + """ + import http.client # noqa: PLC0415 + import ssl # noqa: PLC0415 + from urllib.parse import urlparse # noqa: PLC0415 + + u = urlparse(self.url) + ctx = ssl.create_default_context() + if self.endpoint_url is not None: + ctx.check_hostname = False + ctx.verify_mode = ssl.CERT_NONE + conn = http.client.HTTPSConnection(u.hostname, u.port or 443, context=ctx, timeout=30) + conn.putrequest(method, path, skip_host=True, skip_accept_encoding=True) + for k, v in headers: + conn.putheader(k, v) + if body or method == "POST": + conn.putheader("Content-Length", str(len(body))) + conn.endheaders() + if body: + conn.send(body) + resp = conn.getresponse() + text = resp.read().decode(errors="replace") + connection = resp.getheader("Connection") + conn.close() + return resp.status, text, connection + + @pytest.mark.parametrize("path", ["/console/login", "/management/accounts", "/health"]) + def test_duplicate_host_header_is_refused_on_every_route(self, path): + # The refusal is a property of the listener, not of the DynamoDB + # route: the console, the management API and the health check get + # the same 400 with the connection closed. + if self.endpoint_url is None: + pytest.skip("console and management routes exist on extenddb only") + u = urlparse(self.url) + status, text, connection = self._raw_request( + "GET", path, [("Host", u.netloc), ("Host", "evil.example.com")] + ) + assert status == 400, (path, status, text[:200]) + assert text == "400 Bad Request", (path, text[:200]) + assert connection == "close", (path, connection) + + @pytest.mark.parametrize("path", ["/console/login", "/management/accounts", "/health"]) + def test_duplicate_authorization_header_is_refused_on_every_route(self, path): + if self.endpoint_url is None: + pytest.skip("console and management routes exist on extenddb only") + u = urlparse(self.url) + status, text, connection = self._raw_request( + "GET", + path, + [ + ("Host", u.netloc), + ("Authorization", "Basic YWRtaW46eA=="), + ("Authorization", "Basic YWRtaW46eQ=="), + ], + ) + assert status == 400, (path, status, text[:200]) + assert text == "400 Bad Request", (path, text[:200]) + assert connection == "close", (path, connection) + + def test_duplicate_authorization_header_is_refused_on_the_data_plane(self): + # Measured against the service on 2026-10-23: two Authorization + # headers get the front end's 400 Bad Request with the connection + # closed, before authentication runs. + u = urlparse(self.url) + auth = ( + "AWS4-HMAC-SHA256 Credential=AKIAEXAMPLEEXAMPLE01/20260101/us-east-1/dynamodb/aws4_request, " + "SignedHeaders=host;x-amz-date;x-amz-target, Signature=" + "0" * 64 + ) + status, text, connection = self._raw_request( + "POST", + "/", + [ + ("Host", u.netloc), + ("X-Amz-Target", "DynamoDB_20120810.ListTables"), + ("Content-Type", "application/x-amz-json-1.0"), + ("X-Amz-Date", "20260101T000000Z"), + ("Authorization", auth), + ("Authorization", auth), + ], + body=b"{}", + ) + assert status == 400, (status, text[:200]) + assert connection == "close", connection + if self.endpoint_url is not None: + assert text == "400 Bad Request", text[:200] + def test_session_token_with_a_long_term_key_is_rejected(self): _, _, env_token = _env_credentials() if env_token: From 590ada7e0bb1c32ec85e9a3e280cdedd6c4aa7ca Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:50:13 +0000 Subject: [PATCH 05/10] test(auth): cover a signed header repeated after signing 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. --- tests/test_auth_error_fidelity.py | 33 +++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/test_auth_error_fidelity.py b/tests/test_auth_error_fidelity.py index b122d8b9e..b3a252534 100755 --- a/tests/test_auth_error_fidelity.py +++ b/tests/test_auth_error_fidelity.py @@ -495,6 +495,39 @@ def test_repeated_signed_header_is_canonicalized_comma_joined(self): conn.close() assert resp.status == 200, text + @pytest.mark.parametrize( + "name,second_value", + [("X-Amz-Date", "20260101T000000Z"), ("X-Amz-Target", "DynamoDB_20120810.ListTables")], + ) + def test_signed_header_repeated_after_signing_is_a_signature_mismatch(self, name, second_value): + # Measured against the service on 2026-10-23: a 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; the canonical headers join both values, so the + # signature no longer matches. The second X-Amz-Target value is the + # same string as the first. + access_key, secret_key, token = _env_credentials() + body_bytes = json.dumps({"Limit": 1}).encode("utf-8") + aws_req = AWSRequest( + method="POST", + url=self.url, + data=body_bytes, + headers={ + "X-Amz-Target": "DynamoDB_20120810.ListTables", + "Content-Type": "application/x-amz-json-1.0", + }, + ) + SigV4Auth(Credentials(access_key, secret_key, token), "dynamodb", self.region).add_auth(aws_req) + u = urlparse(self.url) + headers = [("Host", u.netloc)] + headers += [(k, v) for k, v in aws_req.headers.items() if k.lower() != "host"] + headers.append((name, second_value)) + status, text, _ = self._raw_request("POST", "/", headers, body=body_bytes) + assert status == 400, (status, text[:300]) + payload = json.loads(text) + assert payload["__type"].endswith("InvalidSignatureException"), text + assert payload["message"].startswith("The request signature we calculated does not match"), text + def test_unsigned_payload_literal_is_a_plain_signature_mismatch(self): # A client that signs the payload line as the S3 literal fails with the # ordinary mismatch: the service does not special-case the literal. From 942c0bf3467740813a067d1f719f1f277398fee6 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:50:13 +0000 Subject: [PATCH 06/10] fix(auth): check the timestamp and the credential scope before the key 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. --- crates/auth/src/lib.rs | 334 ++------------------------ crates/auth/src/tests.rs | 380 ++++++++++++++++++++++++++++++ tests/test_auth_error_fidelity.py | 21 ++ 3 files changed, 423 insertions(+), 312 deletions(-) mode change 100755 => 100644 crates/auth/src/lib.rs create mode 100644 crates/auth/src/tests.rs diff --git a/crates/auth/src/lib.rs b/crates/auth/src/lib.rs old mode 100755 new mode 100644 index e64be6766..a67c4de41 --- a/crates/auth/src/lib.rs +++ b/crates/auth/src/lib.rs @@ -104,8 +104,9 @@ pub trait CredentialStore: Send + Sync { /// `SigV4` auth provider with local credential store. /// -/// Parses the `Authorization` header, looks up the access key, decrypts the -/// secret, verifies the `SigV4` signature, and validates the request timestamp. +/// Parses the `Authorization` header, validates the request timestamp, checks +/// the credential scope, looks up the access key, decrypts the secret and +/// verifies the `SigV4` signature, in that order (the service's order). /// Handles both long-lived (AKIA*) and temporary (ASIA* + X-Amz-Security-Token) /// credentials. The credential scope must name this server's region. pub struct BuiltinAuthProvider { @@ -144,8 +145,22 @@ impl AuthProvider for BuiltinAuthProvider { // Parse Authorization header let parsed = sigv4::parse::parse_authorization(auth_header)?; - // Look up credential before timestamp validation — real DynamoDB returns - // an invalid-key error even when the timestamp is also expired. + // The service checks the request timestamp, then the credential + // scope, then looks the key up (measured 2026-10-23: an unknown key + // with a stale date answers the timestamp error, an unknown key with + // a wrong region answers the scope message, and a stale date with a + // wrong region answers the timestamp error). Neither check reads the + // stored credential, so running them first leaves the S-5 timing + // argument below unchanged. + // + // Validate timestamp (±15 minute window). + sigv4::verify::validate_timestamp(headers)?; + + // The credential scope must name this server's region and the + // dynamodb service; the service reports both complaints in one + // message when both are wrong. + sigv4::verify::check_scope(&parsed, &self.region)?; + let credential = self .credential_store .lookup_credential(&parsed.access_key_id) @@ -157,19 +172,10 @@ impl AuthProvider for BuiltinAuthProvider { })?; // S-5: Track inactive status but do NOT return early. Continue through - // timestamp validation and signature verification to ensure constant-time - // failure paths — no timing difference between inactive, invalid, and - // absent keys. + // signature verification to ensure constant-time failure paths — no + // timing difference between inactive, invalid, and absent keys. let is_inactive = !credential.is_active; - // Validate timestamp (±15 minute window) after credential lookup. - sigv4::verify::validate_timestamp(headers)?; - - // The credential scope must name this server's region and the - // dynamodb service; the service reports both complaints in one - // message when both are wrong. - sigv4::verify::check_scope(&parsed, &self.region)?; - // Session tokens. For a session credential the presented token must // match the stored one. For a long-term key no token may be presented: // the service validates any presented token and a long-term key has @@ -238,300 +244,4 @@ impl AuthProvider for BuiltinAuthProvider { } #[cfg(test)] -mod tests { - use super::*; - use axum::http::{HeaderMap, HeaderValue}; - - /// In-memory credential store for unit tests. - struct MockCredentialStore { - /// `Some(credential)` for found credentials, `None` for not found. - credential: Option, - /// If set, `lookup_credential` returns this error instead. - error: Option<&'static str>, - } - - #[async_trait::async_trait] - impl CredentialStore for MockCredentialStore { - async fn lookup_credential( - &self, - _access_key_id: &str, - ) -> Result, DynamoDbError> { - if let Some(msg) = self.error { - return Err(DynamoDbError::ExpiredTokenException(msg.to_owned())); - } - Ok(self.credential.clone()) - } - } - - fn make_headers_with_auth(access_key: &str, token: Option<&str>) -> HeaderMap { - let mut headers = HeaderMap::new(); - // Use current time for the date to pass timestamp validation. - let now = time::OffsetDateTime::now_utc(); - let date_str = format!( - "{:04}{:02}{:02}T{:02}{:02}{:02}Z", - now.year(), - now.month() as u8, - now.day(), - now.hour(), - now.minute(), - now.second(), - ); - let date_short = &date_str[..8]; - // Signature is fake — these tests exercise error paths (expired token, - // unknown key) that trigger before signature verification in - // authenticate(). - let auth = format!( - "AWS4-HMAC-SHA256 Credential={}/{}/us-east-1/dynamodb/aws4_request, \ - SignedHeaders=content-type;host;x-amz-date;x-amz-target, \ - Signature=0000000000000000000000000000000000000000000000000000000000000000", - access_key, date_short - ); - headers.insert("authorization", HeaderValue::from_str(&auth).unwrap()); - headers.insert("x-amz-date", HeaderValue::from_str(&date_str).unwrap()); - headers.insert("host", HeaderValue::from_static("localhost:18443")); - headers.insert( - "content-type", - HeaderValue::from_static("application/x-amz-json-1.0"), - ); - headers.insert( - "x-amz-target", - HeaderValue::from_static("DynamoDB_20120810.ListTables"), - ); - if let Some(t) = token { - headers.insert("x-amz-security-token", HeaderValue::from_str(t).unwrap()); - } - headers - } - - #[tokio::test] - async fn expired_session_returns_expired_token_exception() { - // CB-12: The credential store returns ExpiredTokenException directly - // for expired sessions (fail-closed). The auth layer propagates it. - let store = MockCredentialStore { - credential: None, - error: Some("The security token included in the request is expired"), - }; - let provider = BuiltinAuthProvider::new(store, "us-east-1"); - let headers = make_headers_with_auth("ASIAEXTENDDB00000000", Some("test-token-value")); - - let result = provider.authenticate(&headers, b"{}").await; - match result { - Err(DynamoDbError::ExpiredTokenException(msg)) => { - assert!( - msg.contains("expired"), - "Expected 'expired' in message: {msg}" - ); - } - other => panic!("Expected ExpiredTokenException, got: {other:?}"), - } - } - - #[tokio::test] - async fn nonexistent_key_returns_unrecognized_client() { - let store = MockCredentialStore { - credential: None, - error: None, - }; - let provider = BuiltinAuthProvider::new(store, "us-east-1"); - let headers = make_headers_with_auth("AKIAXXXXXXXXXXXXXXXX", None); - - let result = provider.authenticate(&headers, b"{}").await; - match result { - Err(DynamoDbError::UnrecognizedClientException(msg)) => { - assert!( - msg.contains("invalid"), - "Expected 'invalid' in message: {msg}" - ); - } - other => panic!("Expected UnrecognizedClientException, got: {other:?}"), - } - } - - const SECRET: &str = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"; - - fn credential(is_session: bool) -> StoredCredential { - StoredCredential { - secret_key: SECRET.to_owned(), - account_id: "123456789012".to_owned(), - principal_name: "alice".to_owned(), - session_name: is_session.then(|| "sess".to_owned()), - is_session, - session_token: is_session.then(|| "stored-token".to_owned()), - is_active: true, - expires_at: None, - } - } - - /// Headers for a request correctly signed by a client whose credential - /// scope names `region`. `token`, when present, is sent and signed. - fn signed_headers( - access_key: &str, - region: &str, - token: Option<&str>, - body: &[u8], - ) -> HeaderMap { - let mut headers = make_headers_with_auth(access_key, token); - headers.remove("authorization"); - let date_str = headers["x-amz-date"].to_str().unwrap().to_owned(); - let date_short = date_str[..8].to_owned(); - let mut names = vec!["content-type", "host", "x-amz-date", "x-amz-target"]; - if token.is_some() { - names.push("x-amz-security-token"); - } - let signed = names.join(";"); - let creq = sigv4::canonical::canonical_request("POST", "/", "", &headers, &signed, body); - let scope = format!("{date_short}/{region}/dynamodb/aws4_request"); - let sts = sigv4::canonical::string_to_sign(&date_str, &scope, &creq); - let key = sigv4::signing_key::derive_signing_key(SECRET, &date_short, region, "dynamodb"); - let sig = sigv4::signing_key::compute_signature(&key, &sts); - let auth = format!( - "AWS4-HMAC-SHA256 Credential={access_key}/{scope}, SignedHeaders={signed}, Signature={sig}" - ); - headers.insert("authorization", HeaderValue::from_str(&auth).unwrap()); - headers - } - - fn provider(is_session: bool) -> BuiltinAuthProvider { - BuiltinAuthProvider::new( - MockCredentialStore { - credential: Some(credential(is_session)), - error: None, - }, - "us-east-1", - ) - } - - #[tokio::test] - async fn correctly_signed_long_term_key_authenticates() { - let headers = signed_headers("AKIAEXTENDDB00000000", "us-east-1", None, b"{}"); - let id = provider(false).authenticate(&headers, b"{}").await.unwrap(); - assert!(matches!(id, AuthIdentity::User { .. })); - } - - /// Measured 2026-09-16: a scope for another region is refused with this - /// exact message; the region is not named and the message ends with a space. - #[tokio::test] - async fn scope_for_another_region_is_rejected() { - let headers = signed_headers("AKIAEXTENDDB00000000", "us-west-2", None, b"{}"); - match provider(false).authenticate(&headers, b"{}").await { - Err(DynamoDbError::InvalidSignatureException(msg)) => { - assert_eq!(msg, "Credential should be scoped to a valid region. "); - } - other => panic!("Expected InvalidSignatureException, got: {other:?}"), - } - } - - /// A long-term key has no session token; presenting one is an invalid token. - #[tokio::test] - async fn session_token_with_a_long_term_key_is_rejected() { - let headers = signed_headers( - "AKIAEXTENDDB00000000", - "us-east-1", - Some("any-token"), - b"{}", - ); - match provider(false).authenticate(&headers, b"{}").await { - Err(DynamoDbError::UnrecognizedClientException(msg)) => { - assert_eq!(msg, "The security token included in the request is invalid"); - } - other => panic!("Expected UnrecognizedClientException, got: {other:?}"), - } - } - - #[tokio::test] - async fn session_key_with_matching_token_authenticates() { - let headers = signed_headers( - "ASIAEXTENDDB00000000", - "us-east-1", - Some("stored-token"), - b"{}", - ); - let id = provider(true).authenticate(&headers, b"{}").await.unwrap(); - assert!(matches!(id, AuthIdentity::RoleSession { .. })); - } - - /// Measured 2026-09-16: a presented token that does not resolve has no - /// trailing period; a missing token does. - #[tokio::test] - async fn session_key_token_messages_match_the_service() { - let wrong = signed_headers("ASIAEXTENDDB00000000", "us-east-1", Some("bogus"), b"{}"); - match provider(true).authenticate(&wrong, b"{}").await { - Err(DynamoDbError::UnrecognizedClientException(msg)) => { - assert_eq!(msg, "The security token included in the request is invalid"); - } - other => panic!("Expected UnrecognizedClientException, got: {other:?}"), - } - let missing = signed_headers("ASIAEXTENDDB00000000", "us-east-1", None, b"{}"); - match provider(true).authenticate(&missing, b"{}").await { - Err(DynamoDbError::UnrecognizedClientException(msg)) => { - assert_eq!( - msg, - "The security token included in the request is invalid." - ); - } - other => panic!("Expected UnrecognizedClientException, got: {other:?}"), - } - } - - /// The scope region is compared byte for byte: region names are lowercase - /// and the service does not fold case either. - #[tokio::test] - async fn scope_region_comparison_is_exact() { - for other in ["US-EAST-1", "us-east-1 ", ""] { - let headers = signed_headers("AKIAEXTENDDB00000000", other, None, b"{}"); - match provider(false).authenticate(&headers, b"{}").await { - Err(DynamoDbError::InvalidSignatureException(msg)) => { - assert_eq!( - msg, "Credential should be scoped to a valid region. ", - "{other:?}" - ); - } - other_result => { - panic!("{other:?}: expected InvalidSignatureException, got {other_result:?}") - } - } - } - } - - /// An empty token header on a long-term key is a presented token, and is - /// rejected like any other; only an absent header passes. - #[tokio::test] - async fn empty_session_token_header_with_a_long_term_key_is_rejected() { - let headers = signed_headers("AKIAEXTENDDB00000000", "us-east-1", Some(""), b"{}"); - match provider(false).authenticate(&headers, b"{}").await { - Err(DynamoDbError::UnrecognizedClientException(msg)) => { - assert_eq!(msg, "The security token included in the request is invalid"); - } - other => panic!("Expected UnrecognizedClientException, got: {other:?}"), - } - } - - /// Measured 2026-09-16: a scope wrong in both region and service reports - /// both sentences in one message, region first. - #[tokio::test] - async fn scope_wrong_in_region_and_service_reports_both() { - let mut headers = make_headers_with_auth("AKIAEXTENDDB00000000", None); - headers.remove("authorization"); - let date_str = headers["x-amz-date"].to_str().unwrap().to_owned(); - let date_short = date_str[..8].to_owned(); - let signed = "content-type;host;x-amz-date;x-amz-target"; - let creq = sigv4::canonical::canonical_request("POST", "/", "", &headers, signed, b"{}"); - let scope = format!("{date_short}/us-west-2/s3/aws4_request"); - let sts = sigv4::canonical::string_to_sign(&date_str, &scope, &creq); - let key = sigv4::signing_key::derive_signing_key(SECRET, &date_short, "us-west-2", "s3"); - let sig = sigv4::signing_key::compute_signature(&key, &sts); - let auth = format!( - "AWS4-HMAC-SHA256 Credential=AKIAEXTENDDB00000000/{scope}, SignedHeaders={signed}, Signature={sig}" - ); - headers.insert("authorization", HeaderValue::from_str(&auth).unwrap()); - match provider(false).authenticate(&headers, b"{}").await { - Err(DynamoDbError::InvalidSignatureException(msg)) => { - assert_eq!( - msg, - "Credential should be scoped to a valid region. Credential should be scoped to correct service: 'dynamodb'. " - ); - } - other => panic!("Expected InvalidSignatureException, got: {other:?}"), - } - } -} +mod tests; diff --git a/crates/auth/src/tests.rs b/crates/auth/src/tests.rs new file mode 100644 index 000000000..11e5248ef --- /dev/null +++ b/crates/auth/src/tests.rs @@ -0,0 +1,380 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Unit tests for `BuiltinAuthProvider::authenticate`. + +use super::*; +use axum::http::{HeaderMap, HeaderValue}; + +/// In-memory credential store for unit tests. +struct MockCredentialStore { + /// `Some(credential)` for found credentials, `None` for not found. + credential: Option, + /// If set, `lookup_credential` returns this error instead. + error: Option<&'static str>, +} + +#[async_trait::async_trait] +impl CredentialStore for MockCredentialStore { + async fn lookup_credential( + &self, + _access_key_id: &str, + ) -> Result, DynamoDbError> { + if let Some(msg) = self.error { + return Err(DynamoDbError::ExpiredTokenException(msg.to_owned())); + } + Ok(self.credential.clone()) + } +} + +fn make_headers_with_auth(access_key: &str, token: Option<&str>) -> HeaderMap { + let mut headers = HeaderMap::new(); + // Use current time for the date to pass timestamp validation. + let now = time::OffsetDateTime::now_utc(); + let date_str = format!( + "{:04}{:02}{:02}T{:02}{:02}{:02}Z", + now.year(), + now.month() as u8, + now.day(), + now.hour(), + now.minute(), + now.second(), + ); + let date_short = &date_str[..8]; + // Signature is fake — these tests exercise error paths (expired token, + // unknown key) that trigger before signature verification in + // authenticate(). + let auth = format!( + "AWS4-HMAC-SHA256 Credential={}/{}/us-east-1/dynamodb/aws4_request, \ + SignedHeaders=content-type;host;x-amz-date;x-amz-target, \ + Signature=0000000000000000000000000000000000000000000000000000000000000000", + access_key, date_short + ); + headers.insert("authorization", HeaderValue::from_str(&auth).unwrap()); + headers.insert("x-amz-date", HeaderValue::from_str(&date_str).unwrap()); + headers.insert("host", HeaderValue::from_static("localhost:18443")); + headers.insert( + "content-type", + HeaderValue::from_static("application/x-amz-json-1.0"), + ); + headers.insert( + "x-amz-target", + HeaderValue::from_static("DynamoDB_20120810.ListTables"), + ); + if let Some(t) = token { + headers.insert("x-amz-security-token", HeaderValue::from_str(t).unwrap()); + } + headers +} + +#[tokio::test] +async fn expired_session_returns_expired_token_exception() { + // CB-12: The credential store returns ExpiredTokenException directly + // for expired sessions (fail-closed). The auth layer propagates it. + let store = MockCredentialStore { + credential: None, + error: Some("The security token included in the request is expired"), + }; + let provider = BuiltinAuthProvider::new(store, "us-east-1"); + let headers = make_headers_with_auth("ASIAEXTENDDB00000000", Some("test-token-value")); + + let result = provider.authenticate(&headers, b"{}").await; + match result { + Err(DynamoDbError::ExpiredTokenException(msg)) => { + assert!( + msg.contains("expired"), + "Expected 'expired' in message: {msg}" + ); + } + other => panic!("Expected ExpiredTokenException, got: {other:?}"), + } +} + +#[tokio::test] +async fn nonexistent_key_returns_unrecognized_client() { + let store = MockCredentialStore { + credential: None, + error: None, + }; + let provider = BuiltinAuthProvider::new(store, "us-east-1"); + let headers = make_headers_with_auth("AKIAXXXXXXXXXXXXXXXX", None); + + let result = provider.authenticate(&headers, b"{}").await; + match result { + Err(DynamoDbError::UnrecognizedClientException(msg)) => { + assert!( + msg.contains("invalid"), + "Expected 'invalid' in message: {msg}" + ); + } + other => panic!("Expected UnrecognizedClientException, got: {other:?}"), + } +} + +const SECRET: &str = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"; + +fn credential(is_session: bool) -> StoredCredential { + StoredCredential { + secret_key: SECRET.to_owned(), + account_id: "123456789012".to_owned(), + principal_name: "alice".to_owned(), + session_name: is_session.then(|| "sess".to_owned()), + is_session, + session_token: is_session.then(|| "stored-token".to_owned()), + is_active: true, + expires_at: None, + } +} + +/// Headers for a request correctly signed by a client whose credential +/// scope names `region`. `token`, when present, is sent and signed. +fn signed_headers(access_key: &str, region: &str, token: Option<&str>, body: &[u8]) -> HeaderMap { + let mut headers = make_headers_with_auth(access_key, token); + headers.remove("authorization"); + let date_str = headers["x-amz-date"].to_str().unwrap().to_owned(); + let date_short = date_str[..8].to_owned(); + let mut names = vec!["content-type", "host", "x-amz-date", "x-amz-target"]; + if token.is_some() { + names.push("x-amz-security-token"); + } + let signed = names.join(";"); + let creq = sigv4::canonical::canonical_request("POST", "/", "", &headers, &signed, body); + let scope = format!("{date_short}/{region}/dynamodb/aws4_request"); + let sts = sigv4::canonical::string_to_sign(&date_str, &scope, &creq); + let key = sigv4::signing_key::derive_signing_key(SECRET, &date_short, region, "dynamodb"); + let sig = sigv4::signing_key::compute_signature(&key, &sts); + let auth = format!( + "AWS4-HMAC-SHA256 Credential={access_key}/{scope}, SignedHeaders={signed}, Signature={sig}" + ); + headers.insert("authorization", HeaderValue::from_str(&auth).unwrap()); + headers +} + +fn provider(is_session: bool) -> BuiltinAuthProvider { + BuiltinAuthProvider::new( + MockCredentialStore { + credential: Some(credential(is_session)), + error: None, + }, + "us-east-1", + ) +} + +#[tokio::test] +async fn correctly_signed_long_term_key_authenticates() { + let headers = signed_headers("AKIAEXTENDDB00000000", "us-east-1", None, b"{}"); + let id = provider(false).authenticate(&headers, b"{}").await.unwrap(); + assert!(matches!(id, AuthIdentity::User { .. })); +} + +/// Measured 2026-09-16: a scope for another region is refused with this +/// exact message; the region is not named and the message ends with a space. +#[tokio::test] +async fn scope_for_another_region_is_rejected() { + let headers = signed_headers("AKIAEXTENDDB00000000", "us-west-2", None, b"{}"); + match provider(false).authenticate(&headers, b"{}").await { + Err(DynamoDbError::InvalidSignatureException(msg)) => { + assert_eq!(msg, "Credential should be scoped to a valid region. "); + } + other => panic!("Expected InvalidSignatureException, got: {other:?}"), + } +} + +/// A long-term key has no session token; presenting one is an invalid token. +#[tokio::test] +async fn session_token_with_a_long_term_key_is_rejected() { + let headers = signed_headers( + "AKIAEXTENDDB00000000", + "us-east-1", + Some("any-token"), + b"{}", + ); + match provider(false).authenticate(&headers, b"{}").await { + Err(DynamoDbError::UnrecognizedClientException(msg)) => { + assert_eq!(msg, "The security token included in the request is invalid"); + } + other => panic!("Expected UnrecognizedClientException, got: {other:?}"), + } +} + +#[tokio::test] +async fn session_key_with_matching_token_authenticates() { + let headers = signed_headers( + "ASIAEXTENDDB00000000", + "us-east-1", + Some("stored-token"), + b"{}", + ); + let id = provider(true).authenticate(&headers, b"{}").await.unwrap(); + assert!(matches!(id, AuthIdentity::RoleSession { .. })); +} + +/// Measured 2026-09-16: a presented token that does not resolve has no +/// trailing period; a missing token does. +#[tokio::test] +async fn session_key_token_messages_match_the_service() { + let wrong = signed_headers("ASIAEXTENDDB00000000", "us-east-1", Some("bogus"), b"{}"); + match provider(true).authenticate(&wrong, b"{}").await { + Err(DynamoDbError::UnrecognizedClientException(msg)) => { + assert_eq!(msg, "The security token included in the request is invalid"); + } + other => panic!("Expected UnrecognizedClientException, got: {other:?}"), + } + let missing = signed_headers("ASIAEXTENDDB00000000", "us-east-1", None, b"{}"); + match provider(true).authenticate(&missing, b"{}").await { + Err(DynamoDbError::UnrecognizedClientException(msg)) => { + assert_eq!( + msg, + "The security token included in the request is invalid." + ); + } + other => panic!("Expected UnrecognizedClientException, got: {other:?}"), + } +} + +/// The scope region is compared byte for byte: region names are lowercase +/// and the service does not fold case either. +#[tokio::test] +async fn scope_region_comparison_is_exact() { + for other in ["US-EAST-1", "us-east-1 ", ""] { + let headers = signed_headers("AKIAEXTENDDB00000000", other, None, b"{}"); + match provider(false).authenticate(&headers, b"{}").await { + Err(DynamoDbError::InvalidSignatureException(msg)) => { + assert_eq!( + msg, "Credential should be scoped to a valid region. ", + "{other:?}" + ); + } + other_result => { + panic!("{other:?}: expected InvalidSignatureException, got {other_result:?}") + } + } + } +} + +/// An empty token header on a long-term key is a presented token, and is +/// rejected like any other; only an absent header passes. +#[tokio::test] +async fn empty_session_token_header_with_a_long_term_key_is_rejected() { + let headers = signed_headers("AKIAEXTENDDB00000000", "us-east-1", Some(""), b"{}"); + match provider(false).authenticate(&headers, b"{}").await { + Err(DynamoDbError::UnrecognizedClientException(msg)) => { + assert_eq!(msg, "The security token included in the request is invalid"); + } + other => panic!("Expected UnrecognizedClientException, got: {other:?}"), + } +} + +/// Measured 2026-09-16: a scope wrong in both region and service reports +/// both sentences in one message, region first. +#[tokio::test] +async fn scope_wrong_in_region_and_service_reports_both() { + let mut headers = make_headers_with_auth("AKIAEXTENDDB00000000", None); + headers.remove("authorization"); + let date_str = headers["x-amz-date"].to_str().unwrap().to_owned(); + let date_short = date_str[..8].to_owned(); + let signed = "content-type;host;x-amz-date;x-amz-target"; + let creq = sigv4::canonical::canonical_request("POST", "/", "", &headers, signed, b"{}"); + let scope = format!("{date_short}/us-west-2/s3/aws4_request"); + let sts = sigv4::canonical::string_to_sign(&date_str, &scope, &creq); + let key = sigv4::signing_key::derive_signing_key(SECRET, &date_short, "us-west-2", "s3"); + let sig = sigv4::signing_key::compute_signature(&key, &sts); + let auth = format!( + "AWS4-HMAC-SHA256 Credential=AKIAEXTENDDB00000000/{scope}, SignedHeaders={signed}, Signature={sig}" + ); + headers.insert("authorization", HeaderValue::from_str(&auth).unwrap()); + match provider(false).authenticate(&headers, b"{}").await { + Err(DynamoDbError::InvalidSignatureException(msg)) => { + assert_eq!( + msg, + "Credential should be scoped to a valid region. Credential should be scoped to correct service: 'dynamodb'. " + ); + } + other => panic!("Expected InvalidSignatureException, got: {other:?}"), + } +} + +/// A provider whose store holds no credential at all. +fn provider_without_credentials() -> BuiltinAuthProvider { + BuiltinAuthProvider::new( + MockCredentialStore { + credential: None, + error: None, + }, + "us-east-1", + ) +} + +/// Move the request date one hour into the past, re-signing the scope +/// date in the `Authorization` header to match. +fn make_date_stale(headers: &mut HeaderMap) { + let fresh = headers["x-amz-date"].to_str().unwrap().to_owned(); + let stale = time::OffsetDateTime::now_utc() - time::Duration::hours(1); + let stale_str = format!( + "{:04}{:02}{:02}T{:02}{:02}{:02}Z", + stale.year(), + stale.month() as u8, + stale.day(), + stale.hour(), + stale.minute(), + stale.second(), + ); + let auth = headers["authorization"] + .to_str() + .unwrap() + .replacen(&fresh[..8], &stale_str[..8], 1); + headers.insert("authorization", HeaderValue::from_str(&auth).unwrap()); + headers.insert("x-amz-date", HeaderValue::from_str(&stale_str).unwrap()); +} + +/// Measured 2026-10-23 (R2-1 case F): an unknown access key scoped to +/// another region answers the scope message, so the scope check runs +/// before the key lookup. +#[tokio::test] +async fn unknown_key_with_a_wrong_region_reports_the_scope() { + let headers = signed_headers("AKIAEXAMPLEEXAMPLE01", "us-west-2", None, b"{}"); + match provider_without_credentials() + .authenticate(&headers, b"{}") + .await + { + Err(DynamoDbError::InvalidSignatureException(msg)) => { + assert_eq!(msg, "Credential should be scoped to a valid region. "); + } + other => panic!("Expected InvalidSignatureException, got: {other:?}"), + } +} + +/// Measured 2026-10-23 (R2-2 case V): an unknown access key with a date +/// one hour old answers "Signature expired", so the timestamp check runs +/// before the key lookup. The service's message continues with the dates +/// and its type is InvalidSignatureException; the server's timestamp +/// error shape predates this change and is not altered here. +#[tokio::test] +async fn unknown_key_with_a_stale_date_reports_the_timestamp() { + let mut headers = make_headers_with_auth("AKIAEXAMPLEEXAMPLE01", None); + make_date_stale(&mut headers); + match provider_without_credentials() + .authenticate(&headers, b"{}") + .await + { + Err(DynamoDbError::UnrecognizedClientException(msg)) => { + assert!(msg.starts_with("Signature expired: "), "{msg}"); + } + other => panic!("Expected the timestamp error, got: {other:?}"), + } +} + +/// Measured 2026-10-23 (R2-2 case D): a stale date scoped to another +/// region answers "Signature expired", so the timestamp check runs +/// before the scope check. +#[tokio::test] +async fn stale_date_with_a_wrong_region_reports_the_timestamp() { + let mut headers = signed_headers("AKIAEXTENDDB00000000", "us-west-2", None, b"{}"); + make_date_stale(&mut headers); + match provider(false).authenticate(&headers, b"{}").await { + Err(DynamoDbError::UnrecognizedClientException(msg)) => { + assert!(msg.starts_with("Signature expired: "), "{msg}"); + } + other => panic!("Expected the timestamp error, got: {other:?}"), + } +} diff --git a/tests/test_auth_error_fidelity.py b/tests/test_auth_error_fidelity.py index b3a252534..9e0f9adb4 100755 --- a/tests/test_auth_error_fidelity.py +++ b/tests/test_auth_error_fidelity.py @@ -329,6 +329,27 @@ def test_scope_wrong_in_region_and_service_reports_both(self): "Credential should be scoped to correct service: 'dynamodb'. " ) + def test_unknown_key_with_another_region_reports_the_scope(self): + # Measured against the service on 2026-10-23: the scope is checked + # before the access key is looked up, so an unknown key scoped to + # another region gets the scope message, not the invalid-key error. + other = "us-west-2" if self.region != "us-west-2" else "us-east-1" + body_bytes = json.dumps({}).encode("utf-8") + aws_req = AWSRequest( + method="POST", + url=self.url, + data=body_bytes, + headers={ + "X-Amz-Target": "DynamoDB_20120810.ListTables", + "Content-Type": "application/x-amz-json-1.0", + }, + ) + SigV4Auth(Credentials("AKIAEXAMPLEEXAMPLE01", "x" * 40, None), "dynamodb", other).add_auth(aws_req) + resp = requests.post(self.url, data=body_bytes, headers=dict(aws_req.headers), verify=self.endpoint_url is None) + assert resp.status_code == 400, resp.text + assert resp.json()["__type"].endswith("InvalidSignatureException"), resp.text + assert resp.json()["message"] == "Credential should be scoped to a valid region. " + def test_duplicate_host_header_is_refused(self): # The service's front end refuses a request with two host headers # before it reaches signing (400, connection closed). From bd744d576692ddefca2ee28fd4c34dad3d0e5744 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:50:13 +0000 Subject: [PATCH 07/10] fix(server): bound the wait for the first bytes of a request on both 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. --- Cargo.lock | 1 + crates/server/Cargo.toml | 2 + crates/server/src/listener.rs | 288 ++++++++++++++-------- crates/server/src/listener/tests.rs | 364 ++++++++++++++++++++++++++++ docs/manuals/05-admin-guide.md | 2 +- 5 files changed, 555 insertions(+), 102 deletions(-) create mode 100644 crates/server/src/listener/tests.rs diff --git a/Cargo.lock b/Cargo.lock index d19d5ed7f..331cc6364 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1848,6 +1848,7 @@ dependencies = [ "libc", "metrics", "rand 0.9.4", + "rcgen", "rustls", "serde", "serde_json", diff --git a/crates/server/Cargo.toml b/crates/server/Cargo.toml index 10f50783f..1090061d7 100755 --- a/crates/server/Cargo.toml +++ b/crates/server/Cargo.toml @@ -52,3 +52,5 @@ libc = { workspace = true } [dev-dependencies] extenddb-cache = { workspace = true, features = ["test-util"] } +# Self-signed certificates for the TLS listener tests. +rcgen = { workspace = true } diff --git a/crates/server/src/listener.rs b/crates/server/src/listener.rs index 7b932918e..900f433d0 100644 --- a/crates/server/src/listener.rs +++ b/crates/server/src/listener.rs @@ -65,10 +65,14 @@ pub(crate) async fn serve_app( // a 301 redirect to HTTPS and reject the connection before the TLS // handshake. This gives users a helpful redirect instead of a // confusing TLS handshake failure. The same acceptor sets - // TCP_NODELAY on every accepted socket before the handshake. - let redirect_acceptor = HttpsRedirectAcceptor { addr: local_addr }; + // TCP_NODELAY on every accepted socket before the handshake, and + // bounds the wait for that first byte by `request_timeout`. + let redirect_acceptor = HttpsRedirectAcceptor { + addr: local_addr, + first_byte_timeout: request_timeout, + }; let server = axum_server::from_tcp_rustls(std_listener, rustls_config)? - .map(|tls| tls.acceptor(redirect_acceptor)) + .map(|tls| FirstByteAcceptor::new(tls.acceptor(redirect_acceptor), request_timeout)) .handle(handle); with_request_timeouts(server, request_timeout) .serve(make_service) @@ -76,7 +80,7 @@ pub(crate) async fn serve_app( } else { tracing::info!("extenddb listening on {local_addr}"); let server = axum_server::from_tcp(std_listener)? - .acceptor(NodelayAcceptor) + .acceptor(FirstByteAcceptor::new(NodelayAcceptor, request_timeout)) .handle(handle); with_request_timeouts(server, request_timeout) .serve(make_service) @@ -99,6 +103,11 @@ pub(crate) async fn serve_app( /// HTTP/2: a connection that stops answering PING frames is closed. A client /// that sends the preface and then stalls holds the connection for at most /// interval plus timeout. +/// +/// Neither timer runs until the connection has delivered its first bytes: +/// `hyper_util`'s auto builder reads the stream to tell HTTP/2 from HTTP/1 +/// before either protocol's timer starts, and that read has no bound of its +/// own. [`FirstByteAcceptor`] bounds it. fn with_request_timeouts( mut server: axum_server::Server, request_timeout: Duration, @@ -117,6 +126,165 @@ fn with_request_timeouts( server } +/// Acceptor wrapper that bounds the wait for the start of a request. +/// +/// Runs the inner acceptor (TLS handshake, `TCP_NODELAY`) and hands the +/// resulting stream on inside a [`FirstByteDeadline`]. A client that +/// connects (and, on the TLS listener, completes the handshake) and then +/// sends nothing, or sends only a prefix of the 24-byte HTTP/2 preface, is +/// closed once `timeout` passes. +#[derive(Clone)] +struct FirstByteAcceptor { + inner: A, + timeout: Duration, +} + +impl FirstByteAcceptor { + fn new(inner: A, timeout: Duration) -> Self { + Self { inner, timeout } + } +} + +impl axum_server::accept::Accept for FirstByteAcceptor +where + A: axum_server::accept::Accept, + A::Future: Send + 'static, + A::Stream: Send + 'static, + A::Service: Send + 'static, +{ + type Stream = FirstByteDeadline; + type Service = A::Service; + type Future = std::pin::Pin< + Box< + dyn std::future::Future> + Send, + >, + >; + + fn accept(&self, stream: I, service: S) -> Self::Future { + let inner = self.inner.accept(stream, service); + let timeout = self.timeout; + Box::pin(async move { + let (stream, service) = inner.await?; + Ok((FirstByteDeadline::new(stream, timeout), service)) + }) + } +} + +/// The HTTP/2 client preface. `hyper_util`'s auto builder reads a new +/// connection until it holds these 24 bytes or a byte that differs from +/// them, and only then hands the stream to HTTP/1 or HTTP/2, whose own +/// timers start at that point. +const H2_PREFACE: &[u8] = b"PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n"; + +/// IO adapter that bounds the version sniff on a new connection. +/// +/// The deadline starts when the adapter is built (after accept, or after the +/// TLS handshake). It is dropped once the bytes read so far decide the +/// protocol: a byte that differs from the HTTP/2 preface, or the whole +/// preface. Until then a pending read also polls the deadline, so the +/// connection task wakes when it fires and the read fails with `TimedOut`; +/// `hyper_util` then drops the connection. Writes pass through unchanged. +pub(crate) struct FirstByteDeadline { + inner: S, + deadline: Option>>, + /// Bytes read so far that match the preface prefix, while undecided. + preface_matched: usize, +} + +impl FirstByteDeadline { + fn new(inner: S, timeout: Duration) -> Self { + Self { + inner, + deadline: Some(Box::pin(tokio::time::sleep(timeout))), + preface_matched: 0, + } + } + + /// Feed newly read bytes to the sniff; returns true once the protocol is + /// decided (the auto builder stops reading at the same point). + fn sniff_decided(&mut self, new_bytes: &[u8]) -> bool { + for &b in new_bytes { + if self.preface_matched >= H2_PREFACE.len() || b != H2_PREFACE[self.preface_matched] { + return true; + } + self.preface_matched += 1; + } + self.preface_matched >= H2_PREFACE.len() + } +} + +impl tokio::io::AsyncRead for FirstByteDeadline { + fn poll_read( + mut self: std::pin::Pin<&mut Self>, + cx: &mut std::task::Context<'_>, + buf: &mut tokio::io::ReadBuf<'_>, + ) -> std::task::Poll> { + use std::task::Poll; + + let before = buf.filled().len(); + let read = std::pin::Pin::new(&mut self.inner).poll_read(cx, buf); + if self.deadline.is_none() { + return read; + } + match read { + Poll::Ready(Ok(())) => { + let new_bytes = buf.filled()[before..].to_vec(); + if self.sniff_decided(&new_bytes) { + self.deadline = None; + } + Poll::Ready(Ok(())) + } + Poll::Pending => { + let deadline = self.deadline.as_mut().expect("checked above"); + match deadline.as_mut().poll(cx) { + Poll::Ready(()) => Poll::Ready(Err(std::io::Error::new( + std::io::ErrorKind::TimedOut, + "no request received within request_timeout_secs", + ))), + Poll::Pending => Poll::Pending, + } + } + other => other, + } + } +} + +impl tokio::io::AsyncWrite for FirstByteDeadline { + fn poll_write( + mut self: std::pin::Pin<&mut Self>, + cx: &mut std::task::Context<'_>, + buf: &[u8], + ) -> std::task::Poll> { + std::pin::Pin::new(&mut self.inner).poll_write(cx, buf) + } + + fn poll_flush( + mut self: std::pin::Pin<&mut Self>, + cx: &mut std::task::Context<'_>, + ) -> std::task::Poll> { + std::pin::Pin::new(&mut self.inner).poll_flush(cx) + } + + fn poll_shutdown( + mut self: std::pin::Pin<&mut Self>, + cx: &mut std::task::Context<'_>, + ) -> std::task::Poll> { + std::pin::Pin::new(&mut self.inner).poll_shutdown(cx) + } + + fn poll_write_vectored( + mut self: std::pin::Pin<&mut Self>, + cx: &mut std::task::Context<'_>, + bufs: &[std::io::IoSlice<'_>], + ) -> std::task::Poll> { + std::pin::Pin::new(&mut self.inner).poll_write_vectored(cx, bufs) + } + + fn is_write_vectored(&self) -> bool { + self.inner.is_write_vectored() + } +} + /// Remove the PID file if it exists. Best-effort — log but don't fail. fn cleanup_pid_file(pid_file: Option<&std::path::Path>) { if let Some(path) = pid_file @@ -179,9 +347,13 @@ impl axum_server::accept::Accept fo /// `axum_server` 0.8 ships a `NoDelayAcceptor`, but it cannot be stacked under /// this acceptor without a generic wrapper, so this acceptor sets the option /// itself, and logs instead of rejecting on failure. +/// +/// The peek waits at most `first_byte_timeout` for the first byte, so a +/// client that connects and sends nothing is dropped before the handshake. #[derive(Clone)] struct HttpsRedirectAcceptor { addr: std::net::SocketAddr, + first_byte_timeout: Duration, } impl axum_server::accept::Accept @@ -197,10 +369,19 @@ impl axum_server::accept::Accept fn accept(&self, stream: tokio::net::TcpStream, service: S) -> Self::Future { let addr = self.addr; + let first_byte_timeout = self.first_byte_timeout; Box::pin(async move { set_tcp_nodelay(&stream); let mut peek_buf = [0u8; 1]; - match stream.peek(&mut peek_buf).await { + let peeked = tokio::time::timeout(first_byte_timeout, stream.peek(&mut peek_buf)) + .await + .map_err(|_| { + std::io::Error::new( + std::io::ErrorKind::TimedOut, + "no bytes received before the TLS handshake within request_timeout_secs", + ) + })?; + match peeked { Ok(0) => { // Connection closed before sending data. Err(std::io::Error::new( @@ -271,99 +452,4 @@ async fn shutdown_signal() { } #[cfg(test)] -mod tests { - use super::*; - use tokio::io::AsyncWriteExt; - use tokio::net::{TcpListener, TcpStream}; - - /// Bind a loopback listener and connect one client to it. - async fn bind_and_connect() -> (TcpListener, TcpStream) { - let listener = TcpListener::bind("127.0.0.1:0") - .await - .expect("bind loopback"); - let addr = listener.local_addr().expect("local addr"); - let client = TcpStream::connect(addr).await.expect("connect"); - (listener, client) - } - - /// The plaintext path: a connection that sends part of its request - /// headers and then stalls is closed once `request_timeout` passes, - /// as it is on the TLS path. The client reads EOF (or a reset) well - /// before a bound of several times the timeout; without the timer the - /// read blocks until that bound. - #[tokio::test] - async fn plaintext_listener_closes_a_stalled_header_phase() { - use tokio::io::AsyncReadExt; - - let timeout = Duration::from_millis(300); - let (listener, mut client) = bind_and_connect().await; - let app = Router::new().route("/", axum::routing::get(|| async { "ok" })); - let server = tokio::spawn(serve_app(listener, app, timeout, None, None)); - - // A request line and one header, no terminating blank line. - client - .write_all(b"GET / HTTP/1.1\r\nHost: localhost\r\n") - .await - .expect("write partial headers"); - let mut buf = [0u8; 64]; - let outcome = tokio::time::timeout(timeout * 10, client.read(&mut buf)).await; - server.abort(); - match outcome { - Ok(Ok(0)) | Ok(Err(_)) => {} - Ok(Ok(n)) => panic!("server answered {n} bytes instead of closing the connection"), - Err(_) => panic!( - "server held a stalled header phase open for {:?} on the plaintext listener", - timeout * 10 - ), - } - } - - /// The TLS path: the acceptor that runs before the TLS handshake sets - /// `TCP_NODELAY` on the accepted socket, read back from the socket the - /// acceptor hands on. - #[tokio::test] - async fn tls_acceptor_sets_tcp_nodelay_before_the_handshake() { - use axum_server::accept::Accept; - - let (listener, mut client) = bind_and_connect().await; - // A TLS ClientHello starts with the handshake record type. - client - .write_all(&[0x16]) - .await - .expect("write ClientHello byte"); - let (accepted, _) = listener.accept().await.expect("accept"); - assert!( - !accepted.nodelay().expect("read TCP_NODELAY"), - "a fresh accepted socket has Nagle on" - ); - - let acceptor = HttpsRedirectAcceptor { - addr: listener.local_addr().expect("local addr"), - }; - let (stream, ()) = acceptor - .accept(accepted, ()) - .await - .expect("a TLS ClientHello passes through to the TLS acceptor"); - assert!(stream.nodelay().expect("read TCP_NODELAY")); - } - - /// The plaintext path: the acceptor sets `TCP_NODELAY` on every socket - /// it accepts. - #[tokio::test] - async fn plaintext_listener_sets_tcp_nodelay_on_accepted_sockets() { - use axum_server::accept::Accept; - - let (listener, client) = bind_and_connect().await; - let (accepted, peer) = listener.accept().await.expect("accept"); - assert_eq!(peer, client.local_addr().expect("client addr")); - assert!( - !accepted.nodelay().expect("read TCP_NODELAY"), - "a fresh accepted socket has Nagle on" - ); - let (stream, ()) = NodelayAcceptor - .accept(accepted, ()) - .await - .expect("the plaintext acceptor hands the socket on"); - assert!(stream.nodelay().expect("read TCP_NODELAY")); - } -} +mod tests; diff --git a/crates/server/src/listener/tests.rs b/crates/server/src/listener/tests.rs new file mode 100644 index 000000000..219254bd4 --- /dev/null +++ b/crates/server/src/listener/tests.rs @@ -0,0 +1,364 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Unit tests for the listener: request timeouts, the first-byte deadline +//! and `TCP_NODELAY` on both listener modes. + +use super::*; +use tokio::io::AsyncWriteExt; +use tokio::net::{TcpListener, TcpStream}; + +/// Bind a loopback listener and connect one client to it. +async fn bind_and_connect() -> (TcpListener, TcpStream) { + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("bind loopback"); + let addr = listener.local_addr().expect("local addr"); + let client = TcpStream::connect(addr).await.expect("connect"); + (listener, client) +} + +/// The plaintext path: a connection that sends part of its request +/// headers and then stalls is closed once `request_timeout` passes, +/// as it is on the TLS path. The client reads EOF (or a reset) well +/// before a bound of several times the timeout; without the timer the +/// read blocks until that bound. +#[tokio::test] +async fn plaintext_listener_closes_a_stalled_header_phase() { + use tokio::io::AsyncReadExt; + + let timeout = Duration::from_millis(300); + let (listener, mut client) = bind_and_connect().await; + let app = Router::new().route("/", axum::routing::get(|| async { "ok" })); + let server = tokio::spawn(serve_app(listener, app, timeout, None, None)); + + // A request line and one header, no terminating blank line. + client + .write_all(b"GET / HTTP/1.1\r\nHost: localhost\r\n") + .await + .expect("write partial headers"); + let mut buf = [0u8; 64]; + let outcome = tokio::time::timeout(timeout * 10, client.read(&mut buf)).await; + server.abort(); + match outcome { + Ok(Ok(0)) | Ok(Err(_)) => {} + Ok(Ok(n)) => panic!("server answered {n} bytes instead of closing the connection"), + Err(_) => panic!( + "server held a stalled header phase open for {:?} on the plaintext listener", + timeout * 10 + ), + } +} + +/// The TLS path: the acceptor that runs before the TLS handshake sets +/// `TCP_NODELAY` on the accepted socket, read back from the socket the +/// acceptor hands on. +#[tokio::test] +async fn tls_acceptor_sets_tcp_nodelay_before_the_handshake() { + use axum_server::accept::Accept; + + let (listener, mut client) = bind_and_connect().await; + // A TLS ClientHello starts with the handshake record type. + client + .write_all(&[0x16]) + .await + .expect("write ClientHello byte"); + let (accepted, _) = listener.accept().await.expect("accept"); + assert!( + !accepted.nodelay().expect("read TCP_NODELAY"), + "a fresh accepted socket has Nagle on" + ); + + let acceptor = HttpsRedirectAcceptor { + addr: listener.local_addr().expect("local addr"), + first_byte_timeout: Duration::from_secs(5), + }; + let (stream, ()) = acceptor + .accept(accepted, ()) + .await + .expect("a TLS ClientHello passes through to the TLS acceptor"); + assert!(stream.nodelay().expect("read TCP_NODELAY")); +} + +/// The plaintext path: the acceptor sets `TCP_NODELAY` on every socket +/// it accepts. +#[tokio::test] +async fn plaintext_listener_sets_tcp_nodelay_on_accepted_sockets() { + use axum_server::accept::Accept; + + let (listener, client) = bind_and_connect().await; + let (accepted, peer) = listener.accept().await.expect("accept"); + assert_eq!(peer, client.local_addr().expect("client addr")); + assert!( + !accepted.nodelay().expect("read TCP_NODELAY"), + "a fresh accepted socket has Nagle on" + ); + let (stream, ()) = NodelayAcceptor + .accept(accepted, ()) + .await + .expect("the plaintext acceptor hands the socket on"); + assert!(stream.nodelay().expect("read TCP_NODELAY")); +} + +/// Read from `client` until EOF, an error, or `bound`. Returns `Ok(())` for +/// EOF or a reset, `Err(n)` for `n` unexpected bytes, and panics with `what` +/// when the server holds the connection past `bound`. +async fn expect_closed_within(client: &mut TcpStream, bound: Duration, what: &str) { + use tokio::io::AsyncReadExt; + + let mut buf = [0u8; 64]; + match tokio::time::timeout(bound, client.read(&mut buf)).await { + Ok(Ok(0)) | Ok(Err(_)) => {} + Ok(Ok(n)) => panic!("{what}: server answered {n} bytes instead of closing the connection"), + Err(_) => panic!("{what}: server held the connection open for {bound:?}"), + } +} + +/// The plaintext path: a connection that sends nothing after connecting is +/// closed once `request_timeout` passes. The header read timer never starts +/// for this shape, because the version sniff runs first. +#[tokio::test] +async fn plaintext_listener_closes_a_connection_that_sends_nothing() { + let timeout = Duration::from_millis(300); + let (listener, mut client) = bind_and_connect().await; + let app = Router::new().route("/", axum::routing::get(|| async { "ok" })); + let server = tokio::spawn(serve_app(listener, app, timeout, None, None)); + + expect_closed_within(&mut client, timeout * 10, "plaintext, zero bytes").await; + server.abort(); +} + +/// The plaintext path: a client that sends 16 bytes of the HTTP/2 preface +/// and stops is still in the version sniff; it is closed at the deadline. +#[tokio::test] +async fn plaintext_listener_closes_a_partial_h2_preface() { + let timeout = Duration::from_millis(300); + let (listener, mut client) = bind_and_connect().await; + let app = Router::new().route("/", axum::routing::get(|| async { "ok" })); + let server = tokio::spawn(serve_app(listener, app, timeout, None, None)); + + client + .write_all(&H2_PREFACE[..16]) + .await + .expect("write partial preface"); + expect_closed_within(&mut client, timeout * 10, "plaintext, 16 preface bytes").await; + server.abort(); +} + +/// Write a self-signed certificate for `localhost` and its key into `dir`, +/// returning the server's TLS config and the certificate's DER bytes for the +/// client's trust store. +fn self_signed_localhost(dir: &std::path::Path) -> (ServerTlsConfig, Vec) { + let params = rcgen::CertificateParams::new(vec!["localhost".to_owned()]).expect("params"); + let key_pair = rcgen::KeyPair::generate().expect("key pair"); + let cert = params.self_signed(&key_pair).expect("self-signed cert"); + let cert_path = dir.join("cert.pem"); + let key_path = dir.join("key.pem"); + std::fs::write(&cert_path, cert.pem()).expect("write cert"); + std::fs::write(&key_path, key_pair.serialize_pem()).expect("write key"); + ( + ServerTlsConfig { + cert_path, + key_path, + }, + cert.der().to_vec(), + ) +} + +/// Complete a TLS handshake with the server at `addr` as a client that +/// trusts `cert_der`, send `after_handshake` as application data, then read +/// until EOF, an error, or `bound`. Runs rustls over a blocking socket so +/// the test needs no async TLS client. Returns `Ok(())` for EOF or an error +/// from the server side, `Err(n)` for `n` application bytes received, and +/// panics when the bound passes with the connection still open. +fn tls_client_expect_closed_within( + addr: std::net::SocketAddr, + cert_der: Vec, + after_handshake: &'static [u8], + bound: Duration, + what: &'static str, +) { + // The rustls client needs the process-level provider as well. serve_app + // installs it in the TLS arm, but this client can reach the builder + // before the server task gets there, and ClientConfig::builder panics + // when no provider is installed. Installing it twice is not an error + // here: the second call reports that one is already set. + let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); + let mut roots = rustls::RootCertStore::empty(); + roots + .add(rustls::pki_types::CertificateDer::from(cert_der)) + .expect("trust the test certificate"); + let config = rustls::ClientConfig::builder() + .with_root_certificates(roots) + .with_no_client_auth(); + let server_name = rustls::pki_types::ServerName::try_from("localhost").expect("server name"); + let mut conn = rustls::ClientConnection::new(std::sync::Arc::new(config), server_name) + .expect("client connection"); + let mut sock = std::net::TcpStream::connect(addr).expect("connect"); + sock.set_read_timeout(Some(bound)).expect("read timeout"); + while conn.is_handshaking() { + conn.complete_io(&mut sock).expect("TLS handshake"); + } + let mut tls = rustls::Stream::new(&mut conn, &mut sock); + if !after_handshake.is_empty() { + std::io::Write::write_all(&mut tls, after_handshake).expect("write application data"); + std::io::Write::flush(&mut tls).expect("flush"); + } + let mut buf = [0u8; 64]; + match std::io::Read::read(&mut tls, &mut buf) { + Ok(0) => {} + Ok(n) => panic!("{what}: server answered {n} bytes instead of closing the connection"), + Err(e) + if matches!( + e.kind(), + std::io::ErrorKind::WouldBlock | std::io::ErrorKind::TimedOut + ) => + { + panic!("{what}: server held the connection open for {bound:?}") + } + // A reset, a close_notify reported as an error, or an unexpected + // EOF all mean the server dropped the connection. + Err(_) => {} + } +} + +/// Start the TLS listener on a loopback port with `timeout`, returning its +/// address, the certificate to trust, the temp dir holding the key pair and +/// the server task. +async fn start_tls_listener( + timeout: Duration, +) -> ( + std::net::SocketAddr, + Vec, + std::path::PathBuf, + tokio::task::JoinHandle>, +) { + let dir = std::env::temp_dir().join(format!( + "extenddb-listener-test-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("clock") + .as_nanos() + )); + std::fs::create_dir_all(&dir).expect("temp dir"); + let (tls, cert_der) = self_signed_localhost(&dir); + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("bind loopback"); + let addr = listener.local_addr().expect("local addr"); + let app = Router::new().route("/", axum::routing::get(|| async { "ok" })); + let server = tokio::spawn(serve_app(listener, app, timeout, Some(tls), None)); + (addr, cert_der, dir, server) +} + +/// The TLS path: a client that completes the handshake and then sends no +/// application data is closed once `request_timeout` passes. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn tls_listener_closes_a_connection_that_sends_nothing_after_the_handshake() { + let timeout = Duration::from_millis(300); + let (addr, cert_der, dir, server) = start_tls_listener(timeout).await; + tokio::task::spawn_blocking(move || { + tls_client_expect_closed_within( + addr, + cert_der, + b"", + timeout * 10, + "TLS, zero bytes after the handshake", + ) + }) + .await + .expect("client task"); + server.abort(); + let _ = std::fs::remove_dir_all(dir); +} + +/// The TLS path: 16 bytes of the HTTP/2 preface after the handshake, then +/// silence, is closed at the deadline. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn tls_listener_closes_a_partial_h2_preface() { + let timeout = Duration::from_millis(300); + let (addr, cert_der, dir, server) = start_tls_listener(timeout).await; + tokio::task::spawn_blocking(move || { + tls_client_expect_closed_within( + addr, + cert_der, + &H2_PREFACE[..16], + timeout * 10, + "TLS, 16 preface bytes", + ) + }) + .await + .expect("client task"); + server.abort(); + let _ = std::fs::remove_dir_all(dir); +} + +/// The TLS path: a connection that sends no byte at all (not even a +/// ClientHello) is dropped by the redirect acceptor's peek at the deadline. +#[tokio::test] +async fn tls_acceptor_drops_a_connection_that_sends_nothing_before_the_handshake() { + use axum_server::accept::Accept; + + let timeout = Duration::from_millis(300); + let (listener, _client) = bind_and_connect().await; + let (accepted, _) = listener.accept().await.expect("accept"); + let acceptor = HttpsRedirectAcceptor { + addr: listener.local_addr().expect("local addr"), + first_byte_timeout: timeout, + }; + let outcome = tokio::time::timeout(timeout * 10, acceptor.accept(accepted, ())).await; + match outcome { + Ok(Err(e)) => assert_eq!(e.kind(), std::io::ErrorKind::TimedOut, "{e}"), + Ok(Ok(_)) => panic!("acceptor passed a connection that sent nothing"), + Err(_) => panic!( + "acceptor waited more than {:?} for the first byte", + timeout * 10 + ), + } +} + +/// The deadline is dropped once the first byte arrives: a client that sends +/// a complete request late, but before the deadline, is served. +#[tokio::test] +async fn plaintext_listener_serves_a_request_that_arrives_before_the_deadline() { + use tokio::io::AsyncReadExt; + + let timeout = Duration::from_millis(600); + let (listener, mut client) = bind_and_connect().await; + let app = Router::new().route("/", axum::routing::get(|| async { "ok" })); + let server = tokio::spawn(serve_app(listener, app, timeout, None, None)); + + tokio::time::sleep(timeout / 3).await; + client + .write_all(b"GET / HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\n\r\n") + .await + .expect("write request"); + let mut response = Vec::new(); + tokio::time::timeout(timeout * 10, client.read_to_end(&mut response)) + .await + .expect("response within the bound") + .expect("read response"); + server.abort(); + let text = String::from_utf8_lossy(&response); + assert!(text.starts_with("HTTP/1.1 200"), "{text}"); + assert!(text.ends_with("ok"), "{text}"); +} + +/// The deadline is dropped exactly where the auto builder stops sniffing: a +/// byte that differs from the preface, or the whole preface; a prefix of it +/// keeps the deadline armed. +#[tokio::test] +async fn deadline_tracks_the_version_sniff() { + let mut adapter = FirstByteDeadline::new(tokio::io::empty(), Duration::from_secs(1)); + assert!(!adapter.sniff_decided(&H2_PREFACE[..16])); + assert!(!adapter.sniff_decided(&H2_PREFACE[16..23])); + assert!(adapter.sniff_decided(&H2_PREFACE[23..])); + + let mut adapter = FirstByteDeadline::new(tokio::io::empty(), Duration::from_secs(1)); + assert!(adapter.sniff_decided(b"G")); + + let mut adapter = FirstByteDeadline::new(tokio::io::empty(), Duration::from_secs(1)); + assert!(!adapter.sniff_decided(b"PRI")); + assert!(adapter.sniff_decided(b" + HTTP")); +} diff --git a/docs/manuals/05-admin-guide.md b/docs/manuals/05-admin-guide.md index ac8215e97..ed7960ccb 100755 --- a/docs/manuals/05-admin-guide.md +++ b/docs/manuals/05-admin-guide.md @@ -91,7 +91,7 @@ These settings require a server restart to take effect. | `bind_addr` | `127.0.0.1` | Network interface to bind | | `port` | `18443` | HTTP port | | `region` | `us-east-1` | AWS region for ARN generation. Requests must carry a credential scope for this region; any other region is rejected with `InvalidSignatureException`, as the service rejects a scope for a region other than the endpoint's. Baked into every backup ARN the server issues, so it must stay within `[a-z0-9-]` (a value outside that class makes the server's own backup ARNs "not valid"; the server logs a warning at startup). A backup ARN of the caller's account in another region is resolved in the server's region first, then as written; see the upgrade manual before changing this on a deployment with backups | -| `request_timeout_secs` | `30` | Seconds a connection may take to deliver its request headers, and seconds a request body may stall without delivering data, before the server closes it (a stalled body gets HTTP 408). The same value bounds how long an idle keep-alive connection waits for its next request, so SDK connection pools see idle connections closed after it (they reopen transparently), and it is the HTTP/2 keep-alive PING interval and timeout, so a stalled HTTP/2 connection is closed within twice the value. Must be at least 1 | +| `request_timeout_secs` | `30` | Seconds a connection may take to deliver its request headers, and seconds a request body may stall without delivering data, before the server closes it (a stalled body gets HTTP 408). A connection that is accepted, or that completes the TLS handshake, and then sends no byte is closed after the same number of seconds, as is one that sends only part of the 24-byte HTTP/2 preface. The same value bounds how long an idle keep-alive connection waits for its next request, so SDK connection pools see idle connections closed after it (they reopen transparently), and it is the HTTP/2 keep-alive PING interval and timeout, so a stalled HTTP/2 connection is closed within twice the value. Must be at least 1 | #### [storage] From 5462a494a5ffa6dad18a32afcea5118ba2f16d29 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:50:13 +0000 Subject: [PATCH 08/10] fix(server): refuse a request timeout above a day at startup 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. --- crates/app/src/init_helpers.rs | 2 +- crates/server/src/serve.rs | 42 ++++++++++++++++++++++++---- crates/server/src/serve/tests.rs | 47 ++++++++++++++++++++++++++++++++ docs/manuals/05-admin-guide.md | 2 +- 4 files changed, 86 insertions(+), 7 deletions(-) create mode 100644 crates/server/src/serve/tests.rs diff --git a/crates/app/src/init_helpers.rs b/crates/app/src/init_helpers.rs index 874a4ea69..849d75231 100755 --- a/crates/app/src/init_helpers.rs +++ b/crates/app/src/init_helpers.rs @@ -250,7 +250,7 @@ bind_addr = "{bind_addr}" # region = "us-east-1" # AWS region for ARN generation; requests must be signed for this region # run_dir = "{run_dir}" # Directory for PID file # throttling_enabled = false # Enable provisioned throughput throttling -# request_timeout_secs = 30 # Close a connection whose headers or body stall this long +# request_timeout_secs = 30 # Close a connection whose headers or body stall this long (1 to 86400) [server.tls] # TLS is mandatory. The server refuses to start with enabled = false. diff --git a/crates/server/src/serve.rs b/crates/server/src/serve.rs index 7c84fc8ff..1d7d47a03 100644 --- a/crates/server/src/serve.rs +++ b/crates/server/src/serve.rs @@ -145,6 +145,38 @@ impl ServeParams { } } +/// The largest `request_timeout_secs` the server accepts. +/// +/// hyper adds the HTTP/1 header read timeout to `Instant::now()` once per +/// connection, and that addition panics on overflow, so a value near +/// `u64::MAX` panics the task of every connection the server accepts. A day +/// is longer than any pause a client takes between connecting and sending a +/// request, and it leaves the addition far from the overflow point. +const MAX_REQUEST_TIMEOUT_SECS: u64 = 86_400; + +/// Check the `request_timeout_secs` value from the `[server]` section. +/// +/// Returns the startup error message when the value is outside the accepted +/// range, so misconfiguration fails fast instead of reaching the listener. +fn check_request_timeout_secs(secs: u64) -> Result<(), String> { + if secs == 0 { + return Err( + "Invalid [server] configuration: request_timeout_secs must be at least 1 \ + (0 would close every connection before it could send a request)." + .to_owned(), + ); + } + if secs > MAX_REQUEST_TIMEOUT_SECS { + return Err(format!( + "Invalid [server] configuration: request_timeout_secs must be at most \ + {MAX_REQUEST_TIMEOUT_SECS} (a day); the per-connection timers add it to the \ + current instant, and a larger value overflows that addition and panics the \ + connection task." + )); + } + Ok(()) +} + /// Run the ExtendDB server on the pre-bound listener in `params` until shutdown. /// /// On any error before the HTTP server starts, the PID file is removed and a @@ -375,11 +407,8 @@ async fn serve_inner(params: ServeParams, port: u16) -> anyhow::Result<()> { soft_ttl_seconds, negative_ttl_seconds, max_entries." ); } - if app_config.server.request_timeout_secs == 0 { - anyhow::bail!( - "Invalid [server] configuration: request_timeout_secs must be at least 1 \ - (0 would close every connection before it could send a request)." - ); + if let Err(e) = check_request_timeout_secs(app_config.server.request_timeout_secs) { + anyhow::bail!("{e}"); } if !cache_enabled { tracing::warn!( @@ -735,3 +764,6 @@ pub fn log_to_syslog_raw(msg: &str) { pub fn log_to_syslog_raw(msg: &str) { eprintln!("{msg}"); } + +#[cfg(test)] +mod tests; diff --git a/crates/server/src/serve/tests.rs b/crates/server/src/serve/tests.rs new file mode 100644 index 000000000..ae58d031f --- /dev/null +++ b/crates/server/src/serve/tests.rs @@ -0,0 +1,47 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Unit tests for the startup configuration checks. + +use super::*; + +#[test] +fn a_request_timeout_of_zero_is_refused() { + let err = check_request_timeout_secs(0).expect_err("0 is refused"); + assert!(err.contains("must be at least 1"), "{err}"); +} + +#[test] +fn a_request_timeout_above_a_day_is_refused() { + let err = + check_request_timeout_secs(MAX_REQUEST_TIMEOUT_SECS + 1).expect_err("86401 is refused"); + assert!(err.contains("must be at most 86400"), "{err}"); + + // The value that panicked every connection task before the ceiling. + let err = check_request_timeout_secs(u64::MAX).expect_err("u64::MAX is refused"); + assert!(err.contains("must be at most 86400"), "{err}"); +} + +#[test] +fn the_accepted_range_runs_from_one_second_to_a_day() { + for secs in [1, 30, 3600, MAX_REQUEST_TIMEOUT_SECS] { + check_request_timeout_secs(secs).unwrap_or_else(|e| panic!("{secs} is accepted: {e}")); + } +} + +/// The ceiling keeps the per-connection timers clear of the overflow that +/// made every connection task panic: the deadline hyper builds for a +/// connection is the current instant plus the timeout. +#[test] +fn the_ceiling_added_to_the_current_instant_does_not_overflow() { + let now = std::time::Instant::now(); + assert!( + now.checked_add(Duration::from_secs(MAX_REQUEST_TIMEOUT_SECS)) + .is_some(), + "the ceiling must be addable to an instant" + ); + assert!( + now.checked_add(Duration::from_secs(u64::MAX)).is_none(), + "u64::MAX seconds overflows, which is what the ceiling refuses" + ); +} diff --git a/docs/manuals/05-admin-guide.md b/docs/manuals/05-admin-guide.md index ed7960ccb..b4a7ffcc2 100755 --- a/docs/manuals/05-admin-guide.md +++ b/docs/manuals/05-admin-guide.md @@ -91,7 +91,7 @@ These settings require a server restart to take effect. | `bind_addr` | `127.0.0.1` | Network interface to bind | | `port` | `18443` | HTTP port | | `region` | `us-east-1` | AWS region for ARN generation. Requests must carry a credential scope for this region; any other region is rejected with `InvalidSignatureException`, as the service rejects a scope for a region other than the endpoint's. Baked into every backup ARN the server issues, so it must stay within `[a-z0-9-]` (a value outside that class makes the server's own backup ARNs "not valid"; the server logs a warning at startup). A backup ARN of the caller's account in another region is resolved in the server's region first, then as written; see the upgrade manual before changing this on a deployment with backups | -| `request_timeout_secs` | `30` | Seconds a connection may take to deliver its request headers, and seconds a request body may stall without delivering data, before the server closes it (a stalled body gets HTTP 408). A connection that is accepted, or that completes the TLS handshake, and then sends no byte is closed after the same number of seconds, as is one that sends only part of the 24-byte HTTP/2 preface. The same value bounds how long an idle keep-alive connection waits for its next request, so SDK connection pools see idle connections closed after it (they reopen transparently), and it is the HTTP/2 keep-alive PING interval and timeout, so a stalled HTTP/2 connection is closed within twice the value. Must be at least 1 | +| `request_timeout_secs` | `30` | Seconds a connection may take to deliver its request headers, and seconds a request body may stall without delivering data, before the server closes it (a stalled body gets HTTP 408). A connection that is accepted, or that completes the TLS handshake, and then sends no byte is closed after the same number of seconds, as is one that sends only part of the 24-byte HTTP/2 preface. The same value bounds how long an idle keep-alive connection waits for its next request, so SDK connection pools see idle connections closed after it (they reopen transparently), and it is the HTTP/2 keep-alive PING interval and timeout, so a stalled HTTP/2 connection is closed within twice the value. Must be at least 1 and at most 86400 (a day); the server refuses a larger value at startup, because the per-connection timers add it to the current instant and a value near `u64::MAX` overflows that addition | #### [storage] From 8a440d8b6ee0ebfc2f37fed4d93e853620b295b1 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:50:14 +0000 Subject: [PATCH 09/10] fix(auth): read a token header that is not visible ASCII as presented 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. --- crates/auth/src/lib.rs | 6 +++++- crates/auth/src/tests.rs | 25 +++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/crates/auth/src/lib.rs b/crates/auth/src/lib.rs index a67c4de41..8904b56f0 100644 --- a/crates/auth/src/lib.rs +++ b/crates/auth/src/lib.rs @@ -188,6 +188,10 @@ impl AuthProvider for BuiltinAuthProvider { // S-5: Compare the session token in constant time and defer the // rejection until after signature verification, so neither the token's // byte contents nor its presence changes the failure-path timing. + // A header value that is not visible ASCII cannot be compared with + // the stored token, and it is still a token the client presented, so + // presence is read from the header map and the value from its text. + let token_header_present = headers.contains_key("x-amz-security-token"); let presented_token = headers .get("x-amz-security-token") .and_then(|v| v.to_str().ok()); @@ -196,7 +200,7 @@ impl AuthProvider for BuiltinAuthProvider { let expected = credential.session_token.as_deref().unwrap_or(""); sigv4::verify::constant_time_eq(token.as_bytes(), expected.as_bytes()) } else { - presented_token.is_none() + !token_header_present }; // Verify SigV4 signature — always, even for inactive keys (S-5). diff --git a/crates/auth/src/tests.rs b/crates/auth/src/tests.rs index 11e5248ef..109fa6c34 100644 --- a/crates/auth/src/tests.rs +++ b/crates/auth/src/tests.rs @@ -265,6 +265,31 @@ async fn empty_session_token_header_with_a_long_term_key_is_rejected() { } } +/// A token header whose value is not visible ASCII is a presented token +/// too. It cannot be compared with a stored token, and for a long-term key +/// the rule is that only an absent header passes, so the request is +/// rejected rather than served as an unauthenticated-token request. +#[tokio::test] +async fn non_ascii_session_token_header_with_a_long_term_key_is_rejected() { + // Signed without a token, then the header is added unsigned: SigV4 + // covers the signed headers only, so the signature stays valid. + let mut headers = signed_headers("AKIAEXTENDDB00000000", "us-east-1", None, b"{}"); + headers.insert( + "x-amz-security-token", + HeaderValue::from_bytes(&[0xc3, 0xa9]).expect("a header value with a byte above 0x7f"), + ); + assert!( + headers["x-amz-security-token"].to_str().is_err(), + "the test header must not be readable as a string" + ); + match provider(false).authenticate(&headers, b"{}").await { + Err(DynamoDbError::UnrecognizedClientException(msg)) => { + assert_eq!(msg, "The security token included in the request is invalid"); + } + other => panic!("Expected UnrecognizedClientException, got: {other:?}"), + } +} + /// Measured 2026-09-16: a scope wrong in both region and service reports /// both sentences in one message, region first. #[tokio::test] From 49fb3e2cf495d796cff338056ccce3b37950385f Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:50:14 +0000 Subject: [PATCH 10/10] test(auth): assert the refusal in the data-plane duplicate host case 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. --- tests/test_auth_error_fidelity.py | 40 +++++++++++++++---------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/tests/test_auth_error_fidelity.py b/tests/test_auth_error_fidelity.py index 9e0f9adb4..ffe37fe70 100755 --- a/tests/test_auth_error_fidelity.py +++ b/tests/test_auth_error_fidelity.py @@ -352,29 +352,27 @@ def test_unknown_key_with_another_region_reports_the_scope(self): def test_duplicate_host_header_is_refused(self): # The service's front end refuses a request with two host headers - # before it reaches signing (400, connection closed). - import http.client # noqa: PLC0415 - import ssl # noqa: PLC0415 - from urllib.parse import urlparse # noqa: PLC0415 - + # before it reaches signing (measured 2026-10-23 on POST /: 400, + # Connection: close, and an HTML error page instead of a JSON + # error). The body assertion runs against extenddb only, and it is + # what separates the listener's refusal from the handler's + # MissingAuthenticationToken, which answers 400 as well. u = urlparse(self.url) - ctx = ssl.create_default_context() + status, text, connection = self._raw_request( + "POST", + "/", + [ + ("Host", u.netloc), + ("Host", "evil.example.com"), + ("X-Amz-Target", "DynamoDB_20120810.ListTables"), + ("Content-Type", "application/x-amz-json-1.0"), + ], + body=b"{}", + ) + assert status == 400, (status, text[:200]) + assert connection == "close", connection if self.endpoint_url is not None: - ctx.check_hostname = False - ctx.verify_mode = ssl.CERT_NONE - conn = http.client.HTTPSConnection(u.hostname, u.port or 443, context=ctx, timeout=30) - conn.putrequest("POST", "/", skip_host=True, skip_accept_encoding=True) - conn.putheader("Host", u.netloc) - conn.putheader("Host", "evil.example.com") - conn.putheader("X-Amz-Target", "DynamoDB_20120810.ListTables") - conn.putheader("Content-Type", "application/x-amz-json-1.0") - conn.putheader("Content-Length", "2") - conn.endheaders() - conn.send(b"{}") - resp = conn.getresponse() - resp.read() - conn.close() - assert resp.status == 400 + assert text == "400 Bad Request", text[:200] def _raw_request(self, method: str, path: str, headers: list[tuple[str, str]], body: bytes = b""): """Send one request with each header on its own line, repeats included.