Thanks for starting this thread, Alex!
I fully support merging Authenticator and ActiveRolesProvider.
Aside from the issues you mentioned, from my perspective, it also makes
sense conceptually. Authenticating a request implies establishing the
principal's
identity and consequently its roles. It is a single integration point.
Mixing one
Authenticator implementation with another Role Resolver looks really
awkward.
If some Authenticator implementations need to break this process down into
two
stages, it is ok, but Polaris core still receives one (immutable) Principal
per request
with relevant roles in it.
Cheers,
Dmitri.
On Mon, Aug 4, 2025 at 6:45 AM Alexandre Dutra <adu...@apache.org> wrote:
> Hi all,
>
> ActiveRolesProvider was introduced back in January, in order to enrich
> SecurityContext with valid roles for a given principal.
>
> But that was before the introduction of Quarkus, and the introduction
> of external authentication with Quarkus Security and OIDC.
>
> TLDR: ActiveRolesProvider became problematic since (see details
> below), and I propose to merge ActiveRolesProvider into Authenticator.
> This would make Authenticator the central component for resolving
> principals and principal roles, significantly simplifying both code
> and configuration.
>
> That's it :-) I'm eager to know what the community thinks.
>
> Thanks,
> Alex
>
> ---------
>
> Now, for the interested readers, some low-level details.
> ActiveRolesProvider is problematic because of its signature, and the
> reliance on conventions:
>
> - Its signature expects an AuthenticatedPolarisPrincipal, thus forcing
> the Authenticator to create a "temporary" instance of
> AuthenticatedPolarisPrincipal, *potentially containing invalid roles*.
> This makes the code error-prone. For example, let's assume a principal
> has been granted "catalog_admin", but presents a JWT containing also
> "service_admin":
>
> 1) JWT roles = [PRINCIPAL:ROLE:catalog_admin, PRINCIPAL:ROLE:service_admin]
> 2) Authenticator: creates a temporary
> AuthenticatedPolarisPrincipal{roles:[PRINCIPAL:ROLE:catalog_admin,
> PRINCIPAL:ROLE:service_admin]}
> 3) This temporary instance contains HIGHER privileges than the user was
> granted!
> 4) ActiveRolesProvider: removes the wrong role
> 5) Final SecurityContext:
> AuthenticatedPolarisPrincipal{roles:[PRINCIPAL_ROLE:catalog_admin]}
>
> While the final security context is correct and does not contain
> "service_admin", the *temporary one does contain "service_admin" and
> is thus dangerous*. It must not be leaked (injected or used).
>
> - Even worse, if this temporary instance contains invalid roles, by
> convention the default Authenticator would filter out those roles,
> while the default ActiveRolesProvider would (again, by convention)
> assume that, since no roles were presented, then all roles should be
> activated. This could allow a user to obtain more roles than the JWT
> allows them to. Let's suppose a user has "service_admin", but presents
> a token with wrong roles:
>
> 1) JWT roles = [foo, bar]
> 2) Authenticator: was expecting "PRINCIPAL_ROLE:XYZ" => removes the roles
> 3) Authenticator: creates a temporary
> AuthenticatedPolarisPrincipal{roles:[]}
> 4) ActiveRolesProvider: roles is empty => assumes PRINCIPAL_ROLE:ALL
> => activates all roles
> 5) Final SecurityContext:
> AuthenticatedPolarisPrincipal{roles:[PRINCIPAL_ROLE:service_admin]}
>
> In the above scenario, the reliance on conventions results in the
> principal getting higher activated roles than the JWT allowed (the JWT
> above should have resulted in no roles activated). It's not a serious
> security issue because only roles previously granted can be activated,
> but it is concerning, because the original JWT did not contain such
> roles.
>
> Merging ActiveRolesProvider into Authenticator fixes both of the issues.
>
