Basics of 'subid ranges' support for IPA provider #5693

alexey-tikhonov · 2021-06-20T19:24:52Z

This is for preliminary review.

Limitations:

only IPA provider
single subid interval pair (subuid+subgid) per user
idviews aren't supported
only forward lookup (user -> subid)

Known TODOs:

delete cached subid ranges in case "not found" on a server
distinguish "user not found" vs "user doesn't have ranges defined"

To test:

build shadow-utils from latest upstream (or get Fedora Rawhide build from https://copr.fedorainfracloud.org/coprs/ipedrosa/subid_ranges/) [Update: now available in Fedora Rawhide starting shadow-utils-4.8.1-11.fc35]
build FreeIPA with Add support subordinate user/group ids as separate entries freeipa/freeipa#5713 (or ping me for details of internal test setup)
build SSSD with this PR (enabling --with-subid)
edit /etc/nsswitch.conf: subid: sss
use for example this utility from shadow-utils upstream to fetch subid ranges from IPA server (or new?idmap)

ikerexxe

I've reviewed the changes and I have found several minor concerns. In any case, the scope of the changes is too big for me so I recommend that you get the feedback from a senior member of the team.

src/sss_client/subid/sss_subid.c

src/providers/ldap/ldap_id_subid.c

pbrezina

Hi, I did not test it yet. See minor comments below and inline in the code.

Please, provide some description to the first commit message.
Why conditional build? Why is build disabled by default?
We don't use two blank lines between function definitions.
Can the range be set also for trusted domain user? If yes, this implementation doesn't seem to support it.

src/db/sysdb.h

src/db/sysdb_subid.c

src/providers/ldap/ldap_id_subid.c

pbrezina · 2021-07-19T13:34:38Z

src/providers/ldap/ldap_id_subid.c

+
+        /* store range */
+        sysdb_store_subid_range(state->domain, state->name,
+                                state->domain->user_timeout,


Introduce its own entry_cache_subid_timeout options?

User's subid range feels and looks like other NSS attributes. Does it really make sense to have different expiration time?

subid range may be for user or group, right? We have expiration time for user and for group so it may be confusing that group subid expires with users.

In general, user might have defined a set of subuid and/or subgid ranges(*) (subuid - a range of subordinate UIDs, subgis - a range of subordinate GIDs). Those ranges (both types) can be used by this user for mapping in parent namespace. So both types of subid ranges are kind of user's attribute.

group (as NSS object) can't have any subids defined.

(*) In current implementation FreeIPA supports only two options:

no subid ranges defined for user

user has both subuid and subgid defined (with the same fixed size 65k and the same interval start, but this implementation detail doesn't matter much for us)

src/providers/ldap/ldap_id_subid.c

src/sss_client/subid/sss_subid.c

alexey-tikhonov · 2021-07-23T11:22:53Z

Hi @pbrezina,

thank you for the review.

Please, provide some description to the first commit message.

Do you think it makes sense to keep 2 commits? I was going to squash them after review.

Why conditional build? Why is build disabled by default?

This is MVP of experimental feature.
Corresponding code in shadow-utils is merged upstream, but there is no upstream release available yet. We brought functionality to Fedora via patches. But I doubt other distributions will do the same. I.e. out of Fedora/RHEL, this is at the moment for enthusiasts who are willing to play with a new feature and who is capable to rebuild shadow-utils from sources.
Moreover, I anticipate significant changes might be required once we have initial feedback.

So... at the very least it's conditional to allow distributions to avoid installing plugin that they can't use yet.

Once shadow-utils upstream release is done and widely used, Podman patches released (containers/storage#882), etc (so it is more or less easy to use), we can change default and build code / install plugin in sssd-ipa, IMO.

* We don't use two blank lines between function definitions.

And this (monolithic text) makes me sad :) I didn't see such restrictions in https://sssd.io/contrib/coding-style.html

* Can the range be set also for trusted domain user? If yes, this implementation doesn't seem to support it.

No, FreeIPA doesn't support it in MVP.

alexey-tikhonov · 2021-07-26T17:59:34Z

make-check-valgrind times out on Rawhide (and a couple of tests fail) but I can't reproduce this locally.

pbrezina · 2021-07-27T10:48:34Z

Hi @pbrezina,

thank you for the review.

Please, provide some description to the first commit message.

Do you think it makes sense to keep 2 commits? I was going to squash them after review.

It's up to you, just make sure it has sufficient description.

Why conditional build? Why is build disabled by default?

This is MVP of experimental feature.
Corresponding code in shadow-utils is merged upstream, but there is no upstream release available yet. We brought functionality to Fedora via patches. But I doubt other distributions will do the same. I.e. out of Fedora/RHEL, this is at the moment for enthusiasts who are willing to play with a new feature and who is capable to rebuild shadow-utils from sources.
Moreover, I anticipate significant changes might be required once we have initial feedback.

So... at the very least it's conditional to allow distributions to avoid installing plugin that they can't use yet.

Ack. This is something to document in the commit message.

Once shadow-utils upstream release is done and widely used, Podman patches released (containers/storage#882), etc (so it is more or less easy to use), we can change default and build code / install plugin in sssd-ipa, IMO.

Ok.

* We don't use two blank lines between function definitions.
And this (monolithic text) makes me sad :) I didn't see such restrictions in https://sssd.io/contrib/coding-style.html

:-) Even though something is not explicitly banned, it is not cool to differentiate from the rest of the project.

* Can the range be set also for trusted domain user? If yes, this implementation doesn't seem to support it.

No, FreeIPA doesn't support it in MVP.

Code looks good to me. I'm going to test it.

pbrezina · 2021-07-28T12:20:27Z

Seems to work fine. Please, do the final touch and I wlll ack it.

alexey-tikhonov · 2021-07-28T21:34:03Z

Squashed patches, removed excessive new lines, added release notes to the commit message.

Should I reference shadow-maint/shadow#154 explicitly in the commit message?

pbrezina · 2021-07-29T10:03:36Z

Squashed patches, removed excessive new lines, added release notes to the commit message.

Should I reference shadow-maint/shadow#154 explicitly in the commit message?

It might be good for other distro maintainers.

Please separate the feature release note with blank line from the rest of description.

:feature: Basic support of user's 'subuid and subgid ranges' for IPA provider and corresponding plugin for shadow-utils were introduced. Limitations: - single subid interval pair (subuid+subgid) per user - idviews aren't supported - only forward lookup (user -> subid ranges) Take a note, this is MVP of experimental feature. Significant changes might be required later, after initial feedback. Corresponding support in shadow-utils was merged upstream, but since there is no upstream release available yet, SSSD feature isn't built by default. Build can be enabled with `--with-subid` configure option. Plugin's install path can be configured with `--with-subid-lib-path=` ("${libdir}" by default) For additional details about support in shadow-utils please see discussion in shadow-maint/shadow#154 and in related PRs. :config: New IPA provider's option `ipa_subid_ranges_search_base` allows configuration of search base for user's subid ranges. Default: `cn=subids,%basedn` Resolves: SSSD#5197

alexey-tikhonov · 2021-07-29T10:37:59Z

Squashed patches, removed excessive new lines, added release notes to the commit message.
Should I reference shadow-maint/shadow#154 explicitly in the commit message?

It might be good for other distro maintainers.

Done.

Please separate the feature release note with blank line from the rest of description.

Well, I meant all this text to be included in the notes...

pbrezina · 2021-07-29T12:19:08Z

Pushed PR: #5693

master
- f546088 - Basics of 'subid ranges' support for IPA provider.

alexey-tikhonov linked an issue Jun 20, 2021 that may be closed by this pull request

Support subid resources in ipa provider #5197

Closed

alexey-tikhonov added the Bugzilla label Jun 21, 2021

ikerexxe reviewed Jun 22, 2021

View reviewed changes

src/sss_client/subid/sss_subid.c Outdated Show resolved Hide resolved

src/sss_client/subid/sss_subid.c Outdated Show resolved Hide resolved

src/sss_client/subid/sss_subid.c Show resolved Hide resolved

src/providers/ldap/ldap_id_subid.c Outdated Show resolved Hide resolved

pbrezina reviewed Jul 19, 2021

View reviewed changes

pbrezina self-assigned this Jul 23, 2021

pbrezina added the Changes requested label Jul 23, 2021

pbrezina assigned ikerexxe Jul 23, 2021

alexey-tikhonov force-pushed the subid branch from 431f82e to 791c522 Compare July 23, 2021 20:05

alexey-tikhonov added Waiting for review and removed Changes requested labels Jul 23, 2021

alexey-tikhonov force-pushed the subid branch from 791c522 to 81d4665 Compare July 26, 2021 19:44

alexey-tikhonov removed the Waiting for review label Jul 28, 2021

alexey-tikhonov force-pushed the subid branch from 81d4665 to e290602 Compare July 28, 2021 21:22

alexey-tikhonov added the Waiting for review label Jul 28, 2021

alexey-tikhonov force-pushed the subid branch from e290602 to d5911f4 Compare July 29, 2021 10:32

pbrezina added Accepted Ready to push Ready to push and removed Waiting for review labels Jul 29, 2021

pbrezina added Pushed and removed Accepted Ready to push Ready to push labels Jul 29, 2021

pbrezina closed this Jul 29, 2021

pbrezina mentioned this pull request Jul 29, 2021

Support subid resources in ipa provider #5197

Closed

Basics of 'subid ranges' support for IPA provider #5693

Basics of 'subid ranges' support for IPA provider #5693

alexey-tikhonov commented Jun 20, 2021 •

edited

ikerexxe left a comment

pbrezina left a comment

pbrezina Jul 19, 2021

alexey-tikhonov Jul 23, 2021

pbrezina Jul 28, 2021

alexey-tikhonov Jul 28, 2021

alexey-tikhonov commented Jul 23, 2021

alexey-tikhonov commented Jul 26, 2021

pbrezina commented Jul 27, 2021

pbrezina commented Jul 28, 2021

alexey-tikhonov commented Jul 28, 2021 •

edited

pbrezina commented Jul 29, 2021

alexey-tikhonov commented Jul 29, 2021

pbrezina commented Jul 29, 2021

Basics of 'subid ranges' support for IPA provider #5693

Basics of 'subid ranges' support for IPA provider #5693

Conversation

alexey-tikhonov commented Jun 20, 2021 • edited

ikerexxe left a comment

Choose a reason for hiding this comment

pbrezina left a comment

Choose a reason for hiding this comment

pbrezina Jul 19, 2021

Choose a reason for hiding this comment

alexey-tikhonov Jul 23, 2021

Choose a reason for hiding this comment

pbrezina Jul 28, 2021

Choose a reason for hiding this comment

alexey-tikhonov Jul 28, 2021

Choose a reason for hiding this comment

alexey-tikhonov commented Jul 23, 2021

alexey-tikhonov commented Jul 26, 2021

pbrezina commented Jul 27, 2021

pbrezina commented Jul 28, 2021

alexey-tikhonov commented Jul 28, 2021 • edited

pbrezina commented Jul 29, 2021

alexey-tikhonov commented Jul 29, 2021

pbrezina commented Jul 29, 2021

alexey-tikhonov commented Jun 20, 2021 •

edited

alexey-tikhonov commented Jul 28, 2021 •

edited