many: add `_daemon_` as valid system username #13052

mvo5 · 2023-08-01T15:45:49Z

With the recent adoption of the _daemon_ user in the spec RK011 by the rocks team we should follow suite and also support the new _daemon_ user for snaps.

This commit implements this support.

We also need to update the documentation once this is in stable and deprecate snap_daemon in favor of _daemon_

codecov-commenter · 2023-08-01T16:06:07Z

Codecov Report

Merging #13052 (4e0207a) into master (22e1eb3) will increase coverage by 0.00%.
Report is 9 commits behind head on master.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff            @@
##           master   #13052    +/-   ##
========================================
  Coverage   78.69%   78.70%            
========================================
  Files        1001     1002     +1     
  Lines      124324   124426   +102     
========================================
+ Hits        97839    97925    +86     
- Misses      20331    20343    +12     
- Partials     6154     6158     +4

Flag	Coverage Δ
unittests	`78.70% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
cmd/snap-seccomp/main.go	`63.38% <100.00%> (ø)`
osutil/user.go	`84.61% <100.00%> (ø)`
snap/validate.go	`95.24% <100.00%> (ø)`

... and 8 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pedronis

looking good, some comments

pedronis · 2023-08-02T11:37:40Z

osutil/user.go

@@ -62,6 +62,11 @@ type AddUserOptions struct {
 // allows as valid usernames
 var IsValidUsername = regexp.MustCompile(`^[a-z0-9][-a-z0-9+._]*$`).MatchString


do we still have use cases for this? do we need to expand its comment?

Right now the thinking is that:
a) IsValidUsername is used to validate the name from osutil.AddUser() which is called by devicestate when adding users from the system-user assertion. Making this slightly more restrictive seems to be good to avoid that those names clash with the "system-usernames" from snap.yaml
b) IsValidSystemUsername is used to validate users from the "system-usernames" snap.yaml via "EnsureUserGroup". It needs to be always a superset of IsValudUsername because e.g. snap-seccomp will use it right now to validate users.

This all points to a problem with terminology though, we have a "system-user" assertion and a "system-usernames" piece in the snap.yaml. I was thinking to rename them to "IsValidAssertionUsername" and "IsValidSystemUsername" maybe? Or "IsValidAddUsername" and "IsValidEnsureUsername" (but the later feels weird).

May leave the first alone and rename IsValidSystemUsername ro IsValidSnapSystemUsername. I think the doc comments of AddUser and EnsureUserGroup should also be updated to clarify their validation rules.

Thanks for the great suggestion, I did this now.

snap/system_usernames.go

With the recent adoption of the `_daemon_` user in the spec RK011 by the rocks team we should follow suite and also support the new `_daemon_` user for snaps. This commit implements this support.

pedronis

thanks, question about the preexisting spread tests

osutil/user.go

pedronis · 2023-08-03T15:53:49Z

tests/main/system-users-are-created/task.yaml

should we change /tests/main/system-usernames to also test extensively daemon as snap_daemon is deprecated ?

Yes, I think this would be a good idea. I will think a bit, either it needs to be a new snap or it needs to be a followup once the _daemon_ user is fully accepted (if we jsut add it as a second username to the existing snap).

Actually I have an idea and prepared https://github.com/snapcore/snapd/compare/master...mvo5:new-daemon-user-rocks-more-tests?expand=1 to allow to test within this PR, I may still do it in a followup for easier reviewing.

And indeed #13060 will be used to update system-usernames but as a separate PR as it adds quite a bit of churn (but could of course merge into this one if desired :)

mvo5 · 2023-08-15T16:01:13Z

@degville we will need to update our docs once this is available in stable that this is the preferred way over snap_daemon.

* many: add `_daemon_` as valid system username With the recent adoption of the `_daemon_` user in the spec RK011 by the rocks team we should follow suite and also support the new `_daemon_` user for snaps. This commit implements this support. * osutil: improve comments * tests: improve system-users-are-created spread test * many: tweak function naming/comment (thanks to Samuele) * osutil: rename EnsureUserGroup->EnsureSnapUserGroup

mvo5 added the Needs Samuele review Needs a review from Samuele before it can land label Aug 1, 2023

pedronis self-requested a review August 2, 2023 08:25

pedronis reviewed Aug 2, 2023

View reviewed changes

mvo5 added the Needs Documentation 📖 label Aug 3, 2023

mvo5 added 2 commits August 3, 2023 09:56

many: add _daemon_ as valid system username

d881685

With the recent adoption of the `_daemon_` user in the spec RK011 by the rocks team we should follow suite and also support the new `_daemon_` user for snaps. This commit implements this support.

osutil: improve comments

b054546

mvo5 force-pushed the new-daemon-user-rocks branch from 09d324a to b054546 Compare August 3, 2023 07:57

pedronis self-requested a review August 3, 2023 08:56

tests: improve system-users-are-created spread test

1556e04

mvo5 marked this pull request as ready for review August 3, 2023 10:26

many: tweak function naming/comment (thanks to Samuele)

4e0207a

pedronis approved these changes Aug 3, 2023

View reviewed changes

osutil: rename EnsureUserGroup->EnsureSnapUserGroup

f083d43

mvo5 mentioned this pull request Aug 3, 2023

tests: modify system-usernames test to also test _daemon_ #13060

Merged

mvo5 added the Squash-merge Please squash this PR when merging. label Aug 3, 2023

mvo5 added this to the 2.61 milestone Aug 4, 2023

Meulengracht approved these changes Aug 14, 2023

View reviewed changes

mvo5 merged commit 83c28ee into snapcore:master Aug 15, 2023
50 of 51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: add `_daemon_` as valid system username #13052

many: add `_daemon_` as valid system username #13052

mvo5 commented Aug 1, 2023 •

edited

codecov-commenter commented Aug 1, 2023 •

edited

pedronis left a comment

pedronis Aug 2, 2023

mvo5 Aug 3, 2023

pedronis Aug 3, 2023

mvo5 Aug 3, 2023

pedronis left a comment

pedronis Aug 3, 2023

mvo5 Aug 3, 2023

mvo5 Aug 3, 2023

mvo5 Aug 3, 2023

mvo5 commented Aug 15, 2023

		@@ -62,6 +62,11 @@ type AddUserOptions struct {
		// allows as valid usernames
		var IsValidUsername = regexp.MustCompile(`^[a-z0-9][-a-z0-9+._]*$`).MatchString

many: add _daemon_ as valid system username #13052

many: add _daemon_ as valid system username #13052

Conversation

mvo5 commented Aug 1, 2023 • edited

codecov-commenter commented Aug 1, 2023 • edited

Codecov Report

pedronis left a 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

pedronis left a 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

mvo5 commented Aug 15, 2023

many: add `_daemon_` as valid system username #13052

many: add `_daemon_` as valid system username #13052

mvo5 commented Aug 1, 2023 •

edited

codecov-commenter commented Aug 1, 2023 •

edited