Selecting OIDC tenant via annotation is not working with RESTEasy Reactive

[Eng-Fouad]

Describe the bug

According to OIDC docs, one can select OIDC tenant via annotation as follows:

@Inherited
@InterceptorBinding
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.METHOD, ElementType.TYPE})
public @interface HrTenant {
}

@Interceptor
@HrTenant
public class HrTenantInterceptor {
    @Inject
    RoutingContext routingContext;

    @AroundInvoke
    Object setTenant(InvocationContext context) throws Exception {
        routingContext.put(OidcUtils.TENANT_ID_ATTRIBUTE, "hr");
        return context.proceed();
    }
}

@Path("/hr")
public class HrResource {

    @HrTenant
    @GET
    @Produces(MediaType.TEXT_PLAIN)
    public String helloWorld() {
        return "Hello World";
    }
}

Expected behavior

Authentication of /hr endpoint should be based on quarkus.oidc.hr.* configs.

Actual behavior

Authentication of /hr endpoint uses the default tenant configs quarkus.oidc.* as authentication is triggered before HrTenantInterceptor, hence the authentication fails.

Quarkus version or git rev

3.2.0.Final

[quarkus-bot]

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

[sberyozkin]

@Eng-Fouad Hey, See the note there about the proactive authentication having to be disabled - this is the only way to capture it

[Eng-Fouad]

@Eng-Fouad Hey, See the note there about the proactive authentication having to be disabled - this is the only way to capture it

It is already disabled:

quarkus.http.auth.proactive=false

[sberyozkin]

@Eng-Fouad Can you check this test:

c6e16a2#diff-413ec7589b4f774d8b6b1dd5e222db550f79bb4084e36af4c986f6f7aa5ae273

May be you need to move @HrTenant to the class-level

[Eng-Fouad]

@Eng-Fouad Can you check this test:

c6e16a2#diff-413ec7589b4f774d8b6b1dd5e222db550f79bb4084e36af4c986f6f7aa5ae273

I believe this test evaluates tenant after authentication succeeded. The authentication uses default tenant config, as application.properties has no quarkus.oidc.hr.* configs.

May be you need to move @HrTenant to the class-level

Same result.

[sberyozkin]

@Eng-Fouad My guess it needs to have a higher priority, the tenant resolver interceptor, than that of the security CDI interepceptors, Also CC @michalvavrik

[Eng-Fouad]

@Eng-Fouad My guess it needs to have a higher priority, the tenant resolver interceptor, than that of the security CDI interepceptors, Also CC @michalvavrik

I tried @Priority(1) on HrTenantInterceptor, I got same result.

[sberyozkin]

@Eng-Fouad I've debugged and it works as expected. I've looked at your code above - where do you set the authentication/rbac requirement ? Please also keep in mind that if you have a registered TenantResolver which returns a non-null tenant id in this case then it will be used instead.

[sberyozkin]

@Eng-Fouad I've opened a PR which will fix this issue - but please reopen it if it will be merged before you get a chance to double check, and provide a reproducer, thanks

[Eng-Fouad]

OK, I figured out what the issue was. I had @Authenticated on JAX-RS resource. That's why it was performing authentication first.

@Path("/hr")
public class HrResource {

    @HrTenant
    @Authenticated // <---------
    @GET
    @Produces(MediaType.TEXT_PLAIN)
    public String helloWorld() {
        return "Hello World";
    }
}

---

EDIT:

After removing @Authenticated, auth mechanisms are not called.

[Eng-Fouad]

@sberyozkin I think I understand now why it fails for me but it works in integration-tests.

In the following test:

quarkus/integration-tests/oidc-wiremock/src/main/java/io/quarkus/it/keycloak/TenantEchoResource.java

