[pbs-devel] [PATCH proxmox] auth-api: add error type to ticket verification
Wolfgang Bumiller
w.bumiller at proxmox.com
Fri Aug 30 10:38:33 CEST 2024
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 at 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 at 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
More information about the pbs-devel
mailing list