Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: Do not double-check VAPID aud #772

Merged
merged 3 commits into from
Oct 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gather jsonwebtoken will now do an equivalent check against our settings.vapid_aud (via validation.set_audience()) -- however I don't see us assigning this new AUTOEND__VAPID_AUD setting anywhere, which I think means it's defaulting to incorrect values (not settings.endpoint_url()).

Do we even need settings.vapid_aud -- might we always set something along the lines of validation.set_audience(&[settings.endpoint_url()]) instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and prod's basically endpoint_url() for reference here

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