Add support for the TLS registry to OIDC and OIDC client extensions

[michalvavrik]

• Closes: Add support for the TLS registry to the OIDC Common HTTP client #41001

[github-actions]

🙈 The PR is closed and the preview is expired.

[sberyozkin]

Hi Michal, @michalvavrik Thanks for this PR, and I'm sure it will work out well, but my immediate concern is that I'm seeing OIDC users are asked to enter one more configuration property and I don't see what is the point. So before me going further with the review, I'd like us agree on it.
As far as I understand, referring to a named TLS configuration should be optional. I can imagine where the same TLS configuration is shared between different Quarkus systems, so referring to it would be nice, but right now, I'm not seeing why we should update all properties to use a named TLS configuration

[michalvavrik]

Hi Michal, @michalvavrik Thanks for this PR, and I'm sure it will work out well, but my immediate concern is that I'm seeing OIDC users are asked to enter one more configuration property and I don't see what is the point. So before me going further with the review, I'd like us agree on it.

Old code still works, I simply c&p original tests and changed config properties, original tests are still in place. Dropping deprecation of config properties and of docs adjustments is matter of minute. I can do that.

As far as I understand, referring to a named TLS configuration should be optional. I can imagine where the same TLS configuration is shared between different Quarkus systems, so referring to it would be nice, but right now, I'm not seeing why we should update all properties to use a named TLS configuration

I deprecated OIDC-specific properties because of this sentence in ADR:

quarkus/adr/0004-using-the-tls-registry-for-clients.adoc

Line 174 in 860900e

The extension-specific configuration should be considered _deprecated_ and users should be invited to use the TLS registry.

So, yes, there can be one extra property in case the TLS registry is only configured for let's say JWT credentials via keystore. On the other hand, OIDC-specific code in place only works for resources and JKS judging by the io.quarkus.oidc.common.runtime.OidcCommonUtils#clientJwtKey. While Quarkus TLS Registry also supports absolute paths and PKCS12.

TL;DR; if you want to keep existing OIDC-specific stuff in place, nothing forces us to deprecate it. I don't have strong opinion, honestly, these are little things. Let me know.

[sberyozkin]

Hi @michalvavrik, thanks, let me get back to you on it, I've been thinking, can we have a convention, like a default oidc tls registry name, say oidc, so that that extra property is implied for simple setups ?

I'd also consider giving an example of the old approach and explain it may be eventually gone.

[cescoffier]

Looks good - I just asked a few questions.

[michalvavrik]

I'd also consider giving an example of the old approach and explain it may be eventually gone.

FYI @sberyozkin , when the old config is deprecated, it disappears from list of configuration properties, so it is either keeping the OIDC-specific properties not deprecated, or removing them from docs. Both does not make sense.

[sberyozkin]

Sorry Michal, all the changes impacting Credentials.JWT should be reverted

[michalvavrik]

Sorry Michal, all the changes impacting Credentials.JWT should be reverted

Well, not that I mind, but I don't understand you. I won't answer individual comments because it makes sense to discuss this at one place. Here it is:

Why cannot user use one shared registry among oidc.credentials.jwt and oidc.tls and possibly others? We require user to specify key id and key id password. I think you are too quick to asking removing this feature why I can't see downside. Could you please suggest what is downside of keeping this extra option? I think there are only upsides, for example, extensions can provide their own TLS config, because TLS registry is extendable.

[michalvavrik]

Sorry Michal, all the changes impacting Credentials.JWT should be reverted

@sberyozkin to quicken this discussion bit and cut down your time, you are basically saying it's called TLS registry not Keystore registry so we cannot use it for this purpose because here, we don't configure TLS, right? Yeah, I don't have answer for that, apart of that I don't care how is it called, it's positive feature.

Let me remove it and wait if there is ever user request for this option.

[sberyozkin]

@michalvavrik Using the private key from the keystore to create a signed authentication token is really nothing to do with TLS or any registry, in fact one of the ideas about using this authentication method is to be able to use plain HTTP since the client secret won't be going on the wire.
Let's just focus on the TLS related aspect

[sberyozkin]

Yeah, thanks @michalvavrik, things like trust all, etc, do not apply in this case. This signed token might travel over MTLS to OIDC but I'd say it would be either MTLS or just plain HTTP plus this token.
FYI, it is a private_kkey_jwt case covered at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication

[michalvavrik]

Yeah, thanks @michalvavrik, things like trust all, etc, do not apply in this case. This signed token might travel over MTLS to OIDC but I'd say it would be either MTLS or just plain HTTP plus this token. FYI, it is a private_kkey_jwt case covered at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication

Copy that, will push changes in couple of minutes.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 46f3904.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.