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

[Alexander Abraham <a.abraham@proxmox.com>]

Superseded by: https://lore.proxmox.com/all/20250603091256.40923-1-a.abraham@proxmox.com/
> Dominik Csapak <d.csapak@proxmox.com> hat am 17.02.2025 09:54 CET geschrieben:
> 
>  
> 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