Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(RHEL-70103) journal: again create user journals for users with high uids #305

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

jamacku
Copy link
Member

@jamacku jamacku commented Dec 17, 2024

This effectively reverts a change in 115d514 'journald: move uid_for_system_journal() to uid-alloc-range.h', which slipped in an additional check of uid_is_container(uid). The problem is that that change is not backwards-compatible at all and very hard for users to handle. There is no common agreement on mappings of high-range uids. Systemd declares ownership of a large range for container uids in https://systemd.io/UIDS-GIDS/, but this is only a recent change and various sites allocated those ranges in a different way, in particular FreeIPA uses (used?) uids from this range for human users. On big sites with lots of users changing uids is obviously a hard problem. We generally assume that uids cannot be "freed" and/or changed and/or reused safely, so we shouldn't demand the same from others.

This is somewhat similar to the situation with SYSTEM_ALLOC_UID_MIN / SYSTEM_UID_MAX, which we tried to define to a fixed value in our code, causing huge problems for existing systems with were created with a different definition and couldn't be easily updated. For that case, we added a configuration time switch and we now parse /etc/login.defs to actually use the value that is appropriate for the local system.

Unfortunately, login.defs doesn't have a concept of container allocation ranges (and we don't have code to parse and use those nonexistent names either), so we can't tell users to adjust logind.defs to work around the changed definition.

login.defs has SUB_UID_{MIN,MAX}, but those aren't really the same thing, because they are used to define where the add allocations for subuids, which is generally a much smaller range. Maybe we should talk with other folks about the appropriate allocation ranges and define some new settings in login.defs. But this would require discussion and coordination with other projects first.

Actualy, it seems that this change was needed at all. The code in the container does not log to the outside journal. It talks to its own journald, which does journal splitting using its internal logic based on shifted uids. So let's revert the change to fix user systems.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2251843.

Upstream PR: systemd/systemd#30846

rhel-only: bugfix

Resolves: RHEL-70103

This effectively reverts a change in 115d514
'journald: move uid_for_system_journal() to uid-alloc-range.h', which slipped
in an additional check of uid_is_container(uid). The problem is that that change
is not backwards-compatible at all and very hard for users to handle.
There is no common agreement on mappings of high-range uids. Systemd declares
ownership of a large range for container uids in https://systemd.io/UIDS-GIDS/,
but this is only a recent change and various sites allocated those ranges
in a different way, in particular FreeIPA uses (used?) uids from this range
for human users. On big sites with lots of users changing uids is obviously a
hard problem. We generally assume that uids cannot be "freed" and/or changed
and/or reused safely, so we shouldn't demand the same from others.

This is somewhat similar to the situation with SYSTEM_ALLOC_UID_MIN /
SYSTEM_UID_MAX, which we tried to define to a fixed value in our code, causing
huge problems for existing systems with were created with a different
definition and couldn't be easily updated. For that case, we added a
configuration time switch and we now parse /etc/login.defs to actually use the
value that is appropriate for the local system.

Unfortunately, login.defs doesn't have a concept of container allocation ranges
(and we don't have code to parse and use those nonexistent names either), so we
can't tell users to adjust logind.defs to work around the changed definition.

login.defs has SUB_UID_{MIN,MAX}, but those aren't really the same thing,
because they are used to define where the add allocations for subuids, which is
generally a much smaller range. Maybe we should talk with other folks about
the appropriate allocation ranges and define some new settings in login.defs.
But this would require discussion and coordination with other projects first.

Actualy, it seems that this change was needed at all. The code in the container
does not log to the outside journal. It talks to its own journald, which does
journal splitting using its internal logic based on shifted uids. So let's
revert the change to fix user systems.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2251843.

Upstream PR: systemd/systemd#30846

rhel-only: bugfix

Resolves: RHEL-70103
@jamacku jamacku added this to the RHEL-9.6.0 milestone Dec 17, 2024
@jamacku jamacku requested a review from msekletar December 17, 2024 07:40
@github-actions github-actions bot changed the title journal: again create user journals for users with high uids (RHEL-70103) journal: again create user journals for users with high uids Dec 17, 2024
@github-actions github-actions bot added pr/needs-ci Formerly needs-ci pr/needs-review Formerly needs-review labels Dec 17, 2024
Copy link

github-actions bot commented Dec 17, 2024

Commit validation

Tracker - RHEL-70103

The following commits meet all requirements

commit upstream
4ab0221 - journal: again create user journals for users with high uids rhel-only: bugfix

Tracker validation

Success

🟢 Tracker RHEL-70103 has set desired product: rhel-9.6
🟢 Tracker RHEL-70103 has set desired component: systemd
🟢 Tracker RHEL-70103 has been approved
🟢 Tracker RHEL-70103 has set severity


Pull Request validation

Success

🟡 CI - Waived
🟢 Review - Reviewed by a member
🟢 Approval - Changes were approved


Auto Merge

Success

🟢 Pull Request is not marked as draft and it's not blocked by dont-merge label
🟢 Pull Request meet requirements, title has correct form
🟢 Pull Request meet requirements, mergeable is true
🟠 Pull Request meet requirements, mergeable_state is unstable
🟢 Pull Request has correct target branch main
🟢 Pull Request was merged

@github-actions github-actions bot added tracker/missing Formerly needs-bz and removed tracker/missing Formerly needs-bz labels Dec 18, 2024
@jamacku jamacku requested a review from dtardon January 7, 2025 13:34
Copy link
Member

@dtardon dtardon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions github-actions bot removed the pr/needs-review Formerly needs-review label Jan 13, 2025
@jamacku
Copy link
Member Author

jamacku commented Jan 13, 2025

CI failures are unrelated to these changes:

Enrolling secure boot keys from directory: \loader\keys\auto
Failed to write PK secure boot variable: Security violation
systemd-stub@0x67db6000,0x67dce000
Overlapping PE sections detected. Boot may fail due to image memory corruption!

@github-actions github-actions bot removed the pr/needs-ci Formerly needs-ci label Jan 13, 2025
@github-actions github-actions bot merged commit abe0be8 into redhat-plumbers:main Jan 13, 2025
27 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants