diff --git a/README.md b/README.md index 9c7ee1f..76873ac 100644 --- a/README.md +++ b/README.md @@ -255,13 +255,13 @@ let app: Router = Router::new() .route( "/orders", get(|user: TokenUser| async move { Json(serde_json::json!({ "sub": user.sub })) }) - .route_layer(verifier.require_scope(["orders:write"])) + .route_layer(verifier.require_scope(["orders:write"])?) .route_layer(verifier.require_auth()), ) .route( "/admin", get(admin) - .route_layer(verifier.require_role(["admin"])) + .route_layer(verifier.require_role(["admin"])?) .route_layer(verifier.require_auth()), ) .route( @@ -300,6 +300,10 @@ A token is accepted only when it is RS256 under a published signing key (`kid` r of `acr_values` (listing `phr` also accepts `phrh`), `amr` (all required) and `max_age` in seconds since `auth_time`. +The guards return an error at setup when they would require nothing: `require_scope` or +`require_role` with no values or an empty value, or `require_step_up` with none of its three +settings. + A token bound to a key (a `cnf` claim) is refused unless you add a `confirmation` hook. It gets the claim and the request, so a DPoP proof can be checked against `cnf.jkt`: diff --git a/conformance/refapp/main.rs b/conformance/refapp/main.rs index 07846e5..c735ed8 100644 --- a/conformance/refapp/main.rs +++ b/conformance/refapp/main.rs @@ -126,18 +126,23 @@ fn verify_only_routes() -> Option { let step_up = verifier .require_step_up(StepUp::new().acr_values(["phr"]).max_age(300)) .unwrap_or_else(|e| panic!("{e}")); + let orders = verifier + .require_scope(["orders:write"]) + .unwrap_or_else(|e| panic!("{e}")); + let admin = verifier + .require_role(["admin"]) + .unwrap_or_else(|e| panic!("{e}")); Some( Router::new() .route("/me", me().route_layer(verifier.require_auth())) .route( "/orders", - me().route_layer(verifier.require_scope(["orders:write"])) + me().route_layer(orders) .route_layer(verifier.require_auth()), ) .route( "/admin", - me().route_layer(verifier.require_role(["admin"])) - .route_layer(verifier.require_auth()), + me().route_layer(admin).route_layer(verifier.require_auth()), ) .route( "/step-up", diff --git a/src/tests/verifier.rs b/src/tests/verifier.rs index 97b7396..3d186f3 100644 --- a/src/tests/verifier.rs +++ b/src/tests/verifier.rs @@ -37,8 +37,8 @@ fn guard_for(verifier: &Verifier, guard: &Value) -> Option { .unwrap_or_default() }; match guard["kind"].as_str()? { - "scope" => Some(verifier.require_scope(strings("scopes"))), - "role" => Some(verifier.require_role(strings("roles"))), + "scope" => Some(verifier.require_scope(strings("scopes")).unwrap()), + "role" => Some(verifier.require_role(strings("roles")).unwrap()), "step_up" => { let mut step_up = StepUp::new() .acr_values(strings("acr_values")) @@ -322,6 +322,22 @@ fn construction_errors() { .require_step_up(StepUp::new().acr_values(["bad\"value"])) .is_err() ); + + let empty: [&str; 0] = []; + assert_eq!( + verifier.require_scope(empty).map(|_| ()), + Err(VerifierBuildError::EmptyGuard) + ); + assert!(verifier.require_scope([""]).is_err()); + assert!(verifier.require_scope(["a b"]).is_err()); + assert_eq!( + verifier.require_role(empty).map(|_| ()), + Err(VerifierBuildError::EmptyGuard) + ); + assert_eq!( + verifier.require_role(["admin", " "]).map(|_| ()), + Err(VerifierBuildError::EmptyGuard) + ); } #[tokio::test] @@ -370,7 +386,7 @@ async fn guard_without_verification_answers_missing_token() { .unwrap(); let app = Router::new().route( "/resource", - get(|| async { "ok" }).route_layer(verifier.require_scope(["x"])), + get(|| async { "ok" }).route_layer(verifier.require_scope(["x"]).unwrap()), ); let (status, challenge, body) = send(app, Some("Bearer whatever")).await; assert_eq!(status, 401); diff --git a/src/token_guard.rs b/src/token_guard.rs index 13c0612..1549638 100644 --- a/src/token_guard.rs +++ b/src/token_guard.rs @@ -112,38 +112,53 @@ impl Verifier { /// A guard that answers 403 `insufficient_scope` unless the token has every /// one of `scopes`. Place it behind [`require_auth`](Self::require_auth). - pub fn require_scope(&self, scopes: I) -> TokenGuardLayer + /// + /// An empty list, or an empty scope, is an error: a guard that requires + /// nothing would let every token through. + pub fn require_scope(&self, scopes: I) -> Result where I: IntoIterator, T: Into, { let required: Vec = scopes.into_iter().map(Into::into).collect(); + if required.is_empty() { + return Err(VerifierBuildError::EmptyGuard); + } + for scope in &required { + check_challenge_value(scope)?; + } let joined = required.join(" "); let challenge = HeaderValue::from_str(&format!( "Bearer error=\"insufficient_scope\", scope=\"{joined}\"" )) - .unwrap_or_else(|_| HeaderValue::from_static("Bearer error=\"insufficient_scope\"")); - TokenGuardLayer { + .map_err(|_| VerifierBuildError::InvalidChallenge(joined.clone()))?; + Ok(TokenGuardLayer { check: Arc::new(Check::Scope { required, joined, challenge, }), - } + }) } /// A guard that answers 403 `insufficient_role` unless one of the token's /// roles grants any one of `roles`, with scoped matching (`orders:write` /// grants `orders:read`, `orders` and `orders:*` grant `orders:...`). Place /// it behind [`require_auth`](Self::require_auth). - pub fn require_role(&self, roles: I) -> TokenGuardLayer + /// + /// An empty list, or an empty role, is an error. + pub fn require_role(&self, roles: I) -> Result where I: IntoIterator, T: Into, { - TokenGuardLayer { - check: Arc::new(Check::Role(roles.into_iter().map(Into::into).collect())), + let required: Vec = roles.into_iter().map(Into::into).collect(); + if required.is_empty() || required.iter().any(|r| r.trim().is_empty()) { + return Err(VerifierBuildError::EmptyGuard); } + Ok(TokenGuardLayer { + check: Arc::new(Check::Role(required)), + }) } /// A guard that answers 401 with the RFC 9470 `insufficient_user_authentication` @@ -154,13 +169,7 @@ impl Verifier { return Err(VerifierBuildError::EmptyStepUp); } for value in &step_up.acr_values { - if value.is_empty() - || !value - .bytes() - .all(|b| b.is_ascii_graphic() && b != b'"' && b != b'\\') - { - return Err(VerifierBuildError::InvalidChallenge(value.clone())); - } + check_challenge_value(value)?; } let mut accepted_acr = step_up.acr_values.clone(); if accepted_acr.iter().any(|v| v == "phr") { @@ -176,6 +185,17 @@ impl Verifier { } } +fn check_challenge_value(value: &str) -> Result<(), VerifierBuildError> { + if value.is_empty() + || !value + .bytes() + .all(|b| b.is_ascii_graphic() && b != b'"' && b != b'\\') + { + return Err(VerifierBuildError::InvalidChallenge(value.to_string())); + } + Ok(()) +} + impl Check { fn reject(&self, user: &TokenUser) -> Option { match self { diff --git a/src/verifier.rs b/src/verifier.rs index b2814d0..c9e959f 100644 --- a/src/verifier.rs +++ b/src/verifier.rs @@ -73,6 +73,8 @@ pub enum VerifierBuildError { InvalidJwksUri, /// A step-up guard names no `acr_values`, `amr` or `max_age`. EmptyStepUp, + /// A scope or role guard names no values, or an empty one. + EmptyGuard, /// A guard value cannot be carried in a `WWW-Authenticate` header. InvalidChallenge(String), /// The HTTP client could not be built. @@ -94,6 +96,9 @@ impl fmt::Display for VerifierBuildError { VerifierBuildError::EmptyStepUp => { f.write_str("seamless-auth: a step-up guard needs acr_values, amr or max_age") } + VerifierBuildError::EmptyGuard => f.write_str( + "seamless-auth: a scope or role guard needs at least one non-empty value", + ), VerifierBuildError::InvalidChallenge(what) => { write!(f, "seamless-auth: {what:?} cannot be sent in a challenge") }