Lines 22 to 30 in 79ccc09

	@Inject
	SecurityIdentity identity;
	@GET
	@Produces(MediaType.TEXT_PLAIN)
	public String getTenant() {
	return OidcUtils.TENANT_ID_ATTRIBUTE + "=" + routingContext.get(OidcUtils.TENANT_ID_ATTRIBUTE)
	+ ", static.tenant.id=" + routingContext.get("static.tenant.id")
	+ ", name=" + identity.getPrincipal().getName();

Access to identity is preformed after JAX-RS resource is called. In other words:

1. TenantResolverInterceptor::setTenant is invoked, which sets tenant as hr.
2. JAX-RS resource TenantEchoResource::getTenant() is invoked.
3. Identity is requested: identity.getPrincipal().getName().
4. OidcAuthenticationMechanism::authenticate is invoked, which reads tenant as hr.
5. Authentication succeeds.

While using @Authenticated or @RolesAllowed on JAX-RS resource, (or even accessing identity in filters):

1. Identity is requested.
2. OidcAuthenticationMechanism::authenticate is invoked, which reads tenant as null.
3. Authentication fails.

BTW, I couldn't re-open this issue :)

[michalvavrik]

Hmm, yes, I think it makes sense @Eng-Fouad , it goes down to EagerSecurityHandler. @sberyozkin I can't tell if this can be fixed before I have a look, but this should be debugged and documented, so I'll re-open issue. Fine with you or am I missing something?

[michalvavrik]

@Eng-Fouad I can't reproduce it though, please provide reproduce but what you describe seems to be already tested.

[michalvavrik]

aaahh! ok, oidc-wiremock is using Resteasy classic, let me re-check again.

[michalvavrik]

Okay, simply replacing RC with RR makes a trick, I'll have a look.

[michalvavrik]

@Eng-Fouad can you confirm you are using RESTEasy Reactive?

[Eng-Fouad]

@Eng-Fouad can you confirm you are using RESTEasy Reactive?

That's correct. I am using RESTEasy Reactive:

implementation("io.quarkus:quarkus-resteasy-reactive-jackson")

[sberyozkin]

Thanks @Eng-Fouad @michalvavrik. The test uses @Authenticated but I did not realize it could be related to the reactive vs classic.

[michalvavrik]

Yeah, CDI interceptors can't be used for this purpose as auth is done before CDI interceptors and it is correct design - so with RR https://quarkus.io/guides/security-openid-connect-multitenancy#resolving-tenant-identifiers-with-annotations wont' work.

@sberyozkin any special reason why it has to be CDI interceptors? There is a way to make this work as long as you won't insist on interceptors in RR, we can simply provide interface and will users implement and then we can check in EagerSecurityHandler what annotations there are on method.

Pros

- it will work with proactive auth enabled not sure, probably not, but I have to think about it

• you can use @HrTenant annotation as well to select tenant

Cons

• it has to be something that is invoked before security checks, so a new form of filter or whatever we call it

[sberyozkin]

@michalvavrik

Yeah, CDI interceptors can't be used for this purpose as auth is done before CDI interceptors and it is correct design

Well, as you see is that it does work for RestEasy classic - so it is related to the way RestEasy Reactive handles it ?

[michalvavrik]

Yes, it is @sberyozkin; it is for long debate that we sort of lead when discussing using @PermissionsAllowed on RR endpoints, but I believe it is a good thing it doesn't work - when CDI interceptors are invoked, a lot of things happened already, including serialization and security should be run first.

[michalvavrik]

fyi @sberyozkin this is similar case to #32049 which shame on me, I'm yet to document (I'll do it this weekend).

[sberyozkin]

We already have TenantResolver interface which works with or without the proactive auth being enabled, for reactive and classic. This feature is about binding tenant id resolution to specific methods.

a lot of things happened already, including serialization and security should be run first.

When it is critical not to read some massive input stream before the security check then sure, users can avoid this feature and we can warn them against using it in such cases, but IMHO, it should work for Reactive same way as it does for Classic

[michalvavrik]

We already have TenantResolver interface which works with or without the proactive auth being enabled, for reactive and classic. This feature is about binding tenant is resolution to specific methods.

Yes, and as I said, this can be done. Let me sort this issue out. Only thing I need from you is acceptance that CDI interceptors won't be required (but still can work for RC).

When it is critical not to read some massive input stream before the security check then sure, users can avoid this feature and we can warn them against using it in such cases, but IMHO, it should work for Reactive same way as it does for Classic

There is a lot of strong opinions in that. I'd like to avoid this discussion as long as we can make it work without interceptors (btw it is purely intentional #19598). But sure, I'm happy to hear your POV.

[michalvavrik]

Maybe I wasn't clear, we can make it possible for RR methods to select tenants with annotation.

[sberyozkin]

Absolutely, yeah, I'm not really that fond of CDI interceptors as such, I'm just not seeing as far, how this can work without them, but have a look please when you'll get a chance, cheers

[michalvavrik]

Thanks!

[sberyozkin]

Thanks Michal

[sberyozkin]

What I'm not sure about is how you'd do it at the Resteasy Reactive level given that it is an OIDC specific feature, setting its tenant identifiers. The reason it works (at least for classic) with CDI interceptors is that CDI Interceptor is feature neutral...

[sberyozkin]

Or, may be we just say, implement RR ServerHandler or something like that... OK, I'll keep quiet now :-)

[michalvavrik]

What I'm not sure about is how you'd do it at the Resteasy Reactive level given that it is an OIDC specific feature, setting its tenant identifiers. The reason it works (at least for classic) with CDI interceptors is they are CDI Interceptor is feature neutral...

I don't have answer for that - I'm wondering about that as well. Only thing that is clear to me is that at this very point

quarkus/extensions/resteasy-reactive/quarkus-resteasy-reactive/runtime/src/main/java/io/quarkus/resteasy/reactive/server/runtime/security/EagerSecurityHandler.java

Line 72 in 65ca2db

if (authorizationController == null) {

(or little later) we have knowledge of method annotation but security checks didn't run yet, so we can do there something.

Or, may be we just say, implement RR ServerHandler or something like that... OK, I'll keep quiet now :-)

I will think, bring suggestions if you have them :-)

[sberyozkin]

My understanding ServerHandler runs very close to the actual method invocations in RR, but I'm not sure how we'd package/implement it at the RR level as we can't ship it with quarkus-oidc

[michalvavrik]

There is actually little hack that could help us, if we refactor SecurityCheckStorageBuilder so that extension can provide it's own SecurityCheckStorage, we can just transform existing

return new SecurityCheckStorage() {
            @Override
            public SecurityCheck getSecurityCheck(MethodDescription methodDescription) {
                return securityChecks.get(methodDescription);
            }
        };

to something like

return new SecurityCheckStorage() {
            @Override
            public SecurityCheck getSecurityCheck(MethodDescription methodDescription) {
                setTenant(methodDescription);
                return securityChecks.get(methodDescription);
            }
        };

and then all that is left is to provide annotation that we are going to detect, something like we can reuse @Tenant("bearer") like this

@GET
@Path("/hello")
@Tenant("hr")
public Hello hello() {
   return "hi";
}

I didn't think this through @sberyozkin , but as POC before I know more, it looks good to me and it shall work for both RC and RR. It is well possible I am missing something, I'll come back to it when I have a time, but feedback is welcomed.

[michalvavrik]

Maybe we could be more explicit though, as getSecurityCheck could be called more than once, It shouldn't be a problem, but we can add some explicitly called method instead. I'll think about it.

[michalvavrik]

Last comment so that I'm not spamming: adding it to getSecurityCheck shouldn't be a big deal considering we just call getTenant that does no computing, just return annotation value (prepared in some form, validated etc.).

[sberyozkin]

Hey @michalvavrik Sorry, missed it, for the annotation based tenant id resolution it would not be a problem at all, it being called multiple times.
Note if it were TenantResolver (which can check request for some headers/queries) or TenantConfigResolver (likewise - when creating OIDC config dynamically) it would be a problem - we actually have tests verifying a given TenantResolver and TenantConfigResolver is called only once, but for annotation based approach it is just a value - this security check should be able to have RoutingContext injected to be able to set the tenant id on it

[michalvavrik]

I think I found bit more generic solution in mind, because ability to connect method to request when proactive=false (and RR started processing request) will help us in the future as well. I'll take my time and look when possible. Thanks for feedback.