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

Regression Issue: Secured endpoints are exposed when no token is provided using Keycloak-Authorization extension #17164

Closed
Sgitario opened this issue May 12, 2021 · 24 comments · Fixed by #17174
Labels
area/security kind/bug Something isn't working
Milestone

Comments

@Sgitario
Copy link
Contributor

Describe the bug

Given a secured endpoint
When we call this endpoint using a invalid token
Then it returns forbidden as expected
When we call this endpoint using a valid token
Then it returns OK as expected
When we call this endpoint with no token
Then it returns OK when it should return 404

We are using the next Quarkus extensions:

<dependency>
    <groupId>io.quarkus</groupId>
    <artifactId>quarkus-keycloak-authorization</artifactId>
</dependency>

To Reproduce

Steps to reproduce the behavior:

  1. git clone https://github.com/Sgitario/quarkus-examples
  2. cd quarkus-examples/quarkus-keycloak-authz
  3. mvn clean verify

All the tests should pass, but this test is failing UserResourceTest.noUser_userResource:

[ERROR] Failures: 
[ERROR]   UserResourceTest.noUser_userResource:76 1 expectation failed.
Expected status code <401> but was <200>.

Configuration

quarkus.oidc.auth-server-url=http://localhost:8180/auth/realms/test-realm
quarkus.oidc.client-id=test-application-client
quarkus.oidc.credentials.secret=test-application-client-secret

quarkus.keycloak.policy-enforcer.enable=true

Environment (please complete the following information):

Quarkus version or git rev

It worked with 1.13.3.Final, but started failing with 2.0.0.Alpha1 and 2.0.0.Alpha2. Using the latest version in Main, it also fails.

@Sgitario Sgitario added the kind/bug Something isn't working label May 12, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented May 12, 2021

/cc @sberyozkin

@sberyozkin
Copy link
Member

@Sgitario Interesting, quarkus-keycloak-authorization is expected to be invoked only after the authentication has completed, so if it has been invoked then it means the method is not secure - can you link me please to the test ?

@sberyozkin
Copy link
Member

sberyozkin commented May 12, 2021

@Sgitario See this PR.
So keycloak-authorization is not an authentication mechanism - it only enforces authorization of the already available SecurityIdentity - so if it has been invoked without a token then most likely your test endpoint has no @Authenticated annotation meaning the identity is anonymous - in this case you need to have a policy set to restrict this path.

CC @pedroigor

@Sgitario
Copy link
Contributor Author

@Sgitario See this PR.
So keycloak-authorization is not an authentication mechanism - it only enforces authorization of the already available SecurityIdentity - so if it has been invoked without a token then most likely your test endpoint has no @Authenticated annotation meaning the identity is anonymous - in this case you need to have a policy set to restrict this path.

CC @pedroigor

@sberyozkin this is the resource that the test is consuming: https://github.com/Sgitario/quarkus-examples/blob/main/quarkus-keycloak-authz/src/main/java/io/quarkus/qe/UserResource.java

As you can see, no @Authenticated annotations are being used, however this is a change of behaviour in comparison to 1.x Quarkus version and I think it's quite important change for users.

@sberyozkin
Copy link
Member

sberyozkin commented May 12, 2021

@Sgitario OK, you linked to it above :-).
It is a test problem - a request is made at the public resource.

I agree this change is important - but I also think it has to be fixed by adding a note to the migration guide - as we are still in the 2.0.0.Alpha space. The PR I linked to was to address a problem where keycloak-authorization would fail the requests authenticated by non-OIDC mechanism, and also remove the unnecessary requirement for the users to create keycloak-authorization policies to ignore public resources thus duplicating the existing HTTP policies for the same public resources.

It has to be made clear in the docs that keycloak-authorization is not an authenticator but an authorizer. If you'd like it to be effective when an otherwise public resource is accessed, then, similarly to the annotation based RBAC or HTTP based policies, you need to add a Keycloak ENFORCING policy for this path.

You were seeing 401 due to this code - but not as a result of Keycloak rejecting request to this path.

@Sgitario
Copy link
Contributor Author

I can confirm that adding @Authenticated annotation to the resources made the test worked. Therefore, as agreed with Sergey, this is a important change that needs to be noted in the migration guide.

@sberyozkin
Copy link
Member

@Sgitario Thanks, I'll keep this issue open until I'll update the keycloak-authorization docs with a few clarifications (and will update the migration guide alongside it)

@sberyozkin
Copy link
Member

@Sgitario If you'd like you can also add something like

quarkus.keycloak.policy-enforcer.paths.7.name=API
quarkus.keycloak.policy-enforcer.paths.7.path=/user-no-token
quarkus.keycloak.policy-enforcer.paths.7.enforcement-mode=ENFORCING

and verify it is blocked even without @Authenticated

@sberyozkin
Copy link
Member

Never mind - Authentication is a pre-requisite

@sberyozkin
Copy link
Member

I'll check if it is consistent with the way HTTP configuration policies are enforced (ex, if the policy is deny what happens if the identity is anonymous ? If it is still denied then I'll update keycloak-authorization to block the anonymous request if the policy for a given path is enforcing)

@stuartwdouglas
Copy link
Member

@sberyozkin this sounds like a pretty major bug. If there is a policy in effect it should not be skipped because the user in not authenticated.

@sberyozkin
Copy link
Member

sberyozkin commented May 13, 2021

@stuartwdouglas there is no policy in effect in this case - it is a no token access to a public resource with no policy around this public resource - so I've opened a linked PR to ensure a policy, if provided, is effective for anonymous requests.
As I said above, 401 was returned not because due to some policy but simply because KC authorization was not detecting a TokenCredential - which is not consistent with the way the rest of Quarkus security works and was causing side-effects (SecurityIdentity created with non-OIDC was blocked)

@sberyozkin
Copy link
Member

@stuartwdouglas I'm also about to add a test to that PR to verify that if a token is provided then if it is a proactive authentication then KC authz will reject the call unless the public access is allowed

@sberyozkin
Copy link
Member

@stuartwdouglas the fact 401 was caused by a KC authorizer with anonymous requests to public resources with no policy was really a bug IMHO

@Sgitario
Copy link
Contributor Author

@sberyozkin to sum up, using the keycloak-authz extension, we need to explicitly define the policies for the resources which is the major change as from now on, when no explicit policies about the resources, all the resources are considered public, right? However, the behaviour we were observing is:

Given endpoint with no policies
When we call this endpoint using a invalid token
Then it returns forbidden as expected
When we call this endpoint using a valid token
Then it returns OK as expected
When we call this endpoint with no token
Then it returns OK when it should return 404 (this was the origin of the issue)

When it should be:

Given endpoint with no policies
When we call this endpoint using a invalid token
Then it should return OK
When we call this endpoint using a valid token
Then it should return OK
When we call this endpoint with no token
Then it should return OK

Can you confirm the above for further clarification?
Thanks in advance!

@sberyozkin
Copy link
Member

Sorry, missed this last comment, let me check

@sberyozkin
Copy link
Member

@Sgitario

to sum up, using the keycloak-authz extension, we need to explicitly define the policies for the resources which is the major change as from now on, when no explicit policies about the resources, all the resources are considered public, right?

Absolutely not.

Your test uses a public endpoint, not requiring any authentication, and no policies set up.
So the only difference is that before you were 401 with anonymous request accessing a public resource - which is wrong and not consistent with the rest of Quarkus Sec.
Now for this case to get 401 you need to set a policy - same as with HTTP policy - please see a linked test in the closed PR.

Your description is not a precise observation of the problem - it is presented as if a secured endpoint access is now broken, but it is based on the earlier wrong KC Authz behaviour - where simply having it enabled blocks things - and this is not correct.

Does it make clearer ?

@sberyozkin
Copy link
Member

sberyozkin commented May 14, 2021

@Sgitario

To sum up - if the token is provided then keycloak-authorization, with the proactive authentication will always check it - irrespective of whether the endpoint requires authentication or not.

If the token is not provided:

  • if the endpoint requires authentication - then it is a not a job of KC authorization to handle it - it will be blocked by quarkus-oidc or other auth mechanism
  • if the token is not provided and the resource is public - well then if you need KC Authz be effective - have a policy set up for it - this is precisely how it is handled with HTTP Policy and this is the only difference in behaviour

@Sgitario
Copy link
Contributor Author

@Sgitario

To sum up - if the token is provided then keycloak-authorization, with the proactive authentication will always check it - irrespective of whether the endpoint requires authentication or not.

If the token is not provided:

  • if the endpoint requires authentication - then it is a not a job of KC authorization to handle it - it will be blocked by quarkus-oidc or other auth mechanism
  • if the token is not provided and the resource is public - well then if you need KC Authz be effective - have a policy set up for it - this is precisely how it is handled with HTTP Policy and this is the only difference in behaviour

Thanks for the clarification!

@sberyozkin
Copy link
Member

Hi @Sgitario No problems at all, let me close this issue now - please reopen as soon as you have some concerns or observer it differently but I'm 99.9% confident you will confirm the above is true with your own tests - it is all tested now in main as well :-)

@sberyozkin
Copy link
Member

sberyozkin commented May 15, 2021

@Sgitario Hey Jose, I've decided to reopen it as it appears that some uncertainty still persists around it and I don't want to give an impression I'd just like to move on past the real issue :-). I'd like to add more clarity here and give QE more time to test/verify the above summary and challenge it.

So as we've already touched on it a few times above, the only difference you can now observe with 2.0.0.Alpha2 compared to 1.13.x and earlier is that when an anonymous access to a public resource is attempted without a KC Authz policy you now get 200 without 401 - and when one sees it it strikes as if a secured resource is now accessed for free.

As I've already said, IMHO it was actually a bug that by simply enabling a keycloak-authorization dependency a public resource access would fail with 401 (with the other side-effect - valid identity was blocked when more than one authentication mechanism was used).

Here is an OIDC Code Flow test - where a public resource is accessed with quarkus-oidc being enabled in the application.properties but with no HTTP access policies related to this public resource.

See the difference ?

is the major change as from now on, when no explicit policies about the resources, all the resources are considered public

Only applies to the anonymous access to the endpoint which has been set up as a public resource. Now, exactly the same it is done for HTTP policies, if one needs to block an access in such case then an enforcing policy should be created for this resource - but I'd recommend to use HTTP policies for it as keycloak-authorization is about dealing with the tokens.

Please remember keycloak-authorization is not an authentication mechanism - it is supposed to authorize the already authenticated identity - so all those 401s which you see with the invalid tokens - they are not reported by keycloak-authorization but by quarkus-oidc.

I have to admit I'm also finding counter-intuitive that by just enabling keycloak-authorization and sending a token to the public resource I'll get 403, without any KC Authz policies - again it does not look consistent to me with the way the rest of Quarkus sec acts - for example, the code which checks @RolesAllowed and HTTP Policies and which is always enabled does not block the requests to the endpoint which has no @RolesAllowed or HTTP policies. But I'm not going to get to change it :-)

But what I'd like to do is to check as part of this issue discussion is how keycloak-authorization acts when proactive-authentication is disabled. For example, in a quarkus-oidc case if a bearer token is sent to the public endpoint then the token will be verified only if it is detected that an authenticated identity is required (i.e - there must be some HTTP Policy or @Authenticated or @RolesAllowed set up) - and if it is not setup - then no token will be verified and the resource is free to access. I think now it should work correctly with keycloak-authorization as well but I'll need to confirm

Thanks

@sberyozkin
Copy link
Member

Note that with keycloak-authorization, even if application.properties has no policies, Keycloak itself can have them prepared in its realm. So the test which @Sgitario created was in fact indeed testing an endpoint with an enforcing policy - and 401 was a correct expectation - it has been fixed for the next 2.0.0.Alpha3 with the linked PR.

However - the important point is that, as mentioned earlier above, is that in 1.13.x.Final the test was getting 401 not because a policy was checked - but because keycloak-authorization was rejecting a call because it was an anonymous identity.

So, in 2.0.0.Alpha3, for the anonymous request:

  • Jose's original test will continue returning 401 as expected (because the policy in the realm is enforcing)
  • But if the policy is set either in the realm or in application.properties to permissive/disabled or no policy is available - then it will be 200 (will still be 401 in 13.1.4.Final)

@sberyozkin
Copy link
Member

Will close this issue again since Jose has confirmed we are are in agreement. I'll update the migration guide first and then will close it. We will reopen it if there will be some more questions/uncertainties.
@Sgitario thanks for coming up with a very comprehensive test suite

@sberyozkin
Copy link
Member

Added a short note at https://github.com/quarkusio/quarkus/wiki/Migration-Guide-2.0, with the docs also updated in the earlier PR

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
4 participants