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

cmd/snap-discard-ns: asssert process capabilities #15144

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bboozzoo
Copy link
Contributor

@bboozzoo bboozzoo commented Feb 27, 2025

Assert process capabilities, to ensure correctness when invoked from snap-confine.

Cherry picked from #15094. This is a preparatory step for having snap-discard-ns be invoked by a user with privileges carried by capabilities.

Related: SNAPDENG-34419

@bboozzoo bboozzoo added the Simple 😃 A small PR which can be reviewed quickly label Feb 27, 2025
Copy link

codecov bot commented Feb 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.07%. Comparing base (a272aac) to head (15d2157).
Report is 76 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #15144    +/-   ##
========================================
  Coverage   78.07%   78.07%            
========================================
  Files        1182     1184     +2     
  Lines      157743   158224   +481     
========================================
+ Hits       123154   123538   +384     
- Misses      26943    27005    +62     
- Partials     7646     7681    +35     
Flag Coverage Δ
unittests 78.07% <ø> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Feb 27, 2025

Fri Feb 28 09:55:13 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13583880631

Failures:

Preparing:

  • google-core:ubuntu-core-24-64:tests/main/snap-user-dir-perms-fixed:test
  • google-core:ubuntu-core-24-64:tests/main/snap-user-dir-perms-fixed:root
  • google-core:ubuntu-core-24-64:tests/main/interfaces-desktop-host-fonts-core
  • google-core:ubuntu-core-24-64:tests/regression/lp-1797556

Executing:

  • google-core:ubuntu-core-24-64:tests/main/interfaces-many-core-provided
  • google-core:ubuntu-core-24-64:tests/main/interfaces-shared-memory-private
  • google-core:ubuntu-core-24-64:tests/main/interfaces-personal-files
  • google-core:ubuntu-core-24-64:tests/core/remove-user
  • google-core:ubuntu-core-24-64:tests/main/parallel-install-store
  • google-core:ubuntu-core-24-64:tests/main/refresh-app-awareness:strict
  • google-core:ubuntu-core-24-64:tests/main/snap-debug-stacktrace
  • google-core:ubuntu-core-24-64:tests/main/snap-session-agent-service-control
  • google-core:ubuntu-core-24-64:tests/main/snap-confine-tmp-mount
  • google-core:ubuntu-core-24-64:tests/main/snap-download
  • google-core:ubuntu-core-24-64:tests/main/snap-user-service
  • google-core:ubuntu-core-24-64:tests/main/snap-user-service-start-on-install
  • google-core:ubuntu-core-24-64:tests/main/cgroup-tracking:root
  • google-core:ubuntu-core-24-64:tests/main/snap-routine-portal-info
  • google-core:ubuntu-core-24-64:tests/main/snapshot-users
  • google-core:ubuntu-core-24-64:tests/main/interfaces-browser-support:disallow
  • google-core:ubuntu-core-24-64:tests/main/snap-user-service-socket-activation
  • google-core:ubuntu-core-24-64:tests/main/system-usernames:snap_daemon
  • google-core:ubuntu-core-24-64:tests/main/snap-session-agent-unavailable-to-snaps
  • google-core:ubuntu-core-24-64:tests/main/interfaces-mount-observe
  • google-core:ubuntu-core-24-64:tests/main/interfaces-browser-support:allow
  • google-core:ubuntu-core-24-64:tests/main/regression-home-snap-root-owned
  • google-core:ubuntu-core-24-64:tests/main/sudo-env
  • google-core:ubuntu-core-24-64:tests/main/snapshot-cross-revno
  • google-core:ubuntu-core-24-64:tests/main/snap-user-service-restart-on-upgrade
  • google-core:ubuntu-core-24-64:tests/core/xdg-open-on-core
  • google-core:ubuntu-core-24-64:tests/main/snap-session-agent-socket-activation
  • google-core:ubuntu-core-24-64:tests/main/system-usernames:daemon
  • google-core:ubuntu-core-24-64:tests/main/dbus-activation-session
  • google-core:ubuntu-core-24-64:tests/main/snap-user-service-upgrade-failure
  • google-core:ubuntu-core-24-64:tests/core/create-user-2
  • google-core:ubuntu-core-24-64:tests/main/snap-remove-terminate:fork_bomb
  • google-core:ubuntu-core-24-64:tests/main/parallel-install-basic
  • google-core:ubuntu-core-24-64:tests/main/parallel-install-local
  • google-core:ubuntu-core-24-64:tests/main/snap-confine-undesired-mode-group
  • google-core:ubuntu-core-24-64:tests/main/interfaces-system-observe
  • google-core:ubuntu-core-24-64:tests/smoke/sandbox
  • google-core:ubuntu-core-24-64:tests/smoke/install
  • google-core:ubuntu-core-24-64:tests/regression/lp-1813365
  • google-core:ubuntu-core-24-64:tests/regression/lp-2065077
  • google:ubuntu-20.04-64:tests/unit/go:gcc
  • google:ubuntu-24.10-64:tests/main/cloud-init

Restoring:

  • google-core:ubuntu-core-24-64:tests/main/snap-routine-portal-info
  • google-core:ubuntu-core-24-64:tests/main/snap-user-dir-perms-fixed:test
  • google-core:ubuntu-core-24-64:tests/main/snap-user-dir-perms-fixed:root
  • google-core:ubuntu-core-24-64:tests/regression/lp-1797556

Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

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

LGTM with a typo fix inline.

@@ -63,6 +90,11 @@ int main(int argc, char** argv) {
sc_instance_name_validate(snap_instance_name, &err);
sc_die_on_error(err);

/* time to asssert we have the right capabilities to perform the job */
assert_caps();
/* TODO: drop superfluous capabiltiies and keep only the ones that are
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/* TODO: drop superfluous capabiltiies and keep only the ones that are
/* TODO: drop superfluous capabilities and keep only the ones that are

Copy link
Contributor

@alexmurray alexmurray left a comment

Choose a reason for hiding this comment

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

Due to a typo assert_caps() repeatedly queries for only CAP_SYS_ADMIN rather than each cap in expected_caps.

const char* cap_name SC_CLEANUP(cap_free) = cap_to_name(cap);

cap_flag_value_t set = CAP_CLEAR;
if (cap_get_flag(current, CAP_SYS_ADMIN, CAP_EFFECTIVE, &set) != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a typo - I am guessing CAP_SYS_ADMIN be cap so we query each capability rather than repeatedly querying for CAP_SYS_ADMIN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching. I've pushed a fixup.

@bboozzoo bboozzoo requested a review from alexmurray February 28, 2025 08:01
Assert process capabilities, to ensure correctness when invoked from
snap-confine.

Signed-off-by: Maciej Borzecki <[email protected]>
@bboozzoo bboozzoo force-pushed the bboozoo/snap-discard-ns-caps-assert branch from bc6da80 to 15d2157 Compare February 28, 2025 08:04
Copy link
Contributor

@alexmurray alexmurray left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Simple 😃 A small PR which can be reviewed quickly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants