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

[Dominik Csapak <d.csapak@proxmox.com>]

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