bug: Make VAPID sub assertion optional (#831)

Only reports `sub` if it is present and non-empty.

Closes: [SYNC-4564](https://mozilla-hub.atlassian.net/browse/SYNC-4564)


[SYNC-4564]:
https://mozilla-hub.atlassian.net/browse/SYNC-4564?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

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(());

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::<VapidClaims>(
         &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]

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<String, VapidError> {
+    pub fn insecure_sub(&self) -> Result<Option<String>, 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<VapidClaims, VapidError> {
@@ -268,7 +265,7 @@ mod tests {
         let returned_header = VapidHeader::parse(VALID_HEADER);
         assert_eq!(
             returned_header.unwrap().insecure_sub(),
-            Ok("mailto:[email protected]".to_owned())
+            Ok(Some("mailto:[email protected]".to_owned()))
         )
     }
 
@@ -277,7 +274,7 @@ mod tests {
         let header = VapidHeader::parse(VALID_HEADER).unwrap();
         assert_eq!(
             header.insecure_sub().unwrap(),
-            "mailto:[email protected]".to_string()
+            Some("mailto:[email protected]".to_string())
         );
     }
 }

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

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",

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": "[email protected]",
+        "sub": "mailto:[email protected]",
     }
     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())