Homed update policy: user changing own settings #31153

AdrianVovk · 2024-01-31T19:55:02Z

Rework of #30109 to deal with changes in #30840 and discussed changes to behavior

Depends on and includes #30840

AdrianVovk · 2024-01-31T19:59:53Z

I mapped out all the ways user_record_authenticate gets called:

Activate -> home_activate -> (check host record)
Activate -> home_activate -> home_activate_luks -> home_setup_luks -> luks_validate_home_record -> (check LUKS embedded record)
Activate -> home_activate -> home_activate_luks -> home_auto_grow_luks -> home_resize_luks -> home_setup_luks -> luks_validate_home_record -> (check LUKS embedded record)
Activate -> home_activate -> ... -> home_refresh -> home_load_embedded_identity -> (check ~/.identity)
Update -> home_update -> (check host record)
Update -> home_update -> home_setup -> home_setup_luks -> luks_validate_home_record -> (check LUKS embedded record)
Update -> home_update -> home_load_embedded_identity
ChangePassword -> home_passwd -> home_load_embedded_identity
ChangePassword -> home_passwd -> home_setup -> home_setup_luks -> luks_validate_home_record -> (check LUKS embedded record)
Resize -> home_resize -> (check host record)
Resize -> home_resize -> home_resize_luks -> home_setup_luks -> luks_validate_home_record -> (check LUKS embedded record)
Fixate -> home_insepct -> (check host record)
Fixate -> home_inspect -> home_setup -> home_setup_luks -> luks_validate_home_record -> (check LUKS embedded record)
Fixate -> home_inspect -> home_load_embedded_identity
Authenticate -> home_inspect -> (check host record)
Authenticate -> home_inspect -> home_setup -> home_setup_luks -> luks_validate_home_record -> (check LUKS embedded record)
Authenticate -> home_inspect -> home_load_embedded_identity
Unlock -> home_unlock -> (check host record)
Deactivate -> home_deactivate -> home_auto_shrink_luks -> home_resize_luks -> home_setup_luks -> luks_validate_home_record -> (check LUKS embedded record)
Acquire can also trigger Activate, Fixate, Authenticate or Unlock depending on what state the home is in

AdrianVovk · 2024-01-31T21:50:38Z

Part of what we discussed with @poettering is changing user_record_authenticate to always look into the kernel keyring and just use that key if it's available. Ultimately this would mean that user_record_authenticate will pass whenever the user is active and unlocked. So given all the code paths above, I want to break down why this is OK. Here we try to keep in mind the motivation for doing all these authentication checks: We never want to be in a situation where the user can unlock a home directory with an out-of-date password, even as we travel between hosts.

All code paths in response to Activate: the user must be inactive, and thus we don't have their key in the keyring and thus user_record_authenticate actually performs authentication
Update
- If the key is available in the keyring, then the user must be active. But Activating the user already verified their password against the embedded records. So this should be safe. In this case, we skip all 3 checks.
- If the key isn't available in the keyring, then the user must be inactive or locked:
  - If the user is locked, we don't permit updates at all and error before even starting homework. Thus the user must be inactive.
  - If we're performing an offline update, we skip the user_record_authenticate and don't even look at the embedded records. So we skip all 3 checks. But then the user will inevitably need to be Activated, which will perform all 3 checks as mentioned above.
  - If we're performing an online update, then we call user_record_authenticate which will actually perform the authentication (since the key isn't in the keyring!). So we do all 3 checks in this case.
ChangePassword
- I think for this one we shouldn't ever load the key from the keyring just to be paranoid
- Thus user_record_authenticate always performs the full authentication check
Resize: Very similar reasoning to Update, except we don't have an offline mode. Thus either the user is active and already verified via Activate, or they're inactive and we perform all three checks.
Fixate: An unfixated user can never be active, and thus we can never have their key in the keyring. So, we perform all 3 checks.
Authenticate: This is like ChangePassword. We shouldn't ever load the key from the keyring. The whole point of this method is to enforce authentication
Unlock: the user's key won't be in keyring, thus user_record_authenticate actually performs authentication
Deactivate: The keyring things are only used to auto-shrink the home area, so the logic from Resize applies. Loading the keys from the keyring to facilitate resize is also just status-quo...
Acquire: this just triggers the other methods depending on the state of the home, so anything we say about those should apply to Acquire

So all this said, what is the practical conclusion?

home_activate shouldn't bother trying to load from the keyring, because it will never find anything
home_update should load from the keyring every time
home_passwd shouldn't load from the keyring, to force password re-entry
home_resize should load from the keyring every time
home_inspect shouldn't load from the keyring. In Fixate's case, it'll never find anything so it shouldn't bother, an in Authenticate's case we don't want it to load from the keyring
home_unlock shouldn't bother
home_deactivate should always load from the keyring so that the eventual resize can work

src/basic/hashmap.c

poettering · 2024-02-13T08:49:23Z

btw, i'd split PRs up a lot more aggressively. If they can stand on their own reasonably I think single-commit PRs are great, too, even. Everything you already got in will make the remainder smaller, and the rebase mess will be reduced.

poettering · 2024-02-13T08:50:06Z

src/basic/hashmap.c

+
+        /* Reuse the array. */
+        FOREACH_ARRAY(e, entries, n)
+                *e = (void*) (*e)->key;


I think I already reviewed this in another PR: the reuse of the array with a specific but different type is not OK.

This MR is stacked on top of #30840

Is there something specific I'm supposed to do so that you don't accidentally review the same things twice over?

src/shared/keyring-util.c

src/home/user-record-util.c

poettering · 2024-04-24T14:24:46Z

conceptually still looks good to me, added some more review points, but the most impotant thing is that the list of editable fields should be itsefl a field on the recrod, as discussed.

Allows the system administrator to configure what fields the user is allowed to edit about themself, along with hard-coded defaults.

This allows an unprivileged user that is active at the console to change the fields that are in the selfModifiable allowlists (introduced in a previous commit) without authenticating as a system administrator. Administrators can disable this behavior per-user by setting the relevant selfModifiable allowlists, or system-wide by changing the policy of the org.freedesktop.home1.update-home-by-owner Polkit action.

github-actions · 2024-04-29T20:00:38Z

Important

An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released.

poettering · 2024-04-30T13:54:39Z

docs/USER_RECORD.md

+is allowed to edit.
+
+`selfModifiablePrivileged` →  Similar to `selfMoidifiableFields`, but it lists fields in
+the `privileged` section that the user is allowed to edit.


why keep this separate from "regular" and "perMachine"? i mean, we'd be really stupid if we'd introduce fields in one section that would exist (or if existance then with different meaning) in the other. Hence I think it should be fine to throw them in the same bin here and keep one list of json fields.

Moidfiable → Modifiable (here and above)

poettering · 2024-04-30T13:55:24Z

src/shared/user-record.c

