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

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

[Thomas Ellmenreich]

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.

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.

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.

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:#}"))
 }
 
 #[sortable]
-- 
2.47.3

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

[Nicolas Frey]

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>

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

[Thomas Ellmenreich]

Thanks for having a look ;)

On Thu Aug 13, 2026 at 11:27 AM CEST, Nicolas Frey wrote:

[snip]

> Consider adding a Fixes trailer, referencing the bug like so:
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7135

Ah, yes, thank you very much for pointing this out. It's not the first time
I've forgotten... xC

[snip]

> personally I'd rather see a verbose error message than none at all

Good to hear!

[snip]

> also, why the alternate formatting ("{err:#}") [0] and not just "{err}"?
>
> [0] https://doc.rust-lang.org/std/fmt/#sign0

I thought this error should display as much information as possible, so it
should print all the 'anyhow' contexts. The 'alternate' formatting with '#'
causes 'anyhow' to display all the different contexts [1]. Had I not used
the 'alternate' formatting, the error I included would have been reduced to:

```
... proxmox-datacenter-privileged-api[616]: could not get opneid auth url: Request failed
```

Which is also exactly what is currently sent to the client.

[1]: https://github.com/dtolnay/anyhow/blob/bf3ed9149f4334c984c1ad252b534107b307078c/src/fmt.rs#L10-L15

[snip]

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

[Nicolas Frey]

On Thu Aug 13, 2026 at 1:02 PM CEST, Thomas Ellmenreich wrote:
> Thanks for having a look ;)
>
> On Thu Aug 13, 2026 at 11:27 AM CEST, Nicolas Frey wrote:
>
> [snip]
>
>> Consider adding a Fixes trailer, referencing the bug like so:
>> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7135
>
> Ah, yes, thank you very much for pointing this out. It's not the first time
> I've forgotten... xC
>
> [snip]
>
>> personally I'd rather see a verbose error message than none at all
>
> Good to hear!
>
> [snip]
>
>> also, why the alternate formatting ("{err:#}") [0] and not just "{err}"?
>>
>> [0] https://doc.rust-lang.org/std/fmt/#sign0
>
> I thought this error should display as much information as possible, so it
> should print all the 'anyhow' contexts. The 'alternate' formatting with '#'
> causes 'anyhow' to display all the different contexts [1]. Had I not used
> the 'alternate' formatting, the error I included would have been reduced to:
>
> ```
> ... proxmox-datacenter-privileged-api[616]: could not get opneid auth url: Request failed
> ```
>
> Which is also exactly what is currently sent to the client.
>
> [1]: https://github.com/dtolnay/anyhow/blob/bf3ed9149f4334c984c1ad252b534107b307078c/src/fmt.rs#L10-L15
>
> [snip]

Ah, thanks for the explanation! I think it might be good to include
that rationale in the commit message (i.e. why we want to have such
verbose logging here)