Re: [pve-devel] [PATCH proxmox/proxmox-openid] fix #5076: Added extra audience verification checks.

[Dominik Csapak <d.csapak@proxmox.com>]

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.abraham@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