[pbs-devel] [PATCH proxmox{,-backup} 0/4] HttpOnly follow-ups

[pbs-devel] [PATCH proxmox{,-backup} 0/4] HttpOnly follow-ups

[Shannon Sterz]

this small series tries to smooth out the transition to HttpOnly cookies
for our users:

- cookies are now removed by the server if a 401 UNAUTHORIZED error is
  encountered. this matches the behaviour of our previous javascript
  browser-based clients. it is necessary, as they can't remove the
  cookie themselves anymore due to the new security measures.
- log-in is possible again, even if an invalid HttpOnly cookie is
  provided.
- `Expire` is removed from the cookies to make them "session cookies"
  again. this should restore security assumptions some users may have
  made about closing their browser and being logged out. note that
  session cookies may still be restored after a browser was closed, if
  the browser uses session restoration.

proxmox:

Shannon Sterz (3):
  rest-server: remove auth cookies via http header on unauthorized
    request
  auth-api: don't set `Expire` for HttpOnly cookies anymore
  auth-api: allow log-in via parameters even if HttpOnly cookie is
    invalid

 proxmox-auth-api/src/api/access.rs    | 67 +++++++++++++++------------
 proxmox-auth-api/src/types.rs         |  2 +-
 proxmox-rest-server/src/api_config.rs |  9 ++++
 proxmox-rest-server/src/rest.rs       | 25 +++++++++-
 4 files changed, 71 insertions(+), 32 deletions(-)


proxmox-backup:

Shannon Sterz (1):
  api/proxy: set auth cookie name in rest server api config

 src/auth.rs                     | 10 +++++++++-
 src/auth_helpers.rs             |  1 +
 src/bin/proxmox-backup-api.rs   |  1 +
 src/bin/proxmox-backup-proxy.rs |  1 +
 4 files changed, 12 insertions(+), 1 deletion(-)


Summary over all repositories:
  8 files changed, 83 insertions(+), 33 deletions(-)

--
Generated by git-murpp 0.8.1


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

[pbs-devel] [PATCH proxmox 1/3] rest-server: remove auth cookies via http header on unauthorized request

[Shannon Sterz]

previously the behaviour of our javascript clients was to remove
authentication cookies if the api returned a 401 UNAUTHORIZED
response. with the switch to httponly cookies, this is no longer
possible. add an option to the ApiConfig to allow the rest-server to
remove such cookies

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 proxmox-rest-server/src/api_config.rs |  9 +++++++++
 proxmox-rest-server/src/rest.rs       | 25 ++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/proxmox-rest-server/src/api_config.rs b/proxmox-rest-server/src/api_config.rs
