OIDC multi-tenancy @Tenant annotation not selecting tenant

[RKaczmarek]

Describe the bug

Using @Tenant annotation on controller or controller method does not select the OIDC tenant.

Expected behavior

quarkus.oidc.auth-server-url=https://example.com

quarkus.oidc.foo.auth-server-url=https://example.com/foo

quarkus.oidc.bar.auth-server-url=https://example.com/bar

...
@Path("foo")
@Tenant("foo")
public class FooController {
    @Path("")
    @Get()
    public void foo() {
    }
}

...
@Path("bar")
@Tenant("bar")
public class BarController {
    @Path("")
    @Get()
    public void bar() {
    }
}

GET "/foo"
should resolve tenant foo and use https://example.com/foo as authorization server

Actual behavior

GET "/foo"
resolves default tenant and uses https://example.com as authorization server

How to Reproduce?

see expected behavior

Output of uname -a or ver

No response

Output of java -version

11

Quarkus version or git rev

3.4.3,3.6.0

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

gradle 8.3

Additional information

No response

[quarkus-bot]

/cc @pedroigor (oidc), @sberyozkin (oidc)

[sberyozkin]

@RKaczmarek It is only effective when the proactive authentication is disabled, see the info fragment at https://quarkus.io/guides/security-openid-connect-multitenancy#annotations-tenant-resolver ? Do you have it disabled ?

CC @michalvavrik

[RKaczmarek]

Yes, proactive authentication is disabled. Explicitly not using a TenantResolver or TenantConfigResolver.

[michalvavrik]

Yes, even if proactive authentication is disabled. Explicitly not using a TenantResolver or TenantConfigResolver.

I believe you, but your reproducer, citing see expected behavior, is not true because we have tests that checks this behavior and implementation differs based on extension you are using (RESTEasy Reactive vs Classic). Please help us to find and fix a bug with more information. Can you provide actual reproducer, please? I'm happy to look at it if you do.

[michalvavrik]

hah @RKaczmarek , I have an idea, are you using some HTTP Security policies like quarkus.http.auth.permissions.*?

[RKaczmarek]

Yes.

quarkus.oidc.enabled = true
quarkus.oidc.token.allow-jwt-introspection = false
quarkus.oidc.token.allow-opaque-token-introspection = false

quarkus.http.auth.basic = true
quarkus.http.auth.proactive = false
quarkus.security.users.embedded.enabled = true

quarkus.http.auth.policy.prometheus-policy.roles-allowed = prometheus
quarkus.http.auth.permission.prometheus.paths = /q/metrics
quarkus.http.auth.permission.prometheus.policy = prometheus-policy
quarkus.http.auth.permission.prometheus.auth-mechanism = basic

