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

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

[Alexander Abraham]

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.

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;
 
 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))?;
-
-        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)
+                    .require_audience_match(true)
+                    .set_other_audience_verifier_fn(|aud| {
+                        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))?;
-
         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))?;
-
         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)?;
-
         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();
             }
         }
-
         Ok(data)
     }
 }
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Dominik Csapak]

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