From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 712381FF183 for ; Wed, 4 Jun 2025 12:16:53 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 347F83962F; Wed, 4 Jun 2025 12:17:11 +0200 (CEST) Date: Wed, 4 Jun 2025 12:17:07 +0200 (CEST) From: Alexander Abraham To: Dominik Csapak , Proxmox VE development discussion Message-ID: <1614010157.2219.1749032227712@webmail.proxmox.com> In-Reply-To: <54539fb1-af0e-4da2-b160-e77ee9c8c2b6@proxmox.com> References: <20250206120154.12288-1-a.abraham@proxmox.com> <54539fb1-af0e-4da2-b160-e77ee9c8c2b6@proxmox.com> MIME-Version: 1.0 X-Priority: 3 Importance: Normal X-Mailer: Open-Xchange Mailer v7.10.6-Rev78 X-Originating-Client: open-xchange-appsuite X-SPAM-LEVEL: Spam detection results: 0 AWL 0.098 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [lib.rs, proxmox.com] Subject: Re: [pve-devel] [PATCH proxmox/proxmox-openid] fix #5076: Added extra audience verification checks. X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Superseded by: https://lore.proxmox.com/all/20250603091256.40923-1-a.abraham@proxmox.com/ > Dominik Csapak 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 > > --- > > 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, > > #[serde(skip_serializing_if = "Option::is_none")] > > pub acr_values: Option>, > > + #[serde(skip_serializing_if = "Option::is_none")] > > + pub aud: Option>, > > } > > > > 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 { > > 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