Resolve HttpCredentialTransport dynamically

[sberyozkin]

This PR replaces #22437, Fixes #22404 (which itself is a consequence of #22437 for @FroMage) and Fixes #22206.

The main problem right now is that quarkus-oidc always returns bearer as its transport which immediately conflicts with smallrye-jwt even when quarkus-oidc uses web-app/authorization code only and @FroMage is not the first one who has seen this problem as a few users have tried to use smallrye-jwt alongside quarkus-oidc/web-app since smallrye-jwt covers a few non-OIDC cases.

Splitting quarkus-oidc into quarkus-oidc-service, quarkus-oidc-web-app will solve this particular issue but I'm not sure it is worth it and will likely have side-effects (ex, service and web-app tenants will not be kept in the same map, not clear how to support the hybrid flow, I think it makes sense to keep it all in one extension).

So this PR proposes to resolve HttpCredentialTransport dynamically - it is the only way for quarkus-oidc to correctly represent the current tenant's cred transport as both service and web-app tenants can be combined.

Originally HttpAuthenticationMechanism.getCredentialTransport was only used to fail when more than one auth mechanism with the same cred transport is used. Now it is also used to find the best candidate for a challenge (for ex, if basic and oidc/web-app are used and basic fails then it is guaranteed basic will return the challenge, even without the priority based sorting). It is also now used for a path-specific authentication: https://quarkus.io/guides/security#path-specific-authentication-mechanism.

I've thought about it and IMHO it is totally safe to avoid failing the application if more than authentication mechanism with the same type is used - in reality it can only happen by mistake and documenting that in such cases unexpected authentication failures can happen is OK. It is impossible for example for smallrye-jwt to accidentally return a verified SecurityIdentity which quarkus-oidc/bearer can also verify unless both share the same verification keys by pointing to Keycloak JWK endpoint, etc. The same applies to any combination of the same cred transport mechanisms - it will likely fail but can only succeed consistently if both mechanism share the verification related data.

I'm not sure it is worth continue using a no-arg HttpAuthenticationMechanism.getCredentialTransport (it is deprecated in this PR) - it is just not a correct way to represent the current OIDC tenant expectation and one can easily enough workaround this restriction, example, register a CustomOidcAuthenticationMechanism extending OidcAuthenticationMechanism but returning for ex hybrid as the transport value. IMHO a workaround specific around the combination of OIDC and MP JWT (#22437 => #22404 as a consequence).

Note this PR has no tests yet and getCredentialTransport(RoutingContext) will return Uni - for now I'd like to see if we can agree with Stuart in principle and if yes then I'll proceed with completing the PR
CC @FroMage

[sberyozkin]

@FroMage I can also suggest a workaround, register a CustomOidcAuthenticationMechanism and inject both Jwt and Oidc authentication mechanisms. CustomOidcAuthenticationMechanism should only have a higher priority than these 2 mechanisms. Now in its authenticate - delegate to MpJwt if the expected cookie header is in the context - otherwise - to Oidc.

In getChallenge you can check which mechanism was used with routingContext.get(HttpAuthenticationMechanism.class.getName()); or check the header again.

[FroMage]

I'll have to try this out in my app as soon as I get it to compile again. It's not clear to me if your PR should be enough or if I need your workaround too.

[sberyozkin]

Hi @FroMage, please try it, I'm quite sure the PR will address the problem you are seeing in a generic way; but the woraround should be also viable, we've had a few cases where the default authentication flow in the Quarkus Security does not work for some applications so in those cases registering a custom mechanism, possibly combing some of those provided by Quarkus works. PR should address it though but it will require Stuart's approval

[geoand]

This now needs a rebase

[geoand]

I can't say I know much about Quarkus OIDC, but this looks reasonable to me

[sberyozkin]

@geoand Thanks, I'll try to handle it before CR1

[geoand]

Thanks! Let me know if I can help somehow

[FroMage]

