From c8b6d4a47043d851983908e61dfce10c82c105cc Mon Sep 17 00:00:00 2001 From: flazouh Date: Fri, 6 Nov 2026 17:30:18 +0100 Subject: [PATCH 1/7] feat(cli): sign in through the browser with PKCE and carry consent restrictions nemo-relay auth login now uses an authorization code with PKCE (S256) returned to a loopback redirect on 127.0.0.1, opening the default browser or accepting the pasted redirect address when the browser runs on another device. The device authorization grant stays available behind --device. [upstream._oidc] accepts organization_id, workspace_id, credit_limit, credit_limit_reset and hidden_fields. Relay encodes them into the OAuth state as the Ori CLI does, so OpenRouter's consent page enforces them. Logout revokes the stored refresh token at the issuer's revocation endpoint, and a new sign-in revokes the one it replaces. Signed-off-by: flazouh --- crates/cli/src/commands/auth.rs | 51 ++- crates/cli/src/oidc/browser.rs | 224 +++++++++++++ crates/cli/src/oidc/client.rs | 129 +++++++- crates/cli/src/oidc/consent.rs | 155 +++++++++ crates/cli/src/oidc/mod.rs | 117 ++++++- .../coverage/shared/oidc_consent_tests.rs | 135 ++++++++ .../cli/tests/coverage/shared/oidc_tests.rs | 306 +++++++++++++++++- docs/nemo-relay-cli/basic-usage.mdx | 65 +++- 8 files changed, 1151 insertions(+), 31 deletions(-) create mode 100644 crates/cli/src/oidc/browser.rs create mode 100644 crates/cli/src/oidc/consent.rs create mode 100644 crates/cli/tests/coverage/shared/oidc_consent_tests.rs diff --git a/crates/cli/src/commands/auth.rs b/crates/cli/src/commands/auth.rs index 08a513982..cf9944fa9 100644 --- a/crates/cli/src/commands/auth.rs +++ b/crates/cli/src/commands/auth.rs @@ -10,7 +10,7 @@ use clap::{Args, Subcommand, ValueEnum}; use super::serve::ServerArgs; use crate::configuration::GatewayConfig; use crate::error::CliError; -use crate::oidc::OidcSource; +use crate::oidc::{Logout, OidcSource, Revocation}; #[derive(Debug, Clone, Args)] pub(crate) struct AuthCommand { @@ -20,8 +20,8 @@ pub(crate) struct AuthCommand { #[derive(Debug, Clone, Subcommand)] pub(crate) enum AuthSubcommand { - /// Sign in to the configured OpenID Connect provider with a code shown in the terminal. - Login(ProviderArgs), + /// Sign in to the configured OpenID Connect provider in a browser. + Login(LoginArgs), /// Print a current access token, refreshing it near expiry. For scripts; the gateway reads /// the sign-in itself. Token(ProviderArgs), @@ -35,6 +35,23 @@ pub(crate) enum ProviderArg { Anthropic, } +#[derive(Debug, Clone, Default, Args)] +pub(crate) struct LoginArgs { + #[command(flatten)] + pub(crate) provider: ProviderArgs, + /// Print the sign-in address instead of opening a browser. + #[arg(long, conflicts_with = "device")] + pub(crate) no_browser: bool, + /// Loopback port for the browser redirect, for example one forwarded over SSH. A free port + /// is picked by default. + #[arg(long, default_value_t = 0, conflicts_with = "device")] + pub(crate) callback_port: u16, + /// Sign in with a code entered on the issuer's own page (OAuth device authorization) instead + /// of the browser redirect. It cannot carry consent restrictions. + #[arg(long)] + pub(crate) device: bool, +} + /// Without flags, the provider comes from `[upstream.openai_oidc]` or /// `[upstream.anthropic_oidc]` in the effective configuration, including `--config`. #[derive(Debug, Clone, Default, Args)] @@ -61,8 +78,13 @@ pub(super) async fn execute( let load = || configured(server); match command.command { AuthSubcommand::Login(args) => { - let source = select_source(args, load)?; - crate::oidc::login(&dir, &source, |prompt| println!("{prompt}")).await?; + let source = select_source(args.provider, load)?; + let method = if args.device { + crate::oidc::LoginMethod::Device + } else { + crate::oidc::browser_method(!args.no_browser, args.callback_port) + }; + crate::oidc::login(&dir, &source, method, |prompt| println!("{prompt}")).await?; println!("Signed in to {}.", source.issuer); } AuthSubcommand::Token(args) => { @@ -72,10 +94,21 @@ pub(super) async fn execute( } AuthSubcommand::Logout(args) => { let source = select_source(args, load)?; - if crate::oidc::logout(&dir, &source).await? { - println!("Removed the sign-in for {}.", source.issuer); - } else { - println!("No sign-in is stored for {}.", source.issuer); + match crate::oidc::logout(&dir, &source).await? { + Logout::NotSignedIn => println!("No sign-in is stored for {}.", source.issuer), + Logout::Removed(Revocation::Confirmed) => println!( + "Removed the sign-in for {} and revoked it at the issuer.", + source.issuer + ), + Logout::Removed(Revocation::Unsupported) => { + println!("Removed the sign-in for {}.", source.issuer); + } + Logout::Removed(Revocation::Failed(reason)) => { + return Err(CliError::Config(format!( + "removed the sign-in for {}, but the issuer did not confirm revoking it ({reason}); it stays valid until it expires", + source.issuer + ))); + } } } } diff --git a/crates/cli/src/oidc/browser.rs b/crates/cli/src/oidc/browser.rs new file mode 100644 index 000000000..63177aff2 --- /dev/null +++ b/crates/cli/src/oidc/browser.rs @@ -0,0 +1,224 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! The browser sign-in: an authorization code with PKCE (RFC 7636), returned to a loopback +//! redirect (RFC 8252) or pasted back into the terminal when the browser runs on another device. + +use std::time::Duration; + +use axum::Router; +use axum::extract::{Query, State}; +use axum::response::Html; +use axum::routing::get; +use base64::Engine; +use base64::engine::general_purpose::URL_SAFE_NO_PAD; +use ring::rand::{SecureRandom, SystemRandom}; +use sha2::{Digest, Sha256}; +use tokio::sync::mpsc; + +use super::auth_error; +use crate::error::CliError; + +/// How long the command waits for the approval. +pub(crate) const APPROVAL_TIMEOUT: Duration = Duration::from_secs(300); +const CALLBACK_PATH: &str = "/callback"; + +const DONE_PAGE: &str = "NeMo Relay

Signed in. You can close this tab and return to the terminal.

"; +const FAILED_PAGE: &str = "NeMo Relay

The sign-in was not completed. Return to the terminal.

"; +const FOREIGN_PAGE: &str = "NeMo Relay

This page belongs to another sign-in attempt. Use the link the terminal printed.

