diff --git a/crates/api-core/src/node_auth.rs b/crates/api-core/src/node_auth.rs index a99724030b..eb89017bb5 100644 --- a/crates/api-core/src/node_auth.rs +++ b/crates/api-core/src/node_auth.rs @@ -70,6 +70,8 @@ enum RejectReason { Algorithm(Algorithm), #[error("no x5c certificate chain in the JWT header")] NoChain, + #[error("x5c chain has {actual} certificates; maximum is {maximum}")] + ChainTooLong { actual: usize, maximum: usize }, #[error("x5c chain did not verify against the trusted roots: {0}")] Chain(rustls::Error), #[error("leaf certificate could not be parsed as X.509")] @@ -135,23 +137,9 @@ impl NodeJwtValidator { }) } - /// Re-reads the root CA bundle from disk and returns a verifier built from - /// it, without installing it. + /// Test-only convenience that reads the root CA bundle from disk. /// - /// The TLS listener reloads the same file every five minutes to pick up - /// cert-manager rotations; without this the validator would keep its - /// startup snapshot and start rejecting tokens whose `x5c` chains to the - /// new CA — which, with `mtls_enabled = false`, locks nodes out until the - /// API restarts. - /// - /// The fallible read is split from [`install_roots`](Self::install_roots) - /// so the listener can build this and its new TLS acceptor before - /// committing either. Both derive from this same bundle, and the listener - /// must never end up trusting one generation of it on the TLS path and - /// another on the token path — nor drop the acceptor and serve plaintext - /// while bearer auth stays armed. A failed build leaves the previous - /// verifier in place: a half-written bundle must not disarm node auth. - /// Test-only convenience. Production goes through + /// Production goes through /// [`build_roots_from_pem`](Self::build_roots_from_pem) so the listener can /// share one read of the bundle with the TLS acceptor. #[cfg(test)] @@ -164,12 +152,21 @@ impl NodeJwtValidator { self.build_roots_from_pem(&pem) } - /// Builds anchors from a bundle the caller already read. + /// Builds anchors from a bundle the caller already read, without installing + /// them. /// - /// The listener uses this so the TLS acceptor and this validator are built - /// from the *same* bytes: reading the file twice lets a rotation land - /// between the two reads, leaving each path trusting a different generation - /// of the client CA. + /// The listener reloads the same file every five minutes to pick up + /// cert-manager rotations. Building both the TLS acceptor and this + /// validator from the *same* bytes prevents a rotation between two reads + /// from leaving each path trusting a different client-CA generation. A + /// stale validator would reject tokens chaining to the new CA and, with + /// mTLS disabled, lock nodes out until the API restarts. + /// + /// The fallible build is separate from [`install_roots`](Self::install_roots) + /// so the listener can build this verifier and its new TLS acceptor before + /// committing either. It must never drop the acceptor and serve plaintext + /// while bearer auth remains armed. A failed build leaves the previous + /// verifier in place: a half-written bundle must not disarm node auth. pub(crate) fn build_roots_from_pem( &self, pem: &[u8], @@ -177,8 +174,11 @@ impl NodeJwtValidator { Self::verifier_from_pem(&self.root_cafile_path, pem) } - /// Installs anchors from [`build_roots`](Self::build_roots). Infallible, so - /// it is safe to call in a commit phase alongside other swaps. + /// Installs anchors from [`build_roots_from_pem`](Self::build_roots_from_pem). + /// + /// The listener calls this only after it has also built a TLS acceptor from + /// the same bundle. Because this method is infallible, it is safe in the + /// commit phase that keeps TLS and bearer token trust anchors in sync. pub(crate) fn install_roots(&self, cert_verifier: Arc) { *self .cert_verifier @@ -229,10 +229,20 @@ impl NodeJwtValidator { } // 1. The certificate chain must verify against the trusted roots. + let encoded_chain = header + .x5c + .as_ref() + .filter(|chain| !chain.is_empty()) + .ok_or(RejectReason::NoChain)?; + if encoded_chain.len() > ::rpc::node_jwt::NODE_JWT_MAX_X5C_CERTIFICATES { + return Err(RejectReason::ChainTooLong { + actual: encoded_chain.len(), + maximum: ::rpc::node_jwt::NODE_JWT_MAX_X5C_CERTIFICATES, + }); + } let chain = header .x5c_der() .map_err(RejectReason::Malformed)? - .filter(|chain| !chain.is_empty()) .ok_or(RejectReason::NoChain)?; let leaf = CertificateDer::from(chain[0].clone()); let intermediates: Vec = chain[1..] @@ -511,6 +521,57 @@ mod tests { assert!(validator.spiffe_id_from_bearer(&no_chain).is_none()); } + /// The encoded list is checked before Base64 decoding so an untrusted + /// header cannot make certificate-path validation scale with its length. + #[test] + fn x5c_certificate_count_is_capped_before_decoding() { + #[derive(Debug, Eq, PartialEq)] + enum Rejection { + Malformed, + ChainTooLong { actual: usize, maximum: usize }, + } + + let dir = tempfile::tempdir().expect("tempdir"); + let pki = test_pki(MACHINE_PATH); + let validator = validator_for(&dir, &pki.ca_pem); + let encoding_key = + jsonwebtoken::EncodingKey::from_ec_pem(pki.key_pem.as_bytes()).expect("encoding key"); + + check_values( + [ + Check { + scenario: "the maximum certificate count still reaches decoding", + input: ::rpc::node_jwt::NODE_JWT_MAX_X5C_CERTIFICATES, + expect: Rejection::Malformed, + }, + Check { + scenario: "one certificate above the limit is rejected before decoding", + input: ::rpc::node_jwt::NODE_JWT_MAX_X5C_CERTIFICATES + 1, + expect: Rejection::ChainTooLong { + actual: ::rpc::node_jwt::NODE_JWT_MAX_X5C_CERTIFICATES + 1, + maximum: ::rpc::node_jwt::NODE_JWT_MAX_X5C_CERTIFICATES, + }, + }, + ], + |certificate_count| { + let mut header = jsonwebtoken::Header::new(Algorithm::ES256); + // This string is deliberately invalid Base64: only the overlong + // case may reach `ChainTooLong` without trying to decode it. + header.x5c = Some(vec!["@".to_string(); certificate_count]); + let token = jsonwebtoken::encode(&header, &serde_json::json!({}), &encoding_key) + .expect("token encodes"); + + match validator.validate(&token) { + Err(RejectReason::Malformed(_)) => Rejection::Malformed, + Err(RejectReason::ChainTooLong { actual, maximum }) => { + Rejection::ChainTooLong { actual, maximum } + } + result => panic!("unexpected node-token validation result: {result:?}"), + } + }, + ); + } + /// Both the identity cross-check and algorithm pin are independent of the /// x5c-chain verification. Keep explicit coverage so an otherwise-valid /// certificate cannot accidentally make either attacker-controlled header diff --git a/crates/rpc/src/node_jwt.rs b/crates/rpc/src/node_jwt.rs index 532cd0a8e7..2435f77f70 100644 --- a/crates/rpc/src/node_jwt.rs +++ b/crates/rpc/src/node_jwt.rs @@ -32,6 +32,7 @@ //! mTLS client cert remains the only credential. use std::io::Cursor; +use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Arc, RwLock}; use std::task::{Context, Poll}; use std::time::{SystemTime, UNIX_EPOCH}; @@ -50,6 +51,19 @@ pub const NODE_JWT_AUDIENCE: &str = "nico-api"; /// re-mint locally, so a leaked one ages out in minutes. pub const NODE_JWT_TTL_SECS: u64 = 300; +/// Maximum certificates carried in a node-auth JWT's `x5c` header. +/// +/// Node certificates normally include a leaf and issuing CA. This leaves room +/// for additional intermediates while bounding the verifier's work on an +/// untrusted request. +pub const NODE_JWT_MAX_X5C_CERTIFICATES: usize = 4; + +/// Avoid repeatedly warning from the per-request token-minting path for the +/// same persistent certificate-bundle configuration error. +const OVER_LIMIT_WARNING_INTERVAL_SECS: u64 = NODE_JWT_TTL_SECS; + +static LAST_OVER_LIMIT_WARNING_AT: AtomicU64 = AtomicU64::new(0); + /// A cached token is reused until it has less than this long left, then /// re-minted. Comfortably above per-request latency, comfortably below TTL. const REMINT_MARGIN_SECS: u64 = 60; @@ -66,6 +80,9 @@ pub enum NodeJwtError { BadKey(String), #[error("client private key does not match the certificate's public key")] KeyCertMismatch, + /// The client certificate bundle exceeds the node-auth JWT header limit. + #[error("client certificate file has {actual} certificates; maximum is {maximum}")] + TooManyCertificates { actual: usize, maximum: usize }, #[error("JWT signing failed: {0}")] Sign(#[from] jsonwebtoken::errors::Error), #[error("system clock is before the UNIX epoch")] @@ -124,9 +141,11 @@ impl NodeJwtMinter { } /// Returns a currently-valid token, re-minting if the cached one is - /// missing or close to expiry. Returns `None` (and logs at debug) when - /// minting is impossible — e.g. the cert/key files don't exist yet — so - /// callers degrade gracefully to mTLS-only. + /// missing or close to expiry. Returns `None` when minting is impossible + /// — e.g. the cert/key files don't exist yet — so callers degrade + /// gracefully to mTLS-only. Transient failures log at debug; an + /// over-limit certificate bundle logs at most once every five minutes per + /// process because it requires an operator configuration change. pub fn current(&self) -> Option { self.current_with_expiry().map(|(token, _)| token) } @@ -158,6 +177,17 @@ impl NodeJwtMinter { .unwrap_or_else(|poisoned| poisoned.into_inner()) = Some(minted); Some(result) } + Err(error @ NodeJwtError::TooManyCertificates { .. }) => { + if should_log_over_limit_warning(now, &LAST_OVER_LIMIT_WARNING_AT) { + tracing::warn!( + target: "node_auth", + cert_path = %self.cert_path, + %error, + "node-auth: client certificate bundle cannot mint node JWT; continuing with mTLS only" + ); + } + None + } Err(error) => { tracing::debug!( target: "node_auth", @@ -178,6 +208,12 @@ impl NodeJwtMinter { .collect::, _>>() .map_err(|e| NodeJwtError::BadCertificate(e.to_string()))?; let leaf = chain.first().ok_or(NodeJwtError::NoCertificate)?; + if chain.len() > NODE_JWT_MAX_X5C_CERTIFICATES { + return Err(NodeJwtError::TooManyCertificates { + actual: chain.len(), + maximum: NODE_JWT_MAX_X5C_CERTIFICATES, + }); + } let sub = spiffe_uri_from_cert(leaf.as_ref())?; let secret = parse_secret_key(&key_pem)?; @@ -214,6 +250,20 @@ fn unix_now() -> Result { .map_err(|_| NodeJwtError::Clock) } +fn should_log_over_limit_warning(now: u64, last_warning_at: &AtomicU64) -> bool { + let mut last = last_warning_at.load(Ordering::Relaxed); + loop { + if last != 0 && now.saturating_sub(last) < OVER_LIMIT_WARNING_INTERVAL_SECS { + return false; + } + match last_warning_at.compare_exchange_weak(last, now, Ordering::Relaxed, Ordering::Relaxed) + { + Ok(_) => return true, + Err(current) => last = current, + } + } +} + /// Extracts the single SPIFFE URI SAN from the certificate — the same field /// the server's authn layer maps to a machine principal for mTLS. fn spiffe_uri_from_cert(der: &[u8]) -> Result { @@ -356,6 +406,9 @@ where #[cfg(test)] mod tests { + use std::sync::atomic::AtomicU64; + + use carbide_test_support::{Check, check_values}; use jsonwebtoken::{DecodingKey, Validation}; use serde::Deserialize; @@ -450,6 +503,81 @@ mod tests { assert_eq!(claims.exp - claims.iat, NODE_JWT_TTL_SECS); } + #[test] + fn certificate_chain_length_must_fit_the_node_jwt_header() { + #[derive(Debug, Eq, PartialEq)] + enum Outcome { + Mints, + TooManyCertificates { actual: usize, maximum: usize }, + } + + let (cert_pem, key_pem) = cert_and_key(); + let dir = tempfile::tempdir().expect("tempdir"); + + check_values( + [ + Check { + scenario: "the maximum certificate count is minted", + input: NODE_JWT_MAX_X5C_CERTIFICATES, + expect: Outcome::Mints, + }, + Check { + scenario: "one certificate above the limit is rejected", + input: NODE_JWT_MAX_X5C_CERTIFICATES + 1, + expect: Outcome::TooManyCertificates { + actual: NODE_JWT_MAX_X5C_CERTIFICATES + 1, + maximum: NODE_JWT_MAX_X5C_CERTIFICATES, + }, + }, + ], + |certificate_count| { + let minter = NodeJwtMinter::new( + write_temp(&dir, "cert.pem", &cert_pem.repeat(certificate_count)), + write_temp(&dir, "key.pem", &key_pem), + ); + + match minter.mint(unix_now().expect("system clock")) { + Ok(_) => Outcome::Mints, + Err(NodeJwtError::TooManyCertificates { actual, maximum }) => { + Outcome::TooManyCertificates { actual, maximum } + } + Err(error) => panic!("unexpected node-token mint error: {error:?}"), + } + }, + ); + } + + #[test] + fn over_limit_bundle_warning_is_rate_limited() { + check_values( + [ + Check { + scenario: "the first process-wide failure is reported", + input: (0, 1_000), + expect: true, + }, + Check { + scenario: "a failure one second later is suppressed", + input: (1_000, 1_001), + expect: false, + }, + Check { + scenario: "a failure just before the interval ends is suppressed", + input: (1_000, 1_000 + OVER_LIMIT_WARNING_INTERVAL_SECS - 1), + expect: false, + }, + Check { + scenario: "a failure at the interval boundary is reported", + input: (1_000, 1_000 + OVER_LIMIT_WARNING_INTERVAL_SECS), + expect: true, + }, + ], + |(last_warning_at, now)| { + should_log_over_limit_warning(now, &AtomicU64::new(last_warning_at)) + }, + ); + } + #[test] fn sec1_key_pem_is_accepted() { // Vault PKI hands out SEC1-encoded EC keys; re-encode the test key the diff --git a/docs/design/machine-identity/node-auth-jwt.md b/docs/design/machine-identity/node-auth-jwt.md index 4c4765d0b0..83c220e9e6 100644 --- a/docs/design/machine-identity/node-auth-jwt.md +++ b/docs/design/machine-identity/node-auth-jwt.md @@ -276,6 +276,14 @@ against the root CA bundle it already holds (`[tls] root_cafile_path`). There is no JWKS endpoint, no key registry, and no key distribution problem — CA rotation is handled wherever the root bundle is handled today. +An `x5c` chain contains at most four certificates, including the leaf. The +node refuses to mint a token when its client-certificate bundle exceeds that +limit, emits a `node_auth` warning at most once every five minutes per process, +and continues with mTLS only. The API rejects an over-limit token before Base64 +decoding its certificates or building a verification path. Keep the node's +bundle within the limit; otherwise, a deployment with `mtls_enabled = false` +has no node authentication path. + **CA rotation** moves both consumers of that bundle together. The TLS listener already re-reads the file every five minutes for cert-manager rotations; the validator's trust anchors are held behind a lock and refreshed