Nope, this doesn't work, and I get:

	Suppressed: java.lang.NullPointerException
		at io.quarkus.vertx.http.runtime.security.HttpAuthenticator.attemptAuthentication(HttpAuthenticator.java:95)
		at io.quarkus.vertx.http.runtime.security.HttpSecurityRecorder$2.handle(HttpSecurityRecorder.java:110)
		at io.quarkus.vertx.http.runtime.security.HttpSecurityRecorder$2.handle(HttpSecurityRecorder.java:60)
		at io.vertx.ext.web.impl.RouteState.handleContext(RouteState.java:1193)

[sberyozkin]

@geoand thanks, sorry I missed your comment :-)

[sberyozkin]

@FroMage It is still a draft, I did some good progress last Friday, see the added integration-tests/smallrye-jwt-oidc-webapp, but one of the tests fails randomly so I'm trying to get to the bottom of it

[geoand]

NP!

[sberyozkin]

@stuartwdouglas, @FroMage This PR is now ready for review, I guess it is too late for 2.7.0.CR1 as it can be considered a bit sensitive. Please see the issue description for the justification for this PR. Summary:

• HttpCredentialTransport is now returned as a Uni item and calculated dynamically - quarkus-oidc can now return a correct tenant-specific transport type. It has not made a big impact as such on this PR, I only had to adjust a bit HttpAuthenticator to deal with it returned as Uni. It will make it easier to combine only OIDC web-app not only with smallrye-jwt, but also use it for the path based authentication.
• I removed the code I added awhile back to HttpCredentialTransport where, given more than one mechanism and the authentication failure, the attempt was made to find the right mechanism to challenge, for ex, we had an issue where with basic and oidc and the basic failing the oidc was asked to challenge. While working in this PR and testing it I've realized that fix was not complete and does not cover all the variations (it worked by trying to find the mechanism which matches Authorization + scheme - which is not good enough when using JWT cookies for example). Instead, I've updated every shipped mechanism to set itself as a RoutingContext attribute if this mechanism attempts to authenticate as opposed to returning a null/optional Uni. So if it fails then it will be asked to create a challenge first - IMHO that works in all the variations. Otherwise, it is impossible to guarantee that given a failure the right mechanism will be selected to challenge.
• Made the OIDC mechanism of higher priority than JWT/etc. We just have to make some decision, which one takes priority to challenge if no credentials are provided.
• Added docs explaining how to enforce a required challenge order if the default order in a given combination of mechanisms does not work.
• Added integration-tests/smallrye-jwt-oidc-webapp tests
• Made sure IdentityProvider used by OIDC and IdentityProvider used by JWT can only be used by these respective extensions to avoid any side-effects (ex, I was seeing NPE initially when I was posting a bearer token, JWT Authenticator would accept it and then OIDC IdentityProvider would pick it up)

@FroMage Can you test this PR please to confirm you can now combine JWT and OIDC web-app, and that #22404 can be resolved as well ? Tests confirm that yes but an extra test would help

[sberyozkin]

Ouch, still a random failure persists in Native, cancelled the build

[FroMage]

Lemme try it

[FroMage]

Yes, this works. Thanks.

[FroMage]

Only reviewing as a tester, this works for me.

[sberyozkin]

@FroMage Thanks, I've just fixed the native test failure, about to force push

[sberyozkin]

Stuart, @stuartwdouglas, please review when you get a chance

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 48b7229

Status	Name	Step	Failures	Logs	Raw logs
✖	Gradle Tests - JDK 11 Windows	Build	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

✖ io.quarkus.gradle.devmode.BasicKotlinApplicationModuleDevModeTest.main line 19 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

[sberyozkin]

This test failure is not related

[sberyozkin]

@stuartwdouglas I think we can give this PR a try and merge it and see if it can settle on main ? Please comment when you get a chance

[trunghoangminh]

@sberyozkin @FroMage I'm facing the issue when using quarkus-smallrye-jwt(local JWT) and quarkus-oidc(JWT Keycloak)
=> there is conflicted when @Inject JsonWebToken. The details here is: https://stackoverflow.com/questions/76444711/how-to-inject-jsonwebtoken-when-having-both-quarkus-smallrye-jwt-and-quarkus-oi
Could you please help me on this.