quarkus.http.auth.policy.foo-policy.roles-allowed = foo
quarkus.http.auth.permission.foo-permission.paths = /foo/*
quarkus.http.auth.permission.foo-permission.policy = foo-policy

quarkus.http.auth.policy.bar-policy.roles-allowed = bar, baz
quarkus.http.auth.permission.bar-permission.paths = /bar/*
quarkus.http.auth.permission.bar-permission.policy = bar-policy

That're the build properties. At runtime the oidc tenants are defined via environment variables.

[michalvavrik]

Yeah, HTTP Security policies are applied before endpoint is matched, so annotation can't have effect. Strictly speaking this can be fixed, but it is far from easy to do. I think we should keep this issue open for proper fix and probably also document current implementation limit for policies.

@RKaczmarek as long as you need HTTP permissions, it won't work for you.

[michalvavrik]

I'll put this on my list, but I can't promise quick fix.

[sberyozkin]

@michalvavrik The interesting situation here is that if the proactive authentication is disabled, then it should apply to policies too, specially to the authorization policy, it should not start before the authentication is completed. Sorry if it is what you did in #36504 (IMHO it has to be split into 2 PRs - one dedicated to everything related to HTTP policies improvements and another one to an annotation based mechanism selection)

[michalvavrik]

I'm going to fix it now, as it will have positive effect for #36504 as well, but there is one design problem I'm going to describe here that can't be worked around:

• HTTP based REST stacks like RESTEasy Reactive, RESTEasy Classic are basically route handlers (however when they've begun processing, you can't work inside them as you do inside other handler, don't call event.fail or event.next)
• There are other route handlers secured with HTTP Security policies, in short, all application and non-application endpoints are also secured with the policies (management interface works on same principle)
• When you annotate endpoint with @Tenant, endpoint matching can only be done during HTTP request processing inside RESTEasy, we will not know this ahead under no circumstances (if it was possible, RESTEasy Reactive would do this beforehand as well). Even more, if we tried to abstract all that logic just for security it would make this very fragile because you basically hope you extracted logic consistently. I'll never go there.
• RESTEasy route handler is applied both on paths that will be endpoints and on prefix paths that might be resolved to endpoint

^^^ means one thing: There is no way we can safely determine that no other route handler should not be secured by HTTP Security policy beforehand. User needs explicitly mark HTTP Security policy as the policy only applied on Jakarta REST and no other handler.

Proposal:

1. quarkus.http.auth.permissions.permission1.applied-on=jakarta-rest with default applied-on=all
2. quarkus.http.auth.permissions.permission1.features=jakarta-rest with default features=all or to match this with what already exist quarkus.http.auth.permissions.permission1.feature=resteasy-reactive (as Quarkus already have features that it logs for users)
3. quarkus.http.auth.permissions.permission1.target=jakarta-rest with default target=all
4. quarkus.http.auth.permissions.permission1.filter=jakarta-rest with default filter=all

And every io.quarkus.vertx.http.runtime.security.HttpSecurityPolicy will naturally need this as well, so:

public class CustomGlobalHttpSecurityPolicy implements HttpSecurityPolicy {
    @Override
    public Uni<CheckResult> checkPermission(RoutingContext request, Uni<SecurityIdentity> identity, AuthorizationRequestContext requestContext) {
        return Uni.createFrom().item(CheckResult.DENY);
    }

    @Override
    public String feature() {
        return "jakarta-rest";
    }
}

NOTE: default will be ALL which means all route handlers, which is how it works now. It is important to realize that quarkus.http.auth.permissions.permission1.paths=/* secures all handlers, not just Jakarta REST one.

cc @sberyozkin

[michalvavrik]

@geoand could you kindly confirm that you can't determine inside Vert.x HTTP that random HTTP request will 100 % be RESTEasy Reactive endpoint? That is you can't reasonable create map where key is [path, method, headers] to boolean [endpoint / not endpoint]?

TBH even then we couldn't do it as we need to secure other handlers, but I want be certain that my assumptions about RESTEasy Reactive are not wrong, you can look at my comment right above, thank you.

[geoand]

Theoretically you could run the same matching algorithm to see if you get a match

[michalvavrik]

Theoretically you could run the same matching algorithm to see if you get a match

So if @Tenant was detected I would run code in org.jboss.resteasy.reactive.server.handlers.ResourceLocatorHandler for every single request somehow from Vert.x HTTP extension. Would it give me 100 % certainty that given HTTP request is going to be matched with resource method?

I can't just extract common parts, because in case anything changes, it will bypass security (postpone authorization to never). I am really worried about such ideas, but if you thing there is sound way to do that, I'll consider it.

[geoand]

I would not do it if I were you, it's bound to turn into a mess :)

[michalvavrik]

I would not do it if I were you, it's bound to turn into a mess :)

agreed, thanks for feedback :-)

[geoand]

🙏🏼

[sberyozkin]

Hi @michalvavrik, I'm just feeling that may be we need to take some time to think about it, may be @stuartwdouglas will also contribute.

I feel that with quarkus.http.auth.permissions.permission1.applied-on=jakarta-rest or similarly named property we are introducing a workaround asking users to be aware of some internal technicalities they are not interested in.

The main point IMHO is that authorization must follow authentication.

So IMHO it is a bug that HTTP authorization check forces the authentication before it is known what the actual authentication requirements are.

When the proactive authentication is disabled, Quarkus can delay the security check till it has confirmed JAX-RS endpoint does require security. IMHO it is worth thinking a bit more if HttpSecurity policy checks can be delayed at least at the JAX-RS path in a similar way.

May be what you have analyzed confirms having a property like this is the best workaround possible, but IMHO it is worth taking some time for more feedback, for now users can use OIDC tenant resolver interfaces

[michalvavrik]

I feel that with quarkus.http.auth.permissions.permission1.applied-on=jakarta-rest or similarly named property we are introducing a workaround asking users to be aware of some internal technicalities they are not interested in.

Quarkus HTTP application is more than some REST endpoints, right now HTTP Security policies secure all route handlers after filter with priority AUTHORIZATION (-100 route order).

• I think it is de-facto contract that other applications are counting on AUTHORIZATION being done like https://github.com/quarkusio/quarkus/blob/main/extensions/smallrye-graphql/deployment/src/main/java/io/quarkus/smallrye/graphql/deployment/SmallRyeGraphQLProcessor.java#L153 and we can't know for user if users do not define any handlers for themselves, how could we?
• Asking for HTTP Security policy to only secure one REST endpoint without any opt-in is sensitive change.
• Detecting whether HTTP request is going to be REST endpoint is not for free (at the very least you will have to run these runtime resource matching algorithm on each HTTP request twice or store it in context but you will run it before RR started, it won't be trivial), while setting one config property is one time developer operation.

So IMHO it is a bug that HTTP authorization check forces the authentication before it is known what the actual authentication requirements are.

I agree.

When the proactive authentication is disabled, Quarkus can delay the security check till it has confirmed JAX-RS endpoint does require security. IMHO it is worth thinking a bit more if HttpSecurity policy checks can be delayed at least at the JAX-RS path in a similar way.

We are talking about Jakarta common security annotations, it is logical they are only applied on REST resources. How I understand HTTP security policies is that they are applied on HTTP requests and that is potentially greater scope than JAX-RS. I agree users will mostly think of JAX-RS endpoints.

May be what you have analyzed confirms having a property like this is the best workaround possible, but IMHO it is worth taking some time for more feedback, for now users can use OIDC tenant resolver interfaces

It's fine with me, @sberyozkin or @stuartwdouglas take as much time as needed. I have nothing left to analyze as I know this area well enough.

Thanks for a feedback.

[michalvavrik]

Next sentence has stuck in my head for 2 days now, so I'll share my view on it.

I feel that with quarkus.http.auth.permissions.permission1.applied-on=jakarta-rest or similarly named property we are introducing a workaround asking users to be aware of some internal technicalities they are not interested in.

It's security first approach, not a workaround. You can't just stop securing other handlers in belief users are mostly interested in JAX-RS endpoints. This is not a closed world, we don't know what other (Quarkiverse? Custom?) extensions are doing after they expect authorization had happened. Difference with a Jakarta annotations is that they are used to secure Jakarta resources, so hardly anyone could expect them to secure for example non-application endpoint handlers like health probes.

[michalvavrik]

And I forgot context - so if you want still to secure all handlers with tenant chosen based on JAX-RS resource, it means you would need to run parts of RESTEasy logic without RESTEasy starting processing (because then flow is different you don't use Vert.x failure handlers, you use their fault chain, don't use event.next() or event.fail in the HTTP authorizer). But RESTEasy (I'm interested in Reactive first) does these things after it begun processing and I've re-checked again how it works in RR, it is possible to extract this, but it is quite complex and we need to hope that whoever will change anything related to selection later will put it into shared part.

[sberyozkin]

@michalvavrik

It's security first approach, not a workaround. You can't just stop securing other handlers in belief users are mostly interested in JAX-RS endpoints.

I'm not sure why you are highlighting it, I was not proposing it. Instead I was thinking along the lines of delaying the enforcement of the policies when the proactive authentication is disabled. Can you please give it another quick check, if it is doable, at least in principle:

If the proactive authentication is disabled - accumulate all the security requirements - routes, JAX-RS and then run them once everything related to security checks is known.

If you already know it is not going to work then it is fine...

I can see how restricting a policy to the JAX-RS chain solves the problem, if this is what will have to be done after all, then, can we at least, rather than introducing HTTP security policy specific property (meaning it does not apply to some policies, and it becomes more confusing with shared policies), try a single property like security-policies-apply-to=jaxrs where the only other (and default) enum value is all (all routes, jaxrs chains). This single property will impact all policies. What do you think ?

[michalvavrik]

I'm not sure why you are highlighting it, I was not proposing it.

Misunderstood you, sorry.

Instead I was thinking along the lines of delaying the enforcement of the policies when the proactive authentication is disabled. Can you please give it another quick check, if it is doable, at least in principle:
If the proactive authentication is disabled - accumulate all the security requirements - routes, JAX-RS and then run them once everything related to security checks is known.

Yes, it is doable.

If you already know it is not going to work then it is fine...

No:

• I think it is going to be complex
• I don't like relying on reusing RR matching because I'd have to change a lot in RR and it is going to be sensitive
• I don't agree that when somehow annotate REST endpoint he should automatically secure with this annotation other route handlers, they are different concepts

I can see how restricting a policy to the JAX-RS chain solves the problem, if this is what will have to be done after all, then, can we at least, rather than introducing HTTP security policy specific property (meaning it does not apply to some policies, and it becomes more confusing with shared policies), try a single property like security-policies-apply-to=jaxrs where the only other (and default) enum value is all (all routes, jaxrs chains). This single property will impact all policies. What do you think ?

I think if security-policies-apply-to=jaxrs is used, you will loose a way to secure other handlers with HTTP Security policies like non-application path.

I'll actually attempt to extract it from RR, though I don't like it the concept of it. Util then.

[sberyozkin]

@michalvavrik

Fair enough re the complexity of trying to rework the way HTTP security is matched now...

I think if security-policies-apply-to=jaxrs is used, you will loose a way to secure other handlers with HTTP Security policies like non-application path.

OK.

So then , as far as quarkus.http.auth.permissions.permission1.applied-on=jakarta-rest and similar proposed variations are concerned, IMHO, we should not try to encode jakarta in the config property values, simply .applies-to=jaxrs will do IMHO

[michalvavrik]

@michalvavrik

Fair enough re the complexity of trying to rework the way HTTP security is matched now...

My main concern is that results need to be identical. When matching is happening in RESTEasy Reactive I can see several phases that needs to be taken into consideration like org.jboss.resteasy.reactive.server.handlers.RestInitialHandler, org.jboss.resteasy.reactive.server.handlers.ClassRoutingHandler#handle, org.jboss.resteasy.reactive.server.handlers.MatrixParamHandler#handle and I suppose there can be others. I can't:

• safely extract this behavior hoping that I extracted it correctly and nothing will change (or will be changed also in extracted mechanism)
• running this inside RR would mean probably to suspend org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext after endpoint match and then run routing handlers with priority higher than REST and then proceed

I'm able to implement it, but they are very sensitive changes for for I see as an edge case. That is why I'd like to go with .applies-to=jaxrs.

[michalvavrik]

Hey @sberyozkin , so quarkus.http.auth.permission.jax-rs1.applies-to=JAXRS won't do as it is runtime property and I need to be able to determine during builtime whether to add ServerRestHandler as alternative is to always create it for endpoints and it can be costly considering this is edge feature.

I suggested to replace it with quarkus.http.auth.jax-rs-permissions=permission-name,permission-name2,permission-name3 that is the build time property. We shall be lenient and won't fail if it doesn't exists in the end. It's imperfect but not a big deal.

I thought I was done when I added tests for endpoint without RBAC and realized this :-/

[michalvavrik]

Ha hm! I think I can do one better. We know that JAX-RS HTTP Security policies are only required with @Tenant annotation and in the future with auth mechanism selection like @Bearer etc. We can enable JAX-RS policies automatically when these annotations are used. Okay, forget my previous comment.

It also puts new light on the quarkus.http.auth.permission.jax-rs1.applies-to as we know when JAX-RS policies are required, however we still can't automatically postpone it for all the paths because non-application paths for example needs to be secured as well (and possibly others). So I'd stick with applies to.