fix(auth): confidential clients may sign in without PKCE, as OpenID certification needs

apps/auth-service/src/deletion/tests.rs

@@ -153,7 +153,7 @@ impl Fixture {
                 user_id: browser.user_id,
                 session_id: browser.session_id,
                 redirect_uri: "https://owned.example/callback".into(),
-                pkce_challenge: "challenge".into(),
+                pkce_challenge: Some("challenge".into()),
                 nonce: None,
                 scopes: vec!["openid".into()],
                 audience: client.to_string(),

apps/auth-service/src/oidc/authorize/parameters.rs

@@ -10,7 +10,7 @@ use crate::oidc::{
 use chrono::{Duration, Utc};
 use serde::Deserialize;
 use serde_json::Value;
-use sourceorigin_common::{UserId, protocol::claims::Acr};
+use sourceorigin_common::{UserId, clients::ClientKind, protocol::claims::Acr};
 use sourceorigin_repository::{InsertAuthorizationRequest, StoredOidcClient};
 use url::Url;
 
@@ -33,10 +33,11 @@ pub(in crate::oidc) struct AuthorizationParameters {
     pub(super) state: String,
     /// Client value echoed in the ID token.
     nonce: Option<String>,
-    /// PKCE challenge the code exchange must answer.
-    code_challenge: String,
-    /// Must be `S256`.
-    code_challenge_method: String,
+    /// PKCE challenge the code exchange must answer. Only a confidential client
+    /// may leave it out.
+    code_challenge: Option<String>,
+    /// Must be `S256` when `code_challenge` is sent.
+    code_challenge_method: Option<String>,
     /// Resource the access token is for; defaults to the client's first audience.
     audience: Option<String>,
     /// Space-separated [`Prompt`] values.
@@ -81,9 +82,8 @@ impl AuthorizationParameters {
         self.supported()?;
         let hinted_user_id = self.hinted_user_id(app, client)?;
         self.response_type()?;
-        self.code_challenge_method()?;
+        let pkce_challenge = self.pkce(client)?;
         self.state()?;
-        self.code_challenge()?;
         let scopes = self.scopes(client)?;
         let audience = self.audience(client)?;
         self.nonce()?;
@@ -104,7 +104,7 @@ impl AuthorizationParameters {
             client_id: client.id.clone(),
             redirect_uri: self.redirect_uri.clone(),
             client_state: self.state.clone(),
-            pkce_challenge: self.code_challenge.clone(),
+            pkce_challenge,
             nonce: self.nonce.clone(),
             scopes: scopes.into_stored(),
             audience,
@@ -194,24 +194,6 @@ impl AuthorizationParameters {
         }
     }
 
-    /// Requires `code_challenge_method=S256`.
-    fn code_challenge_method(&self) -> Result<(), OAuthError> {
-        match self
-            .code_challenge_method
-            .parse::<CodeChallengeMethod>()
-            .map_err(|error| {
-                OAuthCode::InvalidRequest.reject_with(
-                    format!(
-                        "code_challenge_method must be {}",
-                        CodeChallengeMethod::S256
-                    ),
-                    error,
-                )
-            })? {
-            CodeChallengeMethod::S256 => Ok(()),
-        }
-    }
-
     /// Bounds `state`, which comes back with the result.
     fn state(&self) -> Result<(), OAuthError> {
         if self.state.len() > MAX_STATE_BYTES {
@@ -221,13 +203,39 @@ impl AuthorizationParameters {
         Ok(())
     }
 
-    /// Requires the PKCE challenge to be a SHA-256 digest.
-    fn code_challenge(&self) -> Result<(), OAuthError> {
-        if !sourceorigin_crypto::Pkce::valid_challenge(&self.code_challenge) {
+    /// The PKCE challenge the code exchange must answer (RFC 7636): an `S256`
+    /// SHA-256 digest. Public clients must send one. A confidential client may
+    /// leave PKCE out, as RFC 9700 §2.1.1 allows, since it authenticates at the
+    /// token endpoint; a challenge it sends is enforced all the same.
+    fn pkce(&self, client: &StoredOidcClient) -> Result<Option<String>, OAuthError> {
+        let Some(challenge) = &self.code_challenge else {
+            return match (&self.code_challenge_method, client.kind) {
+                (Some(_), _) => Err(OAuthCode::InvalidRequest
+                    .reject("code_challenge_method was sent without a code_challenge")),
+                (None, ClientKind::Confidential) => Ok(None),
+                (None, ClientKind::Public) => Err(OAuthCode::InvalidRequest.reject(format!(
+                    "public clients must send a code_challenge, with code_challenge_method={}",
+                    CodeChallengeMethod::S256
+                ))),
+            };
+        };
+        let method = self.code_challenge_method.as_deref().unwrap_or_default();
+        match method.parse::<CodeChallengeMethod>().map_err(|error| {
+            OAuthCode::InvalidRequest.reject_with(
+                format!(
+                    "code_challenge_method must be {}",
+                    CodeChallengeMethod::S256
+                ),
+                error,
+            )
+        })? {
+            CodeChallengeMethod::S256 => {}
+        }
+        if !sourceorigin_crypto::Pkce::valid_challenge(challenge) {
             return Err(OAuthCode::InvalidRequest
                 .reject("code_challenge must be a base64url SHA-256 digest, 43 characters long"));
         }
-        Ok(())
+        Ok(Some(challenge.clone()))
     }
 
     /// Parses the scopes, which `client` must be allowed to request.

apps/auth-service/src/oidc/tests/authorization.rs

@@ -273,3 +273,61 @@ async fn a_consent_whose_session_ended_while_waiting_is_not_saved() {
     let consents = database.oidc().list_consents(flow.user_id).await;
     assert!(consents.expect("consents").is_empty(), "not saved");
 }
+
+/// PKCE is optional only for a confidential client, which authenticates at the
+/// token endpoint (RFC 9700 §2.1.1): a public client's request without it is
+/// refused, a challenge a confidential client sends is still enforced, and a
+/// verifier for a code issued without a challenge is refused as a downgrade.
+#[tokio::test]
+#[ignore = "requires TEST_DATABASE_URL"]
+async fn pkce_is_optional_only_for_confidential_clients() {
+    /// The scenario's authorization request without PKCE, followed to its code.
+    async fn without_pkce(flow: &Flow) -> Url {
+        let mut url = flow.authorization_url(Scope::OpenId.as_str(), None, &[]);
+        let kept: Vec<_> = url
+            .query_pairs()
+            .into_owned()
+            .filter(|(name, _)| !name.starts_with("code_challenge"))
+            .collect();
+        url.query_pairs_mut().clear().extend_pairs(kept);
+        let uri = format!("{}?{}", url.path(), url.query().expect("query"));
+        flow.consent(flow.request(Method::GET, &uri, &[], None).await)
+            .await
+    }
+    /// Exchanges `code` with the scenario's credentials and no verifier.
+    async fn exchange_without_verifier(flow: &Flow, code: &str) -> Response {
+        flow.request(
+            Method::POST,
+            "/oauth/token",
+            &[
+                ("grant_type", GrantType::AuthorizationCode.as_str()),
+                ("redirect_uri", flow.client.redirect_uris[0].as_str()),
+                ("code", code),
+            ],
+            flow.basic.as_deref(),
+        )
+        .await
+    }
+
+    let public = Flow::new(false, true).await;
+    assert_eq!(
+        parameter(&without_pkce(&public).await, "error"),
+        OAuthCode::InvalidRequest.as_str()
+    );
+
+    let confidential = Flow::new(true, true).await;
+    let downgraded = parameter(&without_pkce(&confidential).await, "code");
+    let basic = confidential.basic.as_deref();
+    Flow::assert_invalid_grant(
+        confidential
+            .exchange(&downgraded, Flow::verifier(), basic)
+            .await,
+    )
+    .await;
+    let code = parameter(&without_pkce(&confidential).await, "code");
+    confidential
+        .id_token(exchange_without_verifier(&confidential, &code).await)
+        .await;
+    let challenged = confidential.code(Scope::OpenId.as_str()).await;
+    Flow::assert_invalid_grant(exchange_without_verifier(&confidential, &challenged).await).await;
+}

apps/auth-service/src/oidc/token.rs

@@ -55,7 +55,7 @@ pub(super) struct TokenForm {
 impl TokenForm {
     /// Whether this form redeems the locked `code` for the authenticated
     /// `client_id`: the code was issued to it, for this `redirect_uri`, and this
-    /// `code_verifier` matches its PKCE challenge. Answers why it does not, with
+    /// `code_verifier` matches its PKCE challenge, or neither was sent. Answers why it does not, with
     /// the rejection.
     fn redeems(
         &self,
@@ -75,17 +75,29 @@ impl TokenForm {
                     .reject("redirect_uri must match the one sent with the authorization request"),
             ));
         }
-        sourceorigin_crypto::Pkce::verify(
-            self.code_verifier.as_deref().unwrap_or_default(),
-            &code.pkce_challenge,
-        )
-        .map_err(|error| {
-            (
+        match (&code.pkce_challenge, self.code_verifier.as_deref()) {
+            (Some(challenge), verifier) => {
+                sourceorigin_crypto::Pkce::verify(verifier.unwrap_or_default(), challenge).map_err(
+                    |error| {
+                        (
+                            CodeRedemptionFailure::InvalidVerifier,
+                            OAuthCode::InvalidGrant
+                                .reject_with(format!("code_verifier is invalid: {error}"), error),
+                        )
+                    },
+                )
+            }
+            // A confidential client sent no challenge, so it sends no verifier.
+            (None, None) => Ok(()),
+            // RFC 9700 §2.1.1: a verifier for a code issued without a challenge
+            // may be an attacker's downgrade of a request that had one.
+            (None, Some(_)) => Err((
                 CodeRedemptionFailure::InvalidVerifier,
-                OAuthCode::InvalidGrant
-                    .reject_with(format!("code_verifier is invalid: {error}"), error),
-            )
-        })
+                OAuthCode::InvalidGrant.reject(
+                    "code_verifier was sent, but the authorization request had no code_challenge",
+                ),
+            )),
+        }
     }
 
     /// Permits refresh scope narrowing while rejecting additions to the original grant.

apps/auth-service/src/tests.rs

@@ -749,7 +749,7 @@ pub(crate) async fn refresh_grant(
             user_id,
             session_id,
             redirect_uri: "https://client.example/callback".into(),
-            pkce_challenge: "challenge".into(),
+            pkce_challenge: Some("challenge".into()),
             nonce: None,
             scopes: vec!["openid".into()],
             audience: client.to_string(),

crates/database/migrations/0048_optional_pkce.sql

@@ -0,0 +1,5 @@
+-- A confidential client may leave PKCE out of an authorization request, as
+-- RFC 9700 allows: it authenticates at the token endpoint. pkce_challenge is
+-- NULL for such a request and the code it issues.
+ALTER TABLE authorization_requests ALTER COLUMN pkce_challenge DROP NOT NULL;
+ALTER TABLE authorization_codes ALTER COLUMN pkce_challenge DROP NOT NULL;

crates/database/src/repositories/oidc/mod.rs

@@ -58,8 +58,8 @@ pub struct InsertAuthorizationRequest {
     pub client_state: String,
     /// The client's `nonce` parameter, echoed in the ID token.
     pub nonce: Option<String>,
-    /// PKCE `S256` code challenge.
-    pub pkce_challenge: String,
+    /// PKCE `S256` code challenge; `None` when a confidential client sent none.
+    pub pkce_challenge: Option<String>,
     /// Requested scopes.
     pub scopes: Vec<String>,
     /// Requested token audience.
@@ -121,8 +121,8 @@ pub struct StoredAuthorizationRequest {
     pub client_state: String,
     /// The client's `nonce` parameter.
     pub nonce: Option<String>,
-    /// PKCE `S256` code challenge.
-    pub pkce_challenge: String,
+    /// PKCE `S256` code challenge; `None` when a confidential client sent none.
+    pub pkce_challenge: Option<String>,
     /// Requested scopes.
     pub scopes: Vec<String>,
     /// Requested token audience.
@@ -158,8 +158,8 @@ pub struct InsertAuthorizationCode {
     pub session_id: SessionId,
     /// Redirect URI the code was delivered to.
     pub redirect_uri: String,
-    /// PKCE `S256` code challenge.
-    pub pkce_challenge: String,
+    /// PKCE `S256` code challenge; `None` when a confidential client sent none.
+    pub pkce_challenge: Option<String>,
     /// The client's `nonce` parameter.
     pub nonce: Option<String>,
     /// Granted scopes.
@@ -187,8 +187,8 @@ pub struct ConsumedAuthorizationCode {
     pub session_id: SessionId,
     /// Redirect URI the code was delivered to.
     pub redirect_uri: String,
-    /// PKCE `S256` code challenge.
-    pub pkce_challenge: String,
+    /// PKCE `S256` code challenge; `None` when a confidential client sent none.
+    pub pkce_challenge: Option<String>,
     /// The client's `nonce` parameter.
     pub nonce: Option<String>,
     /// Granted scopes.

crates/database/src/repositories/oidc/tests.rs

@@ -132,7 +132,7 @@ async fn pushed_requests_are_claimed_only_before_they_expire() {
             redirect_uri: format!("{origin}/callback"),
             client_state: "state".into(),
             nonce: None,
-            pkce_challenge: "challenge".into(),
+            pkce_challenge: Some("challenge".into()),
             scopes: vec![Scope::OpenId.as_str().into()],
             audience: client.id.to_string(),
             interaction: Interaction::AsNeeded,

docs/dev/architecture/oidc.md

@@ -48,7 +48,7 @@ URI, always with `state` and `iss` (`ClientRedirect`).
 pushed requests, one named step per check: no request object and no
 `response_mode` but `query`, an `id_token_hint` that auth signed for this
 client, which names the only account that may answer, `response_type=code`, an
-S256 PKCE challenge, `state`, scopes and audience the client allows, `nonce`,
+S256 PKCE challenge, which only a confidential client may leave out, `state`, scopes and audience the client allows, `nonce`,
 `max_age` and `prompt`. It returns a `ValidatedAuthorization`, whose `store`
 saves the request and returns its token. The request is stored in
 `authorization_requests` for ten minutes, with its first known `acr_values`
@@ -138,7 +138,8 @@ period.
 
 1. It locks the code's account, then the code (`lock_code`), and checks that the
    form redeems it: the same client, the same `redirect_uri`, and a
-   `code_verifier` matching its PKCE challenge. A failed check expires the code
+   `code_verifier` matching its PKCE challenge, or none for a code issued
+   without one. A failed check expires the code
    and records `CodeRedemptionFailed`, so a code is tried once. The account is
    locked whatever its status, and a disabled one isn't refused here. Its code
    fails as any other would: another client presenting it gets the usual

docs/dev/operations/clients.md

@@ -146,7 +146,8 @@ sign-in**. See [passkey sign-in on your own pages](../../user/apps/passkey-sign-
 
 - **Kind.** Clients are public by default, and authenticate with PKCE alone
   (`none`). Confidential clients (`--confidential`) use `client_secret_basic`
-  with a secret auth generates. Both require S256 PKCE.
+  with a secret auth generates. Public clients must send S256 PKCE; confidential
+  clients may leave it out, and a challenge they send is enforced.
 - **Redirect URIs** must match exactly. They use HTTPS, or HTTP on a loopback IP
   for native apps, without credentials or a fragment. Write them as URL parsing
   normalizes them: `https://app.example/`, not `https://APP.example`. The

docs/dev/security/boundaries.md

@@ -168,9 +168,11 @@ back-channel logout tokens.
 
 **What is checked** (`apps/auth-service/src/oidc`):
 
-- Redirect URIs must match a registered one exactly, and every client uses S256
-  PKCE (`oidc/authorize/parameters.rs`). A confidential client also
-  authenticates with its secret.
+- Redirect URIs must match a registered one exactly, and a public client must
+  use S256 PKCE (`oidc/authorize/parameters.rs`). A confidential client
+  authenticates with its secret, so PKCE is optional for it (RFC 9700 §2.1.1);
+  a challenge it sends is enforced, and a `code_verifier` for a code issued
+  without one is refused, so a request can't be downgraded.
 - A code is tried once, and every step re-reads the client's registration
   ([codes, tokens and consent](../../user/reference/oidc.md#codes-tokens-and-consent)).
 - Only the claims a client asked for, and the user consented to, are released.

docs/dev/security/threat-model.md

@@ -59,7 +59,7 @@ own. It builds on the [security boundaries](boundaries.md); the
 
 | Threat | Mitigation |
 | --- | --- |
-| A code or token used by another app, or sent elsewhere | Exact redirect URIs, S256 PKCE for every client, codes tried once, and tokens bound to their audience ([OIDC](../../user/reference/oidc.md#codes-tokens-and-consent)). |
+| A code or token used by another app, or sent elsewhere | Exact redirect URIs, S256 PKCE for every public client and a secret for every confidential one, codes tried once, and tokens bound to their audience ([OIDC](../../user/reference/oidc.md#codes-tokens-and-consent)). |
 | An app impersonating another, or SourceOrigin | Console apps are never trusted, so the consent page shows unless the user already consented to the scopes asked, naming the host it returns to and labelling the app unverified with its owner's handle. `prompt=consent` forces it. Their names may not contain a well-known brand ([OIDC clients](../operations/clients.md)). |
 | An app keeping access | The owner, an administrator or an operator removes an app's access, which ends its refresh grants and outstanding codes. A public client's refresh token rotates, and replaying a spent one revokes the grant, unless it repeats the newest rotation within 10 seconds, which answers with the same new token. |
 | A logout URI aimed at the internal network | Deliveries go only over HTTPS to public addresses, and never follow a redirect ([outbound HTTP](../architecture/delivery-queue.md#outbound-http)). |