diff --git a/conformance/src/bin/server.rs b/conformance/src/bin/server.rs index 3b552d7d3..9cca92618 100644 --- a/conformance/src/bin/server.rs +++ b/conformance/src/bin/server.rs @@ -70,7 +70,8 @@ impl ConformanceServer { subscriptions: Arc::new(Mutex::new(HashMap::new())), next_subscription: Arc::new(AtomicU64::new(0)), log_level: Arc::new(Mutex::new(LoggingLevel::Debug)), - request_state_codec: RequestStateCodec::new(REQUEST_STATE_KEY), + request_state_codec: RequestStateCodec::try_new(REQUEST_STATE_KEY) + .expect("conformance request-state key meets the minimum length"), tasks: TaskManager::new(), } } diff --git a/crates/rmcp/Cargo.toml b/crates/rmcp/Cargo.toml index 486fdf873..fe0e65a5c 100644 --- a/crates/rmcp/Cargo.toml +++ b/crates/rmcp/Cargo.toml @@ -69,6 +69,7 @@ base64 = { version = "0.23", optional = true } # for SEP-2322 requestState integrity sealing (opt-in via the `request-state` feature) hmac = { version = "0.13", optional = true } sha2 = { version = "0.11", optional = true } +zeroize = { version = "1", optional = true } # for HTTP client reqwest = { version = "0.13.2", default-features = false, features = [ @@ -127,7 +128,7 @@ macros = ["dep:rmcp-macros", "dep:pastey"] elicitation = ["dep:url"] # SEP-2322 requestState integrity helper (HMAC-SHA256 seal/open codec) -request-state = ["dep:hmac", "dep:sha2", "base64"] +request-state = ["dep:hmac", "dep:sha2", "dep:zeroize", "base64"] # reqwest http client __reqwest = ["dep:reqwest"] diff --git a/crates/rmcp/src/model/request_state.rs b/crates/rmcp/src/model/request_state.rs index 5ed0f7de8..9075095d6 100644 --- a/crates/rmcp/src/model/request_state.rs +++ b/crates/rmcp/src/model/request_state.rs @@ -40,9 +40,10 @@ //! //! ``` //! use rmcp::model::{RequestStateCodec, SealOptions}; +//! # fn main() -> Result<(), rmcp::model::RequestStateError> { //! //! // Derive the key from a per-process secret; keep it out of client reach. -//! let codec = RequestStateCodec::new(b"a-32-byte-or-longer-secret-key!!!"); +//! let codec = RequestStateCodec::try_new(b"a-32-byte-or-longer-secret-key!!!")?; //! //! // Bind the state to the caller and the originating request. //! let context = b"user:alice|tools/call:weather"; @@ -53,11 +54,13 @@ //! //! // On retry the client echoes `sealed` back untouched; the server re-derives //! // the same context and opens it. -//! let opened = codec.open_with(&sealed, context).expect("integrity check passes"); +//! let opened = codec.open_with(&sealed, context)?; //! assert_eq!(opened, b"step=2"); //! //! // A different principal (different context) is rejected. //! assert!(codec.open_with(&sealed, b"user:bob|tools/call:weather").is_err()); +//! # Ok(()) +//! # } //! ``` use std::time::Duration; @@ -67,6 +70,7 @@ use hmac::{Hmac, KeyInit, Mac}; use serde::{Serialize, de::DeserializeOwned}; use sha2::Sha256; use thiserror::Error; +use zeroize::Zeroizing; type HmacSha256 = Hmac; @@ -81,10 +85,17 @@ const DOMAIN: &[u8] = b"rmcp/mrtr/request-state/v1"; /// front of every sealed body. `0` means "no expiry". const EXPIRY_LEN: usize = 8; -/// Errors returned when opening a sealed [`RequestStateCodec`] value. +/// Errors returned when constructing a [`RequestStateCodec`] or processing a +/// sealed value. #[derive(Debug, Error)] #[non_exhaustive] pub enum RequestStateError { + /// The signing key is shorter than [`RequestStateCodec::MIN_KEY_LENGTH`]. + #[error( + "request state signing key is too short: expected at least {minimum} bytes, got {actual}" + )] + KeyTooShort { minimum: usize, actual: usize }, + /// The value is not a well-formed sealed request state (wrong prefix or /// missing sections). #[error("request state is malformed or uses an unsupported format")] @@ -156,11 +167,12 @@ impl<'a> SealOptions<'a> { /// [`open`](Self::open) a value, so it has to survive across the rounds of a /// single MRTR exchange (e.g. a stable per-process or per-deployment secret). /// -/// The key may be any length; HMAC internally normalizes it. For meaningful -/// security use a high-entropy key of at least 32 bytes. +/// Use [`try_new`](Self::try_new) to require at least +/// [`MIN_KEY_LENGTH`](Self::MIN_KEY_LENGTH) bytes of high-entropy key material. +/// The stored key material is zeroized when the codec is dropped. #[derive(Clone)] pub struct RequestStateCodec { - key: Box<[u8]>, + key: Zeroizing>, } impl std::fmt::Debug for RequestStateCodec { @@ -173,11 +185,33 @@ impl std::fmt::Debug for RequestStateCodec { } impl RequestStateCodec { - /// Creates a codec from a signing key. + /// Minimum accepted signing-key length in bytes. + pub const MIN_KEY_LENGTH: usize = 32; + + /// Creates a codec from a signing key without validating its length. + /// Prefer [`try_new`](Self::try_new) for new integrations. pub fn new(key: impl Into>) -> Self { Self { - key: key.into().into_boxed_slice(), + key: Zeroizing::new(key.into()), + } + } + + /// Creates a codec from a signing key after validating its length. + /// + /// # Errors + /// + /// Returns [`RequestStateError::KeyTooShort`] when `key` contains fewer + /// than [`MIN_KEY_LENGTH`](Self::MIN_KEY_LENGTH) bytes. + pub fn try_new(key: impl Into>) -> Result { + let key = Zeroizing::new(key.into()); + if key.len() < Self::MIN_KEY_LENGTH { + return Err(RequestStateError::KeyTooShort { + minimum: Self::MIN_KEY_LENGTH, + actual: key.len(), + }); } + + Ok(Self { key }) } /// Seals raw bytes into an opaque, integrity-protected string suitable for @@ -347,8 +381,8 @@ impl RequestStateCodec { /// body. The length prefix keeps the `associated_data`/`body` boundary /// unambiguous so distinct inputs cannot collide. fn mac_for(&self, associated_data: &[u8], body: &[u8]) -> HmacSha256 { - let mut mac = - HmacSha256::new_from_slice(&self.key).expect("HMAC accepts keys of any length"); + let mut mac = HmacSha256::new_from_slice(self.key.as_slice()) + .expect("HMAC accepts keys of any length"); mac.update(DOMAIN); mac.update(&(associated_data.len() as u64).to_be_bytes()); mac.update(associated_data); @@ -365,9 +399,41 @@ impl RequestStateCodec { mod tests { use super::*; + #[test] + fn try_new_accepts_key_at_minimum_length() { + let result = RequestStateCodec::try_new(vec![0; RequestStateCodec::MIN_KEY_LENGTH]); + + assert!(result.is_ok(), "unexpected result: {result:?}"); + } + + #[test] + fn try_new_rejects_key_below_minimum_length() { + let actual = RequestStateCodec::MIN_KEY_LENGTH - 1; + let error = RequestStateCodec::try_new(vec![0; actual]).unwrap_err(); + + assert!( + matches!( + &error, + RequestStateError::KeyTooShort { + minimum: RequestStateCodec::MIN_KEY_LENGTH, + actual: error_actual, + } if *error_actual == actual + ), + "unexpected error: {error}" + ); + } + + #[test] + fn new_accepts_short_key_for_backward_compatibility() { + let codec = RequestStateCodec::new(b"key".to_vec()); + + assert_eq!(codec.open(&codec.seal(b"state")).unwrap(), b"state"); + } + #[test] fn seal_open_roundtrips_bytes() { - let codec = RequestStateCodec::new(b"test-key-test-key-test-key-32byte".to_vec()); + let codec = + RequestStateCodec::try_new(b"test-key-test-key-test-key-32byte".to_vec()).unwrap(); let sealed = codec.seal(b"hello world"); assert!(sealed.starts_with("rs1.")); assert_eq!(codec.open(&sealed).unwrap(), b"hello world"); @@ -380,7 +446,8 @@ mod tests { tool: String, round: u32, } - let codec = RequestStateCodec::new(b"another-strong-signing-key-here!!".to_vec()); + let codec = + RequestStateCodec::try_new(b"another-strong-signing-key-here!!".to_vec()).unwrap(); let state = State { tool: "weather".into(), round: 3, @@ -392,14 +459,16 @@ mod tests { #[test] fn empty_payload_roundtrips() { - let codec = RequestStateCodec::new(b"k".to_vec()); + let codec = + RequestStateCodec::try_new(b"empty-payload-test-signing-key!!".to_vec()).unwrap(); let sealed = codec.seal(b""); assert_eq!(codec.open(&sealed).unwrap(), b""); } #[test] fn tampered_payload_is_rejected() { - let codec = RequestStateCodec::new(b"signing-key-signing-key-signing!!".to_vec()); + let codec = + RequestStateCodec::try_new(b"signing-key-signing-key-signing!!".to_vec()).unwrap(); let sealed = codec.seal(b"amount=100"); // Replace the body section but keep the original tag. @@ -416,8 +485,10 @@ mod tests { #[test] fn different_key_is_rejected() { - let signer = RequestStateCodec::new(b"the-real-signing-key-value-here!!".to_vec()); - let attacker = RequestStateCodec::new(b"a-totally-different-forged-key!!!".to_vec()); + let signer = + RequestStateCodec::try_new(b"the-real-signing-key-value-here!!".to_vec()).unwrap(); + let attacker = + RequestStateCodec::try_new(b"a-totally-different-forged-key!!!".to_vec()).unwrap(); let sealed = signer.seal(b"trusted"); assert!(matches!( attacker.open(&sealed), @@ -427,7 +498,8 @@ mod tests { #[test] fn appended_bytes_are_rejected() { - let codec = RequestStateCodec::new(b"key-key-key-key-key-key-key-key!!".to_vec()); + let codec = + RequestStateCodec::try_new(b"key-key-key-key-key-key-key-key!!".to_vec()).unwrap(); let mut sealed = codec.seal(b"state"); sealed.push('x'); assert!(codec.open(&sealed).is_err()); @@ -435,7 +507,8 @@ mod tests { #[test] fn wrong_version_prefix_is_malformed() { - let codec = RequestStateCodec::new(b"key".to_vec()); + let codec = + RequestStateCodec::try_new(b"wrong-version-test-signing-key!!".to_vec()).unwrap(); let sealed = codec.seal(b"state"); let bumped = sealed.replacen("rs1.", "rs2.", 1); assert!(matches!( @@ -446,7 +519,8 @@ mod tests { #[test] fn missing_sections_are_malformed() { - let codec = RequestStateCodec::new(b"key".to_vec()); + let codec = + RequestStateCodec::try_new(b"missing-sections-test-signing-key".to_vec()).unwrap(); assert!(matches!( codec.open("rs1"), Err(RequestStateError::MalformedFormat) @@ -463,7 +537,8 @@ mod tests { #[test] fn non_base64_sections_are_invalid_encoding() { - let codec = RequestStateCodec::new(b"key".to_vec()); + let codec = + RequestStateCodec::try_new(b"invalid-base64-test-signing-key!!".to_vec()).unwrap(); assert!(matches!( codec.open("rs1.!!!!.!!!!"), Err(RequestStateError::InvalidEncoding) @@ -472,7 +547,8 @@ mod tests { #[test] fn debug_does_not_leak_key() { - let codec = RequestStateCodec::new(b"super-secret-key".to_vec()); + let codec = + RequestStateCodec::try_new(b"super-secret-key-super-secret-key!!".to_vec()).unwrap(); let rendered = format!("{codec:?}"); assert!(!rendered.contains("super-secret-key")); assert!(rendered.contains("redacted")); @@ -483,7 +559,8 @@ mod tests { #[test] fn matching_context_opens() { - let codec = RequestStateCodec::new(b"key-key-key-key-key-key-key-key!!".to_vec()); + let codec = + RequestStateCodec::try_new(b"key-key-key-key-key-key-key-key!!".to_vec()).unwrap(); let ctx = b"user:alice|tools/call:weather"; let sealed = codec.seal_with(b"state", &SealOptions::new().associated_data(ctx)); assert_eq!(codec.open_with(&sealed, ctx).unwrap(), b"state"); @@ -491,7 +568,8 @@ mod tests { #[test] fn different_context_is_rejected() { - let codec = RequestStateCodec::new(b"key-key-key-key-key-key-key-key!!".to_vec()); + let codec = + RequestStateCodec::try_new(b"key-key-key-key-key-key-key-key!!".to_vec()).unwrap(); let sealed = codec.seal_with(b"state", &SealOptions::new().associated_data(b"user:alice")); assert!(matches!( @@ -502,7 +580,8 @@ mod tests { #[test] fn missing_context_is_rejected() { - let codec = RequestStateCodec::new(b"key-key-key-key-key-key-key-key!!".to_vec()); + let codec = + RequestStateCodec::try_new(b"key-key-key-key-key-key-key-key!!".to_vec()).unwrap(); let sealed = codec.seal_with(b"state", &SealOptions::new().associated_data(b"user:alice")); // Opening without the associated data must fail closed. @@ -520,7 +599,7 @@ mod tests { #[test] fn within_ttl_opens() { - let codec = RequestStateCodec::new(KEY.to_vec()); + let codec = RequestStateCodec::try_new(KEY.to_vec()).unwrap(); let sealed = codec.seal_at( b"state", &SealOptions::new().ttl(Duration::from_secs(60)), @@ -532,7 +611,7 @@ mod tests { #[test] fn past_ttl_is_expired() { - let codec = RequestStateCodec::new(KEY.to_vec()); + let codec = RequestStateCodec::try_new(KEY.to_vec()).unwrap(); let sealed = codec.seal_at( b"state", &SealOptions::new().ttl(Duration::from_secs(60)), @@ -547,14 +626,14 @@ mod tests { #[test] fn no_ttl_never_expires() { - let codec = RequestStateCodec::new(KEY.to_vec()); + let codec = RequestStateCodec::try_new(KEY.to_vec()).unwrap(); let sealed = codec.seal_at(b"state", &SealOptions::new(), 1_000); assert_eq!(codec.open_at(&sealed, &[], i64::MAX).unwrap(), b"state"); } #[test] fn ttl_and_associated_data_combine() { - let codec = RequestStateCodec::new(KEY.to_vec()); + let codec = RequestStateCodec::try_new(KEY.to_vec()).unwrap(); let ctx = b"user:alice"; let sealed = codec.seal_at( b"state", diff --git a/crates/rmcp/src/transport/auth.rs b/crates/rmcp/src/transport/auth.rs index 2b20391ac..ae4a99d63 100644 --- a/crates/rmcp/src/transport/auth.rs +++ b/crates/rmcp/src/transport/auth.rs @@ -914,7 +914,7 @@ impl JwtSigningAlgorithm { /// This supports two authentication methods: /// - `ClientSecret`: credentials sent in the request body /// - `PrivateKeyJwt`: RFC 7523 signed JWT assertion (requires `auth-client-credentials-jwt` feature) -#[derive(Debug, Clone)] +#[derive(Clone)] #[non_exhaustive] pub enum ClientCredentialsConfig { /// Client secret authentication (credentials in request body) @@ -937,6 +937,42 @@ pub enum ClientCredentialsConfig { }, } +impl std::fmt::Debug for ClientCredentialsConfig { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::ClientSecret { + client_id, + scopes, + resource, + .. + } => f + .debug_struct("ClientSecret") + .field("client_id", client_id) + .field("client_secret", &"") + .field("scopes", scopes) + .field("resource", resource) + .finish(), + #[cfg(feature = "auth-client-credentials-jwt")] + Self::PrivateKeyJwt { + client_id, + signing_algorithm, + token_endpoint_audience, + scopes, + resource, + .. + } => f + .debug_struct("PrivateKeyJwt") + .field("client_id", client_id) + .field("signing_key", &"") + .field("signing_algorithm", signing_algorithm) + .field("token_endpoint_audience", token_endpoint_audience) + .field("scopes", scopes) + .field("resource", resource) + .finish(), + } + } +} + #[cfg(feature = "auth-client-credentials-jwt")] fn client_authentication_audience<'a>( metadata: &'a AuthorizationMetadata, @@ -7390,6 +7426,40 @@ mod tests { assert!(matches!(oauth_client.auth_type(), AuthType::RequestBody)); } + #[test] + fn client_secret_credentials_debug_redacts_secret() { + let config = super::ClientCredentialsConfig::ClientSecret { + client_id: "client-id".to_string(), + client_secret: "client-secret-value".to_string(), + scopes: vec![], + resource: None, + }; + + let rendered = format!("{config:?}"); + + assert!(!rendered.contains("client-secret-value"), "{rendered}"); + } + + #[cfg(feature = "auth-client-credentials-jwt")] + #[test] + fn private_key_jwt_credentials_debug_redacts_signing_key() { + let config = super::ClientCredentialsConfig::PrivateKeyJwt { + client_id: "client-id".to_string(), + signing_key: b"private-signing-key-value".to_vec(), + signing_algorithm: super::JwtSigningAlgorithm::RS256, + token_endpoint_audience: None, + scopes: vec![], + resource: None, + }; + + let rendered = format!("{config:?}"); + + assert!( + !rendered.contains("private-signing-key-value"), + "{rendered}" + ); + } + #[tokio::test] async fn configure_client_credentials_sets_correct_client_id() { let mut mgr = manager_with_metadata(None).await; diff --git a/crates/rmcp/tests/test_mrtr_behavior.rs b/crates/rmcp/tests/test_mrtr_behavior.rs index 4fb24e18e..d5220458b 100644 --- a/crates/rmcp/tests/test_mrtr_behavior.rs +++ b/crates/rmcp/tests/test_mrtr_behavior.rs @@ -598,7 +598,10 @@ async fn request_state_codec_seals_and_verifies_through_the_loop() -> anyhow::Re fn codec() -> &'static RequestStateCodec { static CODEC: OnceLock = OnceLock::new(); - CODEC.get_or_init(|| RequestStateCodec::new(KEY)) + CODEC.get_or_init(|| { + RequestStateCodec::try_new(KEY) + .expect("test request-state key meets the minimum length") + }) } #[derive(Clone, Default)] diff --git a/examples/servers/src/mrtr.rs b/examples/servers/src/mrtr.rs index 40b53c3e3..36a2eb97c 100644 --- a/examples/servers/src/mrtr.rs +++ b/examples/servers/src/mrtr.rs @@ -54,7 +54,8 @@ struct WeatherServer { impl Default for WeatherServer { fn default() -> Self { Self { - codec: RequestStateCodec::new(REQUEST_STATE_KEY), + codec: RequestStateCodec::try_new(REQUEST_STATE_KEY) + .expect("example request-state key meets the minimum length"), } } }