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.

Sign up for GitHub

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

Jump to bottom

bug: Do not double-check VAPID aud #772

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

bug: Do not double-check VAPID aud #772

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d074a54
bug: Do not double-check VAPID aud
jrconlin Oct 2, 2024
7172b90
Merge branch 'master' into bug/770-aud_test
pjenvey Oct 3, 2024
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
42 changes: 19 additions & 23 deletions autoendpoint/src/extractors/subscription.rs
View file
Open in desktop
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 @@ -294,6 +292,8 @@ 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();
// Set the audiences we allow. This obsoletes the need to manually match
// against values later.
validation.set_audience(&audience);
validation.set_required_spec_claims(&["exp", "aud", "sub"]);

Expand Down Expand Up @@ -389,27 +389,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 +491,23 @@ pub mod tests {
));
}

#[test]
fn vapid_aud_valid_for_alternate_host() {
let domain = "https://example.org";
let test_settings = Settings {
vapid_aud: [domain.to_owned()].to_vec(),
..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 Down