Skip to content

Commit

Permalink
bug: Do not double-check VAPID aud (#772)
Browse files Browse the repository at this point in the history
We were currently double-checking the VAPID aud via some legacy code
which may produce invalid results if the declared `vapid_aud` setting
and the `endpoint_url` did not match.

Closes: #770

---------

Co-authored-by: Philip Jenvey <[email protected]>
  • Loading branch information
jrconlin and pjenvey authored Oct 10, 2024
1 parent a6db959 commit ea04db6
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 32 deletions.
47 changes: 21 additions & 26 deletions autoendpoint/src/extractors/subscription.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use std::borrow::Cow;
use std::error::Error;
use std::str::FromStr;

use actix_web::{dev::Payload, web::Data, FromRequest, HttpRequest};
use autopush_common::{
Expand All @@ -12,7 +11,6 @@ use cadence::{CountedExt, StatsdClient};
use futures::{future::LocalBoxFuture, FutureExt};
use jsonwebtoken::{Algorithm, DecodingKey, Validation};
use openssl::hash::MessageDigest;
use url::Url;
use uuid::Uuid;

use crate::error::{ApiError, ApiErrorKind, ApiResult};
Expand Down Expand Up @@ -293,8 +291,9 @@ fn validate_vapid_jwt(

let public_key = decode_public_key(public_key)?;
let mut validation = Validation::new(Algorithm::ES256);
let audience: Vec<&str> = settings.vapid_aud.iter().map(|s| s.as_str()).collect();
validation.set_audience(&audience);
// 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"]);

let token_data = match jsonwebtoken::decode::<VapidClaims>(
Expand Down Expand Up @@ -389,27 +388,6 @@ fn validate_vapid_jwt(
return Err(VapidError::FutureExpirationToken.into());
}

let aud = match Url::from_str(&token_data.claims.aud) {
Ok(v) => v,
Err(_) => {
error!("Bad Aud: Invalid audience {:?}", &token_data.claims.aud);
metrics.clone().incr("notification.auth.bad_vapid.aud");
return Err(VapidError::InvalidAudience.into());
}
};

let domain = &settings.endpoint_url();

if domain != &aud {
info!(
"Bad Aud: I am <{:?}>, asked for <{:?}> ",
domain.as_str(),
token_data.claims.aud
);
metrics.clone().incr("notification.auth.bad_vapid.domain");
return Err(VapidError::InvalidAudience.into());
}

Ok(())
}

Expand Down Expand Up @@ -512,6 +490,23 @@ pub mod tests {
));
}

#[test]
fn vapid_aud_valid_for_alternate_host() {
let domain = "https://example.org";
let test_settings = Settings {
endpoint_url: domain.to_owned(),
..Default::default()
};
let header = make_vapid(
"mailto:[email protected]",
domain,
VapidClaims::default_exp() - 100,
PUB_KEY.to_owned(),
);
let result = validate_vapid_jwt(&header, &test_settings, &Metrics::noop());
assert!(result.is_ok());
}

#[test]
fn vapid_exp_is_string() {
#[derive(Debug, Deserialize, Serialize)]
Expand All @@ -523,7 +518,7 @@ pub mod tests {

let domain = "https://push.services.mozilla.org";
let test_settings = Settings {
endpoint_url: "domain".to_owned(),
endpoint_url: domain.to_owned(),
..Default::default()
};
let jwk_header = jsonwebtoken::Header::new(jsonwebtoken::Algorithm::ES256);
Expand Down
6 changes: 0 additions & 6 deletions autoendpoint/src/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@ pub struct Settings {
pub router_table_name: String,
pub message_table_name: String,

pub vapid_aud: Vec<String>,

/// A stringified JSON list of VAPID public keys which should be tracked internally.
/// This should ONLY include Mozilla generated and consumed messages (e.g. "SendToTab", etc.)
/// These keys should be specified in stripped, b64encoded, X962 format (e.g. a single line of
Expand Down Expand Up @@ -70,10 +68,6 @@ impl Default for Settings {
db_settings: "".to_owned(),
router_table_name: "router".to_string(),
message_table_name: "message".to_string(),
vapid_aud: vec![
"https://push.services.mozilla.org".to_string(),
"http://127.0.0.1:9160".to_string(),
],
// max data is a bit hard to figure out, due to encryption. Using something
// like pywebpush, if you encode a block of 4096 bytes, you'll get a
// 4216 byte data block. Since we're going to be receiving this, we have to
Expand Down

0 comments on commit ea04db6

Please sign in to comment.