Hi Alex Thanks for this. Admittedly, the ActiveRolesProvider was *intended* to support the future OIDC/Quarkus support, but that was long before the current HttpAuthenticationMechanism.
I'm going to check out this branch and validate, but I think it's a good change. It helps with some changes I had planned to make, but hadn't had a chance to work on yet. Mike On Tue, Aug 19, 2025 at 6:49 AM Alexandre Dutra <adu...@apache.org> wrote: > Hi all, > > I went ahead and created a PR implementing the proposed changes: > > https://github.com/apache/polaris/pull/2390 > > Please take a look and let me know what you think. > > Thanks, > Alex > > On Wed, Aug 13, 2025 at 9:58 AM Robert Stupp <sn...@snazy.de> wrote: > > > > Merging those two things SGTM. > > It's what Quarkus/Vert.X > > 'HttpAuthenticationMechanism'/'SecurityIdentity' do (right). > > > > On Wed, Aug 13, 2025 at 1:55 AM Dmitri Bourlatchkov <di...@apache.org> > wrote: > > > > > > 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. > > > > >
