Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failed auth with blocking authentication & a custom authentication failure mapper cause HTTP/2 error #34912

Closed
kdubb opened this issue Jul 21, 2023 · 9 comments · Fixed by #34963
Labels
area/security kind/bug Something isn't working
Milestone

Comments

@kdubb
Copy link
Contributor

kdubb commented Jul 21, 2023

Describe the bug

When the following combination is enabled:

  1. The client request is HTTP/2
  2. quarkus.smallrye-jwt.blocking-authentication is true
  3. Authentication is enabled (e.g. SmallRye JWT)
  4. Authentication throws exception

The request fails with Request has already been read and results in a 500 response instead of the mapped exception response.

Expected behavior

HTTP/2 connections correctly support blocking authentication and authentication exception mappers.

Actual behavior

An exception is thrown with Request has already been read and a 500 response is returned instead of the response returned by the exception mapper.

How to Reproduce?

blocking-auth-mapper-fail-2.16.8.zip
blocking-auth-mapper-fail-3.2.1.zip

Running the included test reproduces the issue in both 2.16 and 3.2 series.

Output of uname -a or ver

macOS 13.4

Output of java -version

Java 17.0.4

GraalVM version (if different from Java)

No response

Quarkus version or git rev

3.2.1 & 2.16.8

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

Gradle 8.1

Additional information

No response

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 21, 2023

/cc @FroMage (resteasy-reactive), @Sgitario (resteasy-reactive), @geoand (resteasy-reactive), @pedroigor (bearer-token), @sberyozkin (bearer-token,jwt,security), @stuartwdouglas (resteasy-reactive)

@Sgitario
Copy link
Contributor

To give better context to this issue, this is the stacktrace I'm seeing:

Unhandled exception in router
java.lang.IllegalStateException: Request has already been read
        at io.vertx.core.http.impl.Http2ServerRequest.checkEnded(Http2ServerRequest.java:217)
        at io.vertx.core.http.impl.Http2ServerRequest.pause(Http2ServerRequest.java:269)
        at io.quarkus.vertx.http.runtime.ResumingRequestWrapper.pause(ResumingRequestWrapper.java:30)
        at io.vertx.core.http.impl.HttpServerRequestWrapper.pause(HttpServerRequestWrapper.java:67)
        at org.jboss.resteasy.reactive.server.vertx.VertxResteasyReactiveRequestContext.<init>(VertxResteasyReactiveRequestContext.java:83)
        at io.quarkus.resteasy.reactive.server.runtime.QuarkusResteasyReactiveRequestContext.<init>(QuarkusResteasyReactiveRequestContext.java:31)
        at io.quarkus.resteasy.reactive.server.runtime.ResteasyReactiveRecorder$5.createContext(ResteasyReactiveRecorder.java:175)
        at org.jboss.resteasy.reactive.server.handlers.RestInitialHandler.beginProcessing(RestInitialHandler.java:52)
        at io.quarkus.resteasy.reactive.server.runtime.ResteasyReactiveRecorder$7.handle(ResteasyReactiveRecorder.java:240)
        at io.quarkus.resteasy.reactive.server.runtime.ResteasyReactiveRecorder$7.handle(ResteasyReactiveRecorder.java:225)
        at io.vertx.ext.web.impl.RouteState.handleFailure(RouteState.java:1290)
        at io.vertx.ext.web.impl.RoutingContextImplBase.iterateNext(RoutingContextImplBase.java:174)
        at io.vertx.ext.web.impl.RoutingContextImpl.next(RoutingContextImpl.java:141)
        at io.vertx.ext.web.impl.RoutingContextImpl.doFail(RoutingContextImpl.java:548)
        at io.vertx.ext.web.impl.RoutingContextImpl.fail(RoutingContextImpl.java:197)
        at io.vertx.ext.web.impl.RoutingContextImpl.fail(RoutingContextImpl.java:186)
        at io.quarkus.vertx.http.runtime.security.HttpSecurityRecorder$AbstractAuthenticationHandler$1.proceed(HttpSecurityRecorder.java:395)
        at io.quarkus.vertx.http.runtime.security.HttpSecurityRecorder$DefaultAuthFailureHandler$1.accept(HttpSecurityRecorder.java:303)
        at io.quarkus.vertx.http.runtime.security.HttpSecurityRecorder$DefaultAuthFailureHandler$1.accept(HttpSecurityRecorder.java:299)
        ....
        at io.quarkus.vertx.http.runtime.security.HttpSecurityRecorder$DefaultAuthFailureHandler.accept(HttpSecurityRecorder.java:299)
        at io.quarkus.vertx.http.runtime.security.HttpSecurityRecorder$DefaultAuthFailureHandler.accept(HttpSecurityRecorder.java:285)
        at io.quarkus.vertx.http.runtime.security.HttpSecurityRecorder$AbstractAuthenticationHandler$3.onFailure(HttpSecurityRecorder.java:463)

