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

Rewrite chuser() for simplicity and correctness #1203

Merged
merged 7 commits into from
Nov 17, 2024
Merged

Rewrite chuser() for simplicity and correctness #1203

merged 7 commits into from
Nov 17, 2024
+111 −67

Conversation

klemensn
Copy link
Contributor

@klemensn klemensn commented Nov 4, 2024 •
edited
Loading

  • Use unambiguous variable names (w/o package name conflict).
  • Fail on invalid input such as the empty string or :.
  • Do not change group without user, i.e. fail on :group.
  • Parse input using mnemonic APIs.
  • Do not juggle between integer types.
  • Unset supplementary groups.
  • Use set[ug]id(2) to follow the idiom of OpenBSD base programs.
    (cannot use setres[ug]id(2) as macOS does not have them.)

Includes/Supersedes #1202.
Fixes #927.

I only tested on OpenBSD (so far), but other systems should just work.

@klemensn klemensn mentioned this pull request Nov 4, 2024
Merged
@neilalexander
Copy link
Member

I'm OK with this, we might be able to write a unit test in core_test.go to prove that it does the right thing on Linux etc.

@klemensn
Rewrite chuser() for simplicity and correctness
a0bfd9d 
- Use unambiguous variable names (w/o package name conflict).
- Fail on invalid input such as the empty string or `:`.
- Do not change group without user, i.e. fail on `:group`.
- Parse input using mnemonic APIs.
- Do not juggle between integer types.
- Unset supplementary groups.
- Use setres[ug]id(2) to match the idiom of OpenBSD base programs.

Includes/Supersedes yggdrasil-network#1202.
Fixes yggdrasil-network#927.

I only tested on OpenBSD (so far), hence the split, but other systems
should just work.
@klemensn
one chuser() to rule them all
3fede90
@klemensn
Copy link
Contributor Author

I'm OK with this, we might be able to write a unit test in core_test.go to prove that it does the right thing on Linux etc.

I simply replaced the existing code.

Looking into tests now...

@klemensn
Copy link
Contributor Author

Now with macOS building, still looks good on OpenBSD:

$ id nobody
uid=32767(nobody) gid=32767(nobody) groups=32767(nobody)
$ ps -c -U nobody -o command,user,ruser,svuid,group,rgroup,svgid,supgrp
COMMAND          USER     RUSER    SVUID GROUP    RGROUP   SVGID SUPGRP
yggdrasil        nobody   nobody   32767 nobody   nobody   32767 nobody

@klemensn
Lookup user/group by ID first, then by name to improve error message
ad3be5a 
`-user foo` would fail with an ugly
	`panic: strconv.Atoi: parsing "foo": invalid syntax`
as returned by `user.LookupId()`, whereas `user.Lookup()` nicely says
	`panic: user: unknown user foo`

In chuser() it does not matter whether we check by ID or name first,
so flip the order to get sensible logs without `fmt.Errorf()` wrapping.
@klemensn
Add chuser() tests
988cd0e 
A bunch of negative cases for the userspace-side parser.

Since the actual set*(2) guts require root, skip accordingly.
@klemensn
Copy link
Contributor Author

I'm OK with this, we might be able to write a unit test in core_test.go to prove that it does the right thing on Linux etc.

test_chuser_unix.go testing chuser() alone is a start.

@klemensn
Omit privileged setgroups(2) call to test set[ug]id(2) to same user
12acd71 
Every user may change its user/group ID to the current one.
With an ugly hack, skip the superuser-only part of chuser()
to exercise this rest of the code path in regular tests.
@klemensn
Copy link
Contributor Author

Unless you run tests as root (who does that?), there is no way to test all of that code automatically.

@neilalexander I've abused an optional function parameter to cover the actual set*id(2) syscalls in CI.
It should work, but it is more of a PoC, which is to say, that I'd rather not have that chuser() change land.

@klemensn
Copy link
Contributor Author

Unless you run tests as root (who does that?), there is no way to test all of that code automatically.

@neilalexander I've abused an optional function parameter to cover the actual set*id(2) syscalls in CI. It should work, but it is more of a PoC, which is to say, that I'd rather not have that chuser() change land.

https://github.com/yggdrasil-network/yggdrasil-go/actions/runs/11787911199/job/32834078977?pr=1203#step:5:19

Linux drops privileges fine in CI.

@neilalexander
Copy link
Member

Is this ready for review in that case?

@klemensn
Copy link
Contributor Author

klemensn commented Nov 17, 2024 •
edited
Loading

Yes, this PR is ready as-is, just like the other two.

neilalexander
neilalexander approved these changes Nov 17, 2024
View reviewed changes
Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@neilalexander neilalexander merged commit c22a746 into yggdrasil-network:develop Nov 17, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neilalexander neilalexander neilalexander approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@klemensn @neilalexander