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

Sudo-less operation #2408

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Sudo-less operation #2408

wants to merge 16 commits into from

Conversation

vista-
Copy link
Contributor

@vista- vista- commented Jan 17, 2025

This PR adds sudo-less operation to Containerlab. See #2307 for more information on this feature.

Sudo-less?

The sudo-less operation is implemented via setting the SUID (or sticky) bit on the Containerlab binary. The changes include handling of SUID-based running of Containerlab, and properly grabbing the correct user for lab directory ACL/ownership management when not run with sudo.

Security?

Root privileges are not used where they are not needed, as Containerlab immediately drops to regular user privilege on startup, and only escalates to root privilege where needed (this is currently done on a per-command granularity).

For use-cases where sudo-less operation is required (for example, Containerlab hosts and labs shared between colleagues, or in education), it is not necessarily desired for every user to be able to perform privileged operations on containerlabs, after all, being able to spin up arbitrary containers is the same as granting sudo rights. However, operations that don't require such privileges, such as saving configs, or creating topology graphs might still be useful for these users.

This feature also includes an optional check for group membership of the clab_admins group:

  • If the group does not exist, the check is bypassed
  • If the group exists, a check is performed whether the user running Containerlab is a member of the clab_admins Unix group before escalating to root privileges

The clab_admins group is created and the user running the command is added to it by default by the upgrade script, the quick install script and the post-install script.

Potential breakages

  • groupadd/usermod might not work on the post-install script for Alpine without shadow utils installed -- this platform is not officially supported by Containerlab, but nfpm is still configured to emit an apk file.

@kaelemc
Copy link
Contributor

kaelemc commented Jan 19, 2025

Brilliant! sudoless working great on deploy/destroy/redeploy. Tested that the group check is working correctly as well.

Windows/minGW! I'd appreciate if someone could test this out.

I see mingw in the get.sh but surely this isn't supported for clab on windows @hellt? The blame says it's from 5 years ago too. If I'm missing something here and it is supported, I'll happily test.

Let me know if there's anything specific you want tested.

@hellt
Copy link
Member

hellt commented Jan 19, 2025

correct, windows is not supported. Windows users should use WSL

@hellt hellt self-assigned this Jan 23, 2025
@vista-
Copy link
Contributor Author

vista- commented Jan 24, 2025

I have added docs about sudoless operations and made the creation of the clab_admins group only happen on the first install/upgrade past v0.6.3.
This is done by creating a file under /etc/containerlab/ to signal that the setup was already done.

This way, admins who would prefer to have no clab_admins group won't have it created on every upgrade.

get.sh Show resolved Hide resolved
docs/install.md Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
@hellt
Copy link
Member

hellt commented Jan 29, 2025

I think what would be good is to change one of the integration tests to use sudo-less commands. Since the binary that we build for integration tests is built with make, we just need to add chmod command to it, I think, and then remove sudo from the smoke test suite

Copy link

codecov bot commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 32.03883% with 70 lines in your changes missing coverage. Please review.

Project coverage is 52.49%. Comparing base (2718217) to head (2845390).

Files with missing lines Patch % Lines
cmd/common/sudo.go 18.64% 43 Missing and 5 partials ⚠️
utils/file.go 51.42% 13 Missing and 4 partials ⚠️
cmd/root.go 25.00% 2 Missing and 1 partial ⚠️
cmd/version/upgrade.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2408      +/-   ##
==========================================
- Coverage   52.61%   52.49%   -0.12%     
==========================================
  Files         170      170              
  Lines       17106    17164      +58     
==========================================
+ Hits         9000     9011      +11     
- Misses       7171     7215      +44     
- Partials      935      938       +3     
Files with missing lines Coverage Δ
cmd/deploy.go 76.84% <ø> (ø)
cmd/destroy.go 77.77% <ø> (ø)
cmd/disableTxOffload.go 12.50% <ø> (ø)
cmd/exec.go 77.27% <ø> (ø)
cmd/inspect.go 75.11% <ø> (ø)
cmd/redeploy.go 90.62% <ø> (ø)
cmd/save.go 73.33% <ø> (ø)
cmd/tools_netem.go 70.87% <100.00%> (+0.28%) ⬆️
cmd/tools_veth.go 72.78% <ø> (ø)
cmd/vxlan.go 61.62% <ø> (ø)
... and 6 more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants