From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 967171FF0EA for ; Thu, 13 Aug 2026 11:27:55 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 48C05214EB; Thu, 13 Aug 2026 11:27:55 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 13 Aug 2026 11:27:50 +0200 Message-Id: Subject: Re: [RFC datacenter-manager 1/1] fix #7135: openid auth: improve error logging From: "Nicolas Frey" To: "Thomas Ellmenreich" , X-Mailer: aerc 0.20.0 References: <20260812084615.56044-1-t.ellmenreich@proxmox.com> In-Reply-To: <20260812084615.56044-1-t.ellmenreich@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1786613254251 X-SPAM-LEVEL: Spam detection results: 0 AWL 1.286 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: L3JPX46LZ27FHXTBU2PZHLEB7BEMMWKZ X-Message-ID-Hash: L3JPX46LZ27FHXTBU2PZHLEB7BEMMWKZ X-MailFrom: n.frey@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: hi, thanks for the patch! some comments inline On Wed Aug 12, 2026 at 10:46 AM CEST, Thomas Ellmenreich wrote: > Improve logging when getting the authorization URL fails. Previously, > the details of the error were completly swallowed, making it difficult > to diagnose problems connecting to the OpenID Connect Server. Now, the > error is logged on the server side to make debugging easier. > Consider adding a Fixes trailer, referencing the bug like so: Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=3D7135 > Signed-off-by: Thomas Ellmenreich > --- > This patch fixes this: [1] Bugzilla issue, although I'm not > completely sure on the implementation. In the issues discussion, an > improvement of the the error sent to the client is proposed, but I > don't think that to be the correct approach. agree, the OICD being misconigured is a concern of the admin that sets it up, not the end user > > To avoid sharing unnecessary information with the client [2], I > instead recommend logging the failed retrieval of an authorization > URL on the server, especially since, in the case of this issue, only > an admin during setup should need this information. > > That said, the error logged with this current implementation can be > very verbose, as seen in the example below, although I still find it > useful to understand the state of things. > personally I'd rather see a verbose error message than none at all > Options Explored > --------------- > > - I considered only providing more information to the client for > certain error cases, but our use of the Anyhow crate makes such a > solution a bigger intervention, which I did not find adequate. > > - Rather than the current solution, I thought there might be a way to > automatically log an error via the api macro. I looked around, but I > could not find anything similar. > > How I tested: > ----------- > > I spun up a Keycloak instance through Docker and added it as an > OpenId Connect Server to my PDM instance. Since I knowingly > misconfigured it with https instead of http, the attempt to login > then logged the following error (after the patch): > > ``` > # Line breaks added for readability > > ... proxmox-datacenter-privileged-api[616]: could not get opneid auth url= : > Request failed: ureq request failed - native-tls: error:0A00010B:SSL > routines:tls_validate_record_header:wrong version number: > ../ssl/record/methods/tlsany_meth.c:77:: native-tls: error:0A00010B:SSL > routines:tls_validate_record_header:wrong version number: > ../ssl/record/methods/tlsany_meth.c:77: > ``` > > [1]: https://bugzilla.proxmox.com/show_bug.cgi?id=3D7135 > [2]: https://cheatsheetseries.owasp.org/cheatsheets/Error_Handling_Cheat_= Sheet.html > > server/src/api/access/openid.rs | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/server/src/api/access/openid.rs b/server/src/api/access/open= id.rs > index 5048fde3..d5308045 100644 > --- a/server/src/api/access/openid.rs > +++ b/server/src/api/access/openid.rs > @@ -271,14 +271,16 @@ pub fn openid_auth_url( > redirect_url: String, > _rpcenv: &mut dyn RpcEnvironment, > ) -> Result { > - let (domains, _digest) =3D pdm_config::domains::config()?; > - let config: OpenIdRealmConfig =3D domains.lookup("openid", &realm)?; > + let url_result: Result =3D try_block!({ > + let (domains, _digest) =3D pdm_config::domains::config()?; > + let config: OpenIdRealmConfig =3D domains.lookup("openid", &real= m)?; > > - let open_id =3D openid_authenticator(&config, &redirect_url)?; > + let open_id =3D openid_authenticator(&config, &redirect_url)?; > > - let url =3D open_id.authorize_url(PDM_RUN_DIR_M!(), &realm)?; > + open_id.authorize_url(PDM_RUN_DIR_M!(), &realm) > + }); > > - Ok(url) > + url_result.inspect_err(|err| log::error!("could not get opneid auth = url: {err:#}")) s/opneid/openid also, why the alternate formatting ("{err:#}") [0] and not just "{err}"? [0] https://doc.rust-lang.org/std/fmt/#sign0 > } > > #[sortable] with the comments taken care of, consider this: Reviewed-by: Nicolas Frey