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.

Sign up for GitHub

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

Jump to bottom

Quadlet: Fix userLevelFilter when UnitDirAdmin is a symlink #23498

Merged
merged 1 commit into from
Aug 11, 2024
Merged

Quadlet: Fix userLevelFilter when UnitDirAdmin is a symlink #23498

merged 1 commit into from
Aug 11, 2024

Conversation

lelemka0
Copy link
Contributor

@lelemka0 lelemka0 commented Aug 4, 2024

Problem

Rootless unit placed in /etc/containers/systemd/users/$(UID) will be loaded for root when /etc/containers/systemd is a symlink.
Fix: #23483

Fix

Try evaluate the path of UnitDirAdmin as a symbolic link.

Does this PR introduce a user-facing change?

None

@rhatdan
Copy link
Member

rhatdan commented Aug 4, 2024

Please fix your commit message.

rhatdan
rhatdan reviewed Aug 4, 2024
View reviewed changes
cmd/quadlet/main.go Outdated Show resolved Hide resolved
@lelemka0 lelemka0 force-pushed the fix/quadlets/userLevelFilter branch from 848b41e to b4a7d0c Compare August 4, 2024 20:38
@lelemka0
Copy link
Contributor Author

lelemka0 commented Aug 4, 2024

Additionally, I found that a similar situation exists in nonNumericFilter.
If UnitDirAdmin/users is a symbolic link, any user will load other users' units.
I will push another commit later.

@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@lelemka0 lelemka0 requested a review from rhatdan August 4, 2024 21:45
ygalblum
ygalblum reviewed Aug 5, 2024
View reviewed changes
cmd/quadlet/main.go Outdated
@@ -183,9 +180,16 @@ func appendSubPaths(dirs []string, path string, isUserFlag bool, filterPtr func(
}

func nonNumericFilter(_path string, isUserFlag bool) bool {
UnitDirAdminUser := filepath.Join(quadlet.UnitDirAdmin, "users")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm missing something here, but can't this code (and the one in userLevelFilter) be executed once somewhere else?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I wasn't clear on my first comment. I wonder if we can compute UnitDirAdminUser and resolvedUnitDirAdmin once instead of every time the filter function is called.

@lelemka0 lelemka0 force-pushed the fix/quadlets/userLevelFilter branch from 29599bf to a7f3831 Compare August 5, 2024 13:26
@lelemka0 lelemka0 requested a review from ygalblum August 5, 2024 13:34
ygalblum
ygalblum reviewed Aug 5, 2024
View reviewed changes
Copy link
Contributor

@ygalblum ygalblum left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.
As you may have noticed, you will also need to add some sort of a test to make sure the functionality works and not less more importantly does not break in future PRs

cmd/quadlet/main.go Outdated
@@ -183,9 +180,16 @@ func appendSubPaths(dirs []string, path string, isUserFlag bool, filterPtr func(
}

func nonNumericFilter(_path string, isUserFlag bool) bool {
UnitDirAdminUser := filepath.Join(quadlet.UnitDirAdmin, "users")
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I wasn't clear on my first comment. I wonder if we can compute UnitDirAdminUser and resolvedUnitDirAdmin once instead of every time the filter function is called.

@lelemka0 lelemka0 force-pushed the fix/quadlets/userLevelFilter branch 2 times, most recently from 88b0e58 to 209f1cd Compare August 5, 2024 17:03
@lelemka0
Copy link
Contributor Author

lelemka0 commented Aug 5, 2024

UnitDirAdminUser and resolvedUnitDirAdmin are computed in getUnitDirs now, so that they will be computed only once during the whole process.
test added.

podman/cmd/quadlet/main_test.go

Lines 102 to 104 in 0bd43f4

t.Setenv("QUADLET_UNIT_DIRS", actualDir)
unitDirs = getUnitDirs(true)
assert.Equal(t, unitDirs, []string{actualDir, innerDir}, "directory resolution should follow symlink")

In the original test for symlinks, because the env is set to actualDir, the test result will always pass. Fixed.
https://github.com/containers/podman/blob/209f1cd9c08c248678d6a73166f1d05bcfb1eeb7/cmd/quadlet/main_test.go#L116

@lelemka0 lelemka0 requested a review from ygalblum August 5, 2024 17:12
ygalblum
ygalblum reviewed Aug 6, 2024
View reviewed changes
cmd/quadlet/main.go Outdated
@@ -59,6 +55,12 @@ var (
}
)

var (
UnitDirAdminUser string
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the nit. But, is there a reason why UnitDirAdminUser and SystemUserDirLevel are exposed and resolvedUnitDirAdminUser isn't? Are they used outside of the module?

@lelemka0 lelemka0 force-pushed the fix/quadlets/userLevelFilter branch from 209f1cd to 0304b30 Compare August 6, 2024 21:01
@lelemka0
Copy link
Contributor Author

lelemka0 commented Aug 6, 2024

It looks like my test fails in Windows Cross because I use syscall.Chroot which doesn't exist in windows.
I think chroot is necessary to verify a specific path, otherwise it will cause modifications to the system.
Actually quadlet is not used in windows, can this part be skipped?

@rhatdan
Copy link
Member

rhatdan commented Aug 6, 2024

Yes this package should only be compiled on Linux (Really for systemd based systems.)

@lelemka0 lelemka0 force-pushed the fix/quadlets/userLevelFilter branch 2 times, most recently from 7f2d946 to f9b79fc Compare August 9, 2024 09:54
@rhatdan
Copy link
Member

rhatdan commented Aug 9, 2024

@lelemka0 could you answer @ygalblum question on variable naming.

@lelemka0 lelemka0 force-pushed the fix/quadlets/userLevelFilter branch from f9b79fc to e159a02 Compare August 9, 2024 10:15
@lelemka0
Copy link
Contributor Author

lelemka0 commented Aug 9, 2024

@rhatdan I have already answered. Actually all three variables are exposed, I don't understand why @ygalblum says resolvedUnitDirAdminUser is not exposed.
https://github.com/containers/podman/blob/e159a025dda19a0a9af0caca364d084c565d7cea/cmd/quadlet/main.go#L58-L62

test works without privilege now.

@rhatdan
Copy link
Member

rhatdan commented Aug 9, 2024

He wants you to change:

 (
	UnitDirAdminUser         string 
 	resolvedUnitDirAdminUser string 
 	SystemUserDirLevel       int 
 )

to

 (
	unitDirAdminUser         string 
 	resolvedUnitDirAdminUser string 
 	systemUserDirLevel       int 
 )

@lelemka0 lelemka0 force-pushed the fix/quadlets/userLevelFilter branch from e159a02 to 53c8d63 Compare August 9, 2024 10:33
@lelemka0
Copy link
Contributor Author

lelemka0 commented Aug 9, 2024

@rhatdan Sorry, I misunderstood.
corrected.

@rhatdan
Copy link
Member

rhatdan commented Aug 9, 2024

LGTM
@ygalblum PTAL

@lelemka0 lelemka0 force-pushed the fix/quadlets/userLevelFilter branch from 53c8d63 to c0a4f0d Compare August 9, 2024 12:27
@lelemka0
Copy link
Contributor Author

lelemka0 commented Aug 9, 2024

test with coverage works now.

ygalblum
ygalblum approved these changes Aug 11, 2024
View reviewed changes
Copy link
Contributor

@ygalblum ygalblum left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Aug 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lelemka0, ygalblum

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2024
@ygalblum
Copy link
Contributor

@lelemka0 sorry for not being clear. Please rebase you branch, it should fix the test failures

@lelemka0 lelemka0 force-pushed the fix/quadlets/userLevelFilter branch from c0a4f0d to 79adafa Compare August 11, 2024 09:59
@lelemka0
Quadlet: fix filters failure when the search paths are symlinks
b0948a5 
Rootless units placed in `users` would be loaded for root when
`/etc/containers/systemd` is a symlink. In this case, since
`UnitDirAdmin` is hardcoded, `userLevelFilter` always returns `true`.
If `/etc/containers/systemd/users` is a symlink, any user would load
other users' units.
Fix the above two problems.

Fixes: containers#23483

Signed-off-by: Uzinn Kagurazaka <[email protected]>
@lelemka0 lelemka0 force-pushed the fix/quadlets/userLevelFilter branch from 79adafa to b0948a5 Compare August 11, 2024 10:01
@ygalblum
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 11, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 277e061 into containers:main Aug 11, 2024
83 checks passed
@Luap99 Luap99 mentioned this pull request Nov 1, 2024
Closed
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Nov 10, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Nov 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ygalblum ygalblum ygalblum approved these changes

@rhatdan rhatdan Awaiting requested review from rhatdan

Assignees

@ygalblum ygalblum

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Milestone
No milestone
3 participants
@lelemka0 @rhatdan @ygalblum