Issue 10343: Disabled default quarkus-oidc tenant blocks access to th…

[Sgitario]

No description provided.

[Sgitario]

This commit is linked to this issue: #10343
@sberyozkin do you mind taking a look? Thanks!

[Sgitario]

I can enable/disable the security in public endpoints now using this configuration:

quarkus.http.auth.permission.1.paths=/*
quarkus.http.auth.permission.1.policy=permit
quarkus.oidc.tenant-enabled=false
quarkus.http.auth.proactive=false
%secured.quarkus.http.auth.permission.1.policy=authorized
%secured.quarkus.oidc.tenant-enabled=true
%secured.quarkus.http.auth.proactive=true

Source code to reproduce in here.

[sberyozkin]

@Sgitario Thanks for this PR, the build fails with the Mockito failure, have a look please.

[sberyozkin]

Hi Stuart @stuartwdouglas What is not clear to me is why the proactive authentication has to be disabled. Proactive authentication is about verifying the token even if the resource is public. But @Sgitario explains that no token is available, therefore, given that the current request path is public, OIDC (or other active) authentication mechanism must not be invoked at all ?

[Sgitario]

@Sgitario Thanks for this PR, the build fails with the Mockito failure, have a look please.

Fixed by adding the vertx-codegen dependency in the tests (this is the root cause of this issue which only affects to Java 8)

[Sgitario]

Hi Stuart @stuartwdouglas What is not clear to me is why the proactive authentication has to be disabled. Proactive authentication is about verifying the token even if the resource is public. But @Sgitario explains that no token is available, therefore, given that the current request path is public, OIDC (or other active) authentication mechanism must not be invoked at all ?

FYI, I had to disable the proactive authentication in order to avoid that the authentication was invoked in this line.

[sberyozkin]

@Sgitario We have a public resource test but I think it works because at the OidcAuthenticationMechanism level we have the tenant resolved.
Note both CodeAuthenticationMechanism and BearerAuthenticationmechanism also return a deferred identity. Can you check please if the deferred can be removed there given that OidcAuthenticationMechanism does it now ?

[Sgitario]

@Sgitario We have a public resource test but I think it works because at the OidcAuthenticationMechanism level we have the tenant resolved.
Note both CodeAuthenticationMechanism and BearerAuthenticationmechanism also return a deferred identity. Can you check please if the deferred can be removed there given that OidcAuthenticationMechanism does it now ?

Not sure what you meant, I could not see any .invocation to deferred() in either CodeAuthenticationMechanism or BearerAuthenticationmechanism.

[sberyozkin]

@Sgitario Sorry, my bad, it is done here:
https://github.com/quarkusio/quarkus/blob/master/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcIdentityProvider.java#L46 which both mechanisms delegate to
Interesting...so I wonder if it can be removed there

[Sgitario]

@Sgitario Sorry, my bad, it is done here:
https://github.com/quarkusio/quarkus/blob/master/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcIdentityProvider.java#L46 which both mechanisms delegate to
Interesting...

So, if I understand it correctly, the OidcAuthenticationMechanism checks which mechanism to use (either Code or Bearer). And then, each of these mechanism delegate to the identity provider.
However, it's failing in the "isWebApp" method in OidcAuthenticationMechanism. Another solution would be to not raise any exception here, but in the identity provider, and I'm doubt this is something we can do as we need to know which mechanism to use beforehand.
Let me know if I need to do something else to address the issue though.

[sberyozkin]

Hi @Sgitario Your PR is the step in the right direction :-), it just raises the question if the (earlier) decision to return a null context if the tenant is disabled is correct. I'll need to check, I'm a bit overwhelmed right now, but we are on it with your help now :-)

[stuartwdouglas]

In the proactive case io.quarkus.vertx.http.runtime.security.HttpAuthenticationMechanism#authenticate should be called, but not io.quarkus.vertx.http.runtime.security.HttpAuthenticationMechanism#getChallenge.

If this is taking over the request then this is a bug, the authenticate() method should just return an empty identity if there is no token.

[Sgitario]

In the proactive case io.quarkus.vertx.http.runtime.security.HttpAuthenticationMechanism#authenticate should be called, but not io.quarkus.vertx.http.runtime.security.HttpAuthenticationMechanism#getChallenge.

If this is taking over the request then this is a bug, the authenticate() method should just return an empty identity if there is no token.

The objective is to allow to configure the security of endpoints in runtime. The problem is that at the moment, it's checking the tenant configuration always, regardless these properperties:

quarkus.http.auth.permission.1.paths=/*
quarkus.http.auth.permission.1.policy=permit
quarkus.oidc.tenant-enabled=false
quarkus.http.auth.proactive=false

The issue happens when I configured the endpoints to public access and I didn't configure the tenant configuration. If the proactive is disabled, I think the expected behaviour should be that no tenant configuration is checked and I got access to my public endpoint. What is happening is that it's failing because No Tenant Configuration is provided.

[sberyozkin]

@Sgitario Hi, sorry for a delay, I've opened another PR:
#10506
I was not sure about interposing with one more deferred call, so the solution which I propose there is to simply return an empty SecurityIdentity/Challenge if the current tenant is disabled which is what Stuart implied.
So quarkus.http.auth.proactive=false will not be needed.
Can you check it please against your tests ?
Thanks

[Sgitario]

@Sgitario Hi, sorry for a delay, I've opened another PR:
#10506
I was not sure about interposing with one more deferred call, so the solution which I propose there is to simply return an empty SecurityIdentity/Challenge if the current tenant is disabled which is what Stuart implied.
So quarkus.http.auth.proactive=false will not be needed.
Can you check it please against your tests ?
Thanks

Many thanks @sberyozkin ! I did check these changes and now it's possible to enable/disable the authentication at runtime:

• if the property "quarkus.oidc.tenant-enabled" is false, the tenant won't be checked if the URL is public. Might be an issue when the URL is protected, because it returns HTTP OK with empty response (and it should return 401)
• if the property "quarkus.oidc.tenant-enabled" is true, the tenant is checked at startup and fails as expected if it's not provided. If the tenant is properly configured, everything works as expected.