"; + +/// How a browser sign-in reaches the user and comes back. +pub(crate) struct BrowserApproval<'a> { + /// Loopback port for the redirect; 0 picks a free one. + pub(crate) port: u16, + /// Opens the authorization address; returns whether a browser started. + pub(crate) open: &'a (dyn Fn(&str) -> bool + Send + Sync), + /// Redirect addresses pasted into the terminal. + pub(crate) pasted: mpsc::Receiver, + pub(crate) timeout: Duration, +} + +/// 32 random bytes as unpadded base64url: the PKCE verifier and the OAuth state value. +pub(crate) fn random_value() -> Result { + let mut bytes = [0u8; 32]; + SystemRandom::new() + .fill(&mut bytes) + .map_err(|_| auth_error("the system random number generator failed".into()))?; + Ok(URL_SAFE_NO_PAD.encode(bytes)) +} + +/// The S256 code challenge for a verifier. +pub(crate) fn code_challenge(verifier: &str) -> String { + URL_SAFE_NO_PAD.encode(Sha256::digest(verifier.as_bytes())) +} + +/// The query of a redirect back from the issuer. +#[derive(Debug, Clone, Default, PartialEq, Eq, serde::Deserialize)] +pub(crate) struct Redirect { + pub(crate) code: Option, + pub(crate) state: Option, + pub(crate) iss: Option, + pub(crate) error: Option, + pub(crate) error_description: Option, +} + +/// Reads a pasted redirect address. Accepts the address with or without angle brackets. +pub(crate) fn parse_pasted(text: &str) -> Option { + let trimmed = text + .trim() + .trim_start_matches('<') + .trim_end_matches('>') + .trim(); + let url = reqwest::Url::parse(trimmed).ok()?; + let mut redirect = Redirect::default(); + for (key, value) in url.query_pairs() { + let value = Some(value.into_owned()); + match key.as_ref() { + "code" => redirect.code = value, + "state" => redirect.state = value, + "iss" => redirect.iss = value, + "error" => redirect.error = value, + "error_description" => redirect.error_description = value, + _ => {} + } + } + (redirect.code.is_some() || redirect.error.is_some()).then_some(redirect) +} + +/// The loopback listener the issuer redirects to. +pub(crate) struct Callback { + pub(crate) redirect_uri: String, + received: mpsc::Receiver, + shutdown: Option>, +} + +#[derive(Clone)] +struct CallbackState { + expected_state: String, + sender: mpsc::Sender, +} + +impl Callback { + /// Listens on `127.0.0.1:`; port 0 picks a free one. + pub(crate) async fn listen(port: u16, expected_state: &str) -> Result { + let listener = tokio::net::TcpListener::bind(("127.0.0.1", port)) + .await + .map_err(|error| { + auth_error(format!( + "cannot listen on 127.0.0.1:{port} for the sign-in redirect: {error}" + )) + })?; + let port = listener.local_addr()?.port(); + let (sender, received) = mpsc::channel(4); + let (shutdown, stop) = tokio::sync::oneshot::channel::<()>(); + let app = Router::new() + .route(CALLBACK_PATH, get(receive)) + .with_state(CallbackState { + expected_state: expected_state.to_owned(), + sender, + }); + tokio::spawn(async move { + let _ = axum::serve(listener, app) + .with_graceful_shutdown(async { + let _ = stop.await; + }) + .await; + }); + Ok(Self { + redirect_uri: format!("http://127.0.0.1:{port}{CALLBACK_PATH}"), + received, + shutdown: Some(shutdown), + }) + } + + /// Waits for the redirect that carries this attempt's state, from the browser or from a + /// pasted address. A paste for another attempt is reported through `show` and ignored. + pub(crate) async fn wait( + mut self, + expected_state: &str, + mut pasted: mpsc::Receiver, + mut show: impl FnMut(String), + timeout: Duration, + ) -> Result { + let deadline = tokio::time::Instant::now() + timeout; + let mut paste_open = true; + let result = loop { + tokio::select! { + Some(redirect) = self.received.recv() => break Ok(redirect), + line = pasted.recv(), if paste_open => match line { + None => paste_open = false, + Some(line) if line.trim().is_empty() => {} + Some(line) => match parse_pasted(&line) { + Some(redirect) if redirect.state.as_deref() == Some(expected_state) => { + break Ok(redirect); + } + Some(_) => show("That address belongs to another sign-in attempt. Paste the address of the tab this command opened.".into()), + None => show("That is not a sign-in redirect. Paste the whole http://127.0.0.1:/callback?code=...&state=... address from the browser.".into()), + }, + }, + () = tokio::time::sleep_until(deadline) => { + break Err(auth_error("timed out after 5 minutes waiting for the sign-in; run the login again".into())); + } + } + }; + if let Some(shutdown) = self.shutdown.take() { + let _ = shutdown.send(()); + } + result + } +} + +async fn receive( + State(state): State, + Query(redirect): Query, +) -> Html<&'static str> { + if redirect.state.as_deref() != Some(state.expected_state.as_str()) { + return Html(FOREIGN_PAGE); + } + let page = if redirect.code.is_some() { + DONE_PAGE + } else { + FAILED_PAGE + }; + let _ = state.sender.send(redirect).await; + Html(page) +} + +/// Lines typed into the terminal, read on a thread of their own so a blocked read never holds +/// the runtime open. The channel closes at end of input. +pub(crate) fn terminal_lines() -> mpsc::Receiver { + let (sender, receiver) = mpsc::channel(4); + std::thread::spawn(move || { + let stdin = std::io::stdin(); + let mut line = String::new(); + while matches!(stdin.read_line(&mut line), Ok(read) if read > 0) { + if sender.blocking_send(std::mem::take(&mut line)).is_err() { + break; + } + } + }); + receiver +} + +/// Opens the address in the default browser. Returns whether a browser was started. +pub(crate) fn open_browser(url: &str) -> bool { + let mut command = if cfg!(target_os = "macos") { + std::process::Command::new("open") + } else if cfg!(windows) { + let mut command = std::process::Command::new("rundll32"); + command.arg("url.dll,FileProtocolHandler"); + command + } else if std::env::var_os("DISPLAY").is_some() || std::env::var_os("WAYLAND_DISPLAY").is_some() + { + std::process::Command::new("xdg-open") + } else { + return false; + }; + command + .arg(url) + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .spawn() + .is_ok() +} diff --git a/crates/cli/src/oidc/client.rs b/crates/cli/src/oidc/client.rs index daa96e13c..62eb7eb0e 100644 --- a/crates/cli/src/oidc/client.rs +++ b/crates/cli/src/oidc/client.rs @@ -9,6 +9,7 @@ use http::header::{ACCEPT, CONTENT_TYPE}; use percent_encoding::{NON_ALPHANUMERIC, utf8_percent_encode}; use serde::Deserialize; +use super::browser::{self, BrowserApproval}; use super::session::Session; use super::{Clock, HTTP_TIMEOUT, OidcSource, auth_error, login_required, now_secs}; use crate::error::CliError; @@ -22,7 +23,18 @@ const SLOW_DOWN_SECS: u64 = 5; struct Discovery { issuer: String, token_endpoint: String, + authorization_endpoint: Option, device_authorization_endpoint: Option, + revocation_endpoint: Option, +} + +/// What the issuer said to a revocation request. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum Revocation { + Confirmed, + /// The issuer publishes no revocation endpoint. + Unsupported, + Failed(String), } #[derive(Deserialize)] @@ -67,7 +79,9 @@ pub(crate) struct OidcClient { issuer: String, client_id: String, pub(super) token_endpoint: String, + authorization_endpoint: Option, device_endpoint: Option, + revocation_endpoint: Option, } impl OidcClient { @@ -93,7 +107,9 @@ impl OidcClient { } // Endpoints on another origin could send the refresh token to an unintended host. let endpoints = std::iter::once(&discovery.token_endpoint) - .chain(discovery.device_authorization_endpoint.as_ref()); + .chain(discovery.authorization_endpoint.as_ref()) + .chain(discovery.device_authorization_endpoint.as_ref()) + .chain(discovery.revocation_endpoint.as_ref()); for endpoint in endpoints { if secure_origin(endpoint)? != origin { return Err(auth_error(format!( @@ -106,10 +122,121 @@ impl OidcClient { issuer: issuer.to_owned(), client_id: source.client_id.clone(), token_endpoint: discovery.token_endpoint, + authorization_endpoint: discovery.authorization_endpoint, device_endpoint: discovery.device_authorization_endpoint, + revocation_endpoint: discovery.revocation_endpoint, }) } + /// The address the user opens to approve the sign-in. + pub(crate) fn authorization_url( + &self, + scope: &str, + redirect_uri: &str, + state: &str, + challenge: &str, + ) -> Result { + let endpoint = self.authorization_endpoint.as_deref().ok_or_else(|| { + auth_error(format!( + "{} does not offer browser sign-in (no authorization_endpoint); use --device", + self.issuer + )) + })?; + let mut url = reqwest::Url::parse(endpoint) + .map_err(|_| auth_error(format!("{endpoint} is not a valid URL")))?; + url.query_pairs_mut() + .append_pair("response_type", "code") + .append_pair("client_id", &self.client_id) + .append_pair("redirect_uri", redirect_uri) + .append_pair("scope", scope) + .append_pair("state", state) + .append_pair("code_challenge", challenge) + .append_pair("code_challenge_method", "S256"); + Ok(url.into()) + } + + /// Signs in through the browser and returns the issued tokens. + pub(crate) async fn browser_login( + &self, + scope: &str, + state: &str, + approval: BrowserApproval<'_>, + mut show: impl FnMut(String), + ) -> Result { + let verifier = browser::random_value()?; + let callback = browser::Callback::listen(approval.port, state).await?; + let redirect_uri = callback.redirect_uri.clone(); + let url = self.authorization_url( + scope, + &redirect_uri, + state, + &browser::code_challenge(&verifier), + )?; + let opened = (approval.open)(&url); + show(if opened { + format!("Opened the browser to sign in. If it did not open, visit:\n{url}") + } else { + format!("To sign in, open this address in a browser on any device:\n{url}") + }); + show(format!( + "If the browser runs on another device, its last page fails to load. Paste that page's address ({redirect_uri}?code=...) here and press Enter." + )); + let redirect = callback + .wait(state, approval.pasted, &mut show, approval.timeout) + .await?; + if redirect + .iss + .as_deref() + .is_some_and(|iss| iss.trim_end_matches('/') != self.issuer) + { + return Err(auth_error( + "the sign-in redirect names another issuer".into(), + )); + } + if let Some(error) = redirect.error { + return Err(device_failure( + &error, + &describe(&error, redirect.error_description.as_deref()), + )); + } + let code = redirect + .code + .ok_or_else(|| auth_error("the sign-in redirect carries no code".into()))?; + let grant = self + .token_request(&[ + ("grant_type", "authorization_code"), + ("code", &code), + ("redirect_uri", &redirect_uri), + ("client_id", &self.client_id), + ("code_verifier", &verifier), + ]) + .await; + match grant { + Grant::Issued(reply) => self.session(reply, None, now_secs()), + Grant::Refused { detail, .. } | Grant::Unavailable(detail) => Err(auth_error(format!( + "{} did not issue a token for the approved sign-in: {detail}", + self.issuer + ))), + } + } + + /// Revokes a refresh token (RFC 7009). + pub(crate) async fn revoke(&self, refresh_token: &str) -> Revocation { + let Some(endpoint) = self.revocation_endpoint.as_deref() else { + return Revocation::Unsupported; + }; + let form = [ + ("client_id", self.client_id.as_str()), + ("token", refresh_token), + ("token_type_hint", "refresh_token"), + ]; + match self.post(endpoint, &form).await { + Ok(response) if response.status().is_success() => Revocation::Confirmed, + Ok(response) => Revocation::Failed(format!("HTTP {}", response.status())), + Err(_) => Revocation::Failed("the issuer could not be reached".into()), + } + } + pub(crate) async fn device_login( &self, scope: &str, diff --git a/crates/cli/src/oidc/consent.rs b/crates/cli/src/oidc/consent.rs new file mode 100644 index 000000000..b272ecafd --- /dev/null +++ b/crates/cli/src/oidc/consent.rs @@ -0,0 +1,155 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Consent restrictions an administrator pins in `[upstream._oidc]`. +//! +//! OpenRouter's consent page reads them from the OAuth `state`: a random value, a `.`, and an +//! unpadded base64url JSON object with the members `organizationId`, `workspaceId`, +//! `creditLimit`, `creditLimitReset` and `hiddenFields`, in that order. The encoding matches the +//! Ori CLI byte for byte, so the one consent service enforces both clients the same way. + +use base64::Engine; +use base64::engine::general_purpose::URL_SAFE_NO_PAD; +use serde::{Deserialize, Serialize}; + +use super::OidcSource; +use crate::error::CliError; + +/// The consent service refuses a longer restriction segment. +pub(crate) const MAX_SEGMENT_LEN: usize = 1024; +/// Largest integer a JSON number keeps exactly in JavaScript. +const MAX_SAFE_INTEGER: f64 = 9_007_199_254_740_991.0; + +/// How often a pinned credit limit resets. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize, Serialize)] +#[serde(rename_all = "lowercase")] +pub(crate) enum CreditLimitReset { + Daily, + Weekly, + Monthly, +} + +/// A consent page control the restriction hides. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize, Serialize)] +#[serde(rename_all = "camelCase")] +pub(crate) enum ConsentField { + KeyLabel, + Expiration, + CreditLimit, + ExistingKey, +} + +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] +struct RestrictionClaim<'a> { + #[serde(skip_serializing_if = "Option::is_none")] + organization_id: Option<&'a str>, + #[serde(skip_serializing_if = "Option::is_none")] + workspace_id: Option<&'a str>, + #[serde(skip_serializing_if = "Option::is_none")] + credit_limit: Option, + #[serde(skip_serializing_if = "Option::is_none")] + credit_limit_reset: Option, + #[serde(skip_serializing_if = "<[_]>::is_empty")] + hidden_fields: &'a [ConsentField], +} + +impl OidcSource { + pub(crate) fn has_consent_restrictions(&self) -> bool { + self.organization_id.is_some() + || self.workspace_id.is_some() + || self.credit_limit.is_some() + || self.credit_limit_reset.is_some() + || !self.hidden_fields.is_empty() + } + + /// The encoded restriction segment, or `None` when the section pins nothing. + pub(crate) fn restriction_segment(&self) -> Option { + if !self.has_consent_restrictions() { + return None; + } + let claim = RestrictionClaim { + organization_id: self.organization_id.as_deref(), + workspace_id: self.workspace_id.as_deref(), + credit_limit: self.credit_limit.as_ref().map(javascript_number), + credit_limit_reset: self.credit_limit_reset, + hidden_fields: &self.hidden_fields, + }; + let json = serde_json::to_string(&claim).unwrap_or_default(); + Some(URL_SAFE_NO_PAD.encode(json)) + } + + /// The OAuth `state`: the random value alone, or followed by `.` and the restriction. + pub(crate) fn authorization_state(&self, random: &str) -> String { + match self.restriction_segment() { + Some(segment) => format!("{random}.{segment}"), + None => random.to_owned(), + } + } + + /// Refuses a restriction the consent service would refuse, when the configuration loads. + pub(crate) fn validate_consent_restrictions(&self, name: &str) -> Result<(), CliError> { + if !self.has_consent_restrictions() { + return Ok(()); + } + let invalid = |detail: &str| Err(CliError::Config(format!("{name}: {detail}"))); + let blank = + |value: &Option| value.as_deref().is_some_and(|id| id.trim().is_empty()); + if blank(&self.organization_id) || blank(&self.workspace_id) { + return invalid("organization_id and workspace_id must not be empty"); + } + if let Some(limit) = &self.credit_limit + && !limit + .as_f64() + .is_some_and(|value| value.is_finite() && value >= 0.0) + { + return invalid("credit_limit must be a finite amount of zero or more"); + } + if self.credit_limit_reset.is_some() && self.credit_limit.is_none() { + return invalid("credit_limit_reset needs credit_limit"); + } + if self.organization_id.is_none() + && self.workspace_id.is_none() + && self.credit_limit.is_none() + { + return invalid("hidden_fields needs organization_id, workspace_id or credit_limit"); + } + let repeated = self + .hidden_fields + .iter() + .enumerate() + .any(|(index, field)| self.hidden_fields[..index].contains(field)); + if repeated { + return invalid("hidden_fields lists a field twice"); + } + if self + .restriction_segment() + .map_or(0, |segment| segment.len()) + > MAX_SEGMENT_LEN + { + return invalid( + "the consent restriction does not fit the OAuth state (1024 characters)", + ); + } + Ok(()) + } +} + +/// `JSON.stringify` writes a whole amount without a fraction, so `25.0` encodes as `25`. +fn javascript_number(number: &serde_json::Number) -> serde_json::Number { + match number.as_f64() { + Some(value) + if number.is_f64() + && value.fract() == 0.0 + && (0.0..=MAX_SAFE_INTEGER).contains(&value) => + { + #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] + serde_json::Number::from(value as u64) + } + _ => number.clone(), + } +} + +#[cfg(test)] +#[path = "../../tests/coverage/shared/oidc_consent_tests.rs"] +mod tests; diff --git a/crates/cli/src/oidc/mod.rs b/crates/cli/src/oidc/mod.rs index 35ccc7f61..e9877efe5 100644 --- a/crates/cli/src/oidc/mod.rs +++ b/crates/cli/src/oidc/mod.rs @@ -4,13 +4,16 @@ //! OpenID Connect sign-in for upstream providers. //! //! `[upstream.openai_oidc]` and `[upstream.anthropic_oidc]` name an issuer and a client. A user -//! signs in once with the OAuth 2.0 device authorization grant (RFC 8628); the sign-in is kept in +//! signs in once in the browser with an authorization code and PKCE (RFC 7636, RFC 8252), or with +//! the device authorization grant (RFC 8628) on request; the sign-in is kept in //! the per-user NeMo Relay config directory with owner-only access. The gateway reads it for each //! upstream request and refreshes the access token near expiry, so the token's real expiry and a //! `logout` apply to the next request. Tokens are written only to that file and to the standard //! output of `nemo-relay auth token`; they are never logged or included in an error. +mod browser; mod client; +mod consent; mod session; use std::path::Path; @@ -18,12 +21,15 @@ use std::time::{Duration, SystemTime, UNIX_EPOCH}; use serde::{Deserialize, Serialize}; +use self::browser::BrowserApproval; use self::client::{OidcClient, secure_origin}; use self::session::{ SessionLock, ensure_sessions_dir, lock_timeout, read_session, session_path, write_session, }; use crate::error::CliError; +pub(crate) use self::client::Revocation; +pub(crate) use self::consent::{ConsentField, CreditLimitReset}; pub(crate) use self::session::sessions_dir; /// Refresh an access token this many seconds before it expires. @@ -49,6 +55,21 @@ pub(crate) struct OidcSource { /// Space-separated scopes requested at sign-in. #[serde(default = "default_scope")] pub(crate) scope: String, + /// Consent restriction: the only organization the consent page offers. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) organization_id: Option, + /// Consent restriction: the only workspace the consent page offers. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) workspace_id: Option, + /// Consent restriction: the credit limit, in USD, of the key the sign-in yields. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) credit_limit: Option, + /// Consent restriction: how often that credit limit resets. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) credit_limit_reset: Option, + /// Consent restriction: consent page controls to hide. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub(crate) hidden_fields: Vec, } impl OidcSource { @@ -57,6 +78,11 @@ impl OidcSource { issuer: issuer.trim_end_matches('/').to_owned(), client_id: client_id.to_owned(), scope: scope.map_or_else(default_scope, str::to_owned), + organization_id: None, + workspace_id: None, + credit_limit: None, + credit_limit_reset: None, + hidden_fields: Vec::new(), } } @@ -72,11 +98,12 @@ impl OidcSource { "{name}.client_id must not be empty" ))); } - secure_origin(self.issuer()).map(|_| ()).map_err(|_| { + secure_origin(self.issuer()).map_err(|_| { CliError::Config(format!( "{name}.issuer must be a valid https URL, or http on a loopback address" )) - }) + })?; + self.validate_consent_restrictions(name) } } @@ -162,32 +189,98 @@ pub(crate) async fn current_token( } } -/// Signs in and stores the session. +/// How `login` asks the user to approve the sign-in. +pub(crate) enum LoginMethod<'a> { + /// Authorization code with PKCE, returned to a loopback redirect or pasted back. + Browser(BrowserApproval<'a>), + /// The device authorization grant: a code entered on the issuer's own page. + Device, +} + +/// The browser sign-in as the command runs it: the system browser unless `open_browser` is +/// false, and redirect addresses pasted into the terminal. +pub(crate) fn browser_method(open_browser: bool, port: u16) -> LoginMethod<'static> { + fn open(url: &str) -> bool { + browser::open_browser(url) + } + fn print_only(_: &str) -> bool { + false + } + LoginMethod::Browser(BrowserApproval { + port, + open: if open_browser { &open } else { &print_only }, + pasted: browser::terminal_lines(), + timeout: browser::APPROVAL_TIMEOUT, + }) +} + +/// Signs in and stores the session. A sign-in it replaces is revoked at the issuer. pub(crate) async fn login( dir: &Path, source: &OidcSource, + method: LoginMethod<'_>, show: impl FnMut(String), ) -> Result<(), CliError> { let client = OidcClient::discover(source).await?; - let session = client.device_login(&source.scope, show).await?; + let session = match method { + LoginMethod::Browser(approval) => { + let state = source.authorization_state(&browser::random_value()?); + client + .browser_login(&source.scope, &state, approval, show) + .await? + } + LoginMethod::Device if source.has_consent_restrictions() => { + return Err(auth_error( + "the device sign-in cannot carry the configured consent restrictions; sign in without --device".into(), + )); + } + LoginMethod::Device => client.device_login(&source.scope, show).await?, + }; ensure_sessions_dir(dir)?; let path = session_path(dir, source); let _lock = SessionLock::acquire(&path).await?; - write_session(&path, &session) + let replaced = read_session(&path, source).ok().flatten(); + write_session(&path, &session)?; + if let Some(old) = replaced.and_then(|old| old.refresh_token) + && session.refresh_token.as_deref() != Some(old.as_str()) + { + client.revoke(&old).await; + } + Ok(()) } -/// Removes a stored sign-in; returns whether one existed. -pub(crate) async fn logout(dir: &Path, source: &OidcSource) -> Result { +/// The result of `logout`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum Logout { + NotSignedIn, + /// The stored sign-in was removed; its refresh token was revoked, or not. + Removed(Revocation), +} + +/// Removes a stored sign-in and revokes its refresh token at the issuer. The local sign-in is +/// removed even when the issuer cannot be reached. +pub(crate) async fn logout(dir: &Path, source: &OidcSource) -> Result { if !dir.exists() { - return Ok(false); + return Ok(Logout::NotSignedIn); } let path = session_path(dir, source); let _lock = SessionLock::acquire(&path).await?; + let stored = read_session(&path, source).ok().flatten(); match std::fs::remove_file(&path) { - Ok(()) => Ok(true), - Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(false), - Err(error) => Err(CliError::Io(error)), + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + return Ok(Logout::NotSignedIn); + } + Err(error) => return Err(CliError::Io(error)), } + let Some(refresh_token) = stored.and_then(|session| session.refresh_token) else { + return Ok(Logout::Removed(Revocation::Unsupported)); + }; + let revocation = match OidcClient::discover(source).await { + Ok(client) => client.revoke(&refresh_token).await, + Err(_) => Revocation::Failed("the issuer could not be reached".into()), + }; + Ok(Logout::Removed(revocation)) } /// Reads the stored sign-in without contacting the issuer. diff --git a/crates/cli/tests/coverage/shared/oidc_consent_tests.rs b/crates/cli/tests/coverage/shared/oidc_consent_tests.rs new file mode 100644 index 000000000..9240ba25d --- /dev/null +++ b/crates/cli/tests/coverage/shared/oidc_consent_tests.rs @@ -0,0 +1,135 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +use base64::Engine; +use base64::engine::general_purpose::URL_SAFE_NO_PAD; + +use super::*; + +const NONCE: &str = "n0nce_Abc-123xyz"; +const WORKSPACE: &str = "0b6f6c8e-6d59-4d5b-9a4a-2f1d8f1c0a11"; + +fn source() -> OidcSource { + OidcSource::new("https://clerk.openrouter.ai", "client", None) +} + +fn parse(toml_text: &str) -> OidcSource { + toml::from_str(&format!( + "issuer = \"https://clerk.openrouter.ai\"\nclient_id = \"client\"\n{toml_text}" + )) + .unwrap() +} + +fn payload(state: &str) -> String { + let (head, segment) = state.split_once('.').expect("a restriction segment"); + assert_eq!(head, NONCE); + assert!( + !segment.contains('.') && !segment.contains('='), + "{segment}" + ); + String::from_utf8(URL_SAFE_NO_PAD.decode(segment).unwrap()).unwrap() +} + +#[test] +fn an_unrestricted_section_sends_the_bare_random_state() { + assert_eq!(source().authorization_state(NONCE), NONCE); +} + +#[test] +fn organization_and_workspace_encode_as_ori_does() { + let state = parse(&format!( + "organization_id = \"org_required\"\nworkspace_id = \"{WORKSPACE}\"" + )) + .authorization_state(NONCE); + + assert_eq!( + payload(&state), + format!(r#"{{"organizationId":"org_required","workspaceId":"{WORKSPACE}"}}"#) + ); +} + +#[test] +fn an_unconstrained_dimension_is_omitted() { + let state = parse("organization_id = \"org_required\"").authorization_state(NONCE); + assert_eq!(payload(&state), r#"{"organizationId":"org_required"}"#); + + let state = parse(&format!("workspace_id = \"{WORKSPACE}\"")).authorization_state(NONCE); + assert_eq!( + payload(&state), + format!(r#"{{"workspaceId":"{WORKSPACE}"}}"#) + ); +} + +#[test] +fn hidden_fields_follow_the_organization_and_workspace() { + let state = parse( + "organization_id = \"org_required\"\nhidden_fields = [\"keyLabel\", \"existingKey\"]", + ) + .authorization_state(NONCE); + + assert_eq!( + payload(&state), + r#"{"organizationId":"org_required","hiddenFields":["keyLabel","existingKey"]}"# + ); +} + +#[test] +fn a_credit_limit_encodes_as_javascript_writes_the_number() { + for (written, encoded) in [("25", "25"), ("25.0", "25"), ("0", "0"), ("12.5", "12.5")] { + let state = parse(&format!("credit_limit = {written}")).authorization_state(NONCE); + assert_eq!(payload(&state), format!(r#"{{"creditLimit":{encoded}}}"#)); + } +} + +#[test] +fn every_member_is_written_in_the_consent_service_order() { + let state = parse(&format!( + "hidden_fields = [\"creditLimit\"]\ncredit_limit_reset = \"monthly\"\ncredit_limit = 25\nworkspace_id = \"{WORKSPACE}\"\norganization_id = \"org_required\"" + )) + .authorization_state(NONCE); + + assert_eq!( + payload(&state), + format!( + r#"{{"organizationId":"org_required","workspaceId":"{WORKSPACE}","creditLimit":25,"creditLimitReset":"monthly","hiddenFields":["creditLimit"]}}"# + ) + ); +} + +#[test] +fn restrictions_the_consent_service_would_refuse_fail_when_the_configuration_loads() { + let refused = |toml_text: &str| { + parse(toml_text) + .validate_consent_restrictions("upstream.openai_oidc") + .unwrap_err() + .to_string() + }; + + assert!(refused("credit_limit_reset = \"monthly\"").contains("needs credit_limit")); + assert!(refused("credit_limit = -1").contains("zero or more")); + assert!(refused("organization_id = \"\"").contains("must not be empty")); + assert!(refused("hidden_fields = [\"keyLabel\"]").contains("needs organization_id")); + assert!( + refused("credit_limit = 5\nhidden_fields = [\"keyLabel\", \"keyLabel\"]").contains("twice") + ); + let long = "o".repeat(800); + assert!(refused(&format!("organization_id = \"{long}\"")).contains("1024")); + assert!( + toml::from_str::( + "issuer = \"https://i.example\"\nclient_id = \"c\"\ncredit_limit_reset = \"yearly\"" + ) + .is_err() + ); + + let accepted = parse("credit_limit = 25\ncredit_limit_reset = \"daily\""); + assert!( + accepted + .validate_consent_restrictions("upstream.openai_oidc") + .is_ok() + ); + assert!( + source() + .validate_consent_restrictions("upstream.openai_oidc") + .is_ok() + ); +} diff --git a/crates/cli/tests/coverage/shared/oidc_tests.rs b/crates/cli/tests/coverage/shared/oidc_tests.rs index bed327438..11b1acdf4 100644 --- a/crates/cli/tests/coverage/shared/oidc_tests.rs +++ b/crates/cli/tests/coverage/shared/oidc_tests.rs @@ -36,6 +36,10 @@ struct Issuer { /// A refresh moves this clock forward by the given seconds, like a slow issuer. refresh_takes: Option<(Arc, u64)>, grants: Vec>, + /// The `code_challenge` of the last authorization address the browser opened. + challenge: Option, + revocation_status: Option, + revoked: Vec>, } type Shared = Arc>; @@ -60,10 +64,21 @@ async fn discovery(State(issuer): State) -> Json { Json(json!({ "issuer": state.advertised_issuer.clone().unwrap_or_else(|| base.clone()), "token_endpoint": state.token_endpoint.clone().unwrap_or_else(|| format!("{base}/token")), + "authorization_endpoint": format!("{base}/authorize"), "device_authorization_endpoint": format!("{base}/device"), + "revocation_endpoint": format!("{base}/revoke"), })) } +async fn revoke(State(issuer): State, body: String) -> Response { + let mut state = issuer.lock().unwrap(); + state.revoked.push(parse_form(&body)); + state + .revocation_status + .unwrap_or(StatusCode::OK) + .into_response() +} + async fn device(body: String) -> Response { if parse_form(&body)["client_id"] == "no-device-grant" { return ( @@ -118,6 +133,14 @@ async fn token(State(issuer): State, body: String) -> Response { error => refused(error), } } + "authorization_code" => { + let challenge = super::browser::code_challenge(&form["code_verifier"]); + if form["code"] == "code-1" && state.challenge.as_deref() == Some(challenge.as_str()) { + issued("access-1", "refresh-1") + } else { + refused("invalid_grant") + } + } "refresh_token" => { if let Some((clock, seconds)) = &state.refresh_takes { clock.fetch_add(*seconds, Ordering::SeqCst); @@ -149,6 +172,7 @@ async fn start_issuer() -> (OidcSource, Shared) { .route("/.well-known/openid-configuration", get(discovery)) .route("/device", post(device)) .route("/token", post(token)) + .route("/revoke", post(revoke)) .with_state(shared.clone()); tokio::spawn(async move { axum::serve(listener, app).await.unwrap() }); (OidcSource::new(&base, CLIENT_ID, None), shared) @@ -326,11 +350,17 @@ async fn logout_applies_to_the_next_gateway_request() { "access-1" ); - assert!(logout(dir.path(), &source).await.unwrap()); + assert_eq!( + logout(dir.path(), &source).await.unwrap(), + Logout::Removed(Revocation::Confirmed) + ); let error = error_text(upstream_token_in(dir.path(), &source, &|| 1_000).await); assert!(error.contains("run `nemo-relay auth login`"), "{error}"); - assert!(!logout(dir.path(), &source).await.unwrap()); + assert_eq!( + logout(dir.path(), &source).await.unwrap(), + Logout::NotSignedIn + ); } #[tokio::test] @@ -616,3 +646,275 @@ async fn a_sign_in_for_one_scope_is_never_used_for_another() { assert!(error.contains("nemo-relay auth login"), "{error}"); assert_eq!(saved(dir.path(), &source).access_token, "access-1"); } + +/// The query of the authorization address, by name. +fn query(url: &str) -> HashMap { + reqwest::Url::parse(url) + .unwrap() + .query_pairs() + .map(|(key, value)| (key.into_owned(), value.into_owned())) + .collect() +} + +/// A browser that approves: it records the challenge, then follows the redirect with `code` +/// and the given state (`None` keeps the attempt's own). +fn approving_browser( + issuer: Shared, + opened: Arc>>, + states: Vec>, +) -> impl Fn(&str) -> bool + Send + Sync { + move |url: &str| { + let params = query(url); + issuer.lock().unwrap().challenge = Some(params["code_challenge"].clone()); + opened.lock().unwrap().push(url.to_owned()); + let states = states.clone(); + tokio::spawn(async move { + for state in states { + let state = state.map_or_else(|| params["state"].clone(), str::to_owned); + let mut redirect = reqwest::Url::parse(¶ms["redirect_uri"]).unwrap(); + redirect + .query_pairs_mut() + .append_pair("code", "code-1") + .append_pair("state", &state); + reqwest::get(redirect).await.unwrap(); + } + }); + true + } +} + +fn approval<'a>( + open: &'a (dyn Fn(&str) -> bool + Send + Sync), + pasted: tokio::sync::mpsc::Receiver, +) -> BrowserApproval<'a> { + BrowserApproval { + port: 0, + open, + pasted, + timeout: Duration::from_secs(10), + } +} + +fn no_paste() -> tokio::sync::mpsc::Receiver { + tokio::sync::mpsc::channel(1).1 +} + +#[tokio::test] +async fn browser_login_exchanges_the_loopback_code_with_the_pkce_verifier() { + let (source, issuer) = start_issuer().await; + let dir = tempfile::tempdir().unwrap(); + let opened = Arc::default(); + let browser = approving_browser(issuer.clone(), Arc::clone(&opened), vec![None]); + + login( + dir.path(), + &source, + LoginMethod::Browser(approval(&browser, no_paste())), + |_| {}, + ) + .await + .unwrap(); + + assert_eq!(saved(dir.path(), &source).access_token, "access-1"); + let url = opened.lock().unwrap()[0].clone(); + let params = query(&url); + assert!( + url.starts_with(&format!("{}/authorize?", source.issuer)), + "{url}" + ); + assert_eq!(params["response_type"], "code"); + assert_eq!(params["client_id"], CLIENT_ID); + assert_eq!(params["scope"], "openid offline_access"); + assert_eq!(params["code_challenge_method"], "S256"); + assert!(params["redirect_uri"].starts_with("http://127.0.0.1:")); + assert!(params["redirect_uri"].ends_with("/callback")); + assert_eq!(params["state"].len(), 43); + let grant = issuer.lock().unwrap().grants[0].clone(); + assert_eq!(grant["grant_type"], "authorization_code"); + assert_eq!(grant["redirect_uri"], params["redirect_uri"]); +} + +#[tokio::test] +async fn a_redirect_for_another_attempt_does_not_end_the_sign_in() { + let (source, issuer) = start_issuer().await; + let opened = Arc::default(); + let browser = approving_browser(issuer.clone(), opened, vec![Some("forged"), None]); + let client = OidcClient::discover(&source).await.unwrap(); + + let session = client + .browser_login( + "openid", + "attempt-state", + approval(&browser, no_paste()), + |_| {}, + ) + .await + .unwrap(); + + assert_eq!(session.access_token, "access-1"); + assert_eq!(issuer.lock().unwrap().grants.len(), 1); +} + +#[tokio::test] +async fn a_pasted_redirect_completes_a_sign_in_without_a_local_browser() { + let (source, issuer) = start_issuer().await; + let client = OidcClient::discover(&source).await.unwrap(); + let captured = issuer.clone(); + let printed = |url: &str| { + captured.lock().unwrap().challenge = Some(query(url)["code_challenge"].clone()); + false + }; + let (paste, pasted) = tokio::sync::mpsc::channel(8); + for line in [ + "not a url\n".to_owned(), + "http://127.0.0.1:1/callback?code=code-1&state=other-attempt\n".to_owned(), + "\n".to_owned(), + ] { + paste.send(line).await.unwrap(); + } + let mut shown = Vec::new(); + + let session = client + .browser_login( + "openid", + "attempt-state", + approval(&printed, pasted), + |line| shown.push(line), + ) + .await + .unwrap(); + + assert_eq!(session.refresh_token.as_deref(), Some("refresh-1")); + let shown = shown.join("\n"); + assert!(shown.contains("open this address in a browser"), "{shown}"); + assert!(shown.contains("not a sign-in redirect"), "{shown}"); + assert!(shown.contains("another sign-in attempt"), "{shown}"); +} + +#[tokio::test] +async fn a_declined_browser_sign_in_reports_it() { + let (source, _issuer) = start_issuer().await; + let client = OidcClient::discover(&source).await.unwrap(); + let printed = |_: &str| false; + let (paste, pasted) = tokio::sync::mpsc::channel(1); + paste + .send("http://127.0.0.1:1/callback?error=access_denied&state=attempt-state".into()) + .await + .unwrap(); + + let error = error_text( + client + .browser_login( + "openid", + "attempt-state", + approval(&printed, pasted), + |_| {}, + ) + .await, + ); + + assert!(error.contains("declined"), "{error}"); +} + +#[tokio::test] +async fn an_unanswered_browser_sign_in_times_out() { + let (source, _issuer) = start_issuer().await; + let client = OidcClient::discover(&source).await.unwrap(); + let printed = |_: &str| false; + let mut waiting = approval(&printed, no_paste()); + waiting.timeout = Duration::from_millis(50); + + let error = error_text(client.browser_login("openid", "s", waiting, |_| {}).await); + + assert!(error.contains("timed out"), "{error}"); +} + +#[tokio::test] +async fn the_browser_sign_in_carries_the_configured_consent_restriction_in_its_state() { + let (mut source, issuer) = start_issuer().await; + source.organization_id = Some("org_required".into()); + source.credit_limit = Some(serde_json::Number::from(25)); + let dir = tempfile::tempdir().unwrap(); + let opened = Arc::default(); + let browser = approving_browser(issuer.clone(), Arc::clone(&opened), vec![None]); + + login( + dir.path(), + &source, + LoginMethod::Browser(approval(&browser, no_paste())), + |_| {}, + ) + .await + .unwrap(); + + let state = query(&opened.lock().unwrap()[0])["state"].clone(); + let (random, segment) = state.split_once('.').unwrap(); + assert_eq!(random.len(), 43); + assert_eq!(Some(segment.to_owned()), source.restriction_segment()); +} + +#[tokio::test] +async fn the_device_sign_in_refuses_consent_restrictions_it_cannot_carry() { + let (mut source, issuer) = start_issuer().await; + source.workspace_id = Some("workspace".into()); + let dir = tempfile::tempdir().unwrap(); + + let error = error_text(login(dir.path(), &source, LoginMethod::Device, |_| {}).await); + + assert!(error.contains("without --device"), "{error}"); + assert!(issuer.lock().unwrap().grants.is_empty()); +} + +#[tokio::test] +async fn logout_revokes_the_refresh_token_at_the_issuer() { + let (source, issuer) = start_issuer().await; + let dir = tempfile::tempdir().unwrap(); + store(dir.path(), &source, Some("refresh-1"), 10_000); + + let outcome = logout(dir.path(), &source).await.unwrap(); + + assert_eq!(outcome, Logout::Removed(Revocation::Confirmed)); + let revoked = issuer.lock().unwrap().revoked.clone(); + assert_eq!(revoked.len(), 1); + assert_eq!(revoked[0]["token"], "refresh-1"); + assert_eq!(revoked[0]["token_type_hint"], "refresh_token"); + assert_eq!(revoked[0]["client_id"], CLIENT_ID); +} + +#[tokio::test] +async fn logout_signs_out_locally_when_the_issuer_refuses_the_revocation() { + let (source, issuer) = start_issuer().await; + issuer.lock().unwrap().revocation_status = Some(StatusCode::SERVICE_UNAVAILABLE); + let dir = tempfile::tempdir().unwrap(); + store(dir.path(), &source, Some("refresh-1"), 10_000); + + let outcome = logout(dir.path(), &source).await.unwrap(); + + assert!( + matches!(outcome, Logout::Removed(Revocation::Failed(_))), + "{outcome:?}" + ); + let error = error_text(upstream_token_in(dir.path(), &source, &|| 1_000).await); + assert!(error.contains("nemo-relay auth login"), "{error}"); +} + +#[tokio::test] +async fn a_new_sign_in_revokes_the_one_it_replaces() { + let (source, issuer) = start_issuer().await; + let dir = tempfile::tempdir().unwrap(); + store(dir.path(), &source, Some("refresh-old"), 10_000); + let browser = approving_browser(issuer.clone(), Arc::default(), vec![None]); + + login( + dir.path(), + &source, + LoginMethod::Browser(approval(&browser, no_paste())), + |_| {}, + ) + .await + .unwrap(); + + let revoked = issuer.lock().unwrap().revoked.clone(); + assert_eq!(revoked.len(), 1); + assert_eq!(revoked[0]["token"], "refresh-old"); +} diff --git a/docs/nemo-relay-cli/basic-usage.mdx b/docs/nemo-relay-cli/basic-usage.mdx index 940ab79e9..20acbc04e 100644 --- a/docs/nemo-relay-cli/basic-usage.mdx +++ b/docs/nemo-relay-cli/basic-usage.mdx @@ -823,10 +823,57 @@ nemo-relay auth login The command reads the provider from the effective configuration, including a file passed with `--config`. When both providers have an `oidc` section with -different values, add `--provider openai` or `--provider anthropic`. The command -uses the OAuth 2.0 device authorization grant. It prints a URL and a code. Open -the URL on any device, enter the code, and approve the sign-in. The command does -not need a local browser, so it also works over SSH. +different values, add `--provider openai` or `--provider anthropic`. + +The command signs in through the browser with an OAuth 2.0 authorization code +and PKCE. It listens on `http://127.0.0.1:/callback`, opens the issuer's +sign-in page in the default browser, and waits up to five minutes for the +issuer to redirect back. The OAuth client must allow that loopback redirect on +any port. The `state` of each attempt is random, and the command accepts only a +redirect that carries it. + +Without a local browser, for example over SSH, the command prints the address. +Open it in a browser on any device and approve the sign-in. The browser's last +page, `http://127.0.0.1:/callback?code=...`, then fails to load, because +the redirect points at the device the browser runs on. Copy that page's address, +paste it into the terminal, and press Enter. Add `--no-browser` to print the +address without opening a browser, and `--callback-port ` to listen on a +fixed port, for example one you forward over SSH. + +`--device` uses the OAuth 2.0 device authorization grant instead: the command +prints a URL and a code to enter on the issuer's own page. It needs no redirect, +but it cannot carry the consent restrictions below. The issuer must offer the +device grant to the client. + +#### Consent Restrictions + +OpenRouter's consent page can pin what a sign-in may approve. An administrator +adds the restrictions to the provider's `oidc` section: + +```toml +[upstream.openai_oidc] +issuer = "https://clerk.openrouter.ai" +client_id = "" +scope = "openid offline_access email inference" +organization_id = "" +workspace_id = "" +credit_limit = 25 +credit_limit_reset = "monthly" +hidden_fields = ["keyLabel", "existingKey"] +``` + +- `organization_id`: the consent page offers only this organization. +- `workspace_id`: the consent page offers only this workspace. +- `credit_limit`: the key the sign-in yields carries this credit limit, in USD. +- `credit_limit_reset`: `daily`, `weekly`, or `monthly`. It needs `credit_limit`. +- `hidden_fields`: consent page controls to hide: `keyLabel`, `expiration`, + `creditLimit`, and `existingKey`. It needs one of the three settings above. + +Every setting is optional. Relay sends them in the OAuth `state` as the Ori CLI +does: the random value, a `.`, and the restrictions as unpadded base64url JSON, +at most 1,024 characters. OpenRouter's consent service enforces them. The +configuration fails to load when a restriction is one the consent service would +refuse. Relay stores the sign-in in `/oidc/` with owner-only access, one file for each issuer, client ID, and scope. The gateway reads it for each request. @@ -834,7 +881,10 @@ When the access token expires within five minutes, the gateway refreshes it with the stored refresh token first. Refreshes from several processes run one at a time, so a rotated refresh token is never used twice. Because the gateway reads the stored sign-in for each request, the token's real expiry applies, and -`nemo-relay auth logout` takes effect on the next request. +`nemo-relay auth logout` takes effect on the next request. Logout also revokes +the stored refresh token at the issuer's revocation endpoint, and a new sign-in +revokes the one it replaces. When the issuer does not confirm the revocation, +logout still removes the local sign-in and exits with an error that says so. Relay adds the token only to requests it authenticates: agents that Relay launches, and daemon routes bound by the managed integration. A client that only @@ -849,8 +899,9 @@ each configured sign-in exists, is valid, has expired but can refresh, or needs a new sign-in. `nemo-relay auth token` prints the current access token for scripts. -The issuer must use `https`, or `http` on a loopback address. Its token and -device endpoints must have the same origin as the issuer. Relay never writes +The issuer must use `https`, or `http` on a loopback address. Its +authorization, token, device, and revocation endpoints must have the same origin +as the issuer. Relay never writes tokens to logs or error messages. ### Operational Logging From d72cd247d8f1f66caf44b2892746769163e918e6 Mon Sep 17 00:00:00 2001 From: flazouh Date: Fri, 6 Nov 2026 18:46:58 +0100 Subject: [PATCH 2/7] refactor(cli): split the browser sign-in into small typed steps Sign-in and logout failures are one SignInError enum, consent restriction failures a RestrictionError enum, and pasted lines a Paste enum. The browser flow lives in browser::sign_in; the client only builds the authorization address, exchanges the code and revokes. Repeated strings are constants. Signed-off-by: flazouh --- crates/cli/src/commands/auth.rs | 78 +++-- crates/cli/src/error.rs | 4 + crates/cli/src/oidc/browser.rs | 298 ++++++++++++------ crates/cli/src/oidc/client.rs | 116 +++---- crates/cli/src/oidc/consent.rs | 160 ++++++---- crates/cli/src/oidc/error.rs | 56 ++++ crates/cli/src/oidc/mod.rs | 132 ++++---- .../cli/tests/coverage/shared/oidc_tests.rs | 58 ++-- 8 files changed, 563 insertions(+), 339 deletions(-) create mode 100644 crates/cli/src/oidc/error.rs diff --git a/crates/cli/src/commands/auth.rs b/crates/cli/src/commands/auth.rs index cf9944fa9..3462844a4 100644 --- a/crates/cli/src/commands/auth.rs +++ b/crates/cli/src/commands/auth.rs @@ -3,6 +3,7 @@ //! `nemo-relay auth`: sign in to the provider named by `[upstream._oidc]`. +use std::path::Path; use std::process::ExitCode; use clap::{Args, Subcommand, ValueEnum}; @@ -10,7 +11,7 @@ use clap::{Args, Subcommand, ValueEnum}; use super::serve::ServerArgs; use crate::configuration::GatewayConfig; use crate::error::CliError; -use crate::oidc::{Logout, OidcSource, Revocation}; +use crate::oidc::{BrowserApproval, LoginMethod, Logout, OidcSource, Revocation, SignInError}; #[derive(Debug, Clone, Args)] pub(crate) struct AuthCommand { @@ -77,42 +78,53 @@ pub(super) async fn execute( let dir = crate::oidc::sessions_dir()?; let load = || configured(server); match command.command { - AuthSubcommand::Login(args) => { - let source = select_source(args.provider, load)?; - let method = if args.device { - crate::oidc::LoginMethod::Device - } else { - crate::oidc::browser_method(!args.no_browser, args.callback_port) - }; - crate::oidc::login(&dir, &source, method, |prompt| println!("{prompt}")).await?; - println!("Signed in to {}.", source.issuer); + AuthSubcommand::Login(args) => login(&dir, args, load).await?, + AuthSubcommand::Token(args) => print_token(&dir, select_source(args, load)?).await?, + AuthSubcommand::Logout(args) => logout(&dir, select_source(args, load)?).await?, + } + Ok(ExitCode::SUCCESS) +} + +async fn login( + dir: &Path, + args: LoginArgs, + load: impl FnOnce() -> Result, +) -> Result<(), CliError> { + let source = select_source(args.provider, load)?; + let method = if args.device { + LoginMethod::Device + } else { + LoginMethod::Browser(BrowserApproval::interactive( + !args.no_browser, + args.callback_port, + )) + }; + crate::oidc::login(dir, &source, method, |prompt| println!("{prompt}")).await?; + println!("Signed in to {}.", source.issuer); + Ok(()) +} + +async fn print_token(dir: &Path, source: OidcSource) -> Result<(), CliError> { + let token = crate::oidc::current_token(dir, &source, &crate::oidc::now_secs).await?; + println!("{token}"); + Ok(()) +} + +async fn logout(dir: &Path, source: OidcSource) -> Result<(), CliError> { + let issuer = source.issuer.clone(); + match crate::oidc::logout(dir, &source).await? { + Logout::NotSignedIn => println!("No sign-in is stored for {issuer}."), + Logout::Removed(Revocation::Confirmed) => { + println!("Removed the sign-in for {issuer} and revoked it at the issuer."); } - AuthSubcommand::Token(args) => { - let source = select_source(args, load)?; - let token = crate::oidc::current_token(&dir, &source, &crate::oidc::now_secs).await?; - println!("{token}"); + Logout::Removed(Revocation::NotOffered | Revocation::NothingToRevoke) => { + println!("Removed the sign-in for {issuer}."); } - AuthSubcommand::Logout(args) => { - let source = select_source(args, load)?; - match crate::oidc::logout(&dir, &source).await? { - Logout::NotSignedIn => println!("No sign-in is stored for {}.", source.issuer), - Logout::Removed(Revocation::Confirmed) => println!( - "Removed the sign-in for {} and revoked it at the issuer.", - source.issuer - ), - Logout::Removed(Revocation::Unsupported) => { - println!("Removed the sign-in for {}.", source.issuer); - } - Logout::Removed(Revocation::Failed(reason)) => { - return Err(CliError::Config(format!( - "removed the sign-in for {}, but the issuer did not confirm revoking it ({reason}); it stays valid until it expires", - source.issuer - ))); - } - } + Logout::Removed(Revocation::Failed(reason)) => { + return Err(SignInError::RevocationUnconfirmed { issuer, reason }.into()); } } - Ok(ExitCode::SUCCESS) + Ok(()) } /// The effective configuration, including the user layer that `--config` replaces. diff --git a/crates/cli/src/error.rs b/crates/cli/src/error.rs index 24fd3e45d..ad94c9649 100644 --- a/crates/cli/src/error.rs +++ b/crates/cli/src/error.rs @@ -112,6 +112,9 @@ pub(crate) enum CliError { /// The upstream needs an OpenID Connect sign-in that is missing or no longer valid. #[error("sign-in required: {0}")] SignInRequired(String), + /// A browser sign-in or a logout did not complete. + #[error(transparent)] + SignIn(#[from] crate::oidc::SignInError), /// The daemon definitively rejected the route credential; retrying cannot succeed. #[error("route credential rejected: {0}")] RouteCredentialRejected(String), @@ -181,6 +184,7 @@ impl CliError { Self::PayloadTooLarge(_) => "payload_too_large", Self::Unauthorized(_) => "unauthorized", Self::SignInRequired(_) => "sign_in_required", + Self::SignIn(_) => "sign_in", Self::RouteCredentialRejected(_) => "route_credential_rejected", Self::Upstream(_) | Self::UpstreamTransport(_) => "upstream", Self::ProviderFailure(_) => "provider_failure", diff --git a/crates/cli/src/oidc/browser.rs b/crates/cli/src/oidc/browser.rs index 63177aff2..f1d3e9b5d 100644 --- a/crates/cli/src/oidc/browser.rs +++ b/crates/cli/src/oidc/browser.rs @@ -14,36 +14,94 @@ use base64::Engine; use base64::engine::general_purpose::URL_SAFE_NO_PAD; use ring::rand::{SecureRandom, SystemRandom}; use sha2::{Digest, Sha256}; -use tokio::sync::mpsc; +use tokio::sync::{mpsc, oneshot}; -use super::auth_error; +use super::client::OidcClient; +use super::error::SignInError; +use super::session::Session; use crate::error::CliError; -/// How long the command waits for the approval. +/// How long the command waits for the user to approve the sign-in. pub(crate) const APPROVAL_TIMEOUT: Duration = Duration::from_secs(300); +/// Bytes of randomness in the PKCE verifier and in the OAuth state. +const RANDOM_BYTES: usize = 32; +const CALLBACK_HOST: &str = "127.0.0.1"; const CALLBACK_PATH: &str = "/callback"; -const DONE_PAGE: &str = "NeMo Relay

