Hi Mike, I guess some of the confusion may come from PR 1353 [1] and this thread being a bit out of sync.
Would you mind removing federated Principals from the API in the PR for the sake of clarity? >From my POV, we can reconsider that if/when SCIM (or something similar) comes. Thanks, Dmitri. [1] https://github.com/apache/polaris/pull/1353 On Wed, Apr 30, 2025 at 11:50 AM Michael Collado <collado.m...@gmail.com> wrote: > Hi Robert > > Unsure if you read the previous emails in this thread. In my earlier > response, I wrote > > > 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. > > I feel that should address the majority of the concern you just expressed. > > Storing grants with references to some kind of FQN is certainly one > approach, but IMO, is a massive undertaking with little benefit. For > example, all of the grant APIs have to be modified to deal with target > entities that don’t exist. How do we do validation for these APIs? Do these > APIs now need to talk to external entities to ensure the caller didn’t > misspell a role name or use all caps instead of mixed case? UI developers > can’t simply populate a list of roles for a dropdown list unless they also > connect directly to the idp. That makes it hard for someone to build a > generic Polaris admin UI without also building support for Okta, Google, > Apple, etc. > > On the other hand, SCIM is an industry standard for a reason. Every major > IdP supports it - and I doubt it’s because they just don’t know any better. > Supporting federated roles + SCIM means all the existing grant APIs Just > Work and building generic privilege management tools/UI could be done > today. > > Given the choice between a massive rewrite of the grant and authorization > primitives with no user-facing benefit and supporting federated roles with > the option of SCIM support in the future, the answer seems clear to me. > > Mike > > On Wed, Apr 30, 2025 at 7:04 AM Robert Stupp <sn...@snazy.de> wrote: > > > Hi, > > > > thanks Mike for the effort. > > > > However, I think that persisting principals, which are not "owned" by > > Polaris, being persisted in Polaris is a good idea. How would changes to > > a principal in the IdP be propagated to Polaris? What happens if a > > principal that's deleted/disabled in the IdP - how is that reflected in > > Polaris? What would happen if two Polaris instances concurrently > > "transparently create" the same "federated principal entity"? > > > > Storing mirrored information that is only owned by an (external) IdP > > leads to a "what's the source-of-truth" problem. > > > > A principal is (mostly) an individual user or service that can access > > Polaris. The actual access-decision is up to the IdP (external IdP or > > Polaris w/ its own principals). Each principal can also be uniquely > > identified (IDP + principal-ID). > > > > I really do not think that principals owned by "external IdPs" should > > ever be persisted in Polaris. > > > > Similar for roles manages by external IdPs. > > > > ACLs (grants) are (generally speaking) lists of grantee+privilege(s) > > tuples. It is IMHO better to use a qualified reference to "IDP + > > principal/role-ID". > > > > AFAIR it's only possbible to grant privileges to roles, not individual > > principals. And if that's the case, I do not see a requirement to have > > "federated principals" being persisted in Polaris at all. > > > > I seems the underlying issue here is the very tight coupling of > > principals + principal-roles + catalog-roles + grants that requires a > > unique integer-ID for each of those and using the same database-oriented > > approach for role-membership management and catalog-entity grants. > > > > What's IMHO needed is a proper separation of those things, which should > > happen now and after that's done integrate external IdPs. > > > > > > On 22.04.25 17:00, Alex Dutra wrote: > > > Hi Mike, > > > > > > Thanks for the historical context, that is really helpful. And I > > completely > > > agree with your last paragraph, and especially this: > > > > > > My preference would be to make this an immutable class that has the > > >> principal entity and all active principal roles all in the > constructor. > > > > > > I was arriving at the exact same conclusion – it's a pity that > > > SecurityContext doesn't expose the principal roles. I guess, the > problem > > > now is how to move forward with this suggestion – I imagine a change > > > in AuthenticatedPolarisPrincipal or in the Resolver would create > > > repercussions in a lot of places in the code base as well as in > existing > > > PRs. > > > > > > Thanks, > > > > > > Alex > > > > > > On Mon, Apr 21, 2025 at 10:30 PM Michael Collado < > collado.m...@gmail.com > > > > > > wrote: > > > > > >> 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 > > >>>>>>>>> > > -- > > Robert Stupp > > @snazy > > > > >