The problem is that after getting a response with auth failures, RESTEasy Reactive tries to customise the response (see this line), and for doing it, it starts a new context (see here) and pauses the already ended request in here which is fine for HTTP/1.1, but not for HTTP/2 (Vert.x throws the above exception).

I'm not very familiar with the above flow, but at first, I don't follow why RESTEasy Reactive creates a new context with the already ended request to customize the response, which I think it's the issue. @michalvavrik can you clarify this?

If the above is done on purpose, then we should somehow reset the HTTP/2 connection in VertxResteasyReactiveRequestContext or RoutingContext.request() (which provides the request).

cc @michalvavrik @geoand @sberyozkin

By the way, I had to modify the test from the reproducer to assert that the expected status code is 401, not 200.

@michalvavrik
Copy link
Member

michalvavrik commented Jul 24, 2023

The problem is that after getting a response with auth failures, RESTEasy Reactive tries to customise the response (see this line), and for doing it, it starts a new context (see here) and pauses the already ended request in here which is fine for HTTP/1.1, but not for HTTP/2 (Vert.x throws the above exception).

I'm not very familiar with the above flow, but at first, I don't follow why RESTEasy Reactive creates a new context with the already ended request to customize the response, which I think it's the issue. @michalvavrik can you clarify this?

Next context should only be started if RESTEasy Reactive didn't start yet processing. The fact that it is happening after

signal bug in that auth failure handler should not be applied and exception should be handled in RESTEasy Reactive fail chain (=we don't catch exception then abort chain handlers apply exception mapper IIRC).

@Sgitario ^^ I only base on your analysis. I'll need to debug this myself to tell more, but proper fix should be avoiding default auth failure handler. I'll have a look this week, unless you want to handle it yourself.

@michalvavrik
Copy link
Member

michalvavrik commented Jul 24, 2023

Basically, my expectation is to remove default auth failure handler from routing context inside same Vert.X route handler that begin processing, which does a trick. And only start new context when Routing context was failed before RR created one.

@michalvavrik
Copy link
Member

michalvavrik commented Jul 24, 2023

Ah, I think I understand you now. So the problem is that I create context and then invoke handleException?

public void beginProcessing(Object externalHttpContext, Throwable throwable) {
        ResteasyReactiveRequestContext rq = requestContextFactory.createContext(deployment, externalHttpContext,
                requestContext,
                initialChain, deployment.getAbortHandlerChain());
        rq.handleException(throwable);
        rq.run();
    }

If not, I'll leave any other comment till I have time to look so that I don't waste your time.

@Sgitario
Copy link
Contributor

Ah, I think I understand you now. So the problem is that I create context and then invoke handleException?

public void beginProcessing(Object externalHttpContext, Throwable throwable) {
        ResteasyReactiveRequestContext rq = requestContextFactory.createContext(deployment, externalHttpContext,
                requestContext,
                initialChain, deployment.getAbortHandlerChain());
        rq.handleException(throwable);
        rq.run();
    }

If not, I'll leave any other comment till I have time to look so that I don't waste your time.

Exactly, this is creating a new context with an existing request (which in the case of HTTP/2.0, can't be paused because it's already closed).

Take your time next week to have a look at this flow. I'm not familiar with it, but I'd be rather than happy to help with anything).

@michalvavrik
Copy link
Member

Thanks, will do.

@michalvavrik
Copy link
Member

FYI it seems like a general problem when exception is thrown inside this

blocking executor and HTTP2 is used, because I can't see any part of our code that results directly in ending HTTP2 request. When I replaced
return context.runBlocking(() -> createSecurityIdentity(request));
with

final io.vertx.core.Context ctx = Vertx.currentContext();
            return Uni.createFrom()
                    .completionStage(ctx
                            .<SecurityIdentity> executeBlocking(tPromise -> tPromise.complete(createSecurityIdentity(request)))
                            .toCompletionStage());

reproducer was fixed. IMHO we should provide similar executor for IdentityProviderManager on Vert.X HTTP level. I'll have to verify what would such change mean and whether it is viable option. In the meanwhile, @Sgitario I don't see this as RESTEasy Reactive bug. Please feel free to prove me wrong. Thanks

also cc @sberyozkin

@Sgitario
Copy link
Contributor

FYI it seems like a general problem when exception is thrown inside this

blocking executor and HTTP2 is used, because I can't see any part of our code that results directly in ending HTTP2 request. When I replaced

return context.runBlocking(() -> createSecurityIdentity(request));

with

final io.vertx.core.Context ctx = Vertx.currentContext();
            return Uni.createFrom()
                    .completionStage(ctx
                            .<SecurityIdentity> executeBlocking(tPromise -> tPromise.complete(createSecurityIdentity(request)))
                            .toCompletionStage());

reproducer was fixed. IMHO we should provide similar executor for IdentityProviderManager on Vert.X HTTP level. I'll have to verify what would such change mean and whether it is viable option. In the meanwhile, @Sgitario I don't see this as RESTEasy Reactive bug. Please feel free to prove me wrong. Thanks

also cc @sberyozkin

I'm sure you're totally right. Removing the resteasy-reactive label. Thanks for checking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants