Re: [pbs-devel] [PATCH proxmox] auth-api: add error type to ticket verification

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Thu, Aug 08, 2024 at 04:35:16PM GMT, Hannes Laimer wrote:
> ... so we can avoid trying to login using an expired ticket as the
> password, mostly important to avoid confusing auth log entries when an
> expired ticket is used.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> This came up in enterprise support where an expired ticket caused
> confusing auth log entries like
> ```
> 2024-08-08T16:09:26+02:00: authentication failure; rhost=[::ffff:192.168.55.1]:42346 user=root@pam msg=authentication error - SUCCESS (0)
> ```
> since the auth code treats not parsable tickets the same way it treats
> invalid and expired ones, it also tries to use expired tickets (,in this
> example,) for PAM authentication, what for obviouse reasons fails.
> 
> Our max pw lengh is 64, so also the case where a user has a valid
> exprired ticket as password is not an issue.
> 
> 
>  proxmox-auth-api/Cargo.toml        |  1 +
>  proxmox-auth-api/src/api/access.rs | 20 +++++++++++------
>  proxmox-auth-api/src/ticket.rs     | 35 ++++++++++++++++++++++++------
>  3 files changed, 42 insertions(+), 14 deletions(-)
> 
> diff --git a/proxmox-auth-api/Cargo.toml b/proxmox-auth-api/Cargo.toml
> index 44db2bf8..69589ff3 100644
> --- a/proxmox-auth-api/Cargo.toml
> +++ b/proxmox-auth-api/Cargo.toml
> @@ -28,6 +28,7 @@ regex = { workspace = true, optional = true }
>  serde = { workspace = true, optional = true, features = [ "derive" ] }
>  serde_json = { workspace = true, optional = true }
>  serde_plain = { workspace = true, optional = true }
> +thiserror = "1"

Please make this a workspace dependency and keep things sorted!