Signed in. You can close this tab and return to the terminal.

"; -const FAILED_PAGE: &str = "NeMo Relay

The sign-in was not completed. Return to the terminal.

"; -const FOREIGN_PAGE: &str = "NeMo Relay

This page belongs to another sign-in attempt. Use the link the terminal printed.

"; +const PAGE_SIGNED_IN: &str = "NeMo Relay

Signed in. You can close this tab and return to the terminal.

"; +const PAGE_NOT_COMPLETED: &str = "NeMo Relay

The sign-in was not completed. Return to the terminal.

"; +const PAGE_FOREIGN_ATTEMPT: &str = "NeMo Relay

This page belongs to another sign-in attempt. Use the link the terminal printed.

"; + +const PASTE_FOREIGN_ATTEMPT: &str = "That address belongs to another sign-in attempt. Paste the address of the tab this command opened."; +const PASTE_NOT_A_REDIRECT: &str = "That is not a sign-in redirect. Paste the whole http://127.0.0.1:/callback?code=...&state=... address from the browser."; + +/// Opens the authorization address; returns whether a browser started. +pub(crate) type Opener = dyn Fn(&str) -> bool + Send + Sync; /// How a browser sign-in reaches the user and comes back. pub(crate) struct BrowserApproval<'a> { /// Loopback port for the redirect; 0 picks a free one. pub(crate) port: u16, - /// Opens the authorization address; returns whether a browser started. - pub(crate) open: &'a (dyn Fn(&str) -> bool + Send + Sync), + pub(crate) open: &'a Opener, /// Redirect addresses pasted into the terminal. pub(crate) pasted: mpsc::Receiver, pub(crate) timeout: Duration, } -/// 32 random bytes as unpadded base64url: the PKCE verifier and the OAuth state value. -pub(crate) fn random_value() -> Result { - let mut bytes = [0u8; 32]; +impl BrowserApproval<'static> { + /// The approval `nemo-relay auth login` uses: the system browser unless `open_browser` is + /// false, and redirect addresses typed into the terminal. + pub(crate) fn interactive(open_browser: bool, port: u16) -> Self { + Self { + port, + open: if open_browser { + &open_in_system_browser + } else { + &print_only + }, + pasted: terminal_lines(), + timeout: APPROVAL_TIMEOUT, + } + } +} + +/// Runs the browser sign-in and returns the issued tokens. +pub(crate) async fn sign_in( + client: &OidcClient, + scope: &str, + state: &str, + approval: BrowserApproval<'_>, + mut show: impl FnMut(String), +) -> Result { + let verifier = random_value()?; + let callback = Callback::listen(approval.port, state).await?; + let redirect_uri = callback.redirect_uri.clone(); + let url = client.authorization_url(scope, &redirect_uri, state, &code_challenge(&verifier))?; + announce(&url, (approval.open)(&url), &redirect_uri, &mut show); + let redirect = callback + .wait(approval.pasted, &mut show, approval.timeout) + .await?; + let code = redirect.into_code(client.issuer())?; + client.exchange_code(&code, &redirect_uri, &verifier).await +} + +fn announce(url: &str, opened: bool, redirect_uri: &str, show: &mut impl FnMut(String)) { + show(if opened { + format!("Opened the browser to sign in. If it did not open, visit:\n{url}") + } else { + format!("To sign in, open this address in a browser on any device:\n{url}") + }); + show(format!( + "If the browser runs on another device, its last page fails to load. Paste that page's address ({redirect_uri}?code=...) here and press Enter." + )); +} + +/// Unpadded base64url of fresh random bytes: the PKCE verifier and the OAuth state value. +pub(crate) fn random_value() -> Result { + let mut bytes = [0u8; RANDOM_BYTES]; SystemRandom::new() .fill(&mut bytes) - .map_err(|_| auth_error("the system random number generator failed".into()))?; + .map_err(|_| SignInError::NoRandomness)?; Ok(URL_SAFE_NO_PAD.encode(bytes)) } @@ -62,34 +120,62 @@ pub(crate) struct Redirect { pub(crate) error_description: Option, } -/// Reads a pasted redirect address. Accepts the address with or without angle brackets. -pub(crate) fn parse_pasted(text: &str) -> Option { - let trimmed = text - .trim() - .trim_start_matches('<') - .trim_end_matches('>') - .trim(); - let url = reqwest::Url::parse(trimmed).ok()?; - let mut redirect = Redirect::default(); - for (key, value) in url.query_pairs() { - let value = Some(value.into_owned()); - match key.as_ref() { - "code" => redirect.code = value, - "state" => redirect.state = value, - "iss" => redirect.iss = value, - "error" => redirect.error = value, - "error_description" => redirect.error_description = value, - _ => {} +impl Redirect { + /// The authorization code, once the redirect is known to come from `issuer` and approve. + fn into_code(self, issuer: &str) -> Result { + if self + .iss + .is_some_and(|iss| iss.trim_end_matches('/') != issuer) + { + return Err(SignInError::ForeignIssuer); + } + if let Some(error) = &self.error { + return Err(SignInError::from_redirect_error( + error, + self.error_description.as_deref(), + )); } + self.code.ok_or(SignInError::MissingCode) + } +} + +/// A line typed into the terminal while the sign-in waits. +#[derive(Debug, PartialEq, Eq)] +pub(crate) enum Paste { + Blank, + NotARedirect, + ForeignAttempt, + Accepted(Redirect), +} + +/// Reads a pasted redirect address, with or without angle brackets around it. +pub(crate) fn read_paste(line: &str, expected_state: &str) -> Paste { + let text = line.trim().trim_start_matches('<').trim_end_matches('>'); + if text.is_empty() { + return Paste::Blank; + } + let Some(redirect) = text + .parse::() + .ok() + .and_then(|uri| Query::::try_from_uri(&uri).ok()) + .map(|Query(redirect)| redirect) + .filter(|redirect| redirect.code.is_some() || redirect.error.is_some()) + else { + return Paste::NotARedirect; + }; + if redirect.state.as_deref() == Some(expected_state) { + Paste::Accepted(redirect) + } else { + Paste::ForeignAttempt } - (redirect.code.is_some() || redirect.error.is_some()).then_some(redirect) } /// The loopback listener the issuer redirects to. pub(crate) struct Callback { pub(crate) redirect_uri: String, + expected_state: String, received: mpsc::Receiver, - shutdown: Option>, + stop: oneshot::Sender<()>, } #[derive(Clone)] @@ -100,85 +186,91 @@ struct CallbackState { impl Callback { /// Listens on `127.0.0.1:`; port 0 picks a free one. - pub(crate) async fn listen(port: u16, expected_state: &str) -> Result { - let listener = tokio::net::TcpListener::bind(("127.0.0.1", port)) + pub(crate) async fn listen(port: u16, expected_state: &str) -> Result { + let listener = tokio::net::TcpListener::bind((CALLBACK_HOST, port)) .await - .map_err(|error| { - auth_error(format!( - "cannot listen on 127.0.0.1:{port} for the sign-in redirect: {error}" - )) - })?; - let port = listener.local_addr()?.port(); + .map_err(|source| SignInError::CallbackUnavailable { port, source })?; + let bound_port = listener + .local_addr() + .map_err(|source| SignInError::CallbackUnavailable { port, source })? + .port(); let (sender, received) = mpsc::channel(4); - let (shutdown, stop) = tokio::sync::oneshot::channel::<()>(); + let (stop, stopped) = oneshot::channel::<()>(); + let state = CallbackState { + expected_state: expected_state.to_owned(), + sender, + }; let app = Router::new() .route(CALLBACK_PATH, get(receive)) - .with_state(CallbackState { - expected_state: expected_state.to_owned(), - sender, - }); - tokio::spawn(async move { - let _ = axum::serve(listener, app) - .with_graceful_shutdown(async { - let _ = stop.await; - }) - .await; - }); + .with_state(state); + tokio::spawn(serve_until_stopped(listener, app, stopped)); Ok(Self { - redirect_uri: format!("http://127.0.0.1:{port}{CALLBACK_PATH}"), + redirect_uri: format!("http://{CALLBACK_HOST}:{bound_port}{CALLBACK_PATH}"), + expected_state: expected_state.to_owned(), received, - shutdown: Some(shutdown), + stop, }) } - /// Waits for the redirect that carries this attempt's state, from the browser or from a - /// pasted address. A paste for another attempt is reported through `show` and ignored. + /// Waits for the redirect of this attempt, from the browser or pasted, then stops listening. pub(crate) async fn wait( mut self, - expected_state: &str, + pasted: mpsc::Receiver, + show: impl FnMut(String), + timeout: Duration, + ) -> Result { + let first = tokio::time::timeout(timeout, self.first_redirect(pasted, show)).await; + let _ = self.stop.send(()); + first.map_err(|_| SignInError::TimedOut) + } + + async fn first_redirect( + &mut self, mut pasted: mpsc::Receiver, mut show: impl FnMut(String), - timeout: Duration, - ) -> Result { - let deadline = tokio::time::Instant::now() + timeout; + ) -> Redirect { let mut paste_open = true; - let result = loop { + loop { tokio::select! { - Some(redirect) = self.received.recv() => break Ok(redirect), - line = pasted.recv(), if paste_open => match line { + Some(redirect) = self.received.recv() => return redirect, + line = pasted.recv(), if paste_open => match line.map(|line| read_paste(&line, &self.expected_state)) { None => paste_open = false, - Some(line) if line.trim().is_empty() => {} - Some(line) => match parse_pasted(&line) { - Some(redirect) if redirect.state.as_deref() == Some(expected_state) => { - break Ok(redirect); - } - Some(_) => show("That address belongs to another sign-in attempt. Paste the address of the tab this command opened.".into()), - None => show("That is not a sign-in redirect. Paste the whole http://127.0.0.1:/callback?code=...&state=... address from the browser.".into()), - }, + Some(Paste::Accepted(redirect)) => return redirect, + Some(Paste::Blank) => {} + Some(Paste::ForeignAttempt) => show(PASTE_FOREIGN_ATTEMPT.into()), + Some(Paste::NotARedirect) => show(PASTE_NOT_A_REDIRECT.into()), }, - () = tokio::time::sleep_until(deadline) => { - break Err(auth_error("timed out after 5 minutes waiting for the sign-in; run the login again".into())); - } + else => std::future::pending::<()>().await, } - }; - if let Some(shutdown) = self.shutdown.take() { - let _ = shutdown.send(()); } - result } } +async fn serve_until_stopped( + listener: tokio::net::TcpListener, + app: Router, + stopped: oneshot::Receiver<()>, +) { + let shutdown = async { + let _ = stopped.await; + }; + let _ = axum::serve(listener, app) + .with_graceful_shutdown(shutdown) + .await; +} + +/// Answers the browser; only a redirect with this attempt's state reaches the command. async fn receive( State(state): State, Query(redirect): Query, ) -> Html<&'static str> { if redirect.state.as_deref() != Some(state.expected_state.as_str()) { - return Html(FOREIGN_PAGE); + return Html(PAGE_FOREIGN_ATTEMPT); } let page = if redirect.code.is_some() { - DONE_PAGE + PAGE_SIGNED_IN } else { - FAILED_PAGE + PAGE_NOT_COMPLETED }; let _ = state.sender.send(redirect).await; Html(page) @@ -186,13 +278,12 @@ async fn receive( /// Lines typed into the terminal, read on a thread of their own so a blocked read never holds /// the runtime open. The channel closes at end of input. -pub(crate) fn terminal_lines() -> mpsc::Receiver { +fn terminal_lines() -> mpsc::Receiver { let (sender, receiver) = mpsc::channel(4); std::thread::spawn(move || { - let stdin = std::io::stdin(); - let mut line = String::new(); - while matches!(stdin.read_line(&mut line), Ok(read) if read > 0) { - if sender.blocking_send(std::mem::take(&mut line)).is_err() { + for line in std::io::stdin().lines() { + let Ok(line) = line else { break }; + if sender.blocking_send(line).is_err() { break; } } @@ -200,18 +291,9 @@ pub(crate) fn terminal_lines() -> mpsc::Receiver { receiver } -/// Opens the address in the default browser. Returns whether a browser was started. -pub(crate) fn open_browser(url: &str) -> bool { - let mut command = if cfg!(target_os = "macos") { - std::process::Command::new("open") - } else if cfg!(windows) { - let mut command = std::process::Command::new("rundll32"); - command.arg("url.dll,FileProtocolHandler"); - command - } else if std::env::var_os("DISPLAY").is_some() || std::env::var_os("WAYLAND_DISPLAY").is_some() - { - std::process::Command::new("xdg-open") - } else { +/// Opens the address in the default browser; returns whether one started. +fn open_in_system_browser(url: &str) -> bool { + let Some(mut command) = system_browser_command() else { return false; }; command @@ -222,3 +304,23 @@ pub(crate) fn open_browser(url: &str) -> bool { .spawn() .is_ok() } + +/// The platform's command that opens an address. On Linux, only with a graphical display. +fn system_browser_command() -> Option { + if cfg!(target_os = "macos") { + return Some(std::process::Command::new("open")); + } + if cfg!(windows) { + let mut command = std::process::Command::new("rundll32"); + command.arg("url.dll,FileProtocolHandler"); + return Some(command); + } + let has_display = + std::env::var_os("DISPLAY").is_some() || std::env::var_os("WAYLAND_DISPLAY").is_some(); + has_display.then(|| std::process::Command::new("xdg-open")) +} + +/// Prints the address only, for `--no-browser`. +fn print_only(_url: &str) -> bool { + false +} diff --git a/crates/cli/src/oidc/client.rs b/crates/cli/src/oidc/client.rs index 62eb7eb0e..6bb45ec89 100644 --- a/crates/cli/src/oidc/client.rs +++ b/crates/cli/src/oidc/client.rs @@ -1,7 +1,8 @@ // SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -//! Requests to the OpenID Connect issuer: discovery, the device sign-in and token refresh. +//! Requests to the OpenID Connect issuer: discovery, the device sign-in, the authorization code +//! exchange, token refresh and revocation. use std::time::Duration; @@ -9,7 +10,7 @@ use http::header::{ACCEPT, CONTENT_TYPE}; use percent_encoding::{NON_ALPHANUMERIC, utf8_percent_encode}; use serde::Deserialize; -use super::browser::{self, BrowserApproval}; +use super::error::SignInError; use super::session::Session; use super::{Clock, HTTP_TIMEOUT, OidcSource, auth_error, login_required, now_secs}; use crate::error::CliError; @@ -28,15 +29,25 @@ struct Discovery { revocation_endpoint: Option, } -/// What the issuer said to a revocation request. +/// What happened to a refresh token when its sign-in was removed or replaced. #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) enum Revocation { + /// The issuer confirmed the revocation. Confirmed, /// The issuer publishes no revocation endpoint. - Unsupported, + NotOffered, + /// The sign-in held no refresh token. + NothingToRevoke, + /// The issuer did not confirm; the reason holds no token. Failed(String), } +/// Why a request did not get an answer; it names no host, token or path. +pub(crate) const ISSUER_UNREACHABLE: &str = "the issuer could not be reached"; +const AUTHORIZATION_CODE_GRANT: &str = "authorization_code"; +const PKCE_METHOD: &str = "S256"; +const REFRESH_TOKEN_HINT: &str = "refresh_token"; + #[derive(Deserialize)] pub(crate) struct DeviceStart { pub(crate) device_code: String, @@ -128,6 +139,10 @@ impl OidcClient { }) } + pub(crate) fn issuer(&self) -> &str { + &self.issuer + } + /// The address the user opens to approve the sign-in. pub(crate) fn authorization_url( &self, @@ -135,15 +150,15 @@ impl OidcClient { redirect_uri: &str, state: &str, challenge: &str, - ) -> Result { + ) -> Result { let endpoint = self.authorization_endpoint.as_deref().ok_or_else(|| { - auth_error(format!( - "{} does not offer browser sign-in (no authorization_endpoint); use --device", - self.issuer - )) + SignInError::BrowserSignInNotOffered { + issuer: self.issuer.clone(), + } + })?; + let mut url = reqwest::Url::parse(endpoint).map_err(|_| SignInError::InvalidEndpoint { + endpoint: endpoint.to_owned(), })?; - let mut url = reqwest::Url::parse(endpoint) - .map_err(|_| auth_error(format!("{endpoint} is not a valid URL")))?; url.query_pairs_mut() .append_pair("response_type", "code") .append_pair("client_id", &self.client_id) @@ -151,89 +166,52 @@ impl OidcClient { .append_pair("scope", scope) .append_pair("state", state) .append_pair("code_challenge", challenge) - .append_pair("code_challenge_method", "S256"); + .append_pair("code_challenge_method", PKCE_METHOD); Ok(url.into()) } - /// Signs in through the browser and returns the issued tokens. - pub(crate) async fn browser_login( + /// Exchanges an approved authorization code and its PKCE verifier for tokens. + pub(crate) async fn exchange_code( &self, - scope: &str, - state: &str, - approval: BrowserApproval<'_>, - mut show: impl FnMut(String), + code: &str, + redirect_uri: &str, + verifier: &str, ) -> Result { - let verifier = browser::random_value()?; - let callback = browser::Callback::listen(approval.port, state).await?; - let redirect_uri = callback.redirect_uri.clone(); - let url = self.authorization_url( - scope, - &redirect_uri, - state, - &browser::code_challenge(&verifier), - )?; - let opened = (approval.open)(&url); - show(if opened { - format!("Opened the browser to sign in. If it did not open, visit:\n{url}") - } else { - format!("To sign in, open this address in a browser on any device:\n{url}") - }); - show(format!( - "If the browser runs on another device, its last page fails to load. Paste that page's address ({redirect_uri}?code=...) here and press Enter." - )); - let redirect = callback - .wait(state, approval.pasted, &mut show, approval.timeout) - .await?; - if redirect - .iss - .as_deref() - .is_some_and(|iss| iss.trim_end_matches('/') != self.issuer) - { - return Err(auth_error( - "the sign-in redirect names another issuer".into(), - )); - } - if let Some(error) = redirect.error { - return Err(device_failure( - &error, - &describe(&error, redirect.error_description.as_deref()), - )); - } - let code = redirect - .code - .ok_or_else(|| auth_error("the sign-in redirect carries no code".into()))?; let grant = self .token_request(&[ - ("grant_type", "authorization_code"), - ("code", &code), - ("redirect_uri", &redirect_uri), + ("grant_type", AUTHORIZATION_CODE_GRANT), + ("code", code), + ("redirect_uri", redirect_uri), ("client_id", &self.client_id), - ("code_verifier", &verifier), + ("code_verifier", verifier), ]) .await; match grant { Grant::Issued(reply) => self.session(reply, None, now_secs()), - Grant::Refused { detail, .. } | Grant::Unavailable(detail) => Err(auth_error(format!( - "{} did not issue a token for the approved sign-in: {detail}", - self.issuer - ))), + Grant::Refused { detail, .. } | Grant::Unavailable(detail) => { + Err(SignInError::TokenNotIssued { + issuer: self.issuer.clone(), + detail, + } + .into()) + } } } /// Revokes a refresh token (RFC 7009). pub(crate) async fn revoke(&self, refresh_token: &str) -> Revocation { let Some(endpoint) = self.revocation_endpoint.as_deref() else { - return Revocation::Unsupported; + return Revocation::NotOffered; }; let form = [ ("client_id", self.client_id.as_str()), ("token", refresh_token), - ("token_type_hint", "refresh_token"), + ("token_type_hint", REFRESH_TOKEN_HINT), ]; match self.post(endpoint, &form).await { Ok(response) if response.status().is_success() => Revocation::Confirmed, Ok(response) => Revocation::Failed(format!("HTTP {}", response.status())), - Err(_) => Revocation::Failed("the issuer could not be reached".into()), + Err(_) => Revocation::Failed(ISSUER_UNREACHABLE.into()), } } @@ -347,7 +325,7 @@ impl OidcClient { async fn token_request(&self, form: &[(&str, &str)]) -> Grant { let response = match self.post(&self.token_endpoint, form).await { Ok(response) => response, - Err(_) => return Grant::Unavailable("the issuer could not be reached".into()), + Err(_) => return Grant::Unavailable(ISSUER_UNREACHABLE.into()), }; let status = response.status(); let Ok(reply) = response.json::().await else { diff --git a/crates/cli/src/oidc/consent.rs b/crates/cli/src/oidc/consent.rs index b272ecafd..f78e52629 100644 --- a/crates/cli/src/oidc/consent.rs +++ b/crates/cli/src/oidc/consent.rs @@ -17,7 +17,8 @@ use crate::error::CliError; /// The consent service refuses a longer restriction segment. pub(crate) const MAX_SEGMENT_LEN: usize = 1024; -/// Largest integer a JSON number keeps exactly in JavaScript. +const STATE_SEPARATOR: char = '.'; +/// Largest whole number JavaScript keeps exact. const MAX_SAFE_INTEGER: f64 = 9_007_199_254_740_991.0; /// How often a pinned credit limit resets. @@ -39,6 +40,24 @@ pub(crate) enum ConsentField { ExistingKey, } +/// A restriction the consent service would refuse. +#[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)] +pub(crate) enum RestrictionError { + #[error("organization_id and workspace_id must not be empty")] + EmptyIdentifier, + #[error("credit_limit must be a finite amount of zero or more")] + InvalidCreditLimit, + #[error("credit_limit_reset needs credit_limit")] + ResetWithoutLimit, + #[error("hidden_fields needs organization_id, workspace_id or credit_limit")] + HiddenFieldsWithoutPin, + #[error("hidden_fields lists a field twice")] + RepeatedHiddenField, + #[error("the consent restriction does not fit the OAuth state ({MAX_SEGMENT_LEN} characters)")] + TooLong, +} + +/// The JSON the consent service decodes; serde writes the members in declaration order. #[derive(Serialize)] #[serde(rename_all = "camelCase")] struct RestrictionClaim<'a> { @@ -54,13 +73,26 @@ struct RestrictionClaim<'a> { hidden_fields: &'a [ConsentField], } +impl<'a> From<&'a OidcSource> for RestrictionClaim<'a> { + fn from(source: &'a OidcSource) -> Self { + Self { + organization_id: source.organization_id.as_deref(), + workspace_id: source.workspace_id.as_deref(), + credit_limit: source.credit_limit.as_ref().map(javascript_number), + credit_limit_reset: source.credit_limit_reset, + hidden_fields: &source.hidden_fields, + } + } +} + impl OidcSource { pub(crate) fn has_consent_restrictions(&self) -> bool { - self.organization_id.is_some() - || self.workspace_id.is_some() - || self.credit_limit.is_some() - || self.credit_limit_reset.is_some() - || !self.hidden_fields.is_empty() + self.has_pin() || self.credit_limit_reset.is_some() || !self.hidden_fields.is_empty() + } + + /// An organization, a workspace or a credit limit: what makes a restriction claim. + fn has_pin(&self) -> bool { + self.organization_id.is_some() || self.workspace_id.is_some() || self.credit_limit.is_some() } /// The encoded restriction segment, or `None` when the section pins nothing. @@ -68,68 +100,90 @@ impl OidcSource { if !self.has_consent_restrictions() { return None; } - let claim = RestrictionClaim { - organization_id: self.organization_id.as_deref(), - workspace_id: self.workspace_id.as_deref(), - credit_limit: self.credit_limit.as_ref().map(javascript_number), - credit_limit_reset: self.credit_limit_reset, - hidden_fields: &self.hidden_fields, - }; - let json = serde_json::to_string(&claim).unwrap_or_default(); + let json = serde_json::to_string(&RestrictionClaim::from(self)).ok()?; Some(URL_SAFE_NO_PAD.encode(json)) } /// The OAuth `state`: the random value alone, or followed by `.` and the restriction. pub(crate) fn authorization_state(&self, random: &str) -> String { match self.restriction_segment() { - Some(segment) => format!("{random}.{segment}"), + Some(segment) => format!("{random}{STATE_SEPARATOR}{segment}"), None => random.to_owned(), } } - /// Refuses a restriction the consent service would refuse, when the configuration loads. + /// Refuses, when the configuration loads, a restriction the consent service would refuse. pub(crate) fn validate_consent_restrictions(&self, name: &str) -> Result<(), CliError> { + self.restriction_error().map_or(Ok(()), |error| { + Err(CliError::Config(format!("{name}: {error}"))) + }) + } + + fn restriction_error(&self) -> Option { if !self.has_consent_restrictions() { - return Ok(()); + return None; } - let invalid = |detail: &str| Err(CliError::Config(format!("{name}: {detail}"))); - let blank = - |value: &Option| value.as_deref().is_some_and(|id| id.trim().is_empty()); + let checks = [ + Self::check_identifiers, + Self::check_credit_limit, + Self::check_reset, + Self::check_pin, + Self::check_hidden_fields, + Self::check_length, + ]; + checks.iter().find_map(|check| check(self).err()) + } + + fn check_identifiers(&self) -> Result<(), RestrictionError> { + let blank = |id: &Option| id.as_deref().is_some_and(|id| id.trim().is_empty()); if blank(&self.organization_id) || blank(&self.workspace_id) { - return invalid("organization_id and workspace_id must not be empty"); + return Err(RestrictionError::EmptyIdentifier); } - if let Some(limit) = &self.credit_limit - && !limit + Ok(()) + } + + fn check_credit_limit(&self) -> Result<(), RestrictionError> { + let valid = |limit: &serde_json::Number| { + limit .as_f64() .is_some_and(|value| value.is_finite() && value >= 0.0) - { - return invalid("credit_limit must be a finite amount of zero or more"); + }; + match &self.credit_limit { + Some(limit) if !valid(limit) => Err(RestrictionError::InvalidCreditLimit), + _ => Ok(()), } + } + + fn check_reset(&self) -> Result<(), RestrictionError> { if self.credit_limit_reset.is_some() && self.credit_limit.is_none() { - return invalid("credit_limit_reset needs credit_limit"); + return Err(RestrictionError::ResetWithoutLimit); } - if self.organization_id.is_none() - && self.workspace_id.is_none() - && self.credit_limit.is_none() - { - return invalid("hidden_fields needs organization_id, workspace_id or credit_limit"); + Ok(()) + } + + fn check_pin(&self) -> Result<(), RestrictionError> { + if self.has_pin() { + Ok(()) + } else { + Err(RestrictionError::HiddenFieldsWithoutPin) } - let repeated = self - .hidden_fields - .iter() - .enumerate() - .any(|(index, field)| self.hidden_fields[..index].contains(field)); + } + + fn check_hidden_fields(&self) -> Result<(), RestrictionError> { + let fields = &self.hidden_fields; + let repeated = (1..fields.len()).any(|index| fields[..index].contains(&fields[index])); if repeated { - return invalid("hidden_fields lists a field twice"); + return Err(RestrictionError::RepeatedHiddenField); } - if self + Ok(()) + } + + fn check_length(&self) -> Result<(), RestrictionError> { + let length = self .restriction_segment() - .map_or(0, |segment| segment.len()) - > MAX_SEGMENT_LEN - { - return invalid( - "the consent restriction does not fit the OAuth state (1024 characters)", - ); + .map_or(0, |segment| segment.len()); + if length > MAX_SEGMENT_LEN { + return Err(RestrictionError::TooLong); } Ok(()) } @@ -137,16 +191,14 @@ impl OidcSource { /// `JSON.stringify` writes a whole amount without a fraction, so `25.0` encodes as `25`. fn javascript_number(number: &serde_json::Number) -> serde_json::Number { - match number.as_f64() { - Some(value) - if number.is_f64() - && value.fract() == 0.0 - && (0.0..=MAX_SAFE_INTEGER).contains(&value) => - { - #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] - serde_json::Number::from(value as u64) - } - _ => number.clone(), + let whole = number + .as_f64() + .filter(|value| number.is_f64() && value.fract() == 0.0) + .filter(|value| (0.0..=MAX_SAFE_INTEGER).contains(value)); + match whole { + #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] + Some(value) => serde_json::Number::from(value as u64), + None => number.clone(), } } diff --git a/crates/cli/src/oidc/error.rs b/crates/cli/src/oidc/error.rs new file mode 100644 index 000000000..a8d016597 --- /dev/null +++ b/crates/cli/src/oidc/error.rs @@ -0,0 +1,56 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Why a browser sign-in or a logout did not complete. No variant holds a token. + +use super::browser::APPROVAL_TIMEOUT; + +#[derive(Debug, thiserror::Error)] +pub(crate) enum SignInError { + #[error("the system random number generator failed")] + NoRandomness, + #[error("cannot listen on 127.0.0.1:{port} for the sign-in redirect: {source}")] + CallbackUnavailable { port: u16, source: std::io::Error }, + #[error("{issuer} does not offer browser sign-in (no authorization_endpoint); use --device")] + BrowserSignInNotOffered { issuer: String }, + #[error("{endpoint} is not a valid URL")] + InvalidEndpoint { endpoint: String }, + #[error( + "timed out after {} minutes waiting for the sign-in; run the login again", + APPROVAL_TIMEOUT.as_secs() / 60 + )] + TimedOut, + #[error("the sign-in redirect names another issuer")] + ForeignIssuer, + #[error("the sign-in redirect carries no code")] + MissingCode, + #[error("the sign-in was declined")] + Declined, + #[error("the sign-in was not completed: {detail}")] + NotCompleted { detail: String }, + #[error("{issuer} did not issue a token for the approved sign-in: {detail}")] + TokenNotIssued { issuer: String, detail: String }, + #[error( + "the device sign-in cannot carry the configured consent restrictions; sign in without --device" + )] + DeviceCannotCarryRestrictions, + #[error( + "removed the sign-in for {issuer}, but the issuer did not confirm revoking it ({reason}); it stays valid until it expires" + )] + RevocationUnconfirmed { issuer: String, reason: String }, +} + +impl SignInError { + /// An OAuth error code from the redirect (RFC 6749 section 4.1.2.1). + pub(crate) fn from_redirect_error(code: &str, description: Option<&str>) -> Self { + match (code, description.filter(|text| !text.trim().is_empty())) { + ("access_denied", _) => Self::Declined, + (code, Some(text)) => Self::NotCompleted { + detail: format!("{code} ({text})"), + }, + (code, None) => Self::NotCompleted { + detail: code.to_owned(), + }, + } + } +} diff --git a/crates/cli/src/oidc/mod.rs b/crates/cli/src/oidc/mod.rs index e9877efe5..2e1fe86d8 100644 --- a/crates/cli/src/oidc/mod.rs +++ b/crates/cli/src/oidc/mod.rs @@ -14,6 +14,7 @@ mod browser; mod client; mod consent; +mod error; mod session; use std::path::Path; @@ -21,15 +22,17 @@ use std::time::{Duration, SystemTime, UNIX_EPOCH}; use serde::{Deserialize, Serialize}; -use self::browser::BrowserApproval; -use self::client::{OidcClient, secure_origin}; +use self::client::{ISSUER_UNREACHABLE, OidcClient, secure_origin}; use self::session::{ - SessionLock, ensure_sessions_dir, lock_timeout, read_session, session_path, write_session, + Session, SessionLock, ensure_sessions_dir, lock_timeout, read_session, session_path, + write_session, }; use crate::error::CliError; +pub(crate) use self::browser::BrowserApproval; pub(crate) use self::client::Revocation; pub(crate) use self::consent::{ConsentField, CreditLimitReset}; +pub(crate) use self::error::SignInError; pub(crate) use self::session::sessions_dir; /// Refresh an access token this many seconds before it expires. @@ -197,21 +200,12 @@ pub(crate) enum LoginMethod<'a> { Device, } -/// The browser sign-in as the command runs it: the system browser unless `open_browser` is -/// false, and redirect addresses pasted into the terminal. -pub(crate) fn browser_method(open_browser: bool, port: u16) -> LoginMethod<'static> { - fn open(url: &str) -> bool { - browser::open_browser(url) - } - fn print_only(_: &str) -> bool { - false - } - LoginMethod::Browser(BrowserApproval { - port, - open: if open_browser { &open } else { &print_only }, - pasted: browser::terminal_lines(), - timeout: browser::APPROVAL_TIMEOUT, - }) +/// The result of `logout`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum Logout { + NotSignedIn, + /// The stored sign-in was removed, and its refresh token revoked or not. + Removed(Revocation), } /// Signs in and stores the session. A sign-in it replaces is revoked at the issuer. @@ -222,65 +216,91 @@ pub(crate) async fn login( show: impl FnMut(String), ) -> Result<(), CliError> { let client = OidcClient::discover(source).await?; - let session = match method { + let session = sign_in(&client, source, method, show).await?; + let replaced = save_session(dir, source, &session).await?; + if let Some(old_token) = replaced.and_then(|old| old.refresh_token) + && session.refresh_token.as_deref() != Some(old_token.as_str()) + { + client.revoke(&old_token).await; + } + Ok(()) +} + +async fn sign_in( + client: &OidcClient, + source: &OidcSource, + method: LoginMethod<'_>, + show: impl FnMut(String), +) -> Result { + match method { LoginMethod::Browser(approval) => { let state = source.authorization_state(&browser::random_value()?); - client - .browser_login(&source.scope, &state, approval, show) - .await? + browser::sign_in(client, &source.scope, &state, approval, show).await } LoginMethod::Device if source.has_consent_restrictions() => { - return Err(auth_error( - "the device sign-in cannot carry the configured consent restrictions; sign in without --device".into(), - )); + Err(SignInError::DeviceCannotCarryRestrictions.into()) } - LoginMethod::Device => client.device_login(&source.scope, show).await?, - }; + LoginMethod::Device => client.device_login(&source.scope, show).await, + } +} + +/// Writes the session and returns the one it replaced. +async fn save_session( + dir: &Path, + source: &OidcSource, + session: &Session, +) -> Result, CliError> { ensure_sessions_dir(dir)?; let path = session_path(dir, source); let _lock = SessionLock::acquire(&path).await?; let replaced = read_session(&path, source).ok().flatten(); - write_session(&path, &session)?; - if let Some(old) = replaced.and_then(|old| old.refresh_token) - && session.refresh_token.as_deref() != Some(old.as_str()) - { - client.revoke(&old).await; + write_session(&path, session)?; + Ok(replaced) +} + +/// Removes a stored sign-in, then revokes its refresh token at the issuer. The local sign-in is +/// removed even when the issuer cannot be reached. +pub(crate) async fn logout(dir: &Path, source: &OidcSource) -> Result { + let outcome = match remove_session(dir, source).await? { + RemovedSession::Absent => Logout::NotSignedIn, + RemovedSession::Removed(Session { + refresh_token: Some(refresh_token), + .. + }) => Logout::Removed(revoke_at_issuer(source, &refresh_token).await), + RemovedSession::Removed(_) | RemovedSession::Unreadable => { + Logout::Removed(Revocation::NothingToRevoke) + } + }; + Ok(outcome) +} + +async fn revoke_at_issuer(source: &OidcSource, refresh_token: &str) -> Revocation { + match OidcClient::discover(source).await { + Ok(client) => client.revoke(refresh_token).await, + Err(_) => Revocation::Failed(ISSUER_UNREACHABLE.into()), } - Ok(()) } -/// The result of `logout`. -#[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) enum Logout { - NotSignedIn, - /// The stored sign-in was removed; its refresh token was revoked, or not. - Removed(Revocation), +/// What deleting the stored sign-in file found. +enum RemovedSession { + Absent, + /// The file existed but did not hold a sign-in for this source. + Unreadable, + Removed(Session), } -/// Removes a stored sign-in and revokes its refresh token at the issuer. The local sign-in is -/// removed even when the issuer cannot be reached. -pub(crate) async fn logout(dir: &Path, source: &OidcSource) -> Result { +async fn remove_session(dir: &Path, source: &OidcSource) -> Result { if !dir.exists() { - return Ok(Logout::NotSignedIn); + return Ok(RemovedSession::Absent); } let path = session_path(dir, source); let _lock = SessionLock::acquire(&path).await?; let stored = read_session(&path, source).ok().flatten(); match std::fs::remove_file(&path) { - Ok(()) => {} - Err(error) if error.kind() == std::io::ErrorKind::NotFound => { - return Ok(Logout::NotSignedIn); - } - Err(error) => return Err(CliError::Io(error)), + Ok(()) => Ok(stored.map_or(RemovedSession::Unreadable, RemovedSession::Removed)), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(RemovedSession::Absent), + Err(error) => Err(CliError::Io(error)), } - let Some(refresh_token) = stored.and_then(|session| session.refresh_token) else { - return Ok(Logout::Removed(Revocation::Unsupported)); - }; - let revocation = match OidcClient::discover(source).await { - Ok(client) => client.revoke(&refresh_token).await, - Err(_) => Revocation::Failed("the issuer could not be reached".into()), - }; - Ok(Logout::Removed(revocation)) } /// Reads the stored sign-in without contacting the issuer. diff --git a/crates/cli/tests/coverage/shared/oidc_tests.rs b/crates/cli/tests/coverage/shared/oidc_tests.rs index 11b1acdf4..01ae4996b 100644 --- a/crates/cli/tests/coverage/shared/oidc_tests.rs +++ b/crates/cli/tests/coverage/shared/oidc_tests.rs @@ -684,7 +684,7 @@ fn approving_browser( } fn approval<'a>( - open: &'a (dyn Fn(&str) -> bool + Send + Sync), + open: &'a super::browser::Opener, pasted: tokio::sync::mpsc::Receiver, ) -> BrowserApproval<'a> { BrowserApproval { @@ -741,15 +741,15 @@ async fn a_redirect_for_another_attempt_does_not_end_the_sign_in() { let browser = approving_browser(issuer.clone(), opened, vec![Some("forged"), None]); let client = OidcClient::discover(&source).await.unwrap(); - let session = client - .browser_login( - "openid", - "attempt-state", - approval(&browser, no_paste()), - |_| {}, - ) - .await - .unwrap(); + let session = super::browser::sign_in( + &client, + "openid", + "attempt-state", + approval(&browser, no_paste()), + |_| {}, + ) + .await + .unwrap(); assert_eq!(session.access_token, "access-1"); assert_eq!(issuer.lock().unwrap().grants.len(), 1); @@ -760,7 +760,7 @@ async fn a_pasted_redirect_completes_a_sign_in_without_a_local_browser() { let (source, issuer) = start_issuer().await; let client = OidcClient::discover(&source).await.unwrap(); let captured = issuer.clone(); - let printed = |url: &str| { + let printed = move |url: &str| { captured.lock().unwrap().challenge = Some(query(url)["code_challenge"].clone()); false }; @@ -774,15 +774,15 @@ async fn a_pasted_redirect_completes_a_sign_in_without_a_local_browser() { } let mut shown = Vec::new(); - let session = client - .browser_login( - "openid", - "attempt-state", - approval(&printed, pasted), - |line| shown.push(line), - ) - .await - .unwrap(); + let session = super::browser::sign_in( + &client, + "openid", + "attempt-state", + approval(&printed, pasted), + |line| shown.push(line), + ) + .await + .unwrap(); assert_eq!(session.refresh_token.as_deref(), Some("refresh-1")); let shown = shown.join("\n"); @@ -803,14 +803,14 @@ async fn a_declined_browser_sign_in_reports_it() { .unwrap(); let error = error_text( - client - .browser_login( - "openid", - "attempt-state", - approval(&printed, pasted), - |_| {}, - ) - .await, + super::browser::sign_in( + &client, + "openid", + "attempt-state", + approval(&printed, pasted), + |_| {}, + ) + .await, ); assert!(error.contains("declined"), "{error}"); @@ -824,7 +824,7 @@ async fn an_unanswered_browser_sign_in_times_out() { let mut waiting = approval(&printed, no_paste()); waiting.timeout = Duration::from_millis(50); - let error = error_text(client.browser_login("openid", "s", waiting, |_| {}).await); + let error = error_text(super::browser::sign_in(&client, "openid", "s", waiting, |_| {}).await); assert!(error.contains("timed out"), "{error}"); } From 2aedb1776351cbffbd25cf39a256fc2e0aff54ba Mon Sep 17 00:00:00 2001 From: flazouh Date: Mon, 9 Nov 2026 17:27:30 +0000 Subject: [PATCH 3/7] refactor(cli): keep only the browser sign-in Remove the consent restrictions and the revocation from this change, as asked in #1697. Keep the authorization endpoint free to sit on another origin, because only the browser opens it. It must still use https, or http on a loopback address. Refs #1697 Co-Authored-By: Claude Opus 5.5 Signed-off-by: flazouh --- crates/cli/src/commands/auth.rs | 20 +- crates/cli/src/error.rs | 2 +- crates/cli/src/oidc/client.rs | 48 +--- crates/cli/src/oidc/consent.rs | 207 ------------------ crates/cli/src/oidc/error.rs | 10 +- crates/cli/src/oidc/mod.rs | 125 ++--------- .../coverage/shared/oidc_consent_tests.rs | 135 ------------ .../cli/tests/coverage/shared/oidc_tests.rs | 128 ++--------- docs/nemo-relay-cli/basic-usage.mdx | 44 +--- 9 files changed, 52 insertions(+), 667 deletions(-) delete mode 100644 crates/cli/src/oidc/consent.rs delete mode 100644 crates/cli/tests/coverage/shared/oidc_consent_tests.rs diff --git a/crates/cli/src/commands/auth.rs b/crates/cli/src/commands/auth.rs index 3462844a4..00e0b1a0b 100644 --- a/crates/cli/src/commands/auth.rs +++ b/crates/cli/src/commands/auth.rs @@ -11,7 +11,7 @@ use clap::{Args, Subcommand, ValueEnum}; use super::serve::ServerArgs; use crate::configuration::GatewayConfig; use crate::error::CliError; -use crate::oidc::{BrowserApproval, LoginMethod, Logout, OidcSource, Revocation, SignInError}; +use crate::oidc::{BrowserApproval, LoginMethod, OidcSource}; #[derive(Debug, Clone, Args)] pub(crate) struct AuthCommand { @@ -48,7 +48,7 @@ pub(crate) struct LoginArgs { #[arg(long, default_value_t = 0, conflicts_with = "device")] pub(crate) callback_port: u16, /// Sign in with a code entered on the issuer's own page (OAuth device authorization) instead - /// of the browser redirect. It cannot carry consent restrictions. + /// of the browser redirect. #[arg(long)] pub(crate) device: bool, } @@ -111,18 +111,10 @@ async fn print_token(dir: &Path, source: OidcSource) -> Result<(), CliError> { } async fn logout(dir: &Path, source: OidcSource) -> Result<(), CliError> { - let issuer = source.issuer.clone(); - match crate::oidc::logout(dir, &source).await? { - Logout::NotSignedIn => println!("No sign-in is stored for {issuer}."), - Logout::Removed(Revocation::Confirmed) => { - println!("Removed the sign-in for {issuer} and revoked it at the issuer."); - } - Logout::Removed(Revocation::NotOffered | Revocation::NothingToRevoke) => { - println!("Removed the sign-in for {issuer}."); - } - Logout::Removed(Revocation::Failed(reason)) => { - return Err(SignInError::RevocationUnconfirmed { issuer, reason }.into()); - } + if crate::oidc::logout(dir, &source).await? { + println!("Removed the sign-in for {}.", source.issuer); + } else { + println!("No sign-in is stored for {}.", source.issuer); } Ok(()) } diff --git a/crates/cli/src/error.rs b/crates/cli/src/error.rs index ad94c9649..a5bea0f5f 100644 --- a/crates/cli/src/error.rs +++ b/crates/cli/src/error.rs @@ -112,7 +112,7 @@ pub(crate) enum CliError { /// The upstream needs an OpenID Connect sign-in that is missing or no longer valid. #[error("sign-in required: {0}")] SignInRequired(String), - /// A browser sign-in or a logout did not complete. + /// A browser sign-in did not complete. #[error(transparent)] SignIn(#[from] crate::oidc::SignInError), /// The daemon definitively rejected the route credential; retrying cannot succeed. diff --git a/crates/cli/src/oidc/client.rs b/crates/cli/src/oidc/client.rs index 6bb45ec89..7414e468d 100644 --- a/crates/cli/src/oidc/client.rs +++ b/crates/cli/src/oidc/client.rs @@ -2,7 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 //! Requests to the OpenID Connect issuer: discovery, the device sign-in, the authorization code -//! exchange, token refresh and revocation. +//! exchange and token refresh. use std::time::Duration; @@ -26,27 +26,10 @@ struct Discovery { token_endpoint: String, authorization_endpoint: Option, device_authorization_endpoint: Option, - revocation_endpoint: Option, } -/// What happened to a refresh token when its sign-in was removed or replaced. -#[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) enum Revocation { - /// The issuer confirmed the revocation. - Confirmed, - /// The issuer publishes no revocation endpoint. - NotOffered, - /// The sign-in held no refresh token. - NothingToRevoke, - /// The issuer did not confirm; the reason holds no token. - Failed(String), -} - -/// Why a request did not get an answer; it names no host, token or path. -pub(crate) const ISSUER_UNREACHABLE: &str = "the issuer could not be reached"; const AUTHORIZATION_CODE_GRANT: &str = "authorization_code"; const PKCE_METHOD: &str = "S256"; -const REFRESH_TOKEN_HINT: &str = "refresh_token"; #[derive(Deserialize)] pub(crate) struct DeviceStart { @@ -92,7 +75,6 @@ pub(crate) struct OidcClient { pub(super) token_endpoint: String, authorization_endpoint: Option, device_endpoint: Option, - revocation_endpoint: Option, } impl OidcClient { @@ -118,9 +100,7 @@ impl OidcClient { } // Endpoints on another origin could send the refresh token to an unintended host. let endpoints = std::iter::once(&discovery.token_endpoint) - .chain(discovery.authorization_endpoint.as_ref()) - .chain(discovery.device_authorization_endpoint.as_ref()) - .chain(discovery.revocation_endpoint.as_ref()); + .chain(discovery.device_authorization_endpoint.as_ref()); for endpoint in endpoints { if secure_origin(endpoint)? != origin { return Err(auth_error(format!( @@ -128,6 +108,10 @@ impl OidcClient { ))); } } + // The browser alone opens the authorization endpoint, so it may sit on another origin. + if let Some(endpoint) = &discovery.authorization_endpoint { + secure_origin(endpoint)?; + } Ok(Self { http, issuer: issuer.to_owned(), @@ -135,7 +119,6 @@ impl OidcClient { token_endpoint: discovery.token_endpoint, authorization_endpoint: discovery.authorization_endpoint, device_endpoint: discovery.device_authorization_endpoint, - revocation_endpoint: discovery.revocation_endpoint, }) } @@ -198,23 +181,6 @@ impl OidcClient { } } - /// Revokes a refresh token (RFC 7009). - pub(crate) async fn revoke(&self, refresh_token: &str) -> Revocation { - let Some(endpoint) = self.revocation_endpoint.as_deref() else { - return Revocation::NotOffered; - }; - let form = [ - ("client_id", self.client_id.as_str()), - ("token", refresh_token), - ("token_type_hint", REFRESH_TOKEN_HINT), - ]; - match self.post(endpoint, &form).await { - Ok(response) if response.status().is_success() => Revocation::Confirmed, - Ok(response) => Revocation::Failed(format!("HTTP {}", response.status())), - Err(_) => Revocation::Failed(ISSUER_UNREACHABLE.into()), - } - } - pub(crate) async fn device_login( &self, scope: &str, @@ -325,7 +291,7 @@ impl OidcClient { async fn token_request(&self, form: &[(&str, &str)]) -> Grant { let response = match self.post(&self.token_endpoint, form).await { Ok(response) => response, - Err(_) => return Grant::Unavailable(ISSUER_UNREACHABLE.into()), + Err(_) => return Grant::Unavailable("the issuer could not be reached".into()), }; let status = response.status(); let Ok(reply) = response.json::().await else { diff --git a/crates/cli/src/oidc/consent.rs b/crates/cli/src/oidc/consent.rs deleted file mode 100644 index f78e52629..000000000 --- a/crates/cli/src/oidc/consent.rs +++ /dev/null @@ -1,207 +0,0 @@ -// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. -// SPDX-License-Identifier: Apache-2.0 - -//! Consent restrictions an administrator pins in `[upstream._oidc]`. -//! -//! OpenRouter's consent page reads them from the OAuth `state`: a random value, a `.`, and an -//! unpadded base64url JSON object with the members `organizationId`, `workspaceId`, -//! `creditLimit`, `creditLimitReset` and `hiddenFields`, in that order. The encoding matches the -//! Ori CLI byte for byte, so the one consent service enforces both clients the same way. - -use base64::Engine; -use base64::engine::general_purpose::URL_SAFE_NO_PAD; -use serde::{Deserialize, Serialize}; - -use super::OidcSource; -use crate::error::CliError; - -/// The consent service refuses a longer restriction segment. -pub(crate) const MAX_SEGMENT_LEN: usize = 1024; -const STATE_SEPARATOR: char = '.'; -/// Largest whole number JavaScript keeps exact. -const MAX_SAFE_INTEGER: f64 = 9_007_199_254_740_991.0; - -/// How often a pinned credit limit resets. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize, Serialize)] -#[serde(rename_all = "lowercase")] -pub(crate) enum CreditLimitReset { - Daily, - Weekly, - Monthly, -} - -/// A consent page control the restriction hides. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize, Serialize)] -#[serde(rename_all = "camelCase")] -pub(crate) enum ConsentField { - KeyLabel, - Expiration, - CreditLimit, - ExistingKey, -} - -/// A restriction the consent service would refuse. -#[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)] -pub(crate) enum RestrictionError { - #[error("organization_id and workspace_id must not be empty")] - EmptyIdentifier, - #[error("credit_limit must be a finite amount of zero or more")] - InvalidCreditLimit, - #[error("credit_limit_reset needs credit_limit")] - ResetWithoutLimit, - #[error("hidden_fields needs organization_id, workspace_id or credit_limit")] - HiddenFieldsWithoutPin, - #[error("hidden_fields lists a field twice")] - RepeatedHiddenField, - #[error("the consent restriction does not fit the OAuth state ({MAX_SEGMENT_LEN} characters)")] - TooLong, -} - -/// The JSON the consent service decodes; serde writes the members in declaration order. -#[derive(Serialize)] -#[serde(rename_all = "camelCase")] -struct RestrictionClaim<'a> { - #[serde(skip_serializing_if = "Option::is_none")] - organization_id: Option<&'a str>, - #[serde(skip_serializing_if = "Option::is_none")] - workspace_id: Option<&'a str>, - #[serde(skip_serializing_if = "Option::is_none")] - credit_limit: Option, - #[serde(skip_serializing_if = "Option::is_none")] - credit_limit_reset: Option, - #[serde(skip_serializing_if = "<[_]>::is_empty")] - hidden_fields: &'a [ConsentField], -} - -impl<'a> From<&'a OidcSource> for RestrictionClaim<'a> { - fn from(source: &'a OidcSource) -> Self { - Self { - organization_id: source.organization_id.as_deref(), - workspace_id: source.workspace_id.as_deref(), - credit_limit: source.credit_limit.as_ref().map(javascript_number), - credit_limit_reset: source.credit_limit_reset, - hidden_fields: &source.hidden_fields, - } - } -} - -impl OidcSource { - pub(crate) fn has_consent_restrictions(&self) -> bool { - self.has_pin() || self.credit_limit_reset.is_some() || !self.hidden_fields.is_empty() - } - - /// An organization, a workspace or a credit limit: what makes a restriction claim. - fn has_pin(&self) -> bool { - self.organization_id.is_some() || self.workspace_id.is_some() || self.credit_limit.is_some() - } - - /// The encoded restriction segment, or `None` when the section pins nothing. - pub(crate) fn restriction_segment(&self) -> Option { - if !self.has_consent_restrictions() { - return None; - } - let json = serde_json::to_string(&RestrictionClaim::from(self)).ok()?; - Some(URL_SAFE_NO_PAD.encode(json)) - } - - /// The OAuth `state`: the random value alone, or followed by `.` and the restriction. - pub(crate) fn authorization_state(&self, random: &str) -> String { - match self.restriction_segment() { - Some(segment) => format!("{random}{STATE_SEPARATOR}{segment}"), - None => random.to_owned(), - } - } - - /// Refuses, when the configuration loads, a restriction the consent service would refuse. - pub(crate) fn validate_consent_restrictions(&self, name: &str) -> Result<(), CliError> { - self.restriction_error().map_or(Ok(()), |error| { - Err(CliError::Config(format!("{name}: {error}"))) - }) - } - - fn restriction_error(&self) -> Option { - if !self.has_consent_restrictions() { - return None; - } - let checks = [ - Self::check_identifiers, - Self::check_credit_limit, - Self::check_reset, - Self::check_pin, - Self::check_hidden_fields, - Self::check_length, - ]; - checks.iter().find_map(|check| check(self).err()) - } - - fn check_identifiers(&self) -> Result<(), RestrictionError> { - let blank = |id: &Option| id.as_deref().is_some_and(|id| id.trim().is_empty()); - if blank(&self.organization_id) || blank(&self.workspace_id) { - return Err(RestrictionError::EmptyIdentifier); - } - Ok(()) - } - - fn check_credit_limit(&self) -> Result<(), RestrictionError> { - let valid = |limit: &serde_json::Number| { - limit - .as_f64() - .is_some_and(|value| value.is_finite() && value >= 0.0) - }; - match &self.credit_limit { - Some(limit) if !valid(limit) => Err(RestrictionError::InvalidCreditLimit), - _ => Ok(()), - } - } - - fn check_reset(&self) -> Result<(), RestrictionError> { - if self.credit_limit_reset.is_some() && self.credit_limit.is_none() { - return Err(RestrictionError::ResetWithoutLimit); - } - Ok(()) - } - - fn check_pin(&self) -> Result<(), RestrictionError> { - if self.has_pin() { - Ok(()) - } else { - Err(RestrictionError::HiddenFieldsWithoutPin) - } - } - - fn check_hidden_fields(&self) -> Result<(), RestrictionError> { - let fields = &self.hidden_fields; - let repeated = (1..fields.len()).any(|index| fields[..index].contains(&fields[index])); - if repeated { - return Err(RestrictionError::RepeatedHiddenField); - } - Ok(()) - } - - fn check_length(&self) -> Result<(), RestrictionError> { - let length = self - .restriction_segment() - .map_or(0, |segment| segment.len()); - if length > MAX_SEGMENT_LEN { - return Err(RestrictionError::TooLong); - } - Ok(()) - } -} - -/// `JSON.stringify` writes a whole amount without a fraction, so `25.0` encodes as `25`. -fn javascript_number(number: &serde_json::Number) -> serde_json::Number { - let whole = number - .as_f64() - .filter(|value| number.is_f64() && value.fract() == 0.0) - .filter(|value| (0.0..=MAX_SAFE_INTEGER).contains(value)); - match whole { - #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] - Some(value) => serde_json::Number::from(value as u64), - None => number.clone(), - } -} - -#[cfg(test)] -#[path = "../../tests/coverage/shared/oidc_consent_tests.rs"] -mod tests; diff --git a/crates/cli/src/oidc/error.rs b/crates/cli/src/oidc/error.rs index a8d016597..e643ffb5f 100644 --- a/crates/cli/src/oidc/error.rs +++ b/crates/cli/src/oidc/error.rs @@ -1,7 +1,7 @@ // SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -//! Why a browser sign-in or a logout did not complete. No variant holds a token. +//! Why a browser sign-in did not complete. No variant holds a token. use super::browser::APPROVAL_TIMEOUT; @@ -30,14 +30,6 @@ pub(crate) enum SignInError { NotCompleted { detail: String }, #[error("{issuer} did not issue a token for the approved sign-in: {detail}")] TokenNotIssued { issuer: String, detail: String }, - #[error( - "the device sign-in cannot carry the configured consent restrictions; sign in without --device" - )] - DeviceCannotCarryRestrictions, - #[error( - "removed the sign-in for {issuer}, but the issuer did not confirm revoking it ({reason}); it stays valid until it expires" - )] - RevocationUnconfirmed { issuer: String, reason: String }, } impl SignInError { diff --git a/crates/cli/src/oidc/mod.rs b/crates/cli/src/oidc/mod.rs index 2e1fe86d8..ab66aaf42 100644 --- a/crates/cli/src/oidc/mod.rs +++ b/crates/cli/src/oidc/mod.rs @@ -13,7 +13,6 @@ mod browser; mod client; -mod consent; mod error; mod session; @@ -22,16 +21,13 @@ use std::time::{Duration, SystemTime, UNIX_EPOCH}; use serde::{Deserialize, Serialize}; -use self::client::{ISSUER_UNREACHABLE, OidcClient, secure_origin}; +use self::client::{OidcClient, secure_origin}; use self::session::{ - Session, SessionLock, ensure_sessions_dir, lock_timeout, read_session, session_path, - write_session, + SessionLock, ensure_sessions_dir, lock_timeout, read_session, session_path, write_session, }; use crate::error::CliError; pub(crate) use self::browser::BrowserApproval; -pub(crate) use self::client::Revocation; -pub(crate) use self::consent::{ConsentField, CreditLimitReset}; pub(crate) use self::error::SignInError; pub(crate) use self::session::sessions_dir; @@ -58,21 +54,6 @@ pub(crate) struct OidcSource { /// Space-separated scopes requested at sign-in. #[serde(default = "default_scope")] pub(crate) scope: String, - /// Consent restriction: the only organization the consent page offers. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub(crate) organization_id: Option, - /// Consent restriction: the only workspace the consent page offers. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub(crate) workspace_id: Option, - /// Consent restriction: the credit limit, in USD, of the key the sign-in yields. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub(crate) credit_limit: Option, - /// Consent restriction: how often that credit limit resets. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub(crate) credit_limit_reset: Option, - /// Consent restriction: consent page controls to hide. - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub(crate) hidden_fields: Vec, } impl OidcSource { @@ -81,11 +62,6 @@ impl OidcSource { issuer: issuer.trim_end_matches('/').to_owned(), client_id: client_id.to_owned(), scope: scope.map_or_else(default_scope, str::to_owned), - organization_id: None, - workspace_id: None, - credit_limit: None, - credit_limit_reset: None, - hidden_fields: Vec::new(), } } @@ -101,12 +77,11 @@ impl OidcSource { "{name}.client_id must not be empty" ))); } - secure_origin(self.issuer()).map_err(|_| { + secure_origin(self.issuer()).map(|_| ()).map_err(|_| { CliError::Config(format!( "{name}.issuer must be a valid https URL, or http on a loopback address" )) - })?; - self.validate_consent_restrictions(name) + }) } } @@ -200,15 +175,7 @@ pub(crate) enum LoginMethod<'a> { Device, } -/// The result of `logout`. -#[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) enum Logout { - NotSignedIn, - /// The stored sign-in was removed, and its refresh token revoked or not. - Removed(Revocation), -} - -/// Signs in and stores the session. A sign-in it replaces is revoked at the issuer. +/// Signs in and stores the session. pub(crate) async fn login( dir: &Path, source: &OidcSource, @@ -216,89 +183,29 @@ pub(crate) async fn login( show: impl FnMut(String), ) -> Result<(), CliError> { let client = OidcClient::discover(source).await?; - let session = sign_in(&client, source, method, show).await?; - let replaced = save_session(dir, source, &session).await?; - if let Some(old_token) = replaced.and_then(|old| old.refresh_token) - && session.refresh_token.as_deref() != Some(old_token.as_str()) - { - client.revoke(&old_token).await; - } - Ok(()) -} - -async fn sign_in( - client: &OidcClient, - source: &OidcSource, - method: LoginMethod<'_>, - show: impl FnMut(String), -) -> Result { - match method { + let session = match method { LoginMethod::Browser(approval) => { - let state = source.authorization_state(&browser::random_value()?); - browser::sign_in(client, &source.scope, &state, approval, show).await + let state = browser::random_value()?; + browser::sign_in(&client, &source.scope, &state, approval, show).await? } - LoginMethod::Device if source.has_consent_restrictions() => { - Err(SignInError::DeviceCannotCarryRestrictions.into()) - } - LoginMethod::Device => client.device_login(&source.scope, show).await, - } -} - -/// Writes the session and returns the one it replaced. -async fn save_session( - dir: &Path, - source: &OidcSource, - session: &Session, -) -> Result, CliError> { + LoginMethod::Device => client.device_login(&source.scope, show).await?, + }; ensure_sessions_dir(dir)?; let path = session_path(dir, source); let _lock = SessionLock::acquire(&path).await?; - let replaced = read_session(&path, source).ok().flatten(); - write_session(&path, session)?; - Ok(replaced) -} - -/// Removes a stored sign-in, then revokes its refresh token at the issuer. The local sign-in is -/// removed even when the issuer cannot be reached. -pub(crate) async fn logout(dir: &Path, source: &OidcSource) -> Result { - let outcome = match remove_session(dir, source).await? { - RemovedSession::Absent => Logout::NotSignedIn, - RemovedSession::Removed(Session { - refresh_token: Some(refresh_token), - .. - }) => Logout::Removed(revoke_at_issuer(source, &refresh_token).await), - RemovedSession::Removed(_) | RemovedSession::Unreadable => { - Logout::Removed(Revocation::NothingToRevoke) - } - }; - Ok(outcome) -} - -async fn revoke_at_issuer(source: &OidcSource, refresh_token: &str) -> Revocation { - match OidcClient::discover(source).await { - Ok(client) => client.revoke(refresh_token).await, - Err(_) => Revocation::Failed(ISSUER_UNREACHABLE.into()), - } -} - -/// What deleting the stored sign-in file found. -enum RemovedSession { - Absent, - /// The file existed but did not hold a sign-in for this source. - Unreadable, - Removed(Session), + write_session(&path, &session) } -async fn remove_session(dir: &Path, source: &OidcSource) -> Result { +/// Removes a stored sign-in; returns whether one existed. +pub(crate) async fn logout(dir: &Path, source: &OidcSource) -> Result { if !dir.exists() { - return Ok(RemovedSession::Absent); + return Ok(false); } let path = session_path(dir, source); let _lock = SessionLock::acquire(&path).await?; - let stored = read_session(&path, source).ok().flatten(); match std::fs::remove_file(&path) { - Ok(()) => Ok(stored.map_or(RemovedSession::Unreadable, RemovedSession::Removed)), - Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(RemovedSession::Absent), + Ok(()) => Ok(true), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(false), Err(error) => Err(CliError::Io(error)), } } diff --git a/crates/cli/tests/coverage/shared/oidc_consent_tests.rs b/crates/cli/tests/coverage/shared/oidc_consent_tests.rs deleted file mode 100644 index 9240ba25d..000000000 --- a/crates/cli/tests/coverage/shared/oidc_consent_tests.rs +++ /dev/null @@ -1,135 +0,0 @@ -// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. -// SPDX-License-Identifier: Apache-2.0 - -use base64::Engine; -use base64::engine::general_purpose::URL_SAFE_NO_PAD; - -use super::*; - -const NONCE: &str = "n0nce_Abc-123xyz"; -const WORKSPACE: &str = "0b6f6c8e-6d59-4d5b-9a4a-2f1d8f1c0a11"; - -fn source() -> OidcSource { - OidcSource::new("https://clerk.openrouter.ai", "client", None) -} - -fn parse(toml_text: &str) -> OidcSource { - toml::from_str(&format!( - "issuer = \"https://clerk.openrouter.ai\"\nclient_id = \"client\"\n{toml_text}" - )) - .unwrap() -} - -fn payload(state: &str) -> String { - let (head, segment) = state.split_once('.').expect("a restriction segment"); - assert_eq!(head, NONCE); - assert!( - !segment.contains('.') && !segment.contains('='), - "{segment}" - ); - String::from_utf8(URL_SAFE_NO_PAD.decode(segment).unwrap()).unwrap() -} - -#[test] -fn an_unrestricted_section_sends_the_bare_random_state() { - assert_eq!(source().authorization_state(NONCE), NONCE); -} - -#[test] -fn organization_and_workspace_encode_as_ori_does() { - let state = parse(&format!( - "organization_id = \"org_required\"\nworkspace_id = \"{WORKSPACE}\"" - )) - .authorization_state(NONCE); - - assert_eq!( - payload(&state), - format!(r#"{{"organizationId":"org_required","workspaceId":"{WORKSPACE}"}}"#) - ); -} - -#[test] -fn an_unconstrained_dimension_is_omitted() { - let state = parse("organization_id = \"org_required\"").authorization_state(NONCE); - assert_eq!(payload(&state), r#"{"organizationId":"org_required"}"#); - - let state = parse(&format!("workspace_id = \"{WORKSPACE}\"")).authorization_state(NONCE); - assert_eq!( - payload(&state), - format!(r#"{{"workspaceId":"{WORKSPACE}"}}"#) - ); -} - -#[test] -fn hidden_fields_follow_the_organization_and_workspace() { - let state = parse( - "organization_id = \"org_required\"\nhidden_fields = [\"keyLabel\", \"existingKey\"]", - ) - .authorization_state(NONCE); - - assert_eq!( - payload(&state), - r#"{"organizationId":"org_required","hiddenFields":["keyLabel","existingKey"]}"# - ); -} - -#[test] -fn a_credit_limit_encodes_as_javascript_writes_the_number() { - for (written, encoded) in [("25", "25"), ("25.0", "25"), ("0", "0"), ("12.5", "12.5")] { - let state = parse(&format!("credit_limit = {written}")).authorization_state(NONCE); - assert_eq!(payload(&state), format!(r#"{{"creditLimit":{encoded}}}"#)); - } -} - -#[test] -fn every_member_is_written_in_the_consent_service_order() { - let state = parse(&format!( - "hidden_fields = [\"creditLimit\"]\ncredit_limit_reset = \"monthly\"\ncredit_limit = 25\nworkspace_id = \"{WORKSPACE}\"\norganization_id = \"org_required\"" - )) - .authorization_state(NONCE); - - assert_eq!( - payload(&state), - format!( - r#"{{"organizationId":"org_required","workspaceId":"{WORKSPACE}","creditLimit":25,"creditLimitReset":"monthly","hiddenFields":["creditLimit"]}}"# - ) - ); -} - -#[test] -fn restrictions_the_consent_service_would_refuse_fail_when_the_configuration_loads() { - let refused = |toml_text: &str| { - parse(toml_text) - .validate_consent_restrictions("upstream.openai_oidc") - .unwrap_err() - .to_string() - }; - - assert!(refused("credit_limit_reset = \"monthly\"").contains("needs credit_limit")); - assert!(refused("credit_limit = -1").contains("zero or more")); - assert!(refused("organization_id = \"\"").contains("must not be empty")); - assert!(refused("hidden_fields = [\"keyLabel\"]").contains("needs organization_id")); - assert!( - refused("credit_limit = 5\nhidden_fields = [\"keyLabel\", \"keyLabel\"]").contains("twice") - ); - let long = "o".repeat(800); - assert!(refused(&format!("organization_id = \"{long}\"")).contains("1024")); - assert!( - toml::from_str::( - "issuer = \"https://i.example\"\nclient_id = \"c\"\ncredit_limit_reset = \"yearly\"" - ) - .is_err() - ); - - let accepted = parse("credit_limit = 25\ncredit_limit_reset = \"daily\""); - assert!( - accepted - .validate_consent_restrictions("upstream.openai_oidc") - .is_ok() - ); - assert!( - source() - .validate_consent_restrictions("upstream.openai_oidc") - .is_ok() - ); -} diff --git a/crates/cli/tests/coverage/shared/oidc_tests.rs b/crates/cli/tests/coverage/shared/oidc_tests.rs index 01ae4996b..d2e2414ef 100644 --- a/crates/cli/tests/coverage/shared/oidc_tests.rs +++ b/crates/cli/tests/coverage/shared/oidc_tests.rs @@ -28,6 +28,7 @@ struct Issuer { base: String, advertised_issuer: Option, token_endpoint: Option, + authorization_endpoint: Option, /// Replies to device-code polls, in order; the last one repeats. device_replies: VecDeque<&'static str>, refresh_status: Option, @@ -38,8 +39,6 @@ struct Issuer { grants: Vec>, /// The `code_challenge` of the last authorization address the browser opened. challenge: Option, - revocation_status: Option, - revoked: Vec>, } type Shared = Arc>; @@ -64,21 +63,11 @@ async fn discovery(State(issuer): State) -> Json { Json(json!({ "issuer": state.advertised_issuer.clone().unwrap_or_else(|| base.clone()), "token_endpoint": state.token_endpoint.clone().unwrap_or_else(|| format!("{base}/token")), - "authorization_endpoint": format!("{base}/authorize"), + "authorization_endpoint": state.authorization_endpoint.clone().unwrap_or_else(|| format!("{base}/authorize")), "device_authorization_endpoint": format!("{base}/device"), - "revocation_endpoint": format!("{base}/revoke"), })) } -async fn revoke(State(issuer): State, body: String) -> Response { - let mut state = issuer.lock().unwrap(); - state.revoked.push(parse_form(&body)); - state - .revocation_status - .unwrap_or(StatusCode::OK) - .into_response() -} - async fn device(body: String) -> Response { if parse_form(&body)["client_id"] == "no-device-grant" { return ( @@ -172,7 +161,6 @@ async fn start_issuer() -> (OidcSource, Shared) { .route("/.well-known/openid-configuration", get(discovery)) .route("/device", post(device)) .route("/token", post(token)) - .route("/revoke", post(revoke)) .with_state(shared.clone()); tokio::spawn(async move { axum::serve(listener, app).await.unwrap() }); (OidcSource::new(&base, CLIENT_ID, None), shared) @@ -350,17 +338,11 @@ async fn logout_applies_to_the_next_gateway_request() { "access-1" ); - assert_eq!( - logout(dir.path(), &source).await.unwrap(), - Logout::Removed(Revocation::Confirmed) - ); + assert!(logout(dir.path(), &source).await.unwrap()); let error = error_text(upstream_token_in(dir.path(), &source, &|| 1_000).await); assert!(error.contains("run `nemo-relay auth login`"), "{error}"); - assert_eq!( - logout(dir.path(), &source).await.unwrap(), - Logout::NotSignedIn - ); + assert!(!logout(dir.path(), &source).await.unwrap()); } #[tokio::test] @@ -490,6 +472,18 @@ async fn discovery_rejects_a_token_endpoint_on_another_origin() { assert!(error.contains("another origin"), "{error}"); } +#[tokio::test] +async fn discovery_accepts_an_authorization_endpoint_on_another_https_origin() { + let (source, issuer) = start_issuer().await; + issuer.lock().unwrap().authorization_endpoint = + Some("https://login.elsewhere.example/authorize".into()); + assert!(OidcClient::discover(&source).await.is_ok()); + issuer.lock().unwrap().authorization_endpoint = + Some("http://login.elsewhere.example/authorize".into()); + let error = error_text(OidcClient::discover(&source).await); + assert!(error.contains("must use https"), "{error}"); +} + #[test] fn issuer_urls_must_use_https_unless_they_are_loopback() { assert!(secure_origin("https://clerk.openrouter.ai").is_ok()); @@ -828,93 +822,3 @@ async fn an_unanswered_browser_sign_in_times_out() { assert!(error.contains("timed out"), "{error}"); } - -#[tokio::test] -async fn the_browser_sign_in_carries_the_configured_consent_restriction_in_its_state() { - let (mut source, issuer) = start_issuer().await; - source.organization_id = Some("org_required".into()); - source.credit_limit = Some(serde_json::Number::from(25)); - let dir = tempfile::tempdir().unwrap(); - let opened = Arc::default(); - let browser = approving_browser(issuer.clone(), Arc::clone(&opened), vec![None]); - - login( - dir.path(), - &source, - LoginMethod::Browser(approval(&browser, no_paste())), - |_| {}, - ) - .await - .unwrap(); - - let state = query(&opened.lock().unwrap()[0])["state"].clone(); - let (random, segment) = state.split_once('.').unwrap(); - assert_eq!(random.len(), 43); - assert_eq!(Some(segment.to_owned()), source.restriction_segment()); -} - -#[tokio::test] -async fn the_device_sign_in_refuses_consent_restrictions_it_cannot_carry() { - let (mut source, issuer) = start_issuer().await; - source.workspace_id = Some("workspace".into()); - let dir = tempfile::tempdir().unwrap(); - - let error = error_text(login(dir.path(), &source, LoginMethod::Device, |_| {}).await); - - assert!(error.contains("without --device"), "{error}"); - assert!(issuer.lock().unwrap().grants.is_empty()); -} - -#[tokio::test] -async fn logout_revokes_the_refresh_token_at_the_issuer() { - let (source, issuer) = start_issuer().await; - let dir = tempfile::tempdir().unwrap(); - store(dir.path(), &source, Some("refresh-1"), 10_000); - - let outcome = logout(dir.path(), &source).await.unwrap(); - - assert_eq!(outcome, Logout::Removed(Revocation::Confirmed)); - let revoked = issuer.lock().unwrap().revoked.clone(); - assert_eq!(revoked.len(), 1); - assert_eq!(revoked[0]["token"], "refresh-1"); - assert_eq!(revoked[0]["token_type_hint"], "refresh_token"); - assert_eq!(revoked[0]["client_id"], CLIENT_ID); -} - -#[tokio::test] -async fn logout_signs_out_locally_when_the_issuer_refuses_the_revocation() { - let (source, issuer) = start_issuer().await; - issuer.lock().unwrap().revocation_status = Some(StatusCode::SERVICE_UNAVAILABLE); - let dir = tempfile::tempdir().unwrap(); - store(dir.path(), &source, Some("refresh-1"), 10_000); - - let outcome = logout(dir.path(), &source).await.unwrap(); - - assert!( - matches!(outcome, Logout::Removed(Revocation::Failed(_))), - "{outcome:?}" - ); - let error = error_text(upstream_token_in(dir.path(), &source, &|| 1_000).await); - assert!(error.contains("nemo-relay auth login"), "{error}"); -} - -#[tokio::test] -async fn a_new_sign_in_revokes_the_one_it_replaces() { - let (source, issuer) = start_issuer().await; - let dir = tempfile::tempdir().unwrap(); - store(dir.path(), &source, Some("refresh-old"), 10_000); - let browser = approving_browser(issuer.clone(), Arc::default(), vec![None]); - - login( - dir.path(), - &source, - LoginMethod::Browser(approval(&browser, no_paste())), - |_| {}, - ) - .await - .unwrap(); - - let revoked = issuer.lock().unwrap().revoked.clone(); - assert_eq!(revoked.len(), 1); - assert_eq!(revoked[0]["token"], "refresh-old"); -} diff --git a/docs/nemo-relay-cli/basic-usage.mdx b/docs/nemo-relay-cli/basic-usage.mdx index 20acbc04e..712143420 100644 --- a/docs/nemo-relay-cli/basic-usage.mdx +++ b/docs/nemo-relay-cli/basic-usage.mdx @@ -841,39 +841,8 @@ address without opening a browser, and `--callback-port ` to listen on a fixed port, for example one you forward over SSH. `--device` uses the OAuth 2.0 device authorization grant instead: the command -prints a URL and a code to enter on the issuer's own page. It needs no redirect, -but it cannot carry the consent restrictions below. The issuer must offer the -device grant to the client. - -#### Consent Restrictions - -OpenRouter's consent page can pin what a sign-in may approve. An administrator -adds the restrictions to the provider's `oidc` section: - -```toml -[upstream.openai_oidc] -issuer = "https://clerk.openrouter.ai" -client_id = "" -scope = "openid offline_access email inference" -organization_id = "" -workspace_id = "" -credit_limit = 25 -credit_limit_reset = "monthly" -hidden_fields = ["keyLabel", "existingKey"] -``` - -- `organization_id`: the consent page offers only this organization. -- `workspace_id`: the consent page offers only this workspace. -- `credit_limit`: the key the sign-in yields carries this credit limit, in USD. -- `credit_limit_reset`: `daily`, `weekly`, or `monthly`. It needs `credit_limit`. -- `hidden_fields`: consent page controls to hide: `keyLabel`, `expiration`, - `creditLimit`, and `existingKey`. It needs one of the three settings above. - -Every setting is optional. Relay sends them in the OAuth `state` as the Ori CLI -does: the random value, a `.`, and the restrictions as unpadded base64url JSON, -at most 1,024 characters. OpenRouter's consent service enforces them. The -configuration fails to load when a restriction is one the consent service would -refuse. +prints a URL and a code to enter on the issuer's own page. It needs no redirect. +The issuer must offer the device grant to the client. Relay stores the sign-in in `/oidc/` with owner-only access, one file for each issuer, client ID, and scope. The gateway reads it for each request. @@ -881,10 +850,7 @@ When the access token expires within five minutes, the gateway refreshes it with the stored refresh token first. Refreshes from several processes run one at a time, so a rotated refresh token is never used twice. Because the gateway reads the stored sign-in for each request, the token's real expiry applies, and -`nemo-relay auth logout` takes effect on the next request. Logout also revokes -the stored refresh token at the issuer's revocation endpoint, and a new sign-in -revokes the one it replaces. When the issuer does not confirm the revocation, -logout still removes the local sign-in and exits with an error that says so. +`nemo-relay auth logout` takes effect on the next request. Relay adds the token only to requests it authenticates: agents that Relay launches, and daemon routes bound by the managed integration. A client that only @@ -900,8 +866,8 @@ a new sign-in. `nemo-relay auth token` prints the current access token for scripts. The issuer must use `https`, or `http` on a loopback address. Its -authorization, token, device, and revocation endpoints must have the same origin -as the issuer. Relay never writes +authorization, token, and device endpoints must have the same origin as the +issuer. Relay never writes tokens to logs or error messages. ### Operational Logging From 108b1f31465815d67db9f0f3fdba37ccc75dcfa0 Mon Sep 17 00:00:00 2001 From: flazouh Date: Mon, 9 Nov 2026 17:42:23 +0000 Subject: [PATCH 4/7] test(cli): cover the issuer, missing endpoint and refused code paths A redirect whose iss names another issuer is refused before any code exchange. Discovery without an authorization_endpoint points to --device. A refused code exchange reports that the issuer issued no token. Refs #1697 Co-Authored-By: Claude Opus 5.5 Signed-off-by: flazouh --- .../cli/tests/coverage/shared/oidc_tests.rs | 66 ++++++++++++++++++- 1 file changed, 64 insertions(+), 2 deletions(-) diff --git a/crates/cli/tests/coverage/shared/oidc_tests.rs b/crates/cli/tests/coverage/shared/oidc_tests.rs index d2e2414ef..e25c9ddbc 100644 --- a/crates/cli/tests/coverage/shared/oidc_tests.rs +++ b/crates/cli/tests/coverage/shared/oidc_tests.rs @@ -29,6 +29,7 @@ struct Issuer { advertised_issuer: Option, token_endpoint: Option, authorization_endpoint: Option, + omit_authorization_endpoint: bool, /// Replies to device-code polls, in order; the last one repeats. device_replies: VecDeque<&'static str>, refresh_status: Option, @@ -60,12 +61,19 @@ fn parse_form(body: &str) -> HashMap { async fn discovery(State(issuer): State) -> Json { let state = issuer.lock().unwrap(); let base = state.base.clone(); - Json(json!({ + let mut document = json!({ "issuer": state.advertised_issuer.clone().unwrap_or_else(|| base.clone()), "token_endpoint": state.token_endpoint.clone().unwrap_or_else(|| format!("{base}/token")), "authorization_endpoint": state.authorization_endpoint.clone().unwrap_or_else(|| format!("{base}/authorize")), "device_authorization_endpoint": format!("{base}/device"), - })) + }); + if state.omit_authorization_endpoint { + document + .as_object_mut() + .unwrap() + .remove("authorization_endpoint"); + } + Json(document) } async fn device(body: String) -> Response { @@ -822,3 +830,57 @@ async fn an_unanswered_browser_sign_in_times_out() { assert!(error.contains("timed out"), "{error}"); } +async fn pasted_sign_in_error(client: &OidcClient, redirect: &str) -> String { + let printed = |_: &str| false; + let (paste, pasted) = tokio::sync::mpsc::channel(1); + paste.send(redirect.to_owned()).await.unwrap(); + error_text( + super::browser::sign_in( + client, + "openid", + "attempt-state", + approval(&printed, pasted), + |_| {}, + ) + .await, + ) +} +#[tokio::test] +async fn a_redirect_that_names_another_issuer_is_refused() { + let (source, issuer) = start_issuer().await; + let client = OidcClient::discover(&source).await.unwrap(); + let error = pasted_sign_in_error( + &client, + "http://127.0.0.1:1/callback?code=code-1&state=attempt-state&iss=https%3A%2F%2Fother.example", + ) + .await; + assert!(error.contains("names another issuer"), "{error}"); + assert!(issuer.lock().unwrap().grants.is_empty()); +} +#[tokio::test] +async fn browser_sign_in_without_an_authorization_endpoint_points_to_device() { + let (source, issuer) = start_issuer().await; + issuer.lock().unwrap().omit_authorization_endpoint = true; + let client = OidcClient::discover(&source).await.unwrap(); + let error = pasted_sign_in_error( + &client, + "http://127.0.0.1:1/callback?code=code-1&state=attempt-state", + ) + .await; + assert!(error.contains("does not offer browser sign-in"), "{error}"); + assert!(error.contains("--device"), "{error}"); + assert!(issuer.lock().unwrap().grants.is_empty()); +} +#[tokio::test] +async fn a_refused_code_exchange_reports_that_no_token_was_issued() { + let (source, issuer) = start_issuer().await; + let client = OidcClient::discover(&source).await.unwrap(); + let error = pasted_sign_in_error( + &client, + "http://127.0.0.1:1/callback?code=unknown-code&state=attempt-state", + ) + .await; + assert!(error.contains("did not issue a token"), "{error}"); + assert!(error.contains("invalid_grant"), "{error}"); + assert_eq!(issuer.lock().unwrap().grants.len(), 1); +} From a17234c80d10fc81ca6834fb320892c755076df5 Mon Sep 17 00:00:00 2001 From: flazouh Date: Mon, 9 Nov 2026 17:56:44 +0000 Subject: [PATCH 5/7] fix(cli): match iss exactly and stop the browser page claiming success RFC 9207 compares the iss of a redirect with the discovered metadata issuer as an exact string. The check trimmed a trailing slash, so it accepted an iss that differed only by that slash. The client now keeps the metadata issuer as published and compares with it. The loopback page said "Signed in" before the code exchange, which can still fail. It now says the response was received and the terminal shows the result. Refs #1697 Co-Authored-By: Claude Opus 5.5 Signed-off-by: flazouh --- crates/cli/src/oidc/browser.rs | 12 +++++------- crates/cli/src/oidc/client.rs | 7 +++++-- crates/cli/tests/coverage/shared/oidc_tests.rs | 17 +++++++++++++++++ 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/crates/cli/src/oidc/browser.rs b/crates/cli/src/oidc/browser.rs index f1d3e9b5d..e211f98d7 100644 --- a/crates/cli/src/oidc/browser.rs +++ b/crates/cli/src/oidc/browser.rs @@ -28,7 +28,7 @@ const RANDOM_BYTES: usize = 32; const CALLBACK_HOST: &str = "127.0.0.1"; const CALLBACK_PATH: &str = "/callback"; -const PAGE_SIGNED_IN: &str = "NeMo Relay

