* [pbs-devel] [PATCH proxmox] auth-api: add error type to ticket verification
@ 2024-08-08 14:35 Hannes Laimer
2024-08-30 8:38 ` Wolfgang Bumiller
0 siblings, 1 reply; 2+ messages in thread
From: Hannes Laimer @ 2024-08-08 14:35 UTC (permalink / raw)
To: 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 <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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [pbs-devel] [PATCH proxmox] auth-api: add error type to ticket verification
2024-08-08 14:35 [pbs-devel] [PATCH proxmox] auth-api: add error type to ticket verification Hannes Laimer
@ 2024-08-30 8:38 ` Wolfgang Bumiller
0 siblings, 0 replies; 2+ messages in thread
From: Wolfgang Bumiller @ 2024-08-30 8:38 UTC (permalink / raw)
To: Hannes Laimer; +Cc: 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 <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"
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
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-08-30 8:38 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-08-08 14:35 [pbs-devel] [PATCH proxmox] auth-api: add error type to ticket verification Hannes Laimer
2024-08-30 8:38 ` Wolfgang Bumiller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox