From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 27E771FF170 for ; Thu, 7 Aug 2025 13:45:14 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 485881CC47; Thu, 7 Aug 2025 13:46:47 +0200 (CEST) Message-ID: <9127e7be-360f-4df8-844d-3c27b0873c91@proxmox.com> Date: Thu, 7 Aug 2025 13:46:42 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: pbs-devel@lists.proxmox.com References: <20250807104909.198914-2-s.sterz@proxmox.com> <20250807104909.198914-4-s.sterz@proxmox.com> Content-Language: en-US From: Mira Limbeck In-Reply-To: <20250807104909.198914-4-s.sterz@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1754567179632 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.285 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup 1/2] api: openid: allow users of openid to opt into the HttpOnly cookies X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" On 8/7/25 12:49, Shannon Sterz wrote: > add a `http-only` api parameter that allows users to opt into the the > HttpOnly cookie based authentication flow. here the server will set a > cookie via a `Set-Cookie` header instead of providing it in the > response's body. this protects users better against cookie stealing > attacks and other similar attacks. > > note that this has the side effect of always returning extjs-like > responses here. > > Analyzed-by: Mira Limbeck > Signed-off-by: Shannon Sterz > --- > src/api2/access/openid.rs | 346 ++++++++++++++++++++++---------------- > 1 file changed, 205 insertions(+), 141 deletions(-) > > diff --git a/src/api2/access/openid.rs b/src/api2/access/openid.rs > index 8e39cbc9..1beb77bc 100644 > --- a/src/api2/access/openid.rs > +++ b/src/api2/access/openid.rs > @@ -1,13 +1,16 @@ > //! OpenID redirect/login API > use anyhow::{bail, format_err, Error}; > +use hyper::http::request::Parts; > +use hyper::Response; > use serde_json::{json, Value}; > > use proxmox_auth_api::api::ApiTicket; > use proxmox_auth_api::ticket::Ticket; > use proxmox_router::{ > - http_err, list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap, > + http_err, list_subdirs_api_method, ApiHandler, ApiMethod, ApiResponseFuture, Permission, > + Router, RpcEnvironment, SubdirMap, > }; > -use proxmox_schema::api; > +use proxmox_schema::{api, BooleanSchema, ObjectSchema, ParameterSchema, StringSchema}; > use proxmox_sortable_macro::sortable; > > use proxmox_openid::{OpenIdAuthenticator, OpenIdConfig}; > @@ -58,167 +61,228 @@ fn openid_authenticator( > OpenIdAuthenticator::discover(&config, redirect_url) > } > > -#[api( > - input: { > - properties: { > - state: { > - description: "OpenId state.", > - type: String, > - }, > - code: { > - description: "OpenId authorization code.", > - type: String, > - }, > - "redirect-url": { > - description: "Redirection Url. The client should set this to used server url.", > - type: String, > - }, > - }, > - }, > - returns: { > - properties: { > - username: { > - type: String, > - description: "User name.", > - }, > - ticket: { > - type: String, > - description: "Auth ticket.", > - }, > - CSRFPreventionToken: { > - type: String, > - description: "Cross Site Request Forgery Prevention Token.", > - }, > - }, > - }, > - protected: true, > - access: { > - permission: &Permission::World, > - }, > -)] > -/// Verify OpenID authorization code and create a ticket > -/// > -/// Returns: An authentication ticket with additional infos. > -pub fn openid_login( > - state: String, > - code: String, > - redirect_url: String, > - rpcenv: &mut dyn RpcEnvironment, > -) -> Result { > - use proxmox_rest_server::RestEnvironment; > +#[sortable] > +pub const API_METHOD_OPENID_LOGIN: ApiMethod = ApiMethod::new_full( > + &ApiHandler::AsyncHttpBodyParameters(&create_ticket_http_only), > + ParameterSchema::Object(&ObjectSchema::new( > + "Get a new ticket as an HttpOnly cookie. Supports tickets via cookies.", > + &sorted!([ > + ("state", false, &StringSchema::new("OpenId state.").schema()), > + ( > + "code", > + false, > + &StringSchema::new("OpenId authorization code.").schema(), > + ), > + ( > + "redirect-url", > + false, > + &StringSchema::new( > + "Redirection Url. The client should set this to used server url.", > + ) > + .schema(), > + ), > + ( > + "http-only", > + true, > + &BooleanSchema::new("Whether the HttpOnly authentication flow should be used.") > + .default(false) > + .schema(), > + ), > + ]), > + )), > +) > +.returns(::proxmox_schema::ReturnType::new( > + false, > + &ObjectSchema::new( > + "An authentication ticket with additional infos.", > + &sorted!([ > + ("username", false, &StringSchema::new("User name.").schema()), > + ( > + "ticket", > + true, > + &StringSchema::new( > + "Auth ticket, present if http-only was not provided or is false." > + ) > + .schema() > + ), > + ("ticket-info", > + true, > + &StringSchema::new( > + "Informational ticket, can only be used to check if the ticket is expired. Present if http-only was true." > + ).schema()), > + ( > + "CSRFPreventionToken", > + false, > + &StringSchema::new("Cross Site Request Forgery Prevention Token.").schema(), > + ), > + ]), > + ) > + .schema(), > +)) > +.protected(true) > +.access(None, &Permission::World) > +.reload_timezone(true); > > - let env: &RestEnvironment = rpcenv > - .as_any() > - .downcast_ref::() > - .ok_or_else(|| format_err!("detected wrong RpcEnvironment type"))?; > +fn create_ticket_http_only( > + _parts: Parts, > + param: Value, > + _info: &ApiMethod, > + rpcenv: Box, > +) -> ApiResponseFuture { > + Box::pin(async move { > + use proxmox_rest_server::RestEnvironment; > > - let user_info = CachedUserInfo::new()?; > + let code = param["code"] > + .as_str() > + .ok_or_else(|| format_err!("missing non-optional parameter: code"))? > + .to_owned(); > + let state = param["state"] > + .as_str() > + .ok_or_else(|| format_err!("missing non-optional parameter: state"))? > + .to_owned(); > + let redirect_url = param["redirect-url"] > + .as_str() > + .ok_or_else(|| format_err!("missing non-optional parameter: redirect-url"))? > + .to_owned(); > > - let mut tested_username = None; > + let env: &RestEnvironment = rpcenv > + .as_any() > + .downcast_ref::() > + .ok_or_else(|| format_err!("detected wrong RpcEnvironment type"))?; > > - let result = proxmox_lang::try_block!({ > - let (realm, private_auth_state) = > - OpenIdAuthenticator::verify_public_auth_state(PROXMOX_BACKUP_RUN_DIR_M!(), &state)?; > + let user_info = CachedUserInfo::new()?; > > - let (domains, _digest) = pbs_config::domains::config()?; > - let config: OpenIdRealmConfig = domains.lookup("openid", &realm)?; > + let mut tested_username = None; > > - let open_id = openid_authenticator(&config, &redirect_url)?; > + let result = proxmox_lang::try_block!({ > + let (realm, private_auth_state) = > + OpenIdAuthenticator::verify_public_auth_state(PROXMOX_BACKUP_RUN_DIR_M!(), &state)?; > > - let info = open_id.verify_authorization_code_simple(&code, &private_auth_state)?; > + let (domains, _digest) = pbs_config::domains::config()?; > + let config: OpenIdRealmConfig = domains.lookup("openid", &realm)?; > > - // eprintln!("VERIFIED {:?}", info); > + let open_id = openid_authenticator(&config, &redirect_url)?; > > - let name_attr = config.username_claim.as_deref().unwrap_or("sub"); > + let info = open_id.verify_authorization_code_simple(&code, &private_auth_state)?; > > - // Try to be compatible with previous versions > - let try_attr = match name_attr { > - "subject" => Some("sub"), > - "username" => Some("preferred_username"), > - _ => None, > - }; > + // eprintln!("VERIFIED {:?}", info); > > - let unique_name = match info[name_attr].as_str() { > - Some(name) => name.to_owned(), > - None => { > - if let Some(try_attr) = try_attr { > - match info[try_attr].as_str() { > - Some(name) => name.to_owned(), > - None => bail!("missing claim '{}'", name_attr), > - } > - } else { > - bail!("missing claim '{}'", name_attr); > - } > - } > - }; > + let name_attr = config.username_claim.as_deref().unwrap_or("sub"); > > - let user_id = Userid::try_from(format!("{}@{}", unique_name, realm))?; > - tested_username = Some(unique_name); > + // Try to be compatible with previous versions > + let try_attr = match name_attr { > + "subject" => Some("sub"), > + "username" => Some("preferred_username"), > + _ => None, > + }; > > - if !user_info.is_active_user_id(&user_id) { > - if config.autocreate.unwrap_or(false) { > - use pbs_config::user; > - let _lock = open_backup_lockfile(user::USER_CFG_LOCKFILE, None, true)?; > - > - let firstname = info["given_name"] > - .as_str() > - .map(|n| n.to_string()) > - .filter(|n| FIRST_NAME_SCHEMA.parse_simple_value(n).is_ok()); > - > - let lastname = info["family_name"] > - .as_str() > - .map(|n| n.to_string()) > - .filter(|n| LAST_NAME_SCHEMA.parse_simple_value(n).is_ok()); > - > - let email = info["email"] > - .as_str() > - .map(|n| n.to_string()) > - .filter(|n| EMAIL_SCHEMA.parse_simple_value(n).is_ok()); > - > - let user = User { > - userid: user_id.clone(), > - comment: None, > - enable: None, > - expire: None, > - firstname, > - lastname, > - email, > - }; > - let (mut config, _digest) = user::config()?; > - if let Ok(old_user) = config.lookup::("user", user.userid.as_str()) { > - if let Some(false) = old_user.enable { > - bail!("user '{}' is disabled.", user.userid); > + let unique_name = match info[name_attr].as_str() { > + Some(name) => name.to_owned(), > + None => { > + if let Some(try_attr) = try_attr { > + match info[try_attr].as_str() { > + Some(name) => name.to_owned(), > + None => bail!("missing claim '{}'", name_attr), > + } > } else { > - bail!("autocreate user failed - '{}' already exists.", user.userid); > + bail!("missing claim '{}'", name_attr); > } > } > - config.set_data(user.userid.as_str(), "user", &user)?; > - user::save_config(&config)?; > - } else { > - bail!("user account '{}' missing, disabled or expired.", user_id); > + }; > + > + let user_id = Userid::try_from(format!("{}@{}", unique_name, realm))?; > + tested_username = Some(unique_name); > + > + if !user_info.is_active_user_id(&user_id) { > + if config.autocreate.unwrap_or(false) { > + use pbs_config::user; > + let _lock = open_backup_lockfile(user::USER_CFG_LOCKFILE, None, true)?; > + > + let firstname = info["given_name"] > + .as_str() > + .map(|n| n.to_string()) > + .filter(|n| FIRST_NAME_SCHEMA.parse_simple_value(n).is_ok()); > + > + let lastname = info["family_name"] > + .as_str() > + .map(|n| n.to_string()) > + .filter(|n| LAST_NAME_SCHEMA.parse_simple_value(n).is_ok()); > + > + let email = info["email"] > + .as_str() > + .map(|n| n.to_string()) > + .filter(|n| EMAIL_SCHEMA.parse_simple_value(n).is_ok()); > + > + let user = User { > + userid: user_id.clone(), > + comment: None, > + enable: None, > + expire: None, > + firstname, > + lastname, > + email, > + }; > + let (mut config, _digest) = user::config()?; > + if let Ok(old_user) = config.lookup::("user", user.userid.as_str()) { > + if let Some(false) = old_user.enable { > + bail!("user '{}' is disabled.", user.userid); > + } else { > + bail!("autocreate user failed - '{}' already exists.", user.userid); > + } > + } > + config.set_data(user.userid.as_str(), "user", &user)?; > + user::save_config(&config)?; > + } else { > + bail!("user account '{}' missing, disabled or expired.", user_id); > + } > } > - } > > - let api_ticket = ApiTicket::Full(user_id.clone()); > - let ticket = Ticket::new("PBS", &api_ticket)?.sign(private_auth_keyring(), None)?; > - let token = assemble_csrf_prevention_token(csrf_secret(), &user_id); > + let api_ticket = ApiTicket::Full(user_id.clone()); > + let ticket = Ticket::new("PBS", &api_ticket)?; > + let token = assemble_csrf_prevention_token(csrf_secret(), &user_id); > > - env.log_auth(user_id.as_str()); > + env.log_auth(user_id.as_str()); > > - Ok(json!({ > - "username": user_id, > - "ticket": ticket, > - "CSRFPreventionToken": token, > - })) > - }); > + Ok((user_id, ticket, token)) > + }); > > - if let Err(ref err) = result { > - let msg = err.to_string(); > - env.log_failed_auth(tested_username, &msg); > - return Err(http_err!(UNAUTHORIZED, "{}", msg)); > - } > + let (user_id, mut ticket, token) = result.map_err(|err| { > + let msg = err.to_string(); > + env.log_failed_auth(tested_username, &msg); > + http_err!(UNAUTHORIZED, "{}", msg) > + })?; > > - result > + let mut response = > + Response::builder().header(hyper::http::header::CONTENT_TYPE, "application/json"); > + > + let data = if Value::Bool(true) == param["http-only"] { > + let cookie = format!( > + "{}={}; Secure; SameSite=Lax; HttpOnly; Path=/;", > + get_auth_cookie_name(), > + ticket.sign(private_auth_keyring(), None)?, > + ); > + > + response = response.header(hyper::header::SET_COOKIE, cookie); > + > + json!({ > + "username": user_id, > + "ticket_info": ticket.ticket_info(), > + "CSRFPreventionToken": token > + }) > + } else { > + json!({ > + "username": user_id, > + "ticket": ticket.sign(private_auth_keyring(), None)?, > + "CSRFPreventionToken": token > + }) > + }; > + > + Ok(response.body( > + json!({"data": data, "status": 200, "success": true }) > + .to_string() > + .into(), > + )?) > + }) > } > > #[api( It looks like this only sets 1 cookie (__Host-PBSAuthCookie) rather than both (PBSAuthCookie and __Host-PBSAuthCookie). This results in the login mask showing after refreshing the browser tab. _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel