On 2/6/25 13:01, Alexander Abraham wrote:
Two things were added to the proxmox-openid crate to fix
bug #5076: i) the function to require strict audience checking
was called and ii) an extra verifier function was added to check
if the configured audiences match the receieved audiences.
Hi,
first, it would be nice if the three relevant patches
(proxmox/access-control/manager) would get a
combined cover-letter. that way it's easier to see that the patches
belong together.
aside from that, it would also be good if the commit message contain
a 'why'. The 'what' and 'how' should (most often) be self-evident from
the diff, but the why isn't most of the time.
E.g. a short sentence like: We want to verify additional audiences because ...
makes it much easier to reason about the intentions later on.
a few smaller comments inline
Signed-off-by: Alexander Abraham <a.abra...@proxmox.com>
---
proxmox-openid/src/lib.rs | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/proxmox-openid/src/lib.rs b/proxmox-openid/src/lib.rs
index fe65fded..396f55cd 100644
--- a/proxmox-openid/src/lib.rs
+++ b/proxmox-openid/src/lib.rs
@@ -1,10 +1,9 @@
#![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))]
-use std::path::Path;
-
use anyhow::{format_err, Error};
use serde::{Deserialize, Serialize};
use serde_json::Value;
+use std::path::Path;
these two hunks seem unrelated (and wrong), please leave the
'std' imports seperate
mod http_client;
pub use http_client::http_client;
@@ -53,6 +52,8 @@ pub struct OpenIdConfig {
pub prompt: Option<String>,
#[serde(skip_serializing_if = "Option::is_none")]
pub acr_values: Option<Vec<String>>,
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub aud: Option<Vec<String>>,
}
pub struct OpenIdAuthenticator {
@@ -204,21 +205,32 @@ impl OpenIdAuthenticator {
.set_pkce_verifier(private_auth_state.pkce_verifier())
.request(http_client)
.map_err(|err| format_err!("Failed to contact token endpoint:
{}", err))?;
-
any special reason why you remove the whitespace here?
- let id_token_verifier: CoreIdTokenVerifier =
self.client.id_token_verifier();
let id_token_claims: &CoreIdTokenClaims = token_response
.extra_fields()
.id_token()
.expect("Server did not return an ID token")
- .claims(&id_token_verifier, &private_auth_state.nonce)
+ .claims(
+ &((self.client.id_token_verifier() as CoreIdTokenVerifier)
is this cast here really necessary? AFAICS it shouldn't ?
+ .require_audience_match(true)
+ .set_other_audience_verifier_fn(|aud| {
+ let curr_aud: &String = &**aud;
clippy warns here:
deref which would be done by auto-deref
so you can just write:
let curr_aud: &String = aud;
+ if &self.config.client_id == curr_aud {
+ true
+ } else {
+ match self.config.aud.as_ref() {
+ Some(confd_auds) =>
confd_auds.contains(curr_aud),
+ None => false,
+ }
+ }
+ })),
+ &private_auth_state.nonce,
+ )
.map_err(|err| format_err!("Failed to verify ID token: {}",
err))?;
-
why the white space removal here too?
let userinfo_claims: GenericUserInfoClaims = self
.client
.user_info(token_response.access_token().to_owned(), None)?
.request(http_client)
.map_err(|err| format_err!("Failed to contact userinfo endpoint:
{}", err))?;
-
and here
Ok((id_token_claims.clone(), userinfo_claims))
}
@@ -230,9 +242,7 @@ impl OpenIdAuthenticator {
) -> Result<Value, Error> {
let (id_token_claims, userinfo_claims) =
self.verify_authorization_code(code, private_auth_state)?;
-
let mut data = serde_json::to_value(id_token_claims)?;
-
and here
let data2 = serde_json::to_value(userinfo_claims)?;
if let Some(map) = data2.as_object() {
@@ -243,7 +253,6 @@ impl OpenIdAuthenticator {
data[key] = value.clone();
}
}
-
and here
IMO, white space cleanup can be fine, but please as a separate (upfront) patch,
so it does not pollute the actual patch
that said, in this case, I'd just leave the empty lines in place
Ok(data)
}
}
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
