IDP: avoid logging value of 'idp_client_secret'

[alexey-tikhonov]

No description provided.

[gemini-code-assist]

Code Review

This pull request aims to prevent logging the sensitive idp_client_secret value. The changes introduce a specific check in dp_get_options to avoid printing the secret's value, which is a good security improvement. The PR also refactors the debug logging level for option processing to a more appropriate level and consistently uses a macro for the secret's key. However, the fix is incomplete as a similar logging vulnerability remains in another function within the same file, which could still lead to secret leakage. This critical issue needs to be addressed.

[sumit-bose]

Hi,

thank you for the patch. Have you considered to change the type of the option from DP_OPT_STRING to DP_OPT_BLOB to avoid logging the value as we already do e.g. for ldap_default_authtok?

bye,
Sumit

[alexey-tikhonov]

Have you considered to change the type of the option from DP_OPT_STRING to DP_OPT_BLOB to avoid logging the value as we already do e.g. for ldap_default_authtok?

I did.
But IIUC this would allow to configure non-ascii characters in the value and wasn't sure if 'oidc_child' would need additional filtering then.

[sumit-bose]

Have you considered to change the type of the option from DP_OPT_STRING to DP_OPT_BLOB to avoid logging the value as we already do e.g. for ldap_default_authtok?

I did. But IIUC this would allow to configure non-ascii characters in the value and wasn't sure if 'oidc_child' would need additional filtering then.

I thought it is only about \0 in the value, DP_OPT_STRING would allow non-ascii like UTF-8 as well?

While talking about oidc_child, can you fix the logging in read_client_secret_from_stdin() as well?

Thanks.

bye,
Sumit

[alexey-tikhonov]

Have you considered to change the type of the option from DP_OPT_STRING to DP_OPT_BLOB to avoid logging the value as we already do e.g. for ldap_default_authtok?

I did. But IIUC this would allow to configure non-ascii characters in the value and wasn't sure if 'oidc_child' would need additional filtering then.

I thought it is only about \0 in the value, DP_OPT_STRING would allow non-ascii like UTF-8 as well?

The questions is: what should be the type of this option from idp-provider/oidc_child point of view?
I thought it was 'string', not 'blob'.
If that's the case I still think that dp_get_options() needs to be fixed, not type of the option changed just because this would change logging behavior of some utility function (this is fragile - nothing will prevent changing this utility function to log blob values as well later).

A better solution would be introduction of a new 'DP_OPT_SENSITIVE_STRING' that would have proper d-tor and serve as a reminder "do not log me", but this would be much more invasive.

As to ldap_default_authtok, I thought it has BLOB type because it can have type 'obfuscated_password' and this is really a set of random bytes, no? I might be wrong here.

While talking about oidc_child, can you fix the logging in read_client_secret_from_stdin() as well?

Done.