index 0b847a0c..0a67231e 100644
--- a/proxmox-rest-server/src/api_config.rs
+++ b/proxmox-rest-server/src/api_config.rs
@@ -33,6 +33,9 @@ pub struct ApiConfig {
     auth_handler: Option<AuthHandler>,
     index_handler: Option<IndexHandler>,
     pub(crate) privileged_addr: Option<PrivilegedAddr>,
+    // Name of the auth cookie that should be unset on 401 request. If `None` no cookie will be
+    // removed.
+    pub(crate) auth_cookie_name: Option<String>,
 
     #[cfg(feature = "templates")]
     templates: templates::Templates,
@@ -62,6 +65,7 @@ impl ApiConfig {
             auth_handler: None,
             index_handler: None,
             privileged_addr: None,
+            auth_cookie_name: None,
 
             #[cfg(feature = "templates")]
             templates: templates::Templates::with_escape_fn(),
@@ -82,6 +86,11 @@ impl ApiConfig {
         self.auth_handler(AuthHandler::from_fn(func))
     }
 
+    pub fn auth_cookie_name(mut self, auth_cookie_name: String) -> Self {
+        self.auth_cookie_name = Some(auth_cookie_name);
+        self
+    }
+
     /// This is used for `protected` API calls to proxy to a more privileged service.
     pub fn privileged_addr(mut self, addr: impl Into<PrivilegedAddr>) -> Self {
         self.privileged_addr = Some(addr.into());
diff --git a/proxmox-rest-server/src/rest.rs b/proxmox-rest-server/src/rest.rs
index bff90882..035a9537 100644
--- a/proxmox-rest-server/src/rest.rs
+++ b/proxmox-rest-server/src/rest.rs
@@ -357,8 +357,21 @@ impl Service<Request<Incoming>> for ApiService {
             Some(proxied_peer) => proxied_peer,
             None => self.peer,
         };
+
+        let header = self.api_config
+            .auth_cookie_name
+            .as_ref()
+            .map(|name|{
+                let host_cookie = format!("{name}=; Expires=Thu, 01 Jan 1970 00:00:00 GMT; Secure; SameSite=Lax; HttpOnly; Path=/;");
+
+                // SAFETY: this can only fail if the cookie name is not valid in http headers.
+                // since this is about an authentication cookie, this should never happen.
+                hyper::header::HeaderValue::from_str(&host_cookie)
+                    .expect("auth cookie name has characters that are not valid for http headers")
+             });
+
         async move {
-            let response = match Arc::clone(&config).handle_request(req, &peer).await {
+            let mut response = match Arc::clone(&config).handle_request(req, &peer).await {
                 Ok(response) => response,
                 Err(err) => {
                     let (err, code) = match err.downcast_ref::<HttpError>() {
@@ -371,6 +384,16 @@ impl Service<Request<Incoming>> for ApiService {
                         .body(err.into())?
                 }
             };
+
+            if let Some(cookie_header) = header {
+                // remove auth cookies that javascript based clients can not unset
+                if response.status() == StatusCode::UNAUTHORIZED {
+                    response
+                        .headers_mut()
+                        .insert(hyper::header::SET_COOKIE, cookie_header);
+                }
+            }
+
             let logger = config.get_access_log();
             log_response(logger, &peer, method, &path, &response, user_agent);
             Ok(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 1/3] rest-server: remove auth cookies via http header on unauthorized request

[Dominik Csapak]

Looks good to me.

Tested by invalidating my cookie and sending any http request that 
returns a 401 subsequently.
That successfully deleted my http-only cookie.


Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>

On 7/25/25 13:24, Shannon Sterz wrote:
> previously the behaviour of our javascript clients was to remove
> authentication cookies if the api returned a 401 UNAUTHORIZED
> response. with the switch to httponly cookies, this is no longer
> possible. add an option to the ApiConfig to allow the rest-server to
> remove such cookies
> 
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
>   proxmox-rest-server/src/api_config.rs |  9 +++++++++
>   proxmox-rest-server/src/rest.rs       | 25 ++++++++++++++++++++++++-
>   2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/proxmox-rest-server/src/api_config.rs b/proxmox-rest-server/src/api_config.rs
> index 0b847a0c..0a67231e 100644
> --- a/proxmox-rest-server/src/api_config.rs
> +++ b/proxmox-rest-server/src/api_config.rs
> @@ -33,6 +33,9 @@ pub struct ApiConfig {
>       auth_handler: Option<AuthHandler>,
>       index_handler: Option<IndexHandler>,
>       pub(crate) privileged_addr: Option<PrivilegedAddr>,
> +    // Name of the auth cookie that should be unset on 401 request. If `None` no cookie will be
> +    // removed.
> +    pub(crate) auth_cookie_name: Option<String>,
>   
>       #[cfg(feature = "templates")]
>       templates: templates::Templates,
> @@ -62,6 +65,7 @@ impl ApiConfig {
>               auth_handler: None,
>               index_handler: None,
>               privileged_addr: None,
> +            auth_cookie_name: None,
>   
>               #[cfg(feature = "templates")]
>               templates: templates::Templates::with_escape_fn(),
> @@ -82,6 +86,11 @@ impl ApiConfig {
>           self.auth_handler(AuthHandler::from_fn(func))
>       }
>   
> +    pub fn auth_cookie_name(mut self, auth_cookie_name: String) -> Self {
> +        self.auth_cookie_name = Some(auth_cookie_name);
> +        self
> +    }
> +
>       /// This is used for `protected` API calls to proxy to a more privileged service.
>       pub fn privileged_addr(mut self, addr: impl Into<PrivilegedAddr>) -> Self {
>           self.privileged_addr = Some(addr.into());
> diff --git a/proxmox-rest-server/src/rest.rs b/proxmox-rest-server/src/rest.rs
> index bff90882..035a9537 100644
> --- a/proxmox-rest-server/src/rest.rs
> +++ b/proxmox-rest-server/src/rest.rs
> @@ -357,8 +357,21 @@ impl Service<Request<Incoming>> for ApiService {
>               Some(proxied_peer) => proxied_peer,
>               None => self.peer,
>           };
> +
> +        let header = self.api_config
> +            .auth_cookie_name
> +            .as_ref()
> +            .map(|name|{
> +                let host_cookie = format!("{name}=; Expires=Thu, 01 Jan 1970 00:00:00 GMT; Secure; SameSite=Lax; HttpOnly; Path=/;");
> +
> +                // SAFETY: this can only fail if the cookie name is not valid in http headers.
> +                // since this is about an authentication cookie, this should never happen.
> +                hyper::header::HeaderValue::from_str(&host_cookie)
> +                    .expect("auth cookie name has characters that are not valid for http headers")
> +             });
> +
>           async move {
> -            let response = match Arc::clone(&config).handle_request(req, &peer).await {
> +            let mut response = match Arc::clone(&config).handle_request(req, &peer).await {
>                   Ok(response) => response,
>                   Err(err) => {
>                       let (err, code) = match err.downcast_ref::<HttpError>() {
> @@ -371,6 +384,16 @@ impl Service<Request<Incoming>> for ApiService {
>                           .body(err.into())?
>                   }
>               };
> +
> +            if let Some(cookie_header) = header {
> +                // remove auth cookies that javascript based clients can not unset
> +                if response.status() == StatusCode::UNAUTHORIZED {
> +                    response
> +                        .headers_mut()
> +                        .insert(hyper::header::SET_COOKIE, cookie_header);
> +                }
> +            }
> +
>               let logger = config.get_access_log();
>               log_response(logger, &peer, method, &path, &response, user_agent);
>               Ok(response)



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

[pbs-devel] [PATCH proxmox 2/3] auth-api: don't set `Expire` for HttpOnly cookies anymore

[Shannon Sterz]

previously users may have assumed that closing a browser will log them
out. this usually worked (see note below), as we defined the cookies
as "session cookies" by not setting `Expire` or `Max-Age`. clients
should remove such cookies when they are closed.

by setting `Expire` we broke this assumption as now browsers would
keep the cookie in place, even when closed, until they expired.

note: some browsers may never have behaved as expected here. a lot of
modern browsers have a "session restore" feature that would simply
restore such cookies when the session was restored. see also the
warning over in the mdn docs for `Set-Cookie` [1].

in any case, the tickets within the cookies were always valid for two
hours as we don't "revoke" tickets before they expire.

[1]:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Set-Cookie#expiresdate

Reported-By: Dominik Csapak <d.csapak@proxmox.com>
Suggested-By: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 proxmox-auth-api/src/api/access.rs | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs
index f5111d4a..671a370b 100644
--- a/proxmox-auth-api/src/api/access.rs
+++ b/proxmox-auth-api/src/api/access.rs
@@ -158,25 +158,18 @@ fn create_ticket_http_only(
             // parse the ticket here, so we can use the correct timestamp of the `Expire` parameter
             // take the ticket here, so the option will be `None` in the response
             if let Some(ticket_str) = ticket_response.ticket.take() {
-                let ticket = Ticket::<ApiTicket>::parse(&ticket_str)?;
-
-                // see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#expiresdate
-                // see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Date
-                // see: https://developer.mozilla.org/en-US/docs/Web/Security/Practical_implementation_guides/Cookies#expires
-                let expire =
-                    proxmox_time::epoch_to_http_date(ticket.time() + crate::TICKET_LIFETIME)?;
-
                 // this makes sure that ticket cookies:
                 // - Typically `__Host-`-prefixed: are only send to the specific domain that set
                 //   them and that scripts served via http cannot overwrite the cookie.
-                // - `Expires`: expire at the same time as the encoded timestamp in the ticket.
                 // - `Secure`: are only sent via https.
                 // - `SameSite=Lax`: are only sent on cross-site requests when the user is
                 //   navigating to the origin site from an external site.
                 // - `HttpOnly`: cookies are not readable to client-side javascript code.
-                let cookie = format!(
-                    "{host_cookie}={ticket_str}; Expires={expire}; Secure; SameSite=Lax; HttpOnly; Path=/;",
-                );
+                // - don't set `Expire` to keep cookie a session cookie. otherwise, we may break
+                //   security assumptions made by users previously. the expiration limit is still
+                //   enforced server side.
+                let cookie =
+                    format!("{host_cookie}={ticket_str}; Secure; SameSite=Lax; HttpOnly; Path=/;");
 
                 response = response.header(hyper::header::SET_COOKIE, cookie);
             }
-- 
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 2/3] auth-api: don't set `Expire` for HttpOnly cookies anymore

[Dominik Csapak]

LGTM

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>

On 7/25/25 13:24, Shannon Sterz wrote:
> previously users may have assumed that closing a browser will log them
> out. this usually worked (see note below), as we defined the cookies
> as "session cookies" by not setting `Expire` or `Max-Age`. clients
> should remove such cookies when they are closed.
> 
> by setting `Expire` we broke this assumption as now browsers would
> keep the cookie in place, even when closed, until they expired.
> 
> note: some browsers may never have behaved as expected here. a lot of
> modern browsers have a "session restore" feature that would simply
> restore such cookies when the session was restored. see also the
> warning over in the mdn docs for `Set-Cookie` [1].
> 
> in any case, the tickets within the cookies were always valid for two
> hours as we don't "revoke" tickets before they expire.
> 
> [1]:
> https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Set-Cookie#expiresdate
> 
> Reported-By: Dominik Csapak <d.csapak@proxmox.com>
> Suggested-By: Dominik Csapak <d.csapak@proxmox.com>
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
>   proxmox-auth-api/src/api/access.rs | 17 +++++------------
>   1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs
> index f5111d4a..671a370b 100644
> --- a/proxmox-auth-api/src/api/access.rs
> +++ b/proxmox-auth-api/src/api/access.rs
> @@ -158,25 +158,18 @@ fn create_ticket_http_only(
>               // parse the ticket here, so we can use the correct timestamp of the `Expire` parameter
>               // take the ticket here, so the option will be `None` in the response
>               if let Some(ticket_str) = ticket_response.ticket.take() {
> -                let ticket = Ticket::<ApiTicket>::parse(&ticket_str)?;
> -
> -                // see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#expiresdate
> -                // see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Date
> -                // see: https://developer.mozilla.org/en-US/docs/Web/Security/Practical_implementation_guides/Cookies#expires
> -                let expire =
> -                    proxmox_time::epoch_to_http_date(ticket.time() + crate::TICKET_LIFETIME)?;
> -
>                   // this makes sure that ticket cookies:
>                   // - Typically `__Host-`-prefixed: are only send to the specific domain that set
>                   //   them and that scripts served via http cannot overwrite the cookie.
> -                // - `Expires`: expire at the same time as the encoded timestamp in the ticket.
>                   // - `Secure`: are only sent via https.
>                   // - `SameSite=Lax`: are only sent on cross-site requests when the user is
>                   //   navigating to the origin site from an external site.
>                   // - `HttpOnly`: cookies are not readable to client-side javascript code.
> -                let cookie = format!(
> -                    "{host_cookie}={ticket_str}; Expires={expire}; Secure; SameSite=Lax; HttpOnly; Path=/;",
> -                );
> +                // - don't set `Expire` to keep cookie a session cookie. otherwise, we may break
> +                //   security assumptions made by users previously. the expiration limit is still
> +                //   enforced server side.
> +                let cookie =
> +                    format!("{host_cookie}={ticket_str}; Secure; SameSite=Lax; HttpOnly; Path=/;");
>   
>                   response = response.header(hyper::header::SET_COOKIE, cookie);
>               }



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

[pbs-devel] [PATCH proxmox 3/3] auth-api: allow log-in via parameters even if HttpOnly cookie is invalid

[Shannon Sterz]

previously the new HttpOnly endpoint would fail when a cookie was
provided even if the body of the request contained valid credentials.
this lead to issues when browser-based clients may have gotten invalid
HttpOnly cookies e.g. if a Proxmox Backup Server was re-installed at
the same IP address. the client could not remove the cookie due to the
new protections. while the server did not allow the client to log in
as it trusted the HttpOnly cookie over the parameters.

allow users to log in again in such a scenario, but don't allow a
ticket refresh. if the client has a valid ticket but cannot provide it
via HttpOnly cookie, something is off and forcing the client to
re-authenticate is probably the safer option.

Reported-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
Suggested-By: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 proxmox-auth-api/src/api/access.rs | 50 +++++++++++++++++++-----------
 proxmox-auth-api/src/types.rs      |  2 +-
 2 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs
index 671a370b..490fe5c8 100644
--- a/proxmox-auth-api/src/api/access.rs
+++ b/proxmox-auth-api/src/api/access.rs
@@ -59,7 +59,7 @@ pub async fn create_ticket(
         .downcast_ref::<RestEnvironment>()
         .ok_or_else(|| format_err!("detected wrong RpcEnvironment type"))?;
 
-    handle_ticket_creation(create_params, env)
+    handle_ticket_creation(create_params, true, env)
         .await
         // remove the superfluous ticket_info to not confuse clients
         .map(|mut info| {
@@ -121,6 +121,7 @@ fn create_ticket_http_only(
         let auth_context = auth_context()?;
         let host_cookie = auth_context.prefixed_auth_cookie_name();
         let mut create_params: CreateTicket = serde_json::from_value(param)?;
+        let password = create_params.password.take();
 
         // previously to refresh a ticket, the old ticket was provided as a password via this
         // endpoint's parameters. however, once the ticket is set as an HttpOnly cookie, some
@@ -139,16 +140,22 @@ fn create_ticket_http_only(
             // after this only `__Host-{Cookie Name}` cookies are in the iterator
             .filter_map(|c| extract_cookie(c, host_cookie))
             // so this should just give us the first one if it exists
-            .next()
-            // if not use the parameter
-            .or(create_params.password);
+            .next();
 
         let env: &RestEnvironment = rpcenv
             .as_any()
             .downcast_ref::<RestEnvironment>()
             .ok_or(format_err!("detected wrong RpcEnvironment type"))?;
 
-        let mut ticket_response = handle_ticket_creation(create_params, env).await?;
+        let mut ticket_response = handle_ticket_creation(create_params.clone(), true, env).await;
+
+        if ticket_response.is_err() && password.is_some() {
+            create_params.password = password;
+            ticket_response = handle_ticket_creation(create_params, false, env).await;
+        }
+
+        let mut ticket_response = ticket_response?;
+
         let mut response =
             Response::builder().header(http::header::CONTENT_TYPE, "application/json");
 
@@ -185,6 +192,7 @@ fn create_ticket_http_only(
 
 async fn handle_ticket_creation(
     create_params: CreateTicket,
+    allow_ticket_refresh: bool,
     env: &RestEnvironment,
 ) -> Result<CreateTicketResponse, Error> {
     let username = create_params.username;
@@ -199,6 +207,7 @@ async fn handle_ticket_creation(
         create_params.privs,
         create_params.port,
         create_params.tfa_challenge,
+        allow_ticket_refresh,
         env,
     )
     .await
@@ -240,6 +249,7 @@ async fn handle_ticket_creation(
     }
 }
 
+#[allow(clippy::too_many_arguments)]
 async fn authenticate_user(
     userid: &Userid,
     password: &str,
@@ -247,6 +257,7 @@ async fn authenticate_user(
     privs: Option<String>,
     port: Option<u16>,
     tfa_challenge: Option<String>,
+    allow_ticket_refresh: bool,
     rpcenv: &RestEnvironment,
 ) -> Result<AuthResult, Error> {
     let auth_context = auth_context()?;
@@ -261,21 +272,24 @@ async fn authenticate_user(
         return authenticate_2nd(userid, &tfa_challenge, password);
     }
 
-    if password.starts_with(prefix) && password.as_bytes().get(prefix.len()).copied() == Some(b':')
-    {
-        if let Ok(ticket_userid) = Ticket::<Userid>::parse(password)
-            .and_then(|ticket| ticket.verify(auth_context.keyring(), prefix, None))
+    if allow_ticket_refresh {
+        if password.starts_with(prefix)
+            && password.as_bytes().get(prefix.len()).copied() == Some(b':')
         {
-            if *userid == ticket_userid {
-                return Ok(AuthResult::CreateTicket);
+            if let Ok(ticket_userid) = Ticket::<Userid>::parse(password)
+                .and_then(|ticket| ticket.verify(auth_context.keyring(), prefix, None))
+            {
+                if *userid == ticket_userid {
+                    return Ok(AuthResult::CreateTicket);
+                }
+                bail!("ticket login failed - wrong userid");
+            }
+        } else if let Some(((path, privs), port)) = path.zip(privs).zip(port) {
+            match auth_context.check_path_ticket(userid, password, path, privs, port)? {
+                None => (), // no path based tickets supported, just fall through.
+                Some(true) => return Ok(AuthResult::Success),
+                Some(false) => bail!("No such privilege"),
             }
-            bail!("ticket login failed - wrong userid");
-        }
-    } else if let Some(((path, privs), port)) = path.zip(privs).zip(port) {
-        match auth_context.check_path_ticket(userid, password, path, privs, port)? {
-            None => (), // no path based tickets supported, just fall through.
-            Some(true) => return Ok(AuthResult::Success),
-            Some(false) => bail!("No such privilege"),
         }
     }
 
diff --git a/proxmox-auth-api/src/types.rs b/proxmox-auth-api/src/types.rs
index 0964e072..9bde661c 100644
--- a/proxmox-auth-api/src/types.rs
+++ b/proxmox-auth-api/src/types.rs
@@ -678,7 +678,7 @@ impl TryFrom<String> for Authid {
 
 #[api]
 /// The parameter object for creating new ticket.
-#[derive(Debug, Deserialize, Serialize)]
+#[derive(Debug, Clone, Deserialize, Serialize)]
 pub struct CreateTicket {
     /// User name
     pub username: Userid,
-- 
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 3/3] auth-api: allow log-in via parameters even if HttpOnly cookie is invalid

[Dominik Csapak]

LGTM

tested by invalidating my http-only cookie and logged in via
the login mask. worked successfully

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>

On 7/25/25 13:24, Shannon Sterz wrote:
> previously the new HttpOnly endpoint would fail when a cookie was
> provided even if the body of the request contained valid credentials.
> this lead to issues when browser-based clients may have gotten invalid
> HttpOnly cookies e.g. if a Proxmox Backup Server was re-installed at
> the same IP address. the client could not remove the cookie due to the
> new protections. while the server did not allow the client to log in
> as it trusted the HttpOnly cookie over the parameters.
> 
> allow users to log in again in such a scenario, but don't allow a
> ticket refresh. if the client has a valid ticket but cannot provide it
> via HttpOnly cookie, something is off and forcing the client to
> re-authenticate is probably the safer option.
> 
> Reported-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
> Suggested-By: Dominik Csapak <d.csapak@proxmox.com>
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
>   proxmox-auth-api/src/api/access.rs | 50 +++++++++++++++++++-----------
>   proxmox-auth-api/src/types.rs      |  2 +-
>   2 files changed, 33 insertions(+), 19 deletions(-)
> 
> diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs
> index 671a370b..490fe5c8 100644
> --- a/proxmox-auth-api/src/api/access.rs
> +++ b/proxmox-auth-api/src/api/access.rs
> @@ -59,7 +59,7 @@ pub async fn create_ticket(
>           .downcast_ref::<RestEnvironment>()
>           .ok_or_else(|| format_err!("detected wrong RpcEnvironment type"))?;
>   
> -    handle_ticket_creation(create_params, env)
> +    handle_ticket_creation(create_params, true, env)
>           .await
>           // remove the superfluous ticket_info to not confuse clients
>           .map(|mut info| {
> @@ -121,6 +121,7 @@ fn create_ticket_http_only(
>           let auth_context = auth_context()?;
>           let host_cookie = auth_context.prefixed_auth_cookie_name();
>           let mut create_params: CreateTicket = serde_json::from_value(param)?;
> +        let password = create_params.password.take();
>   
>           // previously to refresh a ticket, the old ticket was provided as a password via this
>           // endpoint's parameters. however, once the ticket is set as an HttpOnly cookie, some
> @@ -139,16 +140,22 @@ fn create_ticket_http_only(
>               // after this only `__Host-{Cookie Name}` cookies are in the iterator
>               .filter_map(|c| extract_cookie(c, host_cookie))
>               // so this should just give us the first one if it exists
> -            .next()
> -            // if not use the parameter
> -            .or(create_params.password);
> +            .next();
>   
>           let env: &RestEnvironment = rpcenv
>               .as_any()
>               .downcast_ref::<RestEnvironment>()
>               .ok_or(format_err!("detected wrong RpcEnvironment type"))?;
>   
> -        let mut ticket_response = handle_ticket_creation(create_params, env).await?;
> +        let mut ticket_response = handle_ticket_creation(create_params.clone(), true, env).await;
> +
> +        if ticket_response.is_err() && password.is_some() {
> +            create_params.password = password;
> +            ticket_response = handle_ticket_creation(create_params, false, env).await;
> +        }
> +
> +        let mut ticket_response = ticket_response?;
> +
>           let mut response =
>               Response::builder().header(http::header::CONTENT_TYPE, "application/json");
>   
> @@ -185,6 +192,7 @@ fn create_ticket_http_only(
>   
>   async fn handle_ticket_creation(
>       create_params: CreateTicket,
> +    allow_ticket_refresh: bool,
>       env: &RestEnvironment,
>   ) -> Result<CreateTicketResponse, Error> {
>       let username = create_params.username;
> @@ -199,6 +207,7 @@ async fn handle_ticket_creation(
>           create_params.privs,
>           create_params.port,
>           create_params.tfa_challenge,
> +        allow_ticket_refresh,
>           env,
>       )
>       .await
> @@ -240,6 +249,7 @@ async fn handle_ticket_creation(
>       }
>   }
>   
> +#[allow(clippy::too_many_arguments)]
>   async fn authenticate_user(
>       userid: &Userid,
>       password: &str,
> @@ -247,6 +257,7 @@ async fn authenticate_user(
>       privs: Option<String>,
>       port: Option<u16>,
>       tfa_challenge: Option<String>,
> +    allow_ticket_refresh: bool,
>       rpcenv: &RestEnvironment,
>   ) -> Result<AuthResult, Error> {
>       let auth_context = auth_context()?;
> @@ -261,21 +272,24 @@ async fn authenticate_user(
>           return authenticate_2nd(userid, &tfa_challenge, password);
>       }
>   
> -    if password.starts_with(prefix) && password.as_bytes().get(prefix.len()).copied() == Some(b':')
> -    {
> -        if let Ok(ticket_userid) = Ticket::<Userid>::parse(password)
> -            .and_then(|ticket| ticket.verify(auth_context.keyring(), prefix, None))
> +    if allow_ticket_refresh {
> +        if password.starts_with(prefix)
> +            && password.as_bytes().get(prefix.len()).copied() == Some(b':')
>           {
> -            if *userid == ticket_userid {
> -                return Ok(AuthResult::CreateTicket);
> +            if let Ok(ticket_userid) = Ticket::<Userid>::parse(password)
> +                .and_then(|ticket| ticket.verify(auth_context.keyring(), prefix, None))
> +            {
> +                if *userid == ticket_userid {
> +                    return Ok(AuthResult::CreateTicket);
> +                }
> +                bail!("ticket login failed - wrong userid");
> +            }
> +        } else if let Some(((path, privs), port)) = path.zip(privs).zip(port) {
> +            match auth_context.check_path_ticket(userid, password, path, privs, port)? {
> +                None => (), // no path based tickets supported, just fall through.
> +                Some(true) => return Ok(AuthResult::Success),
> +                Some(false) => bail!("No such privilege"),
>               }
> -            bail!("ticket login failed - wrong userid");
> -        }
> -    } else if let Some(((path, privs), port)) = path.zip(privs).zip(port) {
> -        match auth_context.check_path_ticket(userid, password, path, privs, port)? {
> -            None => (), // no path based tickets supported, just fall through.
> -            Some(true) => return Ok(AuthResult::Success),
> -            Some(false) => bail!("No such privilege"),
>           }
>       }
>   
> diff --git a/proxmox-auth-api/src/types.rs b/proxmox-auth-api/src/types.rs
> index 0964e072..9bde661c 100644
> --- a/proxmox-auth-api/src/types.rs
> +++ b/proxmox-auth-api/src/types.rs
> @@ -678,7 +678,7 @@ impl TryFrom<String> for Authid {
>   
>   #[api]
>   /// The parameter object for creating new ticket.
> -#[derive(Debug, Deserialize, Serialize)]
> +#[derive(Debug, Clone, Deserialize, Serialize)]
>   pub struct CreateTicket {
>       /// User name
>       pub username: Userid,



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

[pbs-devel] [PATCH proxmox-backup 1/1] api/proxy: set auth cookie name in rest server api config

[Shannon Sterz]

set the name of the auth cookie when configuring apis to allow the
rest server to remove invalid tickets on 401 requests.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 src/auth.rs                     | 10 +++++++++-
 src/auth_helpers.rs             |  1 +
 src/bin/proxmox-backup-api.rs   |  1 +
 src/bin/proxmox-backup-proxy.rs |  1 +
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/auth.rs b/src/auth.rs
index 86b12a76..ac24c8ca 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -13,7 +13,7 @@ use pbs_config::open_backup_lockfile;
 use proxmox_router::http_bail;
 use serde_json::json;
 
-use proxmox_auth_api::api::{Authenticator, LockedTfaConfig};
+use proxmox_auth_api::api::{AuthContext, Authenticator, LockedTfaConfig};
 use proxmox_auth_api::ticket::{Empty, Ticket};
 use proxmox_auth_api::types::Authid;
 use proxmox_auth_api::{HMACKey, Keyring};
@@ -377,6 +377,14 @@ pub fn setup_auth_context(use_private_key: bool) {
     proxmox_auth_api::set_auth_context(AUTH_CONTEXT.get().unwrap());
 }
 
+pub fn get_auth_cookie_name() -> String {
+    AUTH_CONTEXT
+        .get()
+        .expect("auth context needs to be set before accessing the auth cookie name")
+        .prefixed_auth_cookie_name()
+        .to_string()
+}
+
 pub(crate) fn private_auth_keyring() -> &'static Keyring {
     &PRIVATE_KEYRING
 }
diff --git a/src/auth_helpers.rs b/src/auth_helpers.rs
index 65e36308..fdd8d663 100644
--- a/src/auth_helpers.rs
+++ b/src/auth_helpers.rs
@@ -10,6 +10,7 @@ use proxmox_sys::fs::{file_get_contents, replace_file, CreateOptions};
 use pbs_buildcfg::configdir;
 use serde_json::json;
 
+pub use crate::auth::get_auth_cookie_name;
 pub use crate::auth::setup_auth_context;
 pub use proxmox_auth_api::api::assemble_csrf_prevention_token;
 
diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup-api.rs
index 3d80b023..74528236 100644
--- a/src/bin/proxmox-backup-api.rs
+++ b/src/bin/proxmox-backup-api.rs
@@ -88,6 +88,7 @@ async fn run() -> Result<(), Error> {
     let config = ApiConfig::new(pbs_buildcfg::JS_DIR, RpcEnvironmentType::PRIVILEGED)
         .index_handler_func(|_, _| get_index())
         .auth_handler_func(|h, m| Box::pin(check_pbs_auth(h, m)))
+        .auth_cookie_name(proxmox_backup::auth_helpers::get_auth_cookie_name())
         .default_api2_handler(&proxmox_backup::api2::ROUTER)
         .enable_access_log(
             pbs_buildcfg::API_ACCESS_LOG_FN,
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 694c2463..4641fed1 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -195,6 +195,7 @@ async fn run() -> Result<(), Error> {
     let mut config = ApiConfig::new(pbs_buildcfg::JS_DIR, RpcEnvironmentType::PUBLIC)
         .index_handler_func(|e, p| Box::pin(get_index_future(e, p)))
         .auth_handler_func(|h, m| Box::pin(check_pbs_auth(h, m)))
+        .auth_cookie_name(proxmox_backup::auth_helpers::get_auth_cookie_name())
         .register_template("index", &indexpath)?
         .register_template("console", "/usr/share/pve-xtermjs/index.html.hbs")?
         .default_api2_handler(&proxmox_backup::api2::ROUTER)
-- 
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 1/1] api/proxy: set auth cookie name in rest server api config

[Dominik Csapak]

LGTM

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>

On 7/25/25 13:24, Shannon Sterz wrote:
> set the name of the auth cookie when configuring apis to allow the
> rest server to remove invalid tickets on 401 requests.
> 
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
>   src/auth.rs                     | 10 +++++++++-
>   src/auth_helpers.rs             |  1 +
>   src/bin/proxmox-backup-api.rs   |  1 +
>   src/bin/proxmox-backup-proxy.rs |  1 +
>   4 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/auth.rs b/src/auth.rs
> index 86b12a76..ac24c8ca 100644
> --- a/src/auth.rs
> +++ b/src/auth.rs
> @@ -13,7 +13,7 @@ use pbs_config::open_backup_lockfile;
>   use proxmox_router::http_bail;
>   use serde_json::json;
>   
> -use proxmox_auth_api::api::{Authenticator, LockedTfaConfig};
> +use proxmox_auth_api::api::{AuthContext, Authenticator, LockedTfaConfig};
>   use proxmox_auth_api::ticket::{Empty, Ticket};
>   use proxmox_auth_api::types::Authid;
>   use proxmox_auth_api::{HMACKey, Keyring};
> @@ -377,6 +377,14 @@ pub fn setup_auth_context(use_private_key: bool) {
>       proxmox_auth_api::set_auth_context(AUTH_CONTEXT.get().unwrap());
>   }
>   
> +pub fn get_auth_cookie_name() -> String {
> +    AUTH_CONTEXT
> +        .get()
> +        .expect("auth context needs to be set before accessing the auth cookie name")
> +        .prefixed_auth_cookie_name()
> +        .to_string()
> +}
> +
>   pub(crate) fn private_auth_keyring() -> &'static Keyring {
>       &PRIVATE_KEYRING
>   }
> diff --git a/src/auth_helpers.rs b/src/auth_helpers.rs
> index 65e36308..fdd8d663 100644
> --- a/src/auth_helpers.rs
> +++ b/src/auth_helpers.rs
> @@ -10,6 +10,7 @@ use proxmox_sys::fs::{file_get_contents, replace_file, CreateOptions};
>   use pbs_buildcfg::configdir;
>   use serde_json::json;
>   
> +pub use crate::auth::get_auth_cookie_name;
>   pub use crate::auth::setup_auth_context;
>   pub use proxmox_auth_api::api::assemble_csrf_prevention_token;
>   
> diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup-api.rs
> index 3d80b023..74528236 100644
> --- a/src/bin/proxmox-backup-api.rs
> +++ b/src/bin/proxmox-backup-api.rs
> @@ -88,6 +88,7 @@ async fn run() -> Result<(), Error> {
>       let config = ApiConfig::new(pbs_buildcfg::JS_DIR, RpcEnvironmentType::PRIVILEGED)
>           .index_handler_func(|_, _| get_index())
>           .auth_handler_func(|h, m| Box::pin(check_pbs_auth(h, m)))
> +        .auth_cookie_name(proxmox_backup::auth_helpers::get_auth_cookie_name())
>           .default_api2_handler(&proxmox_backup::api2::ROUTER)
>           .enable_access_log(
>               pbs_buildcfg::API_ACCESS_LOG_FN,
> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
> index 694c2463..4641fed1 100644
> --- a/src/bin/proxmox-backup-proxy.rs
> +++ b/src/bin/proxmox-backup-proxy.rs
> @@ -195,6 +195,7 @@ async fn run() -> Result<(), Error> {
>       let mut config = ApiConfig::new(pbs_buildcfg::JS_DIR, RpcEnvironmentType::PUBLIC)
>           .index_handler_func(|e, p| Box::pin(get_index_future(e, p)))
>           .auth_handler_func(|h, m| Box::pin(check_pbs_auth(h, m)))
> +        .auth_cookie_name(proxmox_backup::auth_helpers::get_auth_cookie_name())
>           .register_template("index", &indexpath)?
>           .register_template("console", "/usr/share/pve-xtermjs/index.html.hbs")?
>           .default_api2_handler(&proxmox_backup::api2::ROUTER)



_______________________________________________
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} 0/4] HttpOnly follow-ups

[Shannon Sterz]

On Fri Jul 25, 2025 at 1:23 PM CEST, Shannon Sterz wrote:
> this small series tries to smooth out the transition to HttpOnly cookies
> for our users:
>
> - cookies are now removed by the server if a 401 UNAUTHORIZED error is
>   encountered. this matches the behaviour of our previous javascript
>   browser-based clients. it is necessary, as they can't remove the
>   cookie themselves anymore due to the new security measures.
> - log-in is possible again, even if an invalid HttpOnly cookie is
>   provided.
> - `Expire` is removed from the cookies to make them "session cookies"
>   again. this should restore security assumptions some users may have
>   made about closing their browser and being logged out. note that
>   session cookies may still be restored after a browser was closed, if
>   the browser uses session restoration.
>
> proxmox:
>
> Shannon Sterz (3):
>   rest-server: remove auth cookies via http header on unauthorized
>     request
>   auth-api: don't set `Expire` for HttpOnly cookies anymore
>   auth-api: allow log-in via parameters even if HttpOnly cookie is
>     invalid
>
>  proxmox-auth-api/src/api/access.rs    | 67 +++++++++++++++------------
>  proxmox-auth-api/src/types.rs         |  2 +-
>  proxmox-rest-server/src/api_config.rs |  9 ++++
>  proxmox-rest-server/src/rest.rs       | 25 +++++++++-
>  4 files changed, 71 insertions(+), 32 deletions(-)
>
>
> proxmox-backup:
>
> Shannon Sterz (1):
>   api/proxy: set auth cookie name in rest server api config
>
>  src/auth.rs                     | 10 +++++++++-
>  src/auth_helpers.rs             |  1 +
>  src/bin/proxmox-backup-api.rs   |  1 +
>  src/bin/proxmox-backup-proxy.rs |  1 +
>  4 files changed, 12 insertions(+), 1 deletion(-)
>
>
> Summary over all repositories:
>   8 files changed, 83 insertions(+), 33 deletions(-)
>
> --
> Generated by git-murpp 0.8.1

sorry for sending this a second time, was a bit too quick with `git
send-email`


_______________________________________________
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} 0/4] HttpOnly follow-ups

[Shannon Sterz]

On Fri Jul 25, 2025 at 1:23 PM CEST, Shannon Sterz wrote:
> this small series tries to smooth out the transition to HttpOnly cookies
> for our users:
>
> - cookies are now removed by the server if a 401 UNAUTHORIZED error is
>   encountered. this matches the behaviour of our previous javascript
>   browser-based clients. it is necessary, as they can't remove the
>   cookie themselves anymore due to the new security measures.
> - log-in is possible again, even if an invalid HttpOnly cookie is
>   provided.
> - `Expire` is removed from the cookies to make them "session cookies"
>   again. this should restore security assumptions some users may have
>   made about closing their browser and being logged out. note that
>   session cookies may still be restored after a browser was closed, if
>   the browser uses session restoration.
>
> proxmox:
>
> Shannon Sterz (3):
>   rest-server: remove auth cookies via http header on unauthorized
>     request
>   auth-api: don't set `Expire` for HttpOnly cookies anymore
>   auth-api: allow log-in via parameters even if HttpOnly cookie is
>     invalid
>
>  proxmox-auth-api/src/api/access.rs    | 67 +++++++++++++++------------
>  proxmox-auth-api/src/types.rs         |  2 +-
>  proxmox-rest-server/src/api_config.rs |  9 ++++
>  proxmox-rest-server/src/rest.rs       | 25 +++++++++-
>  4 files changed, 71 insertions(+), 32 deletions(-)
>
>
> proxmox-backup:
>
> Shannon Sterz (1):
>   api/proxy: set auth cookie name in rest server api config
>
>  src/auth.rs                     | 10 +++++++++-
>  src/auth_helpers.rs             |  1 +
>  src/bin/proxmox-backup-api.rs   |  1 +
>  src/bin/proxmox-backup-proxy.rs |  1 +
>  4 files changed, 12 insertions(+), 1 deletion(-)
>
>
> Summary over all repositories:
>   8 files changed, 83 insertions(+), 33 deletions(-)
>
> --
> Generated by git-murpp 0.8.1

seems these patches were applied [1,2]

thanks!

[1]: https://git.proxmox.com/?p=proxmox-backup.git;a=commit;h=8f4e455550e470a670f139d6124a00887962122c
[2]: https://git.proxmox.com/?p=proxmox.git;a=commit;h=2c0b5edda2f778837c4d2eb8a259a04c3dca8ebd


_______________________________________________
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} 0/4] HttpOnly follow-ups

[Thomas Lamprecht]

On Fri, 25 Jul 2025 13:23:53 +0200, Shannon Sterz wrote:
> this small series tries to smooth out the transition to HttpOnly cookies
> for our users:
> 
> - cookies are now removed by the server if a 401 UNAUTHORIZED error is
>   encountered. this matches the behaviour of our previous javascript
>   browser-based clients. it is necessary, as they can't remove the
>   cookie themselves anymore due to the new security measures.
> - log-in is possible again, even if an invalid HttpOnly cookie is
>   provided.
> - `Expire` is removed from the cookies to make them "session cookies"
>   again. this should restore security assumptions some users may have
>   made about closing their browser and being logged out. note that
>   session cookies may still be restored after a browser was closed, if
>   the browser uses session restoration.
> 
> [...]

As you already noticed:

Applied, thanks!

[1/1] api/proxy: set auth cookie name in rest server api config
      commit: 8f4e455550e470a670f139d6124a00887962122c
[1/3] rest-server: remove auth cookies via http header on unauthorized request
      commit: 4ef6a1c012a5e32d3059731094c31864ddd3fa78
[2/3] auth-api: don't set `Expire` for HttpOnly cookies anymore
      commit: f45eb2934c484b652e8b3676b508d0ea1e9142d6
[3/3] auth-api: allow log-in via parameters even if HttpOnly cookie is invalid
      commit: 2c0b5edda2f778837c4d2eb8a259a04c3dca8ebd


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

[pbs-devel] [PATCH proxmox{,-backup} 0/4] HttpOnly follow-ups

[Shannon Sterz]

this small series tries to smooth out the transition to HttpOnly cookies
for our users:

- cookies are now removed by the server if a 401 UNAUTHORIZED error is
  encountered. this matches the behaviour of our previous javascript
  browser-based clients. it is necessary, as they can't remove the
  cookie themselves anymore due to the new security measures.
- log-in is possible again, even if an invalid HttpOnly cookie is
  provided.
- `Expire` is removed from the cookies to make them "session cookies"
  again. this should restore security assumptions some users may have
  made about closing their browser and being logged out. note that
  session cookies may still be restored after a browser was closed, if
  the browser uses session restoration.

proxmox:

Shannon Sterz (3):
  rest-server: remove auth cookies via http header on unauthorized
    request
  auth-api: don't set `Expire` for HttpOnly cookies anymore
  auth-api: allow log-in via parameters even if HttpOnly cookie is
    invalid

 proxmox-auth-api/src/api/access.rs    | 67 +++++++++++++++------------
 proxmox-auth-api/src/types.rs         |  2 +-
 proxmox-rest-server/src/api_config.rs |  9 ++++
 proxmox-rest-server/src/rest.rs       | 25 +++++++++-
 4 files changed, 71 insertions(+), 32 deletions(-)


proxmox-backup:

Shannon Sterz (1):
  api/proxy: set auth cookie name in rest server api config

 src/auth.rs                     | 10 +++++++++-
 src/auth_helpers.rs             |  1 +
 src/bin/proxmox-backup-api.rs   |  1 +
 src/bin/proxmox-backup-proxy.rs |  1 +
 4 files changed, 12 insertions(+), 1 deletion(-)


Summary over all repositories:
  8 files changed, 83 insertions(+), 33 deletions(-)

--
Generated by git-murpp 0.8.1


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

[pbs-devel] [PATCH proxmox 2/3] auth-api: don't set `Expire` for HttpOnly cookies anymore

[Shannon Sterz]

previously users may have assumed that closing a browser will log them
out. this usually worked (see note below), as we defined the cookies
as "session cookies" by not setting `Expire` or `Max-Age`. clients
should remove such cookies when they are closed.

by setting `Expire` we broke this assumption as now browsers would
keep the cookie in place, even when closed, until they expired.

note: some browsers may never have behaved as expected here. a lot of
modern browsers have a "session restore" feature that would simply
restore such cookies when the session was restored. see also the
warning over in the mdn docs for `Set-Cookie` [1].

in any case, the tickets within the cookies were always valid for two
hours as we don't "revoke" tickets before they expire.

[1]:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Set-Cookie#expiresdate

Reported-By: Dominik Csapak <d.csapak@proxmox.com>
Suggested-By: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 proxmox-auth-api/src/api/access.rs | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs
index f5111d4a..671a370b 100644
--- a/proxmox-auth-api/src/api/access.rs
+++ b/proxmox-auth-api/src/api/access.rs
@@ -158,25 +158,18 @@ fn create_ticket_http_only(
             // parse the ticket here, so we can use the correct timestamp of the `Expire` parameter
             // take the ticket here, so the option will be `None` in the response
             if let Some(ticket_str) = ticket_response.ticket.take() {
-                let ticket = Ticket::<ApiTicket>::parse(&ticket_str)?;
-
-                // see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#expiresdate
-                // see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Date
-                // see: https://developer.mozilla.org/en-US/docs/Web/Security/Practical_implementation_guides/Cookies#expires
-                let expire =
-                    proxmox_time::epoch_to_http_date(ticket.time() + crate::TICKET_LIFETIME)?;
-
                 // this makes sure that ticket cookies:
                 // - Typically `__Host-`-prefixed: are only send to the specific domain that set
                 //   them and that scripts served via http cannot overwrite the cookie.
-                // - `Expires`: expire at the same time as the encoded timestamp in the ticket.
                 // - `Secure`: are only sent via https.
                 // - `SameSite=Lax`: are only sent on cross-site requests when the user is
                 //   navigating to the origin site from an external site.
                 // - `HttpOnly`: cookies are not readable to client-side javascript code.
-                let cookie = format!(
-                    "{host_cookie}={ticket_str}; Expires={expire}; Secure; SameSite=Lax; HttpOnly; Path=/;",
-                );
+                // - don't set `Expire` to keep cookie a session cookie. otherwise, we may break
+                //   security assumptions made by users previously. the expiration limit is still
+                //   enforced server side.
+                let cookie =
+                    format!("{host_cookie}={ticket_str}; Secure; SameSite=Lax; HttpOnly; Path=/;");
 
                 response = response.header(hyper::header::SET_COOKIE, cookie);
             }
-- 
2.47.2



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