+         * user to give themselves some unfair advantage over other users on
+         * a given system.
+         */
+        static const char *safe_fields[] = {


static const char *const safe_fields[] = {

poettering · 2024-04-30T13:55:52Z

src/shared/user-record.c

+}
+
+const char **user_record_self_modifiable_blobs(UserRecord *h) {
+        static const char *safe_blobs[] = {


poettering · 2024-04-30T13:56:59Z

src/shared/user-record.h

@@ -431,6 +435,10 @@ uint64_t user_record_capability_bounding_set(UserRecord *h);
 uint64_t user_record_capability_ambient_set(UserRecord *h);
 int user_record_languages(UserRecord *h, char ***ret);

+const char **user_record_self_modifiable_fields(UserRecord *h);
+const char **user_record_self_modifiable_blobs(UserRecord *h);
+const char **user_record_self_modifiable_privileged(UserRecord *h);


please output them in the calls in user-record-show.c

poettering · 2024-04-30T14:00:17Z

src/home/homed-home-bus.c

@@ -423,7 +423,7 @@ int bus_home_update_record(
                Hashmap *blobs,
                uint64_t flags,
                sd_bus_error *error) {
-        int r;
+        int r, safe;


maybe rename "safe" → "relax_access"?

poettering · 2024-04-30T14:02:34Z

src/home/homed-home-bus.c

+                log_warning_errno(safe, "Failed to determine if changes to user record are safe, assuming not: %m");
+                safe = false;
+        } else if (safe) {
+                safe = bus_home_client_is_trusted(h, message);


this of course is slightly skewed here. Because if the client is root this will cause us to use the org.freedesktop.home1.update-home-by-owner PK action, which is of course strictly speaking weird because root is not really the "owner" of such accounts...

hence, either fix this (i.e. add a "bool strict" param to bus_home_client_is_trusted() or so? or at least add a comment explaining why the sloppiness here doesn't matter too much

poettering · 2024-04-30T14:04:15Z

looks good, just some minor things

github-actions bot added documentation build-system util-lib tests sd-boot/sd-stub/bootctl units meson labels Jan 31, 2024

AdrianVovk force-pushed the homed-update-policy-v2 branch from 6ce7881 to c739755 Compare January 31, 2024 22:24

AdrianVovk mentioned this pull request Jan 31, 2024

Update homed user record change policies #30109

Closed

5 tasks

AdrianVovk force-pushed the homed-update-policy-v2 branch 9 times, most recently from e3ae848 to a65fd3d Compare February 2, 2024 20:13

github-actions bot added login sd-bus labels Feb 2, 2024

AdrianVovk force-pushed the homed-update-policy-v2 branch from a65fd3d to 1c0aea3 Compare February 2, 2024 22:18

AdrianVovk marked this pull request as ready for review February 2, 2024 22:18

github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Feb 2, 2024

poettering reviewed Feb 13, 2024

View reviewed changes

src/basic/hashmap.c Outdated Show resolved Hide resolved

poettering added the needs-rebase label Feb 13, 2024

poettering reviewed Feb 13, 2024

View reviewed changes

src/shared/keyring-util.c Outdated Show resolved Hide resolved

github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed needs-rebase labels Mar 11, 2024

bluca removed the rc-blocker 🚧 PRs and Issues tagged this way are blocking the upcoming rc release! label Mar 19, 2024

bluca modified the milestones: v256, v257 Mar 19, 2024

AdrianVovk force-pushed the homed-update-policy-v2 branch from 698ebfc to 529306a Compare March 26, 2024 00:46

AdrianVovk force-pushed the homed-update-policy-v2 branch from 529306a to 6da095b Compare April 23, 2024 22:04

poettering reviewed Apr 24, 2024

View reviewed changes

src/home/user-record-util.c Outdated Show resolved Hide resolved

poettering reviewed Apr 24, 2024

View reviewed changes

src/home/user-record-util.c Outdated Show resolved Hide resolved

poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Apr 24, 2024

AdrianVovk force-pushed the homed-update-policy-v2 branch from 6da095b to d3137bb Compare April 24, 2024 23:50

github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Apr 24, 2024

AdrianVovk force-pushed the homed-update-policy-v2 branch from d3137bb to 2de111d Compare April 25, 2024 22:35

AdrianVovk added 3 commits April 29, 2024 15:56

user-record: Introduce selfModifiable fields

087873a

Allows the system administrator to configure what fields the user is allowed to edit about themself, along with hard-coded defaults.

test: Test user record selfModifiable behavior

a182147

AdrianVovk force-pushed the homed-update-policy-v2 branch from 2de111d to a182147 Compare April 29, 2024 19:59

poettering reviewed Apr 30, 2024

View reviewed changes

poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Apr 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Homed update policy: user changing own settings #31153

Homed update policy: user changing own settings #31153

AdrianVovk commented Jan 31, 2024 •

edited by github-actions bot

AdrianVovk commented Jan 31, 2024 •

edited

AdrianVovk commented Jan 31, 2024 •

edited

poettering commented Feb 13, 2024

poettering Feb 13, 2024

AdrianVovk Feb 13, 2024

poettering commented Apr 24, 2024

github-actions bot commented Apr 29, 2024

poettering Apr 30, 2024

poettering Apr 30, 2024

poettering Apr 30, 2024

poettering Apr 30, 2024

poettering Apr 30, 2024

poettering Apr 30, 2024

poettering Apr 30, 2024

poettering commented Apr 30, 2024

Homed update policy: user changing own settings #31153

Are you sure you want to change the base?

Homed update policy: user changing own settings #31153

Conversation

AdrianVovk commented Jan 31, 2024 • edited by github-actions bot

AdrianVovk commented Jan 31, 2024 • edited

AdrianVovk commented Jan 31, 2024 • edited

poettering commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poettering commented Apr 24, 2024

github-actions bot commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poettering commented Apr 30, 2024

AdrianVovk commented Jan 31, 2024 •

edited by github-actions bot

AdrianVovk commented Jan 31, 2024 •

edited

AdrianVovk commented Jan 31, 2024 •

edited