Hey
1. I'm unsure why the Resolver was written to assume no role means all
roles. But I think it has to do with
2. The Resolver was intended to be able to resolve principal roles, grants,
and target entities all at one "snapshot" based on the entity and grant
versions returned by that loadEntitiesChangeTracking method. In order to do
this in one shot, the authenticator couldn't resolve the principal roles
actually granted to the principal, so it didn't resolve the
PRINCIPAL_ROLE:ALL scope in the token. But that meant the Resolver needed
to verify the grants existed for the roles to be assumed by the principal
(otherwise, a caller could just put whatever role they want in the scope).
To avoid the lookup during authn, ALL needed to be expanded by the Resolver
based on the grants. This was an impedance to the federated role proposal,
which stated that the authn token is authoritative regarding which roles
are granted to the principal (e.g., using a groups claim
<
https://developer.okta.com/docs/guides/customize-tokens-groups-claim/main/
).
That's why
3. I added the ActiveRolesProvider interface so that the default impl would
continue to validate the grants held by the principal for activating
principal roles but the federated alternative would delegate to the token
claims (yet to be committed).
Hopefully, that history helps. In short, I agree that
SecurityIdentity.getRoles() should be the right way to determine activated
principal roles. The Resolver should rely on the authenticator to determine
the active set of principal roles and not rely on grant records. The
AuthenticatedPolarisPrincipal doesn't really need to hold role names
anymore, but the resolver does still need the list of active PrincipalRoles
to load the appropriate catalog roles. My preference would be to make this
an immutable class that has the principal entity and all active principal
roles all in the constructor.
Mike
On Sun, Apr 20, 2025 at 8:34 AM Alex Dutra <alex.du...@dremio.com.invalid>
wrote:
Hi Mike,
I am generally fine with the spec changes.
I have however some concerns around the way we handle principal roles
today
in Polaris, and as I was preparing the PR with support for external IDPs
[1], some oddities stood out:
1. The pseudo-role PRINCIPAL_ROLE:ALL seems, by convention, to be
converted to an empty set of roles by the Authenticator; but I wonder
how we will be able to distinguish a token that contained no role
from a
token that contained the pseudo-role. I added some changes to my PR to
be
able to distinguish the two situations, but I think the general
handling of
this pseudo-role, as well as role mapping in general, deserves some
more
thinking. The spec says this pseudo-role is "non-functional" – but I
don't
know what this implies exactly.
2. AuthenticatedPolarisPrincipal exposes principal roles and this is
imho concerning for two reasons: first, the Resolver seems to update
the
principal roles during resolution, which makes the class mutable,
potentially changing the principal characteristics *after*
authentication.
And secondly, this creates an ambiguity as roles can be accessed in
two
ways: SecurityIdentity.getRoles() vs
AuthenticatedPolarisPrincipal.getActivatedPrincipalRoles(). I would be
in favor of using the former consistently and completely removing the
latter. (Note: SecurityContext.isUserInRole() relies on the former.)
3. There seems to be some overlap between the primary roles
resolution,
done by the ActiveRolesProvider, and the role resolution done later by
the Resolver, cf. method resolveCallerPrincipalAndPrincipalRoles(). I
wonder if this second role resolution is really necessary? Ultimately,
the
source of truth for role membership will always be
SecurityIdentity.hasRole() or its JAX-RS variant,
SecurityContext.isUserInRole(). We should not deviate from that.
Thanks,
Alex
[1] https://github.com/apache/polaris/pull/1397
On Sat, Apr 19, 2025 at 2:47 AM Dmitri Bourlatchkov <di...@apache.org>
wrote:
Re 2: Thanks for the clarification, Mike! I guess my brain swapped out
a
large portion of that doc :)
I'm still not sure how IdentityToPrincipalMapping can help with
resolving
changes from API and from IdP integration.
The doc talks about namespaces, but in PR# 1353, it looks like API
calls
are free to change anything in federated principal roles.
Should IdentityToPrincipalMapping extend to validating changes coming
from
the Admin API?
Alternatively, we could treat that as a new permission and allow all
admin
users to edit federated roles via API.
Re 1: Yes, I guess it would be helpful with SCIM, but then we'd need to
sync role membership, I guess.
My understanding so far was that we'd get role membership at
authentication
time and not persist it in Polaris.
Is that still the plan?
I think this relates to how we connect authentication and authorization
in
Polaris. If Principals are external, we get role membership from the
access
token and all is clear. If Principals are local, and we still get roles
from the access token, then the question arises about whether that data
is
in sync with local data (and the other way around too).
That could be resolved by Polaris owning access tokens (via token
exchange)
and assuming control over role membership based on local data and
access
time (I commented on this in the doc too).
All in all, since SCIM is not part of phase 1 here, maybe we could
defer
dealing with federated Principal persistence, indeed.
Re 3: I guess it also applies to client secret generation code, but
let's
see whether it actually causes too many conditions in code when it
comes
to
that.
Cheers,
Dmitri.
On Thu, Apr 17, 2025 at 7:42 PM Michael Collado <
collado.m...@gmail.com>
wrote:
Thanks for the response.
1. TBH, I have no strong feelings about persisting the federated
principals. I think the biggest advantage I saw was to support SCIM
along
with 3p identity providers. But for this implementation, if we want
to
skip
persisting principal entities, I'm ok with it.
2. In the doc I wrote the following, which I think addresses the
concern
around conflicts.
1.
The IdentityToPrincipalMapping should supply a naming convention
so
that
PrincipalRoles and Principals created from that source do not
conflict
with
entities created by a different IdentityProvider
In the case that a role happens to exist even with the naming
convention
applied, the caller would be unable to authenticate with that role.
3. I don't think we need special classes for federated roles. In
nearly
all
of the Polaris code, they are treated as PolarisRoleEntity without
any
issue. The only code that needs to be aware of federated roles is
a) The Admin API, which prevents granting these roles to principals
and
b) The federated roles provider, which verifies that the federated
principal is assuming federated roles.
The rest of the code (i.e., the resolver and authorizer) treat these
roles
exactly as native Polaris roles.
Mike
On Thu, Apr 17, 2025 at 2:39 PM Dmitri Bourlatchkov <
di...@apache.org>
wrote:
Thanks for reviving this discussion, Mike!
The API spec change by itself LGTM, but I have related concerns in
how
this
feature is meant to work in general.
1) The need to expose Federated Principals in Polaris API.
The design doc [1] discusses the possibility to expose Federated
Principals
in Polaris API, but there are pros and cons. I do not think it is a
trivial
decision. I'd like to discuss this in more detail before we commit
to
doing
that.
From my POV the main benefit of exposing Federated Principals is to
be
able
to alter their properties in Polaris (as mentioned in the doc).
However,
your email indicates that this is actually forbidden.
What use cases do you envision for exposing read-only Federated
Principals?
2) Reconciliation of Federated vs. Non-Federated changes.
Let's focus on Principal Roles here (which are meant to be writable
via
API).
If users are able to make and change (I assume) Principal Roles via
API,
and federation code will also be creating and changing those roles,
what
is
our approach to handling conflicts between those two streams of
changes?
Some examples for context (also mentioned in GH):
A) A user creates normal Principal Role "A", and later role "A" is
discovered through Federation.
B) A user edits Federated Role "B" and later properties of "B" are
updated
through Federation (or the other way around).
Do we want to control property edits based on the origin of the
property
(spec out a "namespace" for Federated properties)?
3) OO types
Currently Principals and Principal Roles are represented by one
java
type
each. When federation comes into play, would it make sense to
develop a
(java) type hierarchy for working with that data in Polaris code?
My
main
concern here is avoiding a maze of if/else statements throughout
the
Polaris codebase for supporting Federation.
I guess our approach to this depends largely on the outcome of
topic
2
above.
WDYT?
Thanks,
Dmitri.
[1]
https://docs.google.com/document/d/15_3ZiRB6Lhzw0nxij341QUdxEIyFGTrI9_18bFIyJVo/edit?tab=t.0#heading=h.cu1a1acu4lc5
On Wed, Apr 16, 2025 at 6:44 PM Michael Collado <
collado.m...@gmail.com>
wrote:
Hey folks
Some of you already know that I posted an initial PR to get
federated
principals/roles added. One thing that came out of the feedback
was a
spec
change to make it clear when federated identities can be used in
the
APIs.
Notably, federated principals cannot be created or updated, but
can
be
returned in get/list calls, whereas federated roles *can* be
created
by
the
API. The latter is useful/necessary in order to be able to assign
privileges to those roles without relying on the JIT creation on
login.
Please check out the spec change here and let me know what you
think
-
https://github.com/apache/polaris/pull/1353/files#diff-52444bc79608edfae86ed0b46d171f7ef63c20090860d877e4e135168311a986
Mike
On Tue, Dec 17, 2024 at 5:15 PM Dmitri Bourlatchkov
<dmitri.bourlatch...@dremio.com.invalid> wrote:
Hi Mike,
I left some comments in the doc, but overall it looks good to
me
:)
I still think there are some hidden dependencies on
Persistence.
For
example, whether and how we can have composite keys for
persisted
federated
entities... but I guess we can work that out later.
Also, I think it is important for the Authorizer API to avoid
assuming
that
all principals are persisted. Specific authorizer
implementations
(including the default one) can certainly expect persisted
principals,
but
the API should require that for the sake of flexibility of
possible
AuthN/Z
extensions. WDYT?
Cheers,
Dmitri.
On Thu, Nov 14, 2024 at 7:43 PM Michael Collado <
collado.m...@gmail.com>
wrote:
Hey folks
As discussed during the community sync, I've put together
some
thoughts
on
how we'd add support for federated identities in Polaris. I
copied
over
some of what I had in the issue at
https://github.com/apache/polaris/issues/441 and put it into
the
doc
here:
https://docs.google.com/document/d/15_3ZiRB6Lhzw0nxij341QUdxEIyFGTrI9_18bFIyJVo/edit?tab=t.0
.
Please take a look when you get some time and let me know
what
you
think.
Given that our next community sync is scheduled for the
Thanksgiving
holiday in the US, it might be useful to schedule a meeting
specifically
for this. I can schedule that sync if needed.
Mike
