diff --git a/Cargo.lock b/Cargo.lock index 81a5692b..7876663f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2569,9 +2569,9 @@ checksum = "68354c5c6bd36d73ff3feceb05efa59b6acb7626617f4962be322a825e61f79a" [[package]] name = "miniz_oxide" -version = "0.8.0" +version = "0.8.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e2d80299ef12ff69b16a84bb182e3b9df68b5a91574d3d4fa6e41b65deec4df1" +checksum = "a2ef2593ffb6958c941575cee70c8e257438749971869c4ae5acf6f91a168a61" dependencies = [ "adler2", ] diff --git a/autoconnect/autoconnect-common/src/registry.rs b/autoconnect/autoconnect-common/src/registry.rs index 81e72379..e6d35c26 100644 --- a/autoconnect/autoconnect-common/src/registry.rs +++ b/autoconnect/autoconnect-common/src/registry.rs @@ -85,7 +85,7 @@ impl ClientRegistry { pub async fn disconnect(&self, uaid: &Uuid, uid: &Uuid) -> Result<()> { trace!("ClientRegistry::disconnect"); let mut clients = self.clients.write().await; - let client_exists = clients.get(uaid).map_or(false, |client| client.uid == *uid); + let client_exists = clients.get(uaid).is_some_and(|client| client.uid == *uid); if client_exists { clients.remove(uaid).expect("Couldn't remove client?"); return Ok(()); diff --git a/autoendpoint/src/extractors/subscription.rs b/autoendpoint/src/extractors/subscription.rs index 23433d4f..61950f26 100644 --- a/autoendpoint/src/extractors/subscription.rs +++ b/autoendpoint/src/extractors/subscription.rs @@ -94,10 +94,10 @@ impl FromRequest for Subscription { // let's just capture if we have a valid VAPID, as well as what sort of bad sub // values we get. if let Some(header) = &vapid { - let sub = header + if let Some(sub) = header .vapid .insecure_sub() - .map_err(|e: VapidError| { + .inspect_err(|e: &VapidError| { // Capture the type of error and add it to metrics. let mut tags = Tags::default(); tags.tags @@ -108,10 +108,12 @@ impl FromRequest for Subscription { .with_tag("error", e.as_metric()) .send(); }) - .unwrap_or_default(); + .unwrap_or_default() + { + info!("VAPID sub: {sub}"); + }; // For now, record that we had a good (?) VAPID sub, app_state.metrics.incr("notification.auth.ok")?; - info!("VAPID sub: {:?}", sub) }; match token_info.api_version { @@ -304,7 +306,7 @@ fn validate_vapid_jwt( // Set the audiences we allow. This obsoletes the need to manually match // against values later. validation.set_audience(&[settings.endpoint_url().origin().ascii_serialization()]); - validation.set_required_spec_claims(&["exp", "aud", "sub"]); + validation.set_required_spec_claims(&["exp", "aud"]); let token_data = match jsonwebtoken::decode::( &vapid.token, @@ -653,13 +655,7 @@ pub mod tests { version_data: VapidVersionData::Version1, }, }; - let vv = validate_vapid_jwt(&header, &test_settings, &Metrics::sink()) - .unwrap_err() - .kind; - assert!(matches![ - vv, - ApiErrorKind::VapidError(VapidError::InvalidVapid(_)) - ]) + assert!(validate_vapid_jwt(&header, &test_settings, &Metrics::sink()).is_ok()); } #[test] diff --git a/autoendpoint/src/headers/vapid.rs b/autoendpoint/src/headers/vapid.rs index a255c98b..6974e0fd 100644 --- a/autoendpoint/src/headers/vapid.rs +++ b/autoendpoint/src/headers/vapid.rs @@ -149,26 +149,23 @@ impl VapidHeader { /// WARNING: THIS FUNCTION DOES NOT VALIDATE THE VAPID HEADER AND SHOULD /// ONLY BE USED FOR LOGGING AND METRIC REPORTING FUNCTIONS. /// Proper validation should be done by [crate::extractors::subscription::validate_vapid_jwt()] - pub fn insecure_sub(&self) -> Result { + pub fn insecure_sub(&self) -> Result, VapidError> { // This parses the VAPID header string let data = VapidClaims::try_from(self.clone()).inspect_err(|e| { warn!("🔐 Vapid: {:?} {:?}", e, &self.token); })?; - if let Some(sub) = data.sub { - if !sub.starts_with("mailto:") && !sub.starts_with("https://") { - info!("🔐 Vapid: Bad Format {:?}", sub); - return Err(VapidError::SubBadFormat); - } - if sub.is_empty() { - info!("🔐 Empty Vapid sub"); - return Err(VapidError::SubEmpty); - } - info!("🔐 Vapid: sub: {:?}", sub); - return Ok(sub.to_owned()); + let Some(sub) = data.sub else { return Ok(None) }; + if !sub.starts_with("mailto:") && !sub.starts_with("https://") { + info!("🔐 Vapid: Bad Format {sub:?}"); + return Err(VapidError::SubBadFormat); + }; + if sub.is_empty() { + info!("🔐 Empty Vapid sub"); + return Err(VapidError::SubEmpty); } - - Err(VapidError::SubMissing) + info!("🔐 Vapid: sub: {sub}"); + Ok(Some(sub)) } pub fn claims(&self) -> Result { @@ -268,7 +265,7 @@ mod tests { let returned_header = VapidHeader::parse(VALID_HEADER); assert_eq!( returned_header.unwrap().insecure_sub(), - Ok("mailto:admin@example.com".to_owned()) + Ok(Some("mailto:admin@example.com".to_owned())) ) } @@ -277,7 +274,7 @@ mod tests { let header = VapidHeader::parse(VALID_HEADER).unwrap(); assert_eq!( header.insecure_sub().unwrap(), - "mailto:admin@example.com".to_string() + Some("mailto:admin@example.com".to_string()) ); } } diff --git a/autoendpoint/src/routers/common.rs b/autoendpoint/src/routers/common.rs index fe8393aa..11b6c846 100644 --- a/autoendpoint/src/routers/common.rs +++ b/autoendpoint/src/routers/common.rs @@ -151,7 +151,9 @@ pub async fn handle_error( if let Some(Ok(claims)) = vapid.map(|v| v.vapid.claims()) { let mut extras = err.extras.unwrap_or_default(); - extras.extend([("sub".to_owned(), claims.sub.unwrap_or_default())]); + if let Some(sub) = claims.sub { + extras.extend([("sub".to_owned(), sub)]); + } err.extras = Some(extras); }; err diff --git a/autopush-common/src/db/bigtable/bigtable_client/mod.rs b/autopush-common/src/db/bigtable/bigtable_client/mod.rs index 15304d45..1f3d56b6 100644 --- a/autopush-common/src/db/bigtable/bigtable_client/mod.rs +++ b/autopush-common/src/db/bigtable/bigtable_client/mod.rs @@ -253,9 +253,9 @@ pub fn retry_policy(max: usize) -> RetryPolicy { fn retryable_internal_err(status: &RpcStatus) -> bool { match status.code() { - RpcStatusCode::UNKNOWN => { - "error occurred when fetching oauth2 token." == status.message().to_ascii_lowercase() - } + RpcStatusCode::UNKNOWN => status + .message() + .eq_ignore_ascii_case("error occurred when fetching oauth2 token."), RpcStatusCode::INTERNAL => [ "rst_stream", "rst stream", diff --git a/tests/integration/test_integration_all_rust.py b/tests/integration/test_integration_all_rust.py index df0cdaf4..67adb04b 100644 --- a/tests/integration/test_integration_all_rust.py +++ b/tests/integration/test_integration_all_rust.py @@ -1378,7 +1378,7 @@ async def test_with_key(test_client: AsyncPushTestClient) -> None: claims = { "aud": f"http://127.0.0.1:{ENDPOINT_PORT}", "exp": int(time.time()) + 86400, - "sub": "a@example.com", + "sub": "mailto:a@example.com", } vapid = _get_vapid(private_key, claims) pk_hex = vapid["crypto-key"] @@ -1397,6 +1397,24 @@ async def test_with_key(test_client: AsyncPushTestClient) -> None: await test_client.send_notification(vapid=vapid, status=401) +async def test_empty_vapid(test_client: AsyncPushTestClient) -> None: + """Test with a minimal VAPID assertion set""" + private_key = ecdsa.SigningKey.generate(curve=ecdsa.NIST256p) + claims = { + "aud": f"http://127.0.0.1:{ENDPOINT_PORT}", + "exp": int(time.time()) + 86400, + } + vapid = _get_vapid(private_key, claims) + pk_hex = vapid["crypto-key"] + chid = str(uuid.uuid4()) + await test_client.connect() + await test_client.hello() + await test_client.register(channel_id=chid, key=pk_hex) + + # Send an update with a properly formatted key. + await test_client.send_notification(vapid=vapid) + + async def test_with_bad_key(test_client: AsyncPushTestClient): """Test that a message registration request with bad VAPID public key is rejected.""" chid: str = str(uuid.uuid4())