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

[Hannes Laimer <h.laimer@proxmox.com>]

... 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"
 
 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,
+
+    #[error("ticket missing signature")]
+    MissingSignature,
+
+    #[error("timestamp newer than expected")]
+    TimestampNewer,
+
+    #[error("ticket is expired")]
+    Expired,
+
+    #[error("ticket with invalid signature")]
+    InvalidSignature,
+
+    #[error("failed to parse contained ticket data: {0}")]
+    ParseError(String),
+}
+
 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> {
         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