`@TenantFeature` is used instead of `@Tenant` with OIDC `TenantIdentityProvider`

sberyozkin commented 5 months ago

Describe the bug

When quarkus-oidc is used, bearer access tokens are usually verified as part of the HTTP request processing, for example:

@Authenticated
public class Service {
   @GET 
   String get() {}
}

The get method is called only when the token available in Authorization: Bearer <token> is verified.

Or, we can also support an out of band token verification, when the request has already completed, see https://quarkus.io/guides/security-oidc-bearer-token-authentication#authentication-after-an-http-request-has-completed, for example:

public class EventService {
   @Inject
   TenantIdentityProvider provider;

   void onMessage(String jsonMessage) {
       provider.authenticate(extractTokenFromJson(jsonMessage));
    }
}

In both cases, the same default OIDC tenant configuration is used to verify the token, the only difference is the timing of this token verification.

Now, if we have, lets say 4 OIDC providers, one can choose which tenant should be used to verify the token.

The annotation which is used to choose a specific OIDC tenant for a token verification is @Tenant, for example, when the token is processed during the request we have:

@Authenticated
@Tenant("github")
public class Service {
   @GET 
   String get() {}
}

but with the out of band verification, another annotation, @TenantFeature is used to select OIDC tenant which will be used to verify the token:

public class EventService {

   @TenantFeature("github")
   @Inject
   TenantIdentityProvider provider;

   void onMessage(String jsonMessage) {
       provider.authenticate(extractTokenFromJson(jsonMessage));
    }
}

@TenantFeature's role is to mark which features a given OIDC tenant can use during the token verification, for example, one tenant can choose to use a custom Jose4jValidator, another one a custom cert chain validator, etc. So in the last example it should be:

public class EventService {

   @Tenant("github")
   @Inject
   TenantIdentityProvider provider;

   void onMessage(String jsonMessage) {
       provider.authenticate(extractTokenFromJson(jsonMessage));
    }
}

Recall, from the first 2 examples, the only difference between the during-the-request and after-the-request token processing is timing of the token verification.

Does it really matter, if TenantIdentityProvider is used with @Tenant as opposed to @TenantFeature ? Probably noone would have noticed a difference for a while, partly because it is an advanced case, where the token is chosen to be verified out of band in the presence of multiple OIDC providers.

But surprisingly, and may be, unexpectedly, this enhancement request, #40358, and the follow up discussion at #40525, identified that using @TenantFeature with @TenantIdentityProvider is a problem if #40358 to be supported.

The enhancement #40358 is about updating @TenantFeature to bind a given feature to more than one tenant configuration. For example, if we have Keycloak, Auth0 and Okta, and a custom Jose4jValidator, a user may want to bind it only to Auth0 and Okta tenants for them to use this validator during the token verification.

Therefore saying something like

@TenantFeature("auth0", "okta")
@Inject
TenantIdentityProvider provider;

Introduces an illegal state situation since for a given token, only a single OIDC tenant can be used to verify it. We can of course fail the build in such cases but then we'd need to explain to the users why users can have

@TenantFearture("auth0", "okta")
class MyJose4jValidator implements Jose4jValidator {
}

but not

@TenantFeature("auth0", "okta")
@Inject
TenantIdentityProvider provider;

I believe we have 2 options to deal with it:

Leave things as they are, let users use @TenantFeature with TenantIdentityProvider - but then #40358 would have to be rejected in order to avoid having a workaround the fact @TenantFeature is not about selecting tenant configurations for the token verification
Use @Tenant with TokenIdentityProvider - IMHO it is the correct fix but a breaking one. However a chance of users being impacted by it would be low due the advanced nature of this combination.

if someone has other ideas then let me know please

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of `uname -a` or `ver`

No response

Output of `java -version`

No response

Quarkus version or git rev

No response

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

No response

Additional information

No response

quarkus-bot[bot] commented 5 months ago

/cc @pedroigor (oidc)

michalvavrik commented 5 months ago

+1 for 2.

FredMencier commented 4 months ago

Hello, i can't see this change in the release notes of 3.12 : https://github.com/quarkusio/quarkus/releases/tag/3.2.12.Final Thanks

michalvavrik commented 4 months ago

Hello, i can't see this change in the release notes of 3.12 : https://github.com/quarkusio/quarkus/releases/tag/3.2.12.Final Thanks

hello, milestone on this issue says 3.12.0.CR1, so it is in different release notes: https://github.com/quarkusio/quarkus/releases/tag/3.12.0.CR1

quarkusio / quarkus