From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 834911FF15D for ; Thu, 8 Aug 2024 16:35:13 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4E2991FF38; Thu, 8 Aug 2024 16:35:23 +0200 (CEST) From: Hannes Laimer To: pbs-devel@lists.proxmox.com Date: Thu, 8 Aug 2024 16:35:16 +0200 Message-Id: <20240808143516.84584-1-h.laimer@proxmox.com> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.011 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pbs-devel] [PATCH proxmox] auth-api: add error type to ticket verification X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" ... 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 --- 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::::parse(password) - .and_then(|ticket| ticket.verify(auth_context.keyring(), prefix, None)) - { - if *userid == ticket_userid { - return Ok(AuthResult::CreateTicket); + let verify_result = Ticket::::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::() { + // 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 ':' + 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, ) -> Result { 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