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

Non-JWT QuarkusPrincipal injected into JsonWebToken field #34897

Closed
martinoneutrino opened this issue Jul 20, 2023 · 17 comments
Closed

Non-JWT QuarkusPrincipal injected into JsonWebToken field #34897

martinoneutrino opened this issue Jul 20, 2023 · 17 comments
Labels

Comments

@martinoneutrino
Copy link

Describe the bug

I have created a custom HTTP Authorization mechanism (from HttpAuthenticationMechanism, UsernamePasswordAuthenticationRequest and IdentityProvider), which is to be used in the same service that uses both Basic and JWT authorization (ie. for an administration endpoint that uses custom/non-Basic authorization).

However, since Quarkus 3.0, an injected JsonWebToken field in a service will be assigned the QuarkusPrincipal provided by the custom authorization, resulting in the error: java.lang.IllegalStateException: Current principal io.quarkus.security.runtime.QuarkusPrincipal is not a JSON web token. The error also occurs when injecting a field with a Claim annotation.

Expected behavior

The injected JsonWebToken value and any Claim fields are null, since no Authorization: Bearer (JWT) header was provided.

Actual behavior

The following error occurs when the JsonWebToken field is accessed (or Claim is decoded):

ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (executor-thread-1) HTTP Request to /secured/permit-all failed, error id: 9dbdf911-bc9e-46ee-99f7-cae7c4a3f32e-1: java.lang.RuntimeException: Error injecting java.lang.String org.acme.security.jwt.TokenSecuredResource.birthdate
        at org.acme.security.jwt.TokenSecuredResource_Bean.doCreate(Unknown Source)
        at org.acme.security.jwt.TokenSecuredResource_Bean.create(Unknown Source)
        at org.acme.security.jwt.TokenSecuredResource_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 org.acme.security.jwt.TokenSecuredResource_ClientProxy.arc$delegate(Unknown Source)
        at org.acme.security.jwt.TokenSecuredResource_ClientProxy.hello(Unknown Source)
        at org.acme.security.jwt.TokenSecuredResource$quarkusrestinvoker$hello_67032e9948ffec8c1b8bdb973e787b16f99d28e7.invoke(Unknown Source)
        at org.jboss.resteasy.reactive.server.handlers.InvocationHandler.handle(InvocationHandler.java:29)
        at io.quarkus.resteasy.reactive.server.runtime.QuarkusResteasyReactiveRequestContext.invokeHandler(QuarkusResteasyReactiveRequestContext.java:141)
        at org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext.run(AbstractResteasyReactiveContext.java:145)
        at io.quarkus.vertx.core.runtime.VertxCoreRecorder$14.runWith(VertxCoreRecorder.java:576)
        at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2513)
        at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1538)
        at org.jboss.threads.DelegatingRunnable.run(DelegatingRunnable.java:29)
        at org.jboss.threads.ThreadLocalResettingRunnable.run(ThreadLocalResettingRunnable.java:29)
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
        at java.base/java.lang.Thread.run(Thread.java:834)
Caused by: java.lang.IllegalStateException: Current principal io.quarkus.security.runtime.QuarkusPrincipal@3c3134e is not a JSON web token
        at io.quarkus.smallrye.jwt.runtime.auth.JwtPrincipalProducer.currentJWTPrincipalOrNull(JwtPrincipalProducer.java:36)
        at io.quarkus.smallrye.jwt.runtime.auth.JwtPrincipalProducer_ProducerMethod_currentJWTPrincipalOrNull_cfb1303ffa51cd11e1f530c7fecb6216b77f5bb8_Bean.doCreate(Unknown Source)
        at io.quarkus.smallrye.jwt.runtime.auth.JwtPrincipalProducer_ProducerMethod_currentJWTPrincipalOrNull_cfb1303ffa51cd11e1f530c7fecb6216b77f5bb8_Bean.create(Unknown Source)
        at io.quarkus.smallrye.jwt.runtime.auth.JwtPrincipalProducer_ProducerMethod_currentJWTPrincipalOrNull_cfb1303ffa51cd11e1f530c7fecb6216b77f5bb8_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 org.eclipse.microprofile.jwt.JwtPrincipalProducer_ProducerMethod_currentJWTPrincipalOrNull_cfb1303ffa51cd11e1f530c7fecb6216b77f5bb8_ClientProxy.arc$delegate(Unknown Source)
        at org.eclipse.microprofile.jwt.JwtPrincipalProducer_ProducerMethod_currentJWTPrincipalOrNull_cfb1303ffa51cd11e1f530c7fecb6216b77f5bb8_ClientProxy.claim(Unknown Source)
        at io.smallrye.jwt.auth.cdi.RawClaimTypeProducer.getClaimAsString(RawClaimTypeProducer.java:62)
        at io.smallrye.jwt.auth.cdi.RawClaimTypeProducer_ProducerMethod_getClaimAsString_1413fbcd36c84912439659fac5da1bcac3671d96_Bean.doCreate(Unknown Source)
        at io.smallrye.jwt.auth.cdi.RawClaimTypeProducer_ProducerMethod_getClaimAsString_1413fbcd36c84912439659fac5da1bcac3671d96_Bean.create(Unknown Source)
        at io.smallrye.jwt.auth.cdi.RawClaimTypeProducer_ProducerMethod_getClaimAsString_1413fbcd36c84912439659fac5da1bcac3671d96_Bean.get(Unknown Source)
        at io.smallrye.jwt.auth.cdi.RawClaimTypeProducer_ProducerMethod_getClaimAsString_1413fbcd36c84912439659fac5da1bcac3671d96_Bean.get(Unknown Source)
        at io.quarkus.arc.impl.CurrentInjectionPointProvider.get(CurrentInjectionPointProvider.java:48)
        ... 18 more

