From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 1DFF31FF16A for ; Fri, 30 Aug 2024 10:38:42 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 60DDC37586; Fri, 30 Aug 2024 10:39:09 +0200 (CEST) Date: Fri, 30 Aug 2024 10:38:33 +0200 From: Wolfgang Bumiller To: Hannes Laimer Message-ID: References: <20240808143516.84584-1-h.laimer@proxmox.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240808143516.84584-1-h.laimer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.086 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 T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [self.data, access.rs, ticket.rs] Subject: Re: [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 Cc: pbs-devel@lists.proxmox.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 > --- > 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::::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, ^ 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, > ) -> Result { 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::() {}`, 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