[pbs-devel] [PATCH proxmox-backup v2 0/2] add support for HttpOnly cookies for OpenID authentication flow

[pbs-devel] [PATCH proxmox-backup v2 0/2] add support for HttpOnly cookies for OpenID authentication flow

[Shannon Sterz]

these two patches do the following:

1. add a `http-only` parameter to the OpenID login endpoint, so clients
   can opt into receive the authentication ticket via a HttpOnly cookie
2. opt the ui dialog into using this new HttpOnly parameter

this should fix a bug where users were instantly logged out again after
a successful OpenID authentication.

changes since v1, thanks @ Mira Limbeck:

- fixed an issue where the open id login endpoint would return a
  `ticket_info` field instead of a `ticket-info` field.

Shannon Sterz (2):
  api: openid: allow users of openid to opt into the HttpOnly cookies
  ui: opt open id authentication flows into the new http only flow

 src/api2/access/openid.rs | 346 ++++++++++++++++++++++----------------
 www/LoginView.js          |   1 +
 2 files changed, 206 insertions(+), 141 deletions(-)

--
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup v2 1/2] api: openid: allow users of openid to opt into the HttpOnly cookies

[Shannon Sterz]

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 <m.limbeck@proxmox.com>
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---

changes since v1, thanks @ Mira Limbeck:

- fixed the login endpoint to return `ticket-info` instead of
  `ticket_info`

 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..383a4e81 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<Value, Error> {
-    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::<RestEnvironment>()
-        .ok_or_else(|| format_err!("detected wrong RpcEnvironment type"))?;
+fn create_ticket_http_only(
+    _parts: Parts,
+    param: Value,
+    _info: &ApiMethod,
+    rpcenv: Box<dyn RpcEnvironment>,
+) -> 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::<RestEnvironment>()
+            .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", 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", 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(
--
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup v2 2/2] ui: opt open id authentication flows into the new http only flow

[Shannon Sterz]

otherwise the cookie would not be properly set, leading to users
potentially getting logged out instantly again.

Analyzed-by: Mira Limbeck <m.limbeck@proxmox.com>
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 www/LoginView.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/www/LoginView.js b/www/LoginView.js
index cf6c2cf7..08786080 100644
--- a/www/LoginView.js
+++ b/www/LoginView.js
@@ -197,6 +197,7 @@ Ext.define('PBS.LoginView', {
                                 state: auth.state,
                                 code: auth.code,
                                 'redirect-url': redirectURL,
+                                'http-only': true,
                             },
                             method: 'POST',
                             failure: function (response) {
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup v2 0/2] add support for HttpOnly cookies for OpenID authentication flow

[Mira Limbeck]

On 8/7/25 14:02, Shannon Sterz wrote:
> these two patches do the following:
> 
> 1. add a `http-only` parameter to the OpenID login endpoint, so clients
>    can opt into receive the authentication ticket via a HttpOnly cookie
> 2. opt the ui dialog into using this new HttpOnly parameter
> 
> this should fix a bug where users were instantly logged out again after
> a successful OpenID authentication.
> 
> changes since v1, thanks @ Mira Limbeck:
> 
> - fixed an issue where the open id login endpoint would return a
>   `ticket_info` field instead of a `ticket-info` field.
> 
> Shannon Sterz (2):
>   api: openid: allow users of openid to opt into the HttpOnly cookies
>   ui: opt open id authentication flows into the new http only flow
> 
>  src/api2/access/openid.rs | 346 ++++++++++++++++++++++----------------
>  www/LoginView.js          |   1 +
>  2 files changed, 206 insertions(+), 141 deletions(-)
> 
> --
> 2.47.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 

Gave this a quick test. Looks good!
Both cookies are now set.

Consider this:
Tested-by: Mira Limbeck <m.limbeck@proxmox.com>


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup v2 0/2] add support for HttpOnly cookies for OpenID authentication flow

[Maximiliano Sandoval]

Shannon Sterz <s.sterz@proxmox.com> writes:

> these two patches do the following:
>
> 1. add a `http-only` parameter to the OpenID login endpoint, so clients
>    can opt into receive the authentication ticket via a HttpOnly cookie
> 2. opt the ui dialog into using this new HttpOnly parameter
>
> this should fix a bug where users were instantly logged out again after
> a successful OpenID authentication.
>
> changes since v1, thanks @ Mira Limbeck:
>
> - fixed an issue where the open id login endpoint would return a
>   `ticket_info` field instead of a `ticket-info` field.
>
> Shannon Sterz (2):
>   api: openid: allow users of openid to opt into the HttpOnly cookies
>   ui: opt open id authentication flows into the new http only flow
>
>  src/api2/access/openid.rs | 346 ++++++++++++++++++++++----------------
>  www/LoginView.js          |   1 +
>  2 files changed, 206 insertions(+), 141 deletions(-)

Tested with a Keycloak instance. It works now and both cookies are set!

Tested-by: Maximiliano Sandoval <m.sandoval@proxmox.com>

-- 
Maximiliano


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] applied: [PATCH proxmox-backup v2 0/2] add support for HttpOnly cookies for OpenID authentication flow

[Thomas Lamprecht]

On Thu, 07 Aug 2025 14:03:07 +0200, Shannon Sterz wrote:
> these two patches do the following:
> 
> 1. add a `http-only` parameter to the OpenID login endpoint, so clients
>    can opt into receive the authentication ticket via a HttpOnly cookie
> 2. opt the ui dialog into using this new HttpOnly parameter
> 
> this should fix a bug where users were instantly logged out again after
> a successful OpenID authentication.
> 
> [...]

Applied, thanks!

[1/2] api: openid: allow users of openid to opt into the HttpOnly cookies
      commit: f341bff9b7098fb1610bad0e1e88bddcaa2b55de
[2/2] ui: opt open id authentication flows into the new http only flow
      commit: cf22c2415edc544b8577cc63cf3a903848e47348


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel