currentVertxRequest.getCurrent in a SecurityIdentityAugmentor became null since 3.2.9 with GraphQL

[JDoumerc]

Describe the bug

From 3.1.X to 3.2.8, the following SecurityIdentityAugmentor implementation:

@ApplicationScoped
public class SecurityAugmentor implements SecurityIdentityAugmentor {

    @Inject
    private OidcClient oidcClient;

    @Inject
    private OidcIdentityProvider oidcProvider;

    @Inject
    private CurrentVertxRequest currentVertxRequest;

    @Override
    public Uni<SecurityIdentity> augment(SecurityIdentity identity, AuthenticationRequestContext context) {
        if (identity.isAnonymous()) {
            // Not authenticated calls come here, grant "Guest" account from configured Service Account
            return buildGuestIdentity(identity, context);
        }
        return context.runBlocking(QuarkusSecurityIdentity.builder(identity)::build);
    }

    @ActivateRequestContext
    public Uni<SecurityIdentity> buildGuestIdentity(SecurityIdentity identity, AuthenticationRequestContext context) {
        return oidcClient.getTokens(Map.of()).chain(tokens -> {

            RefreshToken rt = new RefreshToken(tokens.getRefreshToken());
            AccessTokenCredential atc = new AccessTokenCredential(tokens.getAccessToken(), rt);

            // Prepare an authentication request to call the OIDC identity provider, or else the current
            // request will be seen has not authenticated
            var tar = new TokenAuthenticationRequest(atc);
            // Save the RountingContext (URL ...) of the current request in the authentication request
            if (currentVertxRequest != null && currentVertxRequest.getCurrent() != null) {
                HttpSecurityUtils.setRoutingContextAttribute(tar, currentVertxRequest.getCurrent());
                return oidcProvider.authenticate(tar, context);
            } else {
                // A query is received on a not functional path
                // Either health check or flood, keep the incoming identity (will result in a 401)
                return Uni.createFrom().item(identity);
            }
            // Let call again Keycloak to get proper authenticated context
        }).onItem().transform(s -> build(s).get());
    }

was able to retrieve a currentVertxRequest.getCurrent() value upon unauthenticated requests on any path of my application.
Then the class was granting a limited right service account authentication to simulate kind of a not authenticated Guest access to all endpoints.

From 3.2.9, the returned current Vertx context is null.
Trying to workaround with RoutingContext injection leads to the following exception (in both POST GraphQL or GET rest http requests):

2023-12-01 15:31:29 [vert.x-eventloop-thread-3] ERROR io.quarkus.vertx.http.runtime.QuarkusErrorHandler - HTTP Request to /graphql failed, error id: da5cd711-ea78-46de-8a53-34b1c1a95605-1
jakarta.enterprise.inject.IllegalProductException: Normal scoped producer method may not return null: io.quarkus.vertx.http.runtime.CurrentVertxRequest.getCurrent()
        at io.quarkus.vertx.http.runtime.CurrentVertxRequest_ProducerMethod_getCurrent_6dc23d16d53ba5c34e1e7b6f54290fd7b9aebd76_Bean.doCreate(Unknown Source)
        at io.quarkus.vertx.http.runtime.CurrentVertxRequest_ProducerMethod_getCurrent_6dc23d16d53ba5c34e1e7b6f54290fd7b9aebd76_Bean.create(Unknown Source)
        at io.quarkus.vertx.http.runtime.CurrentVertxRequest_ProducerMethod_getCurrent_6dc23d16d53ba5c34e1e7b6f54290fd7b9aebd76_Bean.create(Unknown Source)
        at io.quarkus.arc.impl.RequestContext.getIfActive(RequestContext.java:74)
        at io.quarkus.arc.impl.ClientProxies.getDelegate(ClientProxies.java:30)
        at io.vertx.ext.web.CurrentVertxRequest_ProducerMethod_getCurrent_6dc23d16d53ba5c34e1e7b6f54290fd7b9aebd76_ClientProxy.arc$delegate(Unknown Source)
        at io.vertx.ext.web.CurrentVertxRequest_ProducerMethod_getCurrent_6dc23d16d53ba5c34e1e7b6f54290fd7b9aebd76_ClientProxy.put(Unknown Source)
        at io.quarkus.oidc.runtime.OidcIdentityProvider.authenticate(OidcIdentityProvider.java:70)
        at io.quarkus.oidc.runtime.OidcIdentityProvider_ClientProxy.authenticate(Unknown Source)
        at <<<< The security Augmentor on line oidcProvider.authenticate >>>>
        at io.smallrye.context.impl.wrappers.SlowContextualFunction.apply(SlowContextualFunction.java:21)
        at io.smallrye.mutiny.operators.uni.UniOnItemTransformToUni$UniOnItemTransformToUniProcessor.performInnerSubscription(UniOnItemTransformToUni.java:68)
        at io.smallrye.mutiny.operators.uni.UniOnItemTransformToUni$UniOnItemTransformToUniProcessor.onItem(UniOnItemTransformToUni.java:57)
        at io.smallrye.mutiny.operators.uni.UniOnItemTransform$UniOnItemTransformProcessor.onItem(UniOnItemTransform.java:43)
        at io.smallrye.mutiny.operators.uni.UniOperatorProcessor.onItem(UniOperatorProcessor.java:47)
        at io.smallrye.mutiny.operators.uni.UniOperatorProcessor.onItem(UniOperatorProcessor.java:47)
        at io.smallrye.mutiny.vertx.AsyncResultUni.lambda$subscribe$1(AsyncResultUni.java:35)
        at io.smallrye.mutiny.vertx.DelegatingHandler.handle(DelegatingHandler.java:25)
        at io.vertx.ext.web.client.impl.HttpContext.handleDispatchResponse(HttpContext.java:397)
        at io.vertx.ext.web.client.impl.HttpContext.execute(HttpContext.java:384)
        at io.vertx.ext.web.client.impl.HttpContext.next(HttpContext.java:362)
        at io.vertx.ext.web.client.impl.HttpContext.fire(HttpContext.java:329)
        at io.vertx.ext.web.client.impl.HttpContext.dispatchResponse(HttpContext.java:291)
        at io.vertx.ext.web.client.impl.HttpContext.lambda$null$7(HttpContext.java:507)
        at io.vertx.core.impl.ContextInternal.dispatch(ContextInternal.java:277)
        at io.vertx.core.impl.ContextInternal.dispatch(ContextInternal.java:259)
        at io.vertx.core.impl.EventLoopContext.lambda$runOnContext$0(EventLoopContext.java:43)
        at io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:173)
        at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:166)
        at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:470)
        at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:569)
        at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997)
        at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
        at java.base/java.lang.Thread.run(Thread.java:833)

Expected behavior

currentVertxRequest.getCurrent() still returns something when an Http query (REST or GraphQL) has been received and routed to the SecurityAugmentor.

Actual behavior

currentVertxRequest.getCurrent() returns null.
Injection of RutingContext leads to an exception.

How to Reproduce?

Steps to reproduce:

1. Add the proposed security Augmentor to any Quarkus application that handle OIDC authentication
2. Launch a query to an authenticated path of the application without token

Output of uname -a or ver

Windows

Output of java -version

java version "17" 2021-09-14 LTS Java(TM) SE Runtime Environment (build 17+35-LTS-2724) Java HotSpot(TM) 64-Bit Server VM (build 17+35-LTS-2724, mixed mode, sharing)

Quarkus version or git rev

3.2.9.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.8.2

Additional information

The 3.2.9 release note includes some backports (from 3.7) related to RoutingContext, GraphQL (and Vertx), mainly for WebSocket, but I didn't see any direct correlation from those commits that could have lead to this current change of behavior.
Neither did I found any other way to retrieve the RoutingContext in the Tests that were added.

[quarkus-bot]

/cc @sberyozkin (security)

[sberyozkin]

@cescoffier Can it be related to the recent duplicated context fixes ? @michalvavrik AFAIK, the fixes you worked upon did not touch on anything related to the current requests, for 3.2.9 specifcally, the duplicated contexts updates in were done earlier and related to the quarkus-oidc work, this null request problem is on an anonymous identity path.

@JDoumerc Can you check please if currentVertxRequest.getCurrent() is null outside of the chain, for example, what does it return when you call it the very first thing when the augmentor is called ?

[cescoffier]

@sberyozkin no unrelated to that change, or it would have been wrong anyway.

[michalvavrik]

this happens when request context is activated and RoutingContext is request before it is there. Reproducer can't be true, for example this does not happen when proactive authentication is disabled and there is no HTTP Sec policy. I'd suggest it would be easy peasy find out if we had reproducer we could start. I honestly don't know from top of my head what changed in this relation, but what augmentor does is wrong.

[michalvavrik]

ad

From 3.1.X to 3.2.8, the following SecurityIdentityAugmentor implementation

what example does will work only in very special situations. But I don't know GraphQL so maybe it works there. In any case, it can't work with proactive auth or before your stack sets the context. Why don't you take RoutingContext from AuthenticationRequestContext? I'd expect it to be there as you use OIDC.

[JDoumerc]

Thanks for your efforts.

Can you check please if currentVertxRequest.getCurrent() is null outside of the chain, for example, what does it return when you call it the very first thing when the augmentor is called ?

Called on the very first line of the public augment method, it is null also.

Why don't you take RoutingContext from AuthenticationRequestContext? I'd expect it to be there as you use OIDC.

I don't know how to do so. The Interface AuthenticationRequestContext does not provide any other method than the runBlocking, and I have found no sub implementation to cast into nor example to follow on how to retrieve the RoutingContext from it. Any doc/example on this subject is welcome, I could try this way.
Note that the incoming request is not authenticated, the purpose of the class is to provide credentials (with service account when not authenticated).

I haven't precised it but the SecurityAugmentor is implemented in a dedicated Jar (that gots its Jandex index), imported into the GraphQL/Rest server.

There was no issue with 3.2.8 on GET on smallrye-metrics or smallrye-health endpoint or GraphQL queries, it is now happening from the 3.2.9. There first was other configuration changes that I completely reverted (what were the authenticated paths) to be sure it was coming from the Quarkus update.

[michalvavrik]

apologies @JDoumerc you are right, in augmentor you can't take it from context. However you should be able to take it from identity like this

quarkus/integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/SecurityEventListener.java

Line 18 in 0686575

RoutingContext vertxContext = event.getSecurityIdentity()

There was no issue with 3.2.8 on GET on smallrye-metrics or smallrye-health endpoint or GraphQL queries, it is now happening from the 3.2.9. There first was other configuration changes that I completely reverted (what were the authenticated paths) to be sure it was coming from the Quarkus update.

alright, do you think you can create some reproducer because trying to reproduce it from issue description would take a lot of effort (try many combinations). you described it as easy, so please push it to github and I'll have a look what changed between 3.2.8 and 3.2.9

@JDoumerc thanks for reporting it

[sberyozkin]

Thanks @cescoffier @michalvavrik for the early feedback on Friday afternoon :-). Michal, that property will likely not work in the anonymous identity case.

@JDoumerc Can you please come up with a simple reproducer as Michal asked ? Perhaps you can try to register a custom HttpAuthenticationMechanim to deal with anonymous cases

[sberyozkin]

@JDoumerc By the way, did it work in 3.2.8 ?

[michalvavrik]

Michal, that property will likely not work in the anonymous identity case.

I was wrong.

[JDoumerc]

Thanks you all for your help.

I made several tests and share the results with you all:

• first I was also wrong : it looks like granting account never worked on standard http REST calls (no RoutingContext)
• it was working on GraphQL queries up to the 3.2.8 included:
• printing stacktraces up to the SecurityAugmentor.augment pointed out a difference, some calls to the SmallRyeGraphQLAbstractHandler and to the SmallRyeGraphQLOverWebSocketHandler present in 3.2.8 are not appearing anymore in 3.2.9, so it might be related to the WebSocket handling.

I attached the whole stack trace on a GraphQL call.
stack_3.2.9_GraphQL_query_not_authenticated.txt
stack_3.2.8._GraphQL_query_not_authenticated.txt

The stacktraces of the REST call do not differ between 3.2.8 and 3.2.9 (and that is also why I checked not authenticated REST on another server without GraphQL).

The not anymore called entries:

at io.vertx.ext.web.impl.RoutingContextImplBase.iterateNext(RoutingContextImplBase.java:177)
at io.vertx.ext.web.impl.RoutingContextImpl.next(RoutingContextImpl.java:144)
at io.quarkus.smallrye.graphql.runtime.SmallRyeGraphQLOverWebSocketHandler.doHandle(SmallRyeGraphQLOverWebSocketHandler.java:68)
at io.quarkus.smallrye.graphql.runtime.SmallRyeGraphQLAbstractHandler.handleWithIdentity(SmallRyeGraphQLAbstractHandler.java:95)
at io.quarkus.smallrye.graphql.runtime.SmallRyeGraphQLAbstractHandler.handle(SmallRyeGraphQLAbstractHandler.java:76)
at io.quarkus.smallrye.graphql.runtime.SmallRyeGraphQLAbstractHandler.handle(SmallRyeGraphQLAbstractHandler.java:30)
at io.vertx.ext.web.impl.RouteState.handleContext(RouteState.java:1286)

I am wondering if I wasn't somehow using a hack using the SecurityAugmentor (well I needed it and I will have to keep it for roles augmentations as proposed in the documented example).

I still don't get why GraphQL queries ends calling the augment method of the SecurityAugmentor, when REST queries don't. And if it is expected to be called, how could a Context (perhaps a GraphQL Context ?) with routing information for calling and OIDC provider could be retrieved/built ?

I keep in mind that using the HttpAuthenticationMechanism sounds like the correct way to do whether if it might be more work. Thanks for the tip.

[sberyozkin]

@JDoumerc Thanks, I've updated the issue title to reflect it is happening on the GraphQL path, CC @jmartisk

[cescoffier]

Oh! We fixed something related in graphql. That might be a consequence of this fix.

[jmartisk]

This is because the websocket handler is now running AFTER the SecurityIdentityAugmentor. The regular handler (for non-websocket requests) runs even later.
One of the handlers (websocker or regular) has to set the CurrentVertxRequest, but because now they both run after the augmentor, the augmentor doesn't see it.
If this worked in 3.2.8, it was basically a coincidence because the current request was set by the websocket handler (even for non-websocket requests).

So how should we fix it? Either the SecurityIdentityAugmentor needs to be changed to have the CurrentVertxRequest set by itself and not rely on some handler to add it,
or we could add a third GraphQL handler that runs very early and sets the CurrentVertxRequest... Or something else?!

[michalvavrik]

So how should we fix it? Either the SecurityIdentityAugmentor needs to be changed to have the CurrentVertxRequest set by itself and not rely on some handler to add it,

This can't be done with SecurityIdentityAugmentor as this library has no knowledge of Vert.x, we could add some properties map, but what I'd like to do for a long time now is to put this to local Vert.x context data. This would do for Vert.x HTTP as it runs on duplicated context. Does GraphQL also run on duplicated context?

I didn't do it yet, as I thought there must be a good reason why it is not there. Can @cescoffier provide some reasons why shouldn't we do it?

or we could add a third GraphQL handler that runs very early and sets the CurrentVertxRequest... Or something else?!

This is not problem only for GraphQL, users keep activating CDI request context and that breaks what RESTEasy Reactive does etc.

[michalvavrik]

I should mention there are other issues that require RoutingContext in augmentor, I remember at least one with mTLS, but I saw it in reproducers of other issues as well.

[michalvavrik]

CurrentVertxRequest set by itself

That can be done, but the problem is activation of CDI request context as a pattern unless there will be changes in RESTEasy Reactive.