How to Reproduce?

Reproducer: IllegalStateException-not-a-JSON-web-token

Adds three classes to security-jwt-quickstart project:
AdminAuthenticationMechanism, UsernamePasswordAuthenticationRequest, AdminRoleIdentityProvider.

Run security-jwt-quickstart project and send a request to any endpoint (ie. localhost:8080/secured/permit-all), and include header: Authorization: Admin ANYDATA

Output of uname -a or ver

Microsoft Windows [Version 10.0.19045.3208]

Output of java -version

openjdk version "11.0.7"

GraalVM version (if different from Java)

No response

Quarkus version or git rev

3.2.1

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

Apache Maven 3.8.6

Additional information

No response

@martinoneutrino martinoneutrino added the kind/bug Something isn't working label Jul 20, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 20, 2023

/cc @sberyozkin (jwt)

@michalvavrik michalvavrik removed the env/windows Impacts Windows machines label Jul 20, 2023
@michalvavrik
Copy link
Member

I'll check what changed tomorrow unless @sberyozkin knows or says this is intentional.

Workaround (probably obvious to you already) should be to have JWT and claim injected into different bean and programmatically access it only from endpoints authenticated with JWT.

@sberyozkin
Copy link
Member

I don't recall anything changing in this regard in 3.0, but great the reproducer is available

@michalvavrik
Copy link
Member

  • I can't see any changes in directly related code
  • I tested it with 2.16.8.Final and 2.13.8.Final and same behavior is there as well :-)

So

@sberyozkin
Copy link
Member

sberyozkin commented Jul 21, 2023

@michalvavrik Thanks for checking.

The reason I might've called that method currentJWTPrincipalOrNull is that indeed I meant to return null and then finding it can't work and instead fixing it by returning NullJsonWebToken :-)

As far as the individual claims injection is concerned, which can be injected as String for example, indeed, I think it can't work by just returning null.

@martinoneutrino what usecase do you have in mind ? If the request is made to the endpoint which expects to interact with the injected JsonWebToken, then creating a custom authenticated non JWT principal and expecting that code to work does not sound right.

I wonder if this issue may be invalid. I think you should inject SecurityIdentity if you'd like this endpoint interact with alternative principals and then, if needed, check a principal instance type and cast as needed

@michalvavrik
Copy link
Member

+1

@michalvavrik
Copy link
Member

I wonder if this issue may be invalid. I think you should inject SecurityIdentity if you'd like this endpoint interact with alternative principals and then, if needed, check a principal instance type and cast as needed

Or you can just have it injected only to resources that actually are authenticated with JWT auth mechanism or as I mentioned Arc.container().... for bean that has injected claims etc.

@martinoneutrino
Copy link
Author

I tested it with 2.16.8.Final and 2.13.8.Final and same behavior is there as well :-)

  • description seems incorrect, which Quarkus version is not affected?

@michalvavrik I only encountered it after I upgraded some services from 2.16.3.Final to 3.1 and later. So apparently my assumption on the cause was incorrect.
I'm going back and reviewing more since I also upgraded some of my own libraries as part of going to Quarkus 3.0+ (mostly just jakarta.* naming though).

  • what usecase do you have in mind ? If the request is made to the endpoint which expects to interact with the injected JsonWebToken, then creating a custom authenticated non JWT principal and expecting
    that code to work does not sound right.

@sberyozkin My use case is a bit more complex than just an endpoint. I have a ContainerRequestFilter that extracts user ID from the JWT that I then use in a custom AgroalPoolInterceptor. This is part of a custom library, so I don't check for paths/endpoints and only want to just add the Admin role (which the endpoint then uses).

@sberyozkin
Copy link
Member

@martinoneutrino Thanks, adding a role in a custom filter should not be a problem, should work for any security identity. The problem is, a non-JWT principal coming into the endpoint which expects a JWT one, which is why I thought having a more generic identity representation injected should help.

@martinoneutrino
Copy link
Author

I tested and it still shows the same error running in Quarkus 1.x, so I'm not sure how this was ever working for me. I'll keep looking...

@martinoneutrino
Copy link
Author

After further testing and research, I think the correct thing is for JwtPrincipalProducer's currentJWTPrincipalOrNull to return NullJsonWebToken in all cases when this check fails: identity.getPrincipal instanceof JsonWebToken.
I can't see a logical reason why an anonymous Principal (no login provided) should give get a NullJsonWebToken, and yet another mechanism providing an authenticated Principal should raise an IllegalStateException. In both cases, there is no JWT, so the NullJsonWebToken is appropriate.

@sberyozkin
Copy link
Member

@martinoneutrino Thanks - but then it would make it even more difficult to identify the problem in the way the application has been designed, i.e, when a valid non-JWT principal is passed to the endpoint - which thinks it should still access an injected JsonWebToken as part of this call - which is an application level bug.

IMHO this issue has to be closed as invalid one, cheers

@martinoneutrino
Copy link
Author

If so, can we then change it from a throwing an exception to producing a warning?

I don't agree with an anonymous user getting favourable treatment (with a NullJsonWebToken), whereas a non-JWT authenticated user attempting to determine the JWT Principal (if any) gets an exception, instead of receiving the same NullJsonWebToken (which to me indicates the same thing: "there is no JWT").

An application design that mixes authentication schemes and injects JsonWebToken,Claim,etc is obviously going beyond the JWT framework provided (config, annotations, etc) for some specific authentication customization, and I don't think an IllegalStateException is appropriate to signal a potential code or configuration issue. (In my case, I'm in a ContainerRequestFilter designed to access JWT info, and it needs to not blow up when other authentication schemes are in place.)

@tkosse
Copy link

tkosse commented Sep 22, 2023

I think I ran into a similar problem. We are using Basic Auth for our endpoint. When the request is coming from a specific application additionally a JWT is present in the request header. If that is the case the userId from that token should be used and added as additional info (next to the Basic Auth user) when tracing the request through the system.

I added the config mp.jwt.token.header=user-token to point to the JWT. When injecting the JsonWebToken in a class and accessing it an exception is thrown java.lang.IllegalStateException: Current principal basic-auth-user is not a JSON web token.

I worked around that by manually getting the header from the ContainerRequestContext and if present parsing it with a JWTParser instance.

@martinoneutrino
Copy link
Author

I worked around that by manually getting the header from the ContainerRequestContext and if present parsing it with a JWTParser instance.

My current workaround is to catch the IllegalStateException exception when the JsonWebToken is referenced.

@martinoneutrino
Copy link
Author

@sberyozkin Can this IllegalStateException exception be changed to just a warning log?

@sberyozkin
Copy link
Member

@tkosse

I think I ran into a similar problem. We are using Basic Auth for our endpoint. When the request is coming from a specific application additionally a JWT is present in the request header. If that is the case the userId from that token should be used and added as additional info (next to the Basic Auth user) when tracing the request through the system.

I think you are talking about the concept of the inclusive authentication, I have #31329 opened awhile back and just can't get back to it, so I'll try to complete it asap, it is an important one.

Please watch #31329.

@martinoneutrino

Can this IllegalStateException exception be changed to just a warning log?

IMHO this is not a very good idea, if you have a non-JWT principal accessing a code expecting JsonWebToken then something is wrong, as suggested, please use a more generic code like SecurityIdentity or have path specific authentications for different paths or have Instance<JsonWebToken> etc.

IMHO there is nothing else that we can do in this regard

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants