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.