Signed in. You can close this tab and return to the terminal.

"; +const PAGE_RECEIVED: &str = "NeMo Relay

The sign-in response was received. Return to the terminal to see the result.

"; const PAGE_NOT_COMPLETED: &str = "NeMo Relay

The sign-in was not completed. Return to the terminal.

"; const PAGE_FOREIGN_ATTEMPT: &str = "NeMo Relay

This page belongs to another sign-in attempt. Use the link the terminal printed.

"; @@ -81,7 +81,7 @@ pub(crate) async fn sign_in( let redirect = callback .wait(approval.pasted, &mut show, approval.timeout) .await?; - let code = redirect.into_code(client.issuer())?; + let code = redirect.into_code(client.metadata_issuer())?; client.exchange_code(&code, &redirect_uri, &verifier).await } @@ -122,11 +122,9 @@ pub(crate) struct Redirect { impl Redirect { /// The authorization code, once the redirect is known to come from `issuer` and approve. + /// RFC 9207 compares `iss` with the metadata issuer as an exact string. fn into_code(self, issuer: &str) -> Result { - if self - .iss - .is_some_and(|iss| iss.trim_end_matches('/') != issuer) - { + if self.iss.is_some_and(|iss| iss != issuer) { return Err(SignInError::ForeignIssuer); } if let Some(error) = &self.error { @@ -268,7 +266,7 @@ async fn receive( return Html(PAGE_FOREIGN_ATTEMPT); } let page = if redirect.code.is_some() { - PAGE_SIGNED_IN + PAGE_RECEIVED } else { PAGE_NOT_COMPLETED }; diff --git a/crates/cli/src/oidc/client.rs b/crates/cli/src/oidc/client.rs index 7414e468d..eecffa354 100644 --- a/crates/cli/src/oidc/client.rs +++ b/crates/cli/src/oidc/client.rs @@ -71,6 +71,7 @@ enum Grant { pub(crate) struct OidcClient { http: reqwest::Client, issuer: String, + metadata_issuer: String, client_id: String, pub(super) token_endpoint: String, authorization_endpoint: Option, @@ -115,6 +116,7 @@ impl OidcClient { Ok(Self { http, issuer: issuer.to_owned(), + metadata_issuer: discovery.issuer, client_id: source.client_id.clone(), token_endpoint: discovery.token_endpoint, authorization_endpoint: discovery.authorization_endpoint, @@ -122,8 +124,9 @@ impl OidcClient { }) } - pub(crate) fn issuer(&self) -> &str { - &self.issuer + /// The issuer exactly as discovery published it, which RFC 9207 compares `iss` with. + pub(crate) fn metadata_issuer(&self) -> &str { + &self.metadata_issuer } /// The address the user opens to approve the sign-in. diff --git a/crates/cli/tests/coverage/shared/oidc_tests.rs b/crates/cli/tests/coverage/shared/oidc_tests.rs index e25c9ddbc..a0b6adf29 100644 --- a/crates/cli/tests/coverage/shared/oidc_tests.rs +++ b/crates/cli/tests/coverage/shared/oidc_tests.rs @@ -884,3 +884,20 @@ async fn a_refused_code_exchange_reports_that_no_token_was_issued() { assert!(error.contains("invalid_grant"), "{error}"); assert_eq!(issuer.lock().unwrap().grants.len(), 1); } +#[tokio::test] +async fn a_redirect_iss_must_match_the_metadata_issuer_exactly() { + let (source, issuer) = start_issuer().await; + let client = OidcClient::discover(&source).await.unwrap(); + let slashed = reqwest::Url::parse_with_params( + "http://127.0.0.1:1/callback", + &[ + ("code", "code-1"), + ("state", "attempt-state"), + ("iss", &format!("{}/", source.issuer)), + ], + ) + .unwrap(); + let error = pasted_sign_in_error(&client, slashed.as_str()).await; + assert!(error.contains("names another issuer"), "{error}"); + assert!(issuer.lock().unwrap().grants.is_empty()); +} From db5aca7eb0caa8e13ec60fd2121f53ec1c2b3f4f Mon Sep 17 00:00:00 2001 From: flazouh Date: Mon, 9 Nov 2026 18:06:56 +0000 Subject: [PATCH 6/7] fix(cli): hold the authorization endpoint to the issuer origin Issue #1697 asks discovery to check authorization_endpoint with the same origin rule as the token endpoint. Discovery now refuses an authorization endpoint on another origin, and the doc says so. Co-Authored-By: Claude Opus 5.5 Signed-off-by: flazouh --- crates/cli/src/oidc/client.rs | 7 ++----- crates/cli/tests/coverage/shared/oidc_tests.rs | 7 ++----- docs/nemo-relay-cli/authentication.mdx | 8 +++----- 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/crates/cli/src/oidc/client.rs b/crates/cli/src/oidc/client.rs index eecffa354..7251f593a 100644 --- a/crates/cli/src/oidc/client.rs +++ b/crates/cli/src/oidc/client.rs @@ -101,7 +101,8 @@ impl OidcClient { } // Endpoints on another origin could send the refresh token to an unintended host. let endpoints = std::iter::once(&discovery.token_endpoint) - .chain(discovery.device_authorization_endpoint.as_ref()); + .chain(discovery.device_authorization_endpoint.as_ref()) + .chain(discovery.authorization_endpoint.as_ref()); for endpoint in endpoints { if secure_origin(endpoint)? != origin { return Err(auth_error(format!( @@ -109,10 +110,6 @@ impl OidcClient { ))); } } - // The browser alone opens the authorization endpoint, so it may sit on another origin. - if let Some(endpoint) = &discovery.authorization_endpoint { - secure_origin(endpoint)?; - } Ok(Self { http, issuer: issuer.to_owned(), diff --git a/crates/cli/tests/coverage/shared/oidc_tests.rs b/crates/cli/tests/coverage/shared/oidc_tests.rs index a0b6adf29..a97f3f9d8 100644 --- a/crates/cli/tests/coverage/shared/oidc_tests.rs +++ b/crates/cli/tests/coverage/shared/oidc_tests.rs @@ -481,15 +481,12 @@ async fn discovery_rejects_a_token_endpoint_on_another_origin() { } #[tokio::test] -async fn discovery_accepts_an_authorization_endpoint_on_another_https_origin() { +async fn discovery_rejects_an_authorization_endpoint_on_another_origin() { let (source, issuer) = start_issuer().await; issuer.lock().unwrap().authorization_endpoint = Some("https://login.elsewhere.example/authorize".into()); - assert!(OidcClient::discover(&source).await.is_ok()); - issuer.lock().unwrap().authorization_endpoint = - Some("http://login.elsewhere.example/authorize".into()); let error = error_text(OidcClient::discover(&source).await); - assert!(error.contains("must use https"), "{error}"); + assert!(error.contains("another origin"), "{error}"); } #[test] diff --git a/docs/nemo-relay-cli/authentication.mdx b/docs/nemo-relay-cli/authentication.mdx index 04bde2682..ccd32a29e 100644 --- a/docs/nemo-relay-cli/authentication.mdx +++ b/docs/nemo-relay-cli/authentication.mdx @@ -281,11 +281,9 @@ each configured sign-in exists, is valid, has expired but can refresh, or needs a new sign-in. `nemo-relay auth token` prints the current access token for scripts. -The issuer must use `https`, or `http` on a loopback address. Its token and -device endpoints must have the same origin as the issuer. Only the browser opens -the authorization endpoint, so it may use another origin, but it must also use -`https`, or `http` on a loopback address. Relay never writes tokens to logs or -error messages. +The issuer must use `https`, or `http` on a loopback address. Its +authorization, token and device endpoints must have the same origin as the +issuer. Relay never writes tokens to logs or error messages. `nemo-relay auth token` prints an access token. Treat its output as a credential From 1629ffbbcc52207986224d5129b189cc7cb50674 Mon Sep 17 00:00:00 2001 From: flazouh Date: Mon, 9 Nov 2026 18:17:16 +0000 Subject: [PATCH 7/7] test(cli): check a redirect iss against the published issuer Discovery may publish its issuer with a trailing slash. A redirect that names that exact value reaches the token grant, and the unslashed value is refused. Co-Authored-By: Claude Opus 5.5 Signed-off-by: flazouh --- .../cli/tests/coverage/shared/oidc_tests.rs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/crates/cli/tests/coverage/shared/oidc_tests.rs b/crates/cli/tests/coverage/shared/oidc_tests.rs index a97f3f9d8..528c80f7b 100644 --- a/crates/cli/tests/coverage/shared/oidc_tests.rs +++ b/crates/cli/tests/coverage/shared/oidc_tests.rs @@ -898,3 +898,27 @@ async fn a_redirect_iss_must_match_the_metadata_issuer_exactly() { assert!(error.contains("names another issuer"), "{error}"); assert!(issuer.lock().unwrap().grants.is_empty()); } +#[tokio::test] +async fn a_redirect_iss_is_compared_with_the_published_issuer() { + let (source, issuer) = start_issuer().await; + let published = format!("{}/", source.issuer); + issuer.lock().unwrap().advertised_issuer = Some(published.clone()); + let client = OidcClient::discover(&source).await.unwrap(); + let redirect_naming = |iss: &str| { + reqwest::Url::parse_with_params( + "http://127.0.0.1:1/callback", + &[ + ("code", "unknown-code"), + ("state", "attempt-state"), + ("iss", iss), + ], + ) + .unwrap() + }; + let error = pasted_sign_in_error(&client, redirect_naming(&published).as_str()).await; + assert!(error.contains("did not issue a token"), "{error}"); + assert_eq!(issuer.lock().unwrap().grants.len(), 1); + let error = pasted_sign_in_error(&client, redirect_naming(&source.issuer).as_str()).await; + assert!(error.contains("names another issuer"), "{error}"); + assert_eq!(issuer.lock().unwrap().grants.len(), 1); +}