From 87611ce3f2b9ce10f5f3c4b73d8077c9b9d045d2 Mon Sep 17 00:00:00 2001 From: David Herberth Date: Wed, 10 Jun 2026 13:51:29 +0200 Subject: [PATCH 1/5] ref(auth): Make signature timestamp and its verification required --- CHANGELOG.md | 1 + relay-auth/src/lib.rs | 314 ++++++++++-------- relay-cabi/src/auth.rs | 33 +- relay-server/src/extractors/signed_json.rs | 14 +- .../src/services/projects/project/info.rs | 17 +- relay-server/src/services/upload.rs | 2 +- 6 files changed, 216 insertions(+), 165 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67b4e4ab4e7..170afdd5ae1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ **Internal**: +- Require timestamps and verification in auth signatures. ([#6069](https://github.com/getsentry/relay/pull/6069)) - Restore top-level _performance_issues_spans. ([#6045](https://github.com/getsentry/relay/pull/6045)) ## 26.5.2 diff --git a/relay-auth/src/lib.rs b/relay-auth/src/lib.rs index c19bba52280..34c057f3c5a 100644 --- a/relay-auth/src/lib.rs +++ b/relay-auth/src/lib.rs @@ -165,8 +165,8 @@ pub enum SignatureAlgorithm { #[derive(Serialize, Deserialize, Debug)] pub struct SignatureHeader { /// The timestamp of when the data was packed and signed. - #[serde(rename = "t", skip_serializing_if = "Option::is_none")] - pub timestamp: Option>, + #[serde(rename = "t")] + pub timestamp: DateTime, /// Represents how this signature was created and how it needs to be verified. /// @@ -176,26 +176,34 @@ pub struct SignatureHeader { pub signature_algorithm: Option, } -impl SignatureHeader { - /// Checks if the signature expired. - pub fn expired(&self, max_age: Duration) -> bool { - if let Some(ts) = self.timestamp { - ts < (Utc::now() - max_age) - } else { - false - } - } -} - impl Default for SignatureHeader { fn default() -> SignatureHeader { SignatureHeader { - timestamp: Some(Utc::now()), + timestamp: Utc::now(), signature_algorithm: None, } } } +/// A [`SignatureHeader`] which has been verified. +#[derive(Debug)] +pub struct VerifiedSignatureHeader { + timestamp: DateTime, + signature_algorithm: SignatureAlgorithm, +} + +impl VerifiedSignatureHeader { + /// Returns the [`SignatureHeader::timestamp`] of the verified header. + pub fn timestamp(&self) -> DateTime { + self.timestamp + } + + /// Returns the [`SignatureHeader::signature_algorithm`] of the verified header. + pub fn signature_algorithm(&self) -> SignatureAlgorithm { + self.signature_algorithm + } +} + /// Represents the secret key of an Relay. /// /// Secret keys are based on ed25519 but this should be considered an @@ -341,9 +349,16 @@ pub struct PublicKey { } impl PublicKey { - /// Verifies the signature and returns the embedded signature - /// header. - pub fn verify_meta(&self, data: &[u8], sig: SignatureRef<'_>) -> Option { + /// Verifies the signature and returns the embedded signature header. + /// + /// Returns `None` when the signature cannot be verified. + pub fn verify( + &self, + data: &[u8], + sig: SignatureRef<'_>, + start_time: DateTime, + max_age: Duration, + ) -> Option { let mut iter = sig.0.splitn(2, '.'); let sig_bytes = { let sig_encoded = iter.next()?; @@ -357,10 +372,17 @@ impl PublicKey { }; let parsed: SignatureHeader = serde_json::from_slice(&header).ok()?; - let verification_result = match parsed + // Make sure the provided timestamp is not expired or too far in the future. + if (start_time - parsed.timestamp).abs() > max_age { + return None; + } + + let signature_algorithm = parsed .signature_algorithm - .unwrap_or(SignatureAlgorithm::Regular) - { + // Default to the regular algorithm for backwards compatibility. + .unwrap_or(SignatureAlgorithm::Regular); + + let verification_result = match signature_algorithm { SignatureAlgorithm::Regular => { let mut to_verify = header.clone(); to_verify.push(b'\x00'); @@ -372,60 +394,28 @@ impl PublicKey { self.inner.verify_digest(digest, &sig) } }; - if verification_result.is_ok() { - Some(parsed) - } else { - None - } - } - - /// Verifies a signature but discards the header. - pub fn verify(&self, data: &[u8], sig: SignatureRef<'_>) -> bool { - self.verify_meta(data, sig).is_some() - } - - /// Verifies a signature and checks the timestamp. - pub fn verify_timestamp( - &self, - data: &[u8], - sig: SignatureRef<'_>, - max_age: Option, - ) -> bool { - self.verify_meta(data, sig) - .map(|header| max_age.is_none() || !header.expired(max_age.unwrap())) - .unwrap_or(false) - } - /// Unpacks signed data and returns it with header. - pub fn unpack_meta( - &self, - data: &[u8], - signature: SignatureRef<'_>, - ) -> Result<(SignatureHeader, D), UnpackError> { - if let Some(header) = self.verify_meta(data, signature) { - serde_json::from_slice(data) - .map(|data| (header, data)) - .map_err(UnpackError::BadPayload) - } else { - Err(UnpackError::BadSignature) + match verification_result { + Ok(()) => Some(VerifiedSignatureHeader { + timestamp: parsed.timestamp, + signature_algorithm, + }), + Err(_) => None, } } - /// Unpacks the data and verifies that it's not too old, then - /// throws away the wrapper. - /// - /// If no `max_age` is set, the embedded timestamp does not get validated. + /// Unpacks signed data and returns it with header. pub fn unpack( &self, data: &[u8], signature: SignatureRef<'_>, - max_age: Option, + start_time: DateTime, + max_age: Duration, ) -> Result { - let (header, data) = self.unpack_meta(data, signature)?; - if max_age.is_none() || !header.expired(max_age.unwrap()) { - Ok(data) + if let Some(_) = self.verify(data, signature, start_time, max_age) { + serde_json::from_slice(data).map_err(UnpackError::BadPayload) } else { - Err(UnpackError::SignatureExpired) + Err(UnpackError::BadSignature) } } } @@ -536,7 +526,8 @@ impl SignedRegisterState { pub fn unpack( &self, secret: &[u8], - max_age: Option, + start_time: DateTime, + max_age: Duration, ) -> Result { let (token, signature) = self.split(); let code = BASE64URL_NOPAD @@ -554,11 +545,9 @@ impl SignedRegisterState { let state = serde_json::from_slice::(&json).map_err(UnpackError::BadPayload)?; - if let Some(max_age) = max_age { - let secs = state.timestamp().as_secs() as i64; - if secs + max_age.num_seconds() < Utc::now().timestamp() { - return Err(UnpackError::SignatureExpired); - } + let secs = state.timestamp().as_secs() as i64; + if secs + max_age.num_seconds() < start_time.timestamp() { + return Err(UnpackError::SignatureExpired); } Ok(state) @@ -640,11 +629,12 @@ impl RegisterRequest { pub fn bootstrap_unpack( data: &[u8], signature: SignatureRef<'_>, - max_age: Option, + start_time: DateTime, + max_age: Duration, ) -> Result { let req: RegisterRequest = serde_json::from_slice(data).map_err(UnpackError::BadPayload)?; let pk = req.public_key(); - pk.unpack(data, signature, max_age) + pk.unpack(data, signature, start_time, max_age) } /// Returns the Relay ID of the registering Relay. @@ -719,18 +709,16 @@ impl RegisterResponse { data: &[u8], signature: SignatureRef<'_>, secret: &[u8], - max_age: Option, + start_time: DateTime, + max_age: Duration, ) -> Result<(Self, RegisterState), UnpackError> { let response: Self = serde_json::from_slice(data).map_err(UnpackError::BadPayload)?; - let state = response.token.unpack(secret, max_age)?; + let state = response.token.unpack(secret, start_time, max_age)?; - if let Some(header) = state.public_key().verify_meta(data, signature) { - if max_age.is_some_and(|m| header.expired(m)) { - return Err(UnpackError::SignatureExpired); - } - } else { - return Err(UnpackError::BadSignature); - } + let _header = state + .public_key() + .verify(data, signature, start_time, max_age) + .ok_or(UnpackError::BadSignature)?; Ok((response, state)) } @@ -767,15 +755,16 @@ impl Signature { /// Returns `true` if the signature is valid with one of the given /// public keys and satisfies the timestamp constraints defined by `start_time` /// and `max_age`. - pub fn verify_any( + pub fn verify_any<'a>( &self, - public_key: &[PublicKey], + public_key: &'a [PublicKey], start_time: DateTime, max_age: Duration, - ) -> bool { - public_key - .iter() - .any(|p| self.verify(&[], p, start_time, max_age)) + ) -> Option<(&'a PublicKey, VerifiedSignatureHeader)> { + public_key.iter().find_map(|p| { + let verified = self.verify(&[], p, start_time, max_age)?; + Some((p, verified)) + }) } /// Verifies the signature using the specified public key. @@ -789,25 +778,8 @@ impl Signature { public_key: &PublicKey, start_time: DateTime, max_age: Duration, - ) -> bool { - let Some(header) = public_key.verify_meta(data, self.as_signature_ref()) else { - return false; - }; - let Some(timestamp) = header.timestamp else { - return false; - }; - let elapsed = start_time - timestamp; - elapsed >= Duration::zero() && elapsed <= max_age - } - - /// Verifies the signature against the given data and public key. - /// - /// Returns `true` if the signature is valid for the provided `data` - /// when verified with the given public key. - pub fn verify_bytes(&self, data: &[u8], public_key: &PublicKey) -> bool { - public_key - .verify_meta(data, self.as_signature_ref()) - .is_some() + ) -> Option { + public_key.verify(data, self.as_signature_ref(), start_time, max_age) } /// Returns a borrowed view of the signature as a `SignatureRef`. @@ -892,10 +864,21 @@ mod tests { let data = b"Hello World!"; let sig = sk.sign(data); - assert!(pk.verify(data, sig.as_signature_ref())); + assert!( + pk.verify( + data, + sig.as_signature_ref(), + Utc::now(), + Duration::seconds(1) + ) + .is_some() + ); let bad_sig = "jgubwSf2wb2wuiRpgt2H9_bdDSMr88hXLp5zVuhbr65EGkSxOfT5ILIWr623twLgLd0bDgHg6xzOaUCX7XvUCw"; - assert!(!pk.verify(data, SignatureRef(bad_sig))); + assert!( + pk.verify(data, SignatureRef(bad_sig), Utc::now(), Duration::MAX) + .is_none() + ); } #[test] @@ -916,7 +899,8 @@ mod tests { let request = RegisterRequest::bootstrap_unpack( &request_bytes, request_sig.as_signature_ref(), - Some(max_age), + Utc::now(), + max_age, ) .unwrap(); assert_eq!(request.relay_id(), relay_id); @@ -932,7 +916,7 @@ mod tests { // check the challenge contains the expected info let state = SignedRegisterState(challenge_token.clone()); - let register_state = state.unpack(upstream_secret, None).unwrap(); + let register_state = state.unpack(upstream_secret, Utc::now(), max_age).unwrap(); assert_eq!(register_state.public_key, pk); assert_eq!(register_state.relay_id, relay_id); @@ -945,7 +929,8 @@ mod tests { &response_bytes, response_sig.as_signature_ref(), upstream_secret, - Some(max_age), + Utc::now(), + max_age, ) .unwrap(); @@ -982,7 +967,8 @@ mod tests { let request = RegisterRequest::bootstrap_unpack( &request_bytes, request_sig.as_signature_ref(), - Some(max_age), + Utc::now(), + max_age, ) .unwrap(); @@ -1056,16 +1042,17 @@ mod tests { #[test] fn test_verify_any() { - let pair1 = generate_key_pair(); - let pair2 = generate_key_pair(); - let pair3 = generate_key_pair(); + let (_, p1) = generate_key_pair(); + let (_, p2) = generate_key_pair(); + let (s3, p3) = generate_key_pair(); - let signature = pair3.0.sign(&[]); - assert!(signature.verify_any( - &[pair1.1, pair2.1, pair3.1], - Utc::now(), - Duration::seconds(10) - )); + let keys = [p1, p2, p3]; + let signature = s3.sign(&[]); + + let verification = signature + .verify_any(&keys, Utc::now(), Duration::seconds(10)) + .unwrap(); + assert_eq!(verification.0, &keys[2]); } #[test] @@ -1074,14 +1061,22 @@ mod tests { let signature = pair.0.sign(&[]); let start_time = Utc::now(); // The signature is valid in general - assert!(signature.verify(&[], &pair.1, start_time, Duration::seconds(10))); + assert!( + signature + .verify(&[], &pair.1, start_time, Duration::seconds(10)) + .is_some() + ); // Signature is no longer valid because too much time elapsed - assert!(!signature.verify( - &[], - &pair.1, - start_time - Duration::seconds(1), - Duration::milliseconds(500) - )) + assert!( + !signature + .verify( + &[], + &pair.1, + start_time - Duration::seconds(1), + Duration::milliseconds(500) + ) + .is_some() + ) } #[test] @@ -1092,7 +1087,7 @@ mod tests { let pair3 = generate_key_pair(); let header = SignatureHeader { - timestamp: Some(start_time), + timestamp: start_time, signature_algorithm: Some(SignatureAlgorithm::Regular), }; let signature = pair3.0.sign_with_header(&[], &header); @@ -1100,35 +1095,59 @@ mod tests { let public_keys = &[pair1.1, pair2.1, pair3.1]; // Signature still valid after 1 second - assert!(signature.verify_any( - public_keys, - start_time + Duration::seconds(1), - Duration::seconds(2) - )); + let v = signature + .verify_any( + public_keys, + start_time + Duration::seconds(1), + Duration::seconds(2), + ) + .unwrap(); + assert_eq!(v.0, &public_keys[2]); // Signature is no longer valid because too much time elapsed - assert!(!signature.verify_any( - public_keys, - start_time + Duration::seconds(3), - Duration::seconds(2) - )) + assert!( + signature + .verify_any( + public_keys, + start_time + Duration::seconds(3), + Duration::seconds(2) + ) + .is_none() + ); + // Signature is valid (and verification doesn't panic) with `Duration::MAX`. + let v = signature + .verify_any( + public_keys, + DateTime::from_timestamp_nanos(0), + Duration::MAX, + ) + .unwrap(); + assert_eq!(v.0, &public_keys[2]); } #[test] fn test_regular_algorithm() { let (secret, public) = generate_key_pair(); let signature = secret.sign(&[]); - assert!(signature.verify(&[], &public, Utc::now(), Duration::seconds(10))); + assert!( + signature + .verify(&[], &public, Utc::now(), Duration::seconds(10)) + .is_some() + ); } #[test] fn test_prehashed_algorithm() { let (secret, public) = generate_key_pair(); let header = SignatureHeader { - timestamp: Some(Utc::now()), + timestamp: Utc::now(), signature_algorithm: Some(SignatureAlgorithm::Prehashed), }; let signature = secret.sign_with_header(&[], &header); - assert!(signature.verify(&[], &public, Utc::now(), Duration::seconds(10))); + assert!( + signature + .verify(&[], &public, Utc::now(), Duration::seconds(10)) + .is_some() + ); } #[test] @@ -1155,6 +1174,15 @@ mod tests { sig_encoded.push('.'); sig_encoded.push_str(BASE64URL_NOPAD.encode(header.as_bytes()).as_str()); - assert!(public.verify(data, SignatureRef(sig_encoded.as_str()))); + assert!( + public + .verify( + data, + SignatureRef(sig_encoded.as_str()), + Utc::now(), + Duration::seconds(3) + ) + .is_some() + ); } } diff --git a/relay-cabi/src/auth.rs b/relay-cabi/src/auth.rs index cdb4a703afb..a830aeaeb55 100644 --- a/relay-cabi/src/auth.rs +++ b/relay-cabi/src/auth.rs @@ -1,4 +1,4 @@ -use chrono::Duration; +use chrono::{DateTime, Duration, Utc}; use relay_auth::{ PublicKey, RegisterRequest, RegisterResponse, RelayId, RelayVersion, SecretKey, SignatureRef, generate_key_pair, generate_relay_id, @@ -61,7 +61,16 @@ pub unsafe extern "C" fn relay_publickey_verify( ) -> bool { let pk = spk as *const PublicKey; let signature = SignatureRef(unsafe { (*sig).as_str() }); - unsafe { (*pk).verify((*data).as_bytes(), signature) } + unsafe { + (*pk) + .verify( + (*data).as_bytes(), + signature, + DateTime::from_timestamp_nanos(0), + Duration::MAX, + ) + .is_some() + } } /// Verifies a signature @@ -74,9 +83,13 @@ pub unsafe extern "C" fn relay_publickey_verify_timestamp( max_age: u32, ) -> bool { let pk = spk as *const PublicKey; - let max_age = Some(Duration::seconds(i64::from(max_age))); + let max_age = Duration::seconds(i64::from(max_age)); let signature = SignatureRef(unsafe { (*sig).as_str() }); - unsafe { (*pk).verify_timestamp((*data).as_bytes(), signature, max_age) } + unsafe { + (*pk) + .verify((*data).as_bytes(), signature, Utc::now(), max_age) + .is_some() + } } /// Parses a secret key from a string. @@ -146,13 +159,14 @@ pub unsafe extern "C" fn relay_create_register_challenge( max_age: u32, ) -> RelayStr { let max_age = match max_age { - 0 => None, - m => Some(Duration::seconds(i64::from(m))), + 0 => Duration::MAX, + m => Duration::seconds(i64::from(m)), }; let signature = SignatureRef(unsafe { (*signature).as_str() }); let challenge = unsafe { - let req = RegisterRequest::bootstrap_unpack((*data).as_bytes(), signature, max_age)?; + let req = + RegisterRequest::bootstrap_unpack((*data).as_bytes(), signature, Utc::now(), max_age)?; req.into_challenge((*secret).as_str().as_bytes()) }; @@ -178,8 +192,8 @@ pub unsafe extern "C" fn relay_validate_register_response( max_age: u32, ) -> RelayStr { let max_age = match max_age { - 0 => None, - m => Some(Duration::seconds(i64::from(m))), + 0 => Duration::MAX, + m => Duration::seconds(i64::from(m)), }; let signature = SignatureRef(unsafe { (*signature).as_str() }); @@ -187,6 +201,7 @@ pub unsafe extern "C" fn relay_validate_register_response( unsafe { (*data).as_bytes() }, signature, unsafe { (*secret).as_str().as_bytes() }, + Utc::now(), max_age, )?; diff --git a/relay-server/src/extractors/signed_json.rs b/relay-server/src/extractors/signed_json.rs index 9878a012c7f..54dbf61d827 100644 --- a/relay-server/src/extractors/signed_json.rs +++ b/relay-server/src/extractors/signed_json.rs @@ -4,6 +4,7 @@ use axum::extract::{FromRequest, Request}; use axum::http::StatusCode; use axum::response::{IntoResponse, Response}; use bytes::Bytes; +use chrono::Utc; use relay_auth::{RelayId, Signature, UnpackError}; use relay_config::RelayInfo; use serde::de::DeserializeOwned; @@ -96,12 +97,15 @@ impl FromRequest for SignedBytes { .await? .ok_or_else(|| SignatureError::MissingHeader("x-sentry-relay-signature"))?; + let max_age = chrono::Duration::from_std(state.config().signature_max_age()) + .unwrap_or(chrono::Duration::MAX); + let body = Bytes::from_request(request, state).await?; - if signature.verify_bytes(body.as_ref(), &relay.public_key) { - Ok(SignedBytes { body, relay }) - } else { - Err(SignatureError::BadSignature(UnpackError::BadSignature)) - } + + signature + .verify(body.as_ref(), &relay.public_key, Utc::now(), max_age) + .map(|_verified| SignedBytes { body, relay }) + .ok_or(SignatureError::BadSignature(UnpackError::BadSignature)) } } diff --git a/relay-server/src/services/projects/project/info.rs b/relay-server/src/services/projects/project/info.rs index 07f2c1935d3..e6a5be9b178 100644 --- a/relay-server/src/services/projects/project/info.rs +++ b/relay-server/src/services/projects/project/info.rs @@ -179,13 +179,16 @@ impl ProjectInfo { SignatureVerification::Disabled => Ok(()), SignatureVerification::Enabled => match envelope.meta().signature() { Some(signature) => { - if signature.verify_any( - &self.config.trusted_relays, - envelope.received_at(), - // conversion should never fail here - Duration::from_std(config.signature_max_age()) - .unwrap_or(Duration::milliseconds(i64::MAX)), - ) { + if signature + .verify_any( + &self.config.trusted_relays, + envelope.received_at(), + // conversion should never fail here + Duration::from_std(config.signature_max_age()) + .unwrap_or(Duration::milliseconds(i64::MAX)), + ) + .is_some() + { Ok(()) } else { Err(DiscardReason::InvalidSignature) diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 0b39761cdaf..7e3910ab793 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -431,7 +431,7 @@ impl Location { .sign_with_header( uri.as_bytes(), &SignatureHeader { - timestamp: Some(Utc::now()), + timestamp: Utc::now(), signature_algorithm: None, }, ); From 6d1d48ea224dec590824c8af7c1f026f54594847 Mon Sep 17 00:00:00 2001 From: David Herberth Date: Wed, 10 Jun 2026 14:33:18 +0200 Subject: [PATCH 2/5] bot comments --- relay-auth/src/lib.rs | 196 ++++++++++++--------- relay-cabi/src/auth.rs | 4 +- relay-server/src/extractors/signed_json.rs | 8 +- relay-server/src/services/upload.rs | 21 +-- 4 files changed, 130 insertions(+), 99 deletions(-) diff --git a/relay-auth/src/lib.rs b/relay-auth/src/lib.rs index 34c057f3c5a..16ab76d071b 100644 --- a/relay-auth/src/lib.rs +++ b/relay-auth/src/lib.rs @@ -131,6 +131,20 @@ pub enum KeyParseError { BadKey, } +/// Raised to indicate errors when verifying a signature. +#[derive(Debug, thiserror::Error, PartialEq, Eq)] +pub enum SignatureError { + /// Raised if the signature is structurally invalid. + #[error("invalid signature")] + Invalid, + /// Raised if the signature is structurally valid but cannot be verified. + #[error("signature cannot be verified")] + Unverifiable, + /// Raised if the signature is expired. + #[error("signature is too old")] + Expired, +} + /// Raised to indicate failure on unpacking. #[derive(Debug, thiserror::Error)] pub enum UnpackError { @@ -148,6 +162,15 @@ pub enum UnpackError { SignatureExpired, } +impl From for UnpackError { + fn from(value: SignatureError) -> Self { + match value { + SignatureError::Invalid | SignatureError::Unverifiable => Self::BadSignature, + SignatureError::Expired => Self::SignatureExpired, + } + } +} + /// Used to tell which algorithm was used for signature creation. #[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] pub enum SignatureAlgorithm { @@ -357,24 +380,30 @@ impl PublicKey { data: &[u8], sig: SignatureRef<'_>, start_time: DateTime, - max_age: Duration, - ) -> Option { + max_age_diff: Duration, + ) -> Result { let mut iter = sig.0.splitn(2, '.'); let sig_bytes = { - let sig_encoded = iter.next()?; - BASE64URL_NOPAD.decode(sig_encoded.as_bytes()).ok()? + let sig_encoded = iter.next().ok_or(SignatureError::Invalid)?; + BASE64URL_NOPAD + .decode(sig_encoded.as_bytes()) + .map_err(|_| SignatureError::Invalid)? }; - let sig = ed25519_dalek::Signature::from_slice(&sig_bytes).ok()?; + let sig = ed25519_dalek::Signature::from_slice(&sig_bytes) + .map_err(|_| SignatureError::Invalid)?; let header = { - let header_encoded = iter.next()?; - BASE64URL_NOPAD.decode(header_encoded.as_bytes()).ok()? + let header_encoded = iter.next().ok_or(SignatureError::Invalid)?; + BASE64URL_NOPAD + .decode(header_encoded.as_bytes()) + .map_err(|_| SignatureError::Invalid)? }; - let parsed: SignatureHeader = serde_json::from_slice(&header).ok()?; + let parsed: SignatureHeader = + serde_json::from_slice(&header).map_err(|_| SignatureError::Invalid)?; // Make sure the provided timestamp is not expired or too far in the future. - if (start_time - parsed.timestamp).abs() > max_age { - return None; + if (start_time - parsed.timestamp).abs() > max_age_diff { + return Err(SignatureError::Expired); } let signature_algorithm = parsed @@ -395,13 +424,14 @@ impl PublicKey { } }; - match verification_result { - Ok(()) => Some(VerifiedSignatureHeader { - timestamp: parsed.timestamp, - signature_algorithm, - }), - Err(_) => None, - } + let Ok(()) = verification_result else { + return Err(SignatureError::Unverifiable); + }; + + Ok(VerifiedSignatureHeader { + timestamp: parsed.timestamp, + signature_algorithm, + }) } /// Unpacks signed data and returns it with header. @@ -410,13 +440,10 @@ impl PublicKey { data: &[u8], signature: SignatureRef<'_>, start_time: DateTime, - max_age: Duration, + max_age_diff: Duration, ) -> Result { - if let Some(_) = self.verify(data, signature, start_time, max_age) { - serde_json::from_slice(data).map_err(UnpackError::BadPayload) - } else { - Err(UnpackError::BadSignature) - } + let _verified = self.verify(data, signature, start_time, max_age_diff)?; + serde_json::from_slice(data).map_err(UnpackError::BadPayload) } } @@ -527,7 +554,7 @@ impl SignedRegisterState { &self, secret: &[u8], start_time: DateTime, - max_age: Duration, + max_age_diff: Duration, ) -> Result { let (token, signature) = self.split(); let code = BASE64URL_NOPAD @@ -546,7 +573,7 @@ impl SignedRegisterState { serde_json::from_slice::(&json).map_err(UnpackError::BadPayload)?; let secs = state.timestamp().as_secs() as i64; - if secs + max_age.num_seconds() < start_time.timestamp() { + if (secs - start_time.timestamp()).abs() > max_age_diff.num_seconds() { return Err(UnpackError::SignatureExpired); } @@ -630,11 +657,11 @@ impl RegisterRequest { data: &[u8], signature: SignatureRef<'_>, start_time: DateTime, - max_age: Duration, + max_age_diff: Duration, ) -> Result { let req: RegisterRequest = serde_json::from_slice(data).map_err(UnpackError::BadPayload)?; let pk = req.public_key(); - pk.unpack(data, signature, start_time, max_age) + pk.unpack(data, signature, start_time, max_age_diff) } /// Returns the Relay ID of the registering Relay. @@ -710,15 +737,14 @@ impl RegisterResponse { signature: SignatureRef<'_>, secret: &[u8], start_time: DateTime, - max_age: Duration, + max_age_diff: Duration, ) -> Result<(Self, RegisterState), UnpackError> { let response: Self = serde_json::from_slice(data).map_err(UnpackError::BadPayload)?; - let state = response.token.unpack(secret, start_time, max_age)?; + let state = response.token.unpack(secret, start_time, max_age_diff)?; - let _header = state + let _verified = state .public_key() - .verify(data, signature, start_time, max_age) - .ok_or(UnpackError::BadSignature)?; + .verify(data, signature, start_time, max_age_diff)?; Ok((response, state)) } @@ -754,15 +780,15 @@ impl Signature { /// /// Returns `true` if the signature is valid with one of the given /// public keys and satisfies the timestamp constraints defined by `start_time` - /// and `max_age`. + /// and `max_age_diff`. pub fn verify_any<'a>( &self, public_key: &'a [PublicKey], start_time: DateTime, - max_age: Duration, + max_age_diff: Duration, ) -> Option<(&'a PublicKey, VerifiedSignatureHeader)> { public_key.iter().find_map(|p| { - let verified = self.verify(&[], p, start_time, max_age)?; + let verified = self.verify(&[], p, start_time, max_age_diff).ok()?; Some((p, verified)) }) } @@ -771,15 +797,15 @@ impl Signature { /// /// The signature is considered valid if it can be verified using the given /// public key and its embedded timestamp falls within the valid time range, - /// starting from `start_time` and not exceeding `max_age`. + /// starting from `start_time` and not exceeding `max_age_diff` (future and past). pub fn verify( &self, data: &[u8], public_key: &PublicKey, start_time: DateTime, - max_age: Duration, - ) -> Option { - public_key.verify(data, self.as_signature_ref(), start_time, max_age) + max_age_diff: Duration, + ) -> Result { + public_key.verify(data, self.as_signature_ref(), start_time, max_age_diff) } /// Returns a borrowed view of the signature as a `SignatureRef`. @@ -864,20 +890,18 @@ mod tests { let data = b"Hello World!"; let sig = sk.sign(data); - assert!( - pk.verify( - data, - sig.as_signature_ref(), - Utc::now(), - Duration::seconds(1) - ) - .is_some() + let _verified = pk.verify( + data, + sig.as_signature_ref(), + Utc::now(), + Duration::seconds(1), ); let bad_sig = "jgubwSf2wb2wuiRpgt2H9_bdDSMr88hXLp5zVuhbr65EGkSxOfT5ILIWr623twLgLd0bDgHg6xzOaUCX7XvUCw"; - assert!( + assert_eq!( pk.verify(data, SignatureRef(bad_sig), Utc::now(), Duration::MAX) - .is_none() + .unwrap_err(), + SignatureError::Invalid ); } @@ -1060,23 +1084,33 @@ mod tests { let pair = generate_key_pair(); let signature = pair.0.sign(&[]); let start_time = Utc::now(); + // The signature is valid in general - assert!( - signature - .verify(&[], &pair.1, start_time, Duration::seconds(10)) - .is_some() - ); + let _verified = signature + .verify(&[], &pair.1, start_time, Duration::seconds(10)) + .unwrap(); + + // Signature is no longer valid because too far in the future. + let err = signature + .verify( + &[], + &pair.1, + start_time - Duration::seconds(1), + Duration::milliseconds(500), + ) + .unwrap_err(); + assert_eq!(err, SignatureError::Expired); + // Signature is no longer valid because too much time elapsed - assert!( - !signature - .verify( - &[], - &pair.1, - start_time - Duration::seconds(1), - Duration::milliseconds(500) - ) - .is_some() - ) + let err = signature + .verify( + &[], + &pair.1, + start_time + Duration::seconds(1), + Duration::milliseconds(500), + ) + .unwrap_err(); + assert_eq!(err, SignatureError::Expired); } #[test] @@ -1128,11 +1162,9 @@ mod tests { fn test_regular_algorithm() { let (secret, public) = generate_key_pair(); let signature = secret.sign(&[]); - assert!( - signature - .verify(&[], &public, Utc::now(), Duration::seconds(10)) - .is_some() - ); + let _verified = signature + .verify(&[], &public, Utc::now(), Duration::seconds(10)) + .unwrap(); } #[test] @@ -1143,11 +1175,9 @@ mod tests { signature_algorithm: Some(SignatureAlgorithm::Prehashed), }; let signature = secret.sign_with_header(&[], &header); - assert!( - signature - .verify(&[], &public, Utc::now(), Duration::seconds(10)) - .is_some() - ); + let _verified = signature + .verify(&[], &public, Utc::now(), Duration::seconds(10)) + .unwrap(); } #[test] @@ -1174,15 +1204,13 @@ mod tests { sig_encoded.push('.'); sig_encoded.push_str(BASE64URL_NOPAD.encode(header.as_bytes()).as_str()); - assert!( - public - .verify( - data, - SignatureRef(sig_encoded.as_str()), - Utc::now(), - Duration::seconds(3) - ) - .is_some() - ); + let _verified = public + .verify( + data, + SignatureRef(sig_encoded.as_str()), + Utc::now(), + Duration::seconds(3), + ) + .unwrap(); } } diff --git a/relay-cabi/src/auth.rs b/relay-cabi/src/auth.rs index a830aeaeb55..eab5e9a3961 100644 --- a/relay-cabi/src/auth.rs +++ b/relay-cabi/src/auth.rs @@ -69,7 +69,7 @@ pub unsafe extern "C" fn relay_publickey_verify( DateTime::from_timestamp_nanos(0), Duration::MAX, ) - .is_some() + .is_ok() } } @@ -88,7 +88,7 @@ pub unsafe extern "C" fn relay_publickey_verify_timestamp( unsafe { (*pk) .verify((*data).as_bytes(), signature, Utc::now(), max_age) - .is_some() + .is_ok() } } diff --git a/relay-server/src/extractors/signed_json.rs b/relay-server/src/extractors/signed_json.rs index 54dbf61d827..8a5e5a116f8 100644 --- a/relay-server/src/extractors/signed_json.rs +++ b/relay-server/src/extractors/signed_json.rs @@ -102,10 +102,12 @@ impl FromRequest for SignedBytes { let body = Bytes::from_request(request, state).await?; - signature + let _verified = signature .verify(body.as_ref(), &relay.public_key, Utc::now(), max_age) - .map(|_verified| SignedBytes { body, relay }) - .ok_or(SignatureError::BadSignature(UnpackError::BadSignature)) + .map_err(Into::into) + .map_err(SignatureError::BadSignature)?; + + Ok(SignedBytes { body, relay }) } } diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 7e3910ab793..3b7219449e2 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -508,16 +508,17 @@ impl SignedLocation { #[cfg(feature = "processing")] pub fn verify(self, received: DateTime, config: &Config) -> Result, Error> { let public_key = config.public_key().ok_or(Error::SigningFailed)?; - let is_valid = self.signature.verify( - self.location.as_uri().as_bytes(), - public_key, - received, - chrono::Duration::seconds(config.upload().max_age), - ); - match is_valid { - true => Ok(self.location), - false => Err(Error::InvalidSignature), - } + let _verified = self + .signature + .verify( + self.location.as_uri().as_bytes(), + public_key, + received, + chrono::Duration::seconds(config.upload().max_age), + ) + .map_err(|_| Error::InvalidSignature)?; + + Ok(self.location) } } From 596890d899132800359a0206970ac3091aff316c Mon Sep 17 00:00:00 2001 From: David Herberth Date: Wed, 10 Jun 2026 14:34:53 +0200 Subject: [PATCH 3/5] bot comments --- relay-auth/src/lib.rs | 48 +++++++++++++++++++++++------------------- relay-cabi/src/auth.rs | 9 ++------ 2 files changed, 28 insertions(+), 29 deletions(-) diff --git a/relay-auth/src/lib.rs b/relay-auth/src/lib.rs index 16ab76d071b..a2ed0ff84b9 100644 --- a/relay-auth/src/lib.rs +++ b/relay-auth/src/lib.rs @@ -140,7 +140,7 @@ pub enum SignatureError { /// Raised if the signature is structurally valid but cannot be verified. #[error("signature cannot be verified")] Unverifiable, - /// Raised if the signature is expired. + /// Raised if the signature timestamp cannot be verified. #[error("signature is too old")] Expired, } @@ -380,7 +380,7 @@ impl PublicKey { data: &[u8], sig: SignatureRef<'_>, start_time: DateTime, - max_age_diff: Duration, + max_age: Duration, ) -> Result { let mut iter = sig.0.splitn(2, '.'); let sig_bytes = { @@ -401,8 +401,7 @@ impl PublicKey { let parsed: SignatureHeader = serde_json::from_slice(&header).map_err(|_| SignatureError::Invalid)?; - // Make sure the provided timestamp is not expired or too far in the future. - if (start_time - parsed.timestamp).abs() > max_age_diff { + if !is_valid_time(parsed.timestamp, start_time, max_age) { return Err(SignatureError::Expired); } @@ -549,12 +548,12 @@ impl SignedRegisterState { /// Unpacks the encoded state and validates the signature. /// /// If `max_age` is specified, then the timestamp in the state is validated against the current - /// time stamp. If the stored timestamp is too old, `UnpackError::SignatureExpired` is returned. + /// time stamp. If the stored timestamp is too old, [`UnpackError::SignatureExpired`] is returned. pub fn unpack( &self, secret: &[u8], start_time: DateTime, - max_age_diff: Duration, + max_age: Duration, ) -> Result { let (token, signature) = self.split(); let code = BASE64URL_NOPAD @@ -572,8 +571,11 @@ impl SignedRegisterState { let state = serde_json::from_slice::(&json).map_err(UnpackError::BadPayload)?; - let secs = state.timestamp().as_secs() as i64; - if (secs - start_time.timestamp()).abs() > max_age_diff.num_seconds() { + let ts = state + .timestamp + .as_datetime() + .ok_or(UnpackError::SignatureExpired)?; + if !is_valid_time(ts, start_time, max_age) { return Err(UnpackError::SignatureExpired); } @@ -657,11 +659,11 @@ impl RegisterRequest { data: &[u8], signature: SignatureRef<'_>, start_time: DateTime, - max_age_diff: Duration, + max_age: Duration, ) -> Result { let req: RegisterRequest = serde_json::from_slice(data).map_err(UnpackError::BadPayload)?; let pk = req.public_key(); - pk.unpack(data, signature, start_time, max_age_diff) + pk.unpack(data, signature, start_time, max_age) } /// Returns the Relay ID of the registering Relay. @@ -737,14 +739,14 @@ impl RegisterResponse { signature: SignatureRef<'_>, secret: &[u8], start_time: DateTime, - max_age_diff: Duration, + max_age: Duration, ) -> Result<(Self, RegisterState), UnpackError> { let response: Self = serde_json::from_slice(data).map_err(UnpackError::BadPayload)?; - let state = response.token.unpack(secret, start_time, max_age_diff)?; + let state = response.token.unpack(secret, start_time, max_age)?; let _verified = state .public_key() - .verify(data, signature, start_time, max_age_diff)?; + .verify(data, signature, start_time, max_age)?; Ok((response, state)) } @@ -780,15 +782,15 @@ impl Signature { /// /// Returns `true` if the signature is valid with one of the given /// public keys and satisfies the timestamp constraints defined by `start_time` - /// and `max_age_diff`. + /// and `max_age`. pub fn verify_any<'a>( &self, public_key: &'a [PublicKey], start_time: DateTime, - max_age_diff: Duration, + max_age: Duration, ) -> Option<(&'a PublicKey, VerifiedSignatureHeader)> { public_key.iter().find_map(|p| { - let verified = self.verify(&[], p, start_time, max_age_diff).ok()?; + let verified = self.verify(&[], p, start_time, max_age).ok()?; Some((p, verified)) }) } @@ -797,7 +799,7 @@ impl Signature { /// /// The signature is considered valid if it can be verified using the given /// public key and its embedded timestamp falls within the valid time range, - /// starting from `start_time` and not exceeding `max_age_diff` (future and past). + /// starting from `start_time` and not exceeding `max_age`. pub fn verify( &self, data: &[u8], @@ -824,6 +826,12 @@ impl Signature { /// This type is typically obtained by borrowing from an owned [`Signature`]. pub struct SignatureRef<'a>(pub &'a str); +/// Verifies a timestamp `ts` is not in the future and not expired. +fn is_valid_time(ts: DateTime, start_time: DateTime, max_age: Duration) -> bool { + let diff = start_time - ts; + diff >= Duration::zero() && diff < max_age +} + #[cfg(test)] mod tests { use super::*; @@ -1149,11 +1157,7 @@ mod tests { ); // Signature is valid (and verification doesn't panic) with `Duration::MAX`. let v = signature - .verify_any( - public_keys, - DateTime::from_timestamp_nanos(0), - Duration::MAX, - ) + .verify_any(public_keys, start_time, Duration::MAX) .unwrap(); assert_eq!(v.0, &public_keys[2]); } diff --git a/relay-cabi/src/auth.rs b/relay-cabi/src/auth.rs index eab5e9a3961..0ddbd1e51c7 100644 --- a/relay-cabi/src/auth.rs +++ b/relay-cabi/src/auth.rs @@ -1,4 +1,4 @@ -use chrono::{DateTime, Duration, Utc}; +use chrono::{Duration, Utc}; use relay_auth::{ PublicKey, RegisterRequest, RegisterResponse, RelayId, RelayVersion, SecretKey, SignatureRef, generate_key_pair, generate_relay_id, @@ -63,12 +63,7 @@ pub unsafe extern "C" fn relay_publickey_verify( let signature = SignatureRef(unsafe { (*sig).as_str() }); unsafe { (*pk) - .verify( - (*data).as_bytes(), - signature, - DateTime::from_timestamp_nanos(0), - Duration::MAX, - ) + .verify((*data).as_bytes(), signature, Utc::now(), Duration::MAX) .is_ok() } } From db5dcd70cd0971818a29e90a79ba0c80e532f527 Mon Sep 17 00:00:00 2001 From: David Herberth Date: Wed, 10 Jun 2026 16:09:19 +0200 Subject: [PATCH 4/5] equal or --- relay-auth/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/relay-auth/src/lib.rs b/relay-auth/src/lib.rs index a2ed0ff84b9..c9d376a1a14 100644 --- a/relay-auth/src/lib.rs +++ b/relay-auth/src/lib.rs @@ -829,7 +829,7 @@ pub struct SignatureRef<'a>(pub &'a str); /// Verifies a timestamp `ts` is not in the future and not expired. fn is_valid_time(ts: DateTime, start_time: DateTime, max_age: Duration) -> bool { let diff = start_time - ts; - diff >= Duration::zero() && diff < max_age + diff >= Duration::zero() && diff <= max_age } #[cfg(test)] From 0df917184d4b57b61c0aefdf6d76f29599af3393 Mon Sep 17 00:00:00 2001 From: David Herberth Date: Wed, 10 Jun 2026 17:28:44 +0200 Subject: [PATCH 5/5] fix nasty nasty bug --- relay-auth/src/lib.rs | 8 ++++---- relay-config/src/config.rs | 18 ++++++++++++------ 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/relay-auth/src/lib.rs b/relay-auth/src/lib.rs index c9d376a1a14..927881ce13c 100644 --- a/relay-auth/src/lib.rs +++ b/relay-auth/src/lib.rs @@ -401,10 +401,6 @@ impl PublicKey { let parsed: SignatureHeader = serde_json::from_slice(&header).map_err(|_| SignatureError::Invalid)?; - if !is_valid_time(parsed.timestamp, start_time, max_age) { - return Err(SignatureError::Expired); - } - let signature_algorithm = parsed .signature_algorithm // Default to the regular algorithm for backwards compatibility. @@ -427,6 +423,10 @@ impl PublicKey { return Err(SignatureError::Unverifiable); }; + if !is_valid_time(parsed.timestamp, start_time, max_age) { + return Err(SignatureError::Expired); + } + Ok(VerifiedSignatureHeader { timestamp: parsed.timestamp, signature_algorithm, diff --git a/relay-config/src/config.rs b/relay-config/src/config.rs index c02a93f0cbe..1ea84164234 100644 --- a/relay-config/src/config.rs +++ b/relay-config/src/config.rs @@ -1596,25 +1596,31 @@ mod config_relay_info { } /// Authentication options. -#[derive(Serialize, Deserialize, Debug, Default)] +#[derive(Serialize, Deserialize, Debug)] +#[serde(default)] pub struct AuthConfig { /// Controls responses from the readiness health check endpoint based on authentication. - #[serde(default, skip_serializing_if = "is_default")] + #[serde(skip_serializing_if = "is_default")] pub ready: ReadinessCondition, /// Statically authenticated downstream relays. - #[serde(default, with = "config_relay_info")] + #[serde(with = "config_relay_info")] pub static_relays: HashMap, /// How old a signature can be before it is considered invalid, in seconds. /// /// Defaults to 5 minutes. - #[serde(default = "default_max_age")] pub signature_max_age: u64, } -fn default_max_age() -> u64 { - 300 +impl Default for AuthConfig { + fn default() -> Self { + Self { + ready: Default::default(), + static_relays: Default::default(), + signature_max_age: 300, + } + } } /// GeoIp database configuration options.