From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 834911FF15D
for <inbox@lore.proxmox.com>; 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 <h.laimer@proxmox.com>
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
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
<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" <pbs-devel-bounces@lists.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