>  
>  proxmox-product-config = { workspace = true, optional = true }
>  proxmox-rest-server = { workspace = true, optional = true }
> diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs
> index f7d52e95..aa5e0e1b 100644
> --- a/proxmox-auth-api/src/api/access.rs
> +++ b/proxmox-auth-api/src/api/access.rs
> @@ -11,7 +11,7 @@ use proxmox_tfa::api::TfaChallenge;
>  
>  use super::ApiTicket;
>  use super::{auth_context, HMACKey};
> -use crate::ticket::Ticket;
> +use crate::ticket::{Ticket, TicketVerifyError};
>  use crate::types::{Authid, Userid};
>  
>  #[allow(clippy::large_enum_variant)]
> @@ -155,13 +155,19 @@ async fn authenticate_user(
>  
>      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 *userid == ticket_userid {
> -                return Ok(AuthResult::CreateTicket);
> +        let verify_result = Ticket::<Userid>::parse(password)
> +            .and_then(|ticket| ticket.verify(auth_context.keyring(), prefix, None));
> +        match verify_result {
> +            Ok(ticket_userid) if *userid == ticket_userid => return Ok(AuthResult::CreateTicket),
> +            Ok(_) => bail!("ticket login failed - wrong userid"),
> +            Err(err) => {
> +                if let Some(TicketVerifyError::Expired) = err.downcast_ref::<TicketVerifyError>() {
> +                    // we don't want to use the 'ticket' as a password iff we know it is an expired
> +                    // ticket, if it is just invalid, it may also be in fact a password that just
> +                    // happens to start with '<prefix>:'
> +                    bail!("ticket login failed - expired");
> +                }
>              }
> -            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)? {
> diff --git a/proxmox-auth-api/src/ticket.rs b/proxmox-auth-api/src/ticket.rs
> index 498e9385..d0709245 100644
> --- a/proxmox-auth-api/src/ticket.rs
> +++ b/proxmox-auth-api/src/ticket.rs
> @@ -3,7 +3,7 @@
>  use std::borrow::Cow;
>  use std::marker::PhantomData;
>  
> -use anyhow::{bail, format_err, Error};
> +use anyhow::{anyhow, bail, format_err, Error};
>  use openssl::hash::MessageDigest;
>  use percent_encoding::{percent_decode_str, percent_encode, AsciiSet};
>  
> @@ -18,6 +18,27 @@ const TICKET_ASCIISET: &AsciiSet = &percent_encoding::CONTROLS.add(b':');
>  /// with no data.
>  pub struct Empty;
>  
> +#[derive(thiserror::Error, Debug)]
> +pub enum TicketVerifyError {
> +    #[error("ticket with invalid prefix")]
> +    InvalidPrefix,

^ I don't think there's code that can make use of this case.
Let's say this could be `Other(String)`.

> +
> +    #[error("ticket missing signature")]
> +    MissingSignature,

^ This is never used, you used `InvalidSignature` there instead.

That's actually fine, as I don't think it's a useful distinction to make
in any code anyway.

> +
> +    #[error("timestamp newer than expected")]
> +    TimestampNewer,

^ This and the `ParseError` currently don't include "ticket" in the
message. Either we assume the caller will add context and drop it
everywhere, or we use it everywhere to avoid *some* messages to be
confusingly out of context.

^ Also, this is another case for `Other`, as I don't see how we'd deal
with this in code.

> +
> +    #[error("ticket is expired")]
> +    Expired,
> +
> +    #[error("ticket with invalid signature")]
> +    InvalidSignature,

IMO, if we already have a specific error type for this, the
`keyring.verify()` error should also go into this, since - apart from
generic openssl errors - that's what *that* actually means.
But I don't know if `thiserror` can do a good job formatting based on an
`Option` inside, and creating 2 separate cases seems excessive. (I'd
actually just implement `Display` manually then, but read on...).

> +
> +    #[error("failed to parse contained ticket data: {0}")]
> +    ParseError(String),

^ Would also fit into the `Other` case.

> +}
> +
>  impl ToString for Empty {
>      fn to_string(&self) -> String {
>          String::new()
> @@ -142,20 +163,20 @@ where
>          time_frame: std::ops::Range<i64>,
>      ) -> Result<T, Error> {

You're changing almost all cases, only the `keyring.verify()` case
remains AFAICT, which should be added to
`TicketVerifyError::InvalidSignature`.

I think the only really *useful* case here is `Expired`. Not just for
now, but given that you'd be dealing with these cases in *code* only, I
don't think the other cases are all that useful.

So I think we should consider one of these 2 options:

1)
- Change the return type's `Error` to `TicketVerifyError` (the caller
  will have an easier time matching on `Expired`, without requiring a
  `downcast`)
- Change the error enum to `Expired | Other(String)`

2)
- Use an empty `struct TicketExpired;` as error for only the case we
  currently need.
- Keep the `anyhow::Error` return type
- Simplifying the call site to `if err.is::<TicketExpired>() {}`,
  which is also shorter than the `downcast`.

>          if self.prefix != prefix {
> -            bail!("ticket with invalid prefix");
> +            bail!(TicketVerifyError::InvalidPrefix);
>          }
>  
>          let signature = match self.signature.as_deref() {
>              Some(sig) => sig,
> -            None => bail!("invalid ticket without signature"),
> +            None => bail!(TicketVerifyError::InvalidSignature),
>          };
>  
>          let age = crate::time::epoch_i64() - self.time;
>          if age < time_frame.start {
> -            bail!("invalid ticket - timestamp newer than expected");
> +            bail!(TicketVerifyError::TimestampNewer);
>          }
>          if age > time_frame.end {
> -            bail!("invalid ticket - expired");
> +            bail!(TicketVerifyError::Expired);
>          }
>  
>          let is_valid = keyring.verify(
> @@ -165,12 +186,12 @@ where
>          )?;
>  
>          if !is_valid {
> -            bail!("ticket with invalid signature");
> +            bail!(TicketVerifyError::InvalidSignature);
>          }
>  
>          self.data
>              .parse()
> -            .map_err(|err| format_err!("failed to parse contained ticket data: {:?}", err))
> +            .map_err(|err| anyhow!(TicketVerifyError::ParseError(format!("{:?}", err))))
>      }
>  
>      /// Verify the ticket with the provided key pair. The additional authentication data needs to
> -- 
> 2.39.2


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