quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.82k stars 2.69k forks source link

JWT Cookie not working in combination with http permission paths #43619

Closed Serkan80 closed 3 weeks ago

Serkan80 commented 1 month ago

Describe the bug

When you have two endpoints, which have different auth mechanisms, like this:

And if you configure these as follows:

# SECURITY
quarkus.http.auth.permission.basic.paths=/auth/token
quarkus.http.auth.permission.basic.policy=authenticated
quarkus.http.auth.permission.basic.auth-mechanism=basic

quarkus.http.auth.permission.bearer.paths=/api/*
quarkus.http.auth.permission.bearer.policy=authenticated
quarkus.http.auth.permission.bearer.auth-mechanism=bearer

# JWT
mp.jwt.token.header=Cookie
mp.jwt.token.cookie=access_token
smallrye.jwt.always-check-authorization=true

Then no matter what you send to /api/* you get a 401.

But however, if you remove this line:

quarkus.http.auth.permission.bearer.auth-mechanism=bearer

Then it accepts cookies, but as well as Basic auth and Authorization bearer.

Furthermore, the REST controllers are annotated with @Authenticated on the /api/* side.

Expected behavior

Only bearer/cookie token should be accepted and basic authentication should not be allowed.

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

Linux

Output of java -version

JDK 17 Temurin

Quarkus version or git rev

3.15.1

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

mvn

Additional information

No response

quarkus-bot[bot] commented 1 month ago

/cc @sberyozkin (jwt)

sberyozkin commented 1 month ago

@Serkan80 For the specific cookie, one needs to use that cookie name

quarkus.http.auth.permission.jwtcookie.paths=/api/*
quarkus.http.auth.permission.jwtcookie.policy=authenticated
quarkus.http.auth.permission.jwtcookie.auth-mechanism=access_token

I'm not sure I'm happy about it, but then, when you have a cookie obtained from the previous bearer token authentication, you may want to access some other parts of the application with that cookie, so it kind of makes sense to have a dedicated block related to restricting how that cookie is used.

Please try it and let me know if it fixes it

Serkan80 commented 1 month ago

@sberyozkin, thanks man, it works now.

But this should be documented on the JWT guide, it's absolutely not clear that it should have been configured like this.

And another improvement on the documentation, can be how to configure signing keys for JWT from a keystore.

I spent almost a half day to figure out how to configure this, and I finally found the solution by debugging some Quarkus classes and checking the errors.

This was my final solution:

mp.jwt.verify.publickey.location=my-keystore.p12
smallrye.jwt.sign.key.location=my-keystore.p12
smallrye.jwt.keystore.password=dummy
smallrye.jwt.keystore.verify.key.alias=at
smallrye.jwt.keystore.sign.key.alias=at

It's also very annoying that some configuration is part of mp.jwt.xyz and others of smallrye.jwt.xyz.

sberyozkin commented 1 month ago

@Serkan80

But this should be documented on the JWT guide, it's absolutely not clear that it should have been configured like this.

Sure, it is just the first time anyone asked for combining a JWT cookie based authentication with other authentication mechanisms :-). I'll fix this issue by documenting it.

And another improvement on the documentation, can be how to configure signing keys for JWT from a keystore. I spent almost a half day to figure out how to configure this, and I finally found the solution by debugging some Quarkus classes and checking the errors.

These properties are documented, so not sure why you had to debug, you could've pinged me on Zulip :-) But sure, if you'd like please open a PR to give a concrete example

It's also very annoying that some configuration is part of mp.jwt.xyz and others of smallrye.jwt.xyz

Agreed, please open an enhancement request, I can take care of it

sberyozkin commented 1 month ago

@Serkan80 Np, let's discuss other issues on Zulip or Discussions, with the path based authentication you should update @TestSecurity accordingly: https://quarkus.io/guides/security-testing#path-based-authentication

gsmet commented 3 weeks ago

I'm closing this as the original question has been answered.

Follow-ups are welcome if you think things should be further improved.

@sberyozkin while all the configuration properties are documented, I think it should be a good idea to start working on what we discussed in Antwerp and document some typical cases with the configuration that should be used.

sberyozkin commented 3 weeks ago

@gsmet Sure, thanks, I actually have a branch to address this specific doc issue but got distracted, will get back to it.

But indeed, we are also considering a tree option too, as my thinking is, that giving how many options and variations are available in the oidc alone, with smallrye-jwt and other mechanisms also in the mix, that if we offer a way for users to go down from a high level answer to a concrete code and/or doc snippet, then it would help some users. I'll keep you up to date, we agreed with @cescoffier I'll start preparing a tree to review. By the way, it may work well for some other extensions too, thanks