Re: [RFC datacenter-manager 1/1] fix #7135: openid auth: improve error logging

["Nicolas Frey" <n.frey@proxmox.com>]

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=7135

> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
> 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=7135
> [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/openid.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<String, Error> {
> -    let (domains, _digest) = pdm_config::domains::config()?;
> -    let config: OpenIdRealmConfig = domains.lookup("openid", &realm)?;
> +    let url_result: Result<String, Error> = try_block!({
> +        let (domains, _digest) = pdm_config::domains::config()?;
> +        let config: OpenIdRealmConfig = domains.lookup("openid", &realm)?;
>
> -    let open_id = openid_authenticator(&config, &redirect_url)?;
> +        let open_id = openid_authenticator(&config, &redirect_url)?;
>
> -    let url = 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 <n.frey@proxmox.com>