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

o/hookstate/ctlcmd: add optional --pid and --apparmor-label arguments to "snapctl is-connected" #9132

Merged
merged 16 commits into from
Feb 3, 2021
Merged

o/hookstate/ctlcmd: add optional --pid and --apparmor-label arguments to "snapctl is-connected" #9132

merged 16 commits into from
Feb 3, 2021

Conversation

jhenstridge
Copy link
Contributor

@jhenstridge jhenstridge commented Aug 11, 2020 •
edited
Loading

This PR is an implementation of @pedronis's snapctl is-connected --pid proposal described in this forum thread:

https://forum.snapcraft.io/t/autoconnect-requests-for-pulseaudio/18926/3?u=jamesh

Unlike regular snapctl is-connected, the --pid option only considers connections with the snap identified by the given process ID, as identified by cgroup.SnapNameFromPid. The intention is to allow a snap daemon using local sockets check that the peer it is talking to has connected a given interface.

For example, a Pulse Audio snap may provide audio-record and audio-playback slots whose connection grant access to the daemon's socket. The daemon can issue a snapctl is-connected --pid $pid audio-record to determine whether a particular client has been granted access to the microphone.

In addition to the normal 0/1 exit codes, the command may also return the following:

  • 10: process ID does not refer to a snap
  • 11: plug/slot is not connected, and process ID refers to a classic confined snap.

These both look like "false" to the shell, but can be used by services that want to communicate with host system processes or classic snaps.

There is a placeholder function to check whether to allow the feature to be used with a particular plug/slot. At present, it hard codes it to allow use on slots of interface cups-control, pulseaudio, or audio-record.

@tillkamppeter
Copy link

CUPS is supposed to allow access from any non-snapped process and any classic-mode-snapped process in addition to standard-snapped processes plugging to "cups-control", so either the mentioned command should return "true" in all these cases or have special answers for non-snapped and classic-mode-snapped processes.

@tillkamppeter
Copy link

Is it also possible for CUPS to do this with an API call or only by calling this command line tool?

@jhenstridge
Copy link
Contributor Author

CUPS is supposed to allow access from any non-snapped process and any classic-mode-snapped process in addition to standard-snapped processes plugging to "cups-control"

Good point about classic confinement snaps. I agree that for the Pulse Audio and CUPS use cases it would make sense to treat classic snaps like unconfined processes. It does muddy things a bit though, since classic snaps can have plugs and slots.

Is it also possible for CUPS to do this with an API call or only by calling this command line tool?

Yes. The snapd-glib library includes a snapd_client_run_snapctl_sync method. You'd probably need to call snapd_client_set_socket_path first to have the SnapdClient talk to /run/snapd-snap.socket instead of /run/snapd.socket though.

Since you'd be dealing with only a process ID, this would remove the need for determining and parsing the AppArmor label of the peer too.

@jhenstridge jhenstridge mentioned this pull request Aug 17, 2020
Merged
zyga
zyga reviewed Aug 24, 2020
View reviewed changes
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.

Technically this looks correct and feels nice.

I left some comments. Please ping for a full review when not a draft.

overlord/hookstate/ctlcmd/is_connected.go Outdated Show resolved Hide resolved
overlord/hookstate/ctlcmd/is_connected.go Outdated Show resolved Hide resolved
zyga
zyga reviewed Aug 28, 2020
View reviewed changes
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.

Quick pass through the spread test (my favourite part of snapd patches)

tests/main/snapctl-is-connected-pid/task.yaml Show resolved Hide resolved
tests/main/snapctl-is-connected-pid/task.yaml Outdated Show resolved Hide resolved
tests/main/snapctl-is-connected-pid/task.yaml Outdated Show resolved Hide resolved
tests/main/snapctl-is-connected-pid/task.yaml Outdated Show resolved Hide resolved
@pedronis pedronis self-requested a review September 3, 2020 08:30
@pedronis pedronis added the Needs Samuele review Needs a review from Samuele before it can land label Sep 3, 2020
@tillkamppeter
Copy link

@jhenstridge, anything still missing to drop the "Draft" status? I am waiting for this to get in, so that the CUPs Snap can finally get into the Snap Store.

@jhenstridge jhenstridge marked this pull request as ready for review September 15, 2020 10:52
@jhenstridge
Copy link
Contributor Author

I've flipped the PR over to ready for review. I added a check to restrict the feature to one of a few slot types. That check probably should be changed a bit or moved to a more appropriate location, but I'm not sure where exactly.

@pedronis
Copy link
Collaborator

@jhenstridge I looked at this and spent some time thinking, let's chat about it

@pedronis pedronis added Needs security review Can only be merged once security gave a :+1: ⛔ Blocked labels Sep 17, 2020
@jhenstridge jhenstridge changed the title o/hookstate/ctlcmd: add optional --pid argument to "snapctl is-connected" o/hookstate/ctlcmd: add optional --pid and --apparmor-label arguments to "snapctl is-connected" Oct 6, 2020
@jhenstridge jhenstridge force-pushed the snapctl-is-connected-pid branch 2 times, most recently from d9f68cb to 722e2c3 Compare October 7, 2020 09:51
@alexmurray
Copy link
Contributor

This looks good to me from a security point of view, there are comprehensive tests and the implementation looks solid. Just waiting to follow up on Samuele's question above before signing off on this with security review.

@pedronis pedronis self-requested a review January 11, 2021 15:39
@alexmurray
Copy link
Contributor

Security review done as per the previous comments / discussion 👍

@alexmurray alexmurray removed the Needs security review Can only be merged once security gave a :+1: label Jan 13, 2021
@anonymouse64 anonymouse64 self-requested a review January 19, 2021 14:10
pedronis
pedronis approved these changes Jan 26, 2021
View reviewed changes
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

thanks, a couple of comments/questions

overlord/hookstate/ctlcmd/is_connected.go Outdated Show resolved Hide resolved
overlord/hookstate/ctlcmd/is_connected.go Outdated Show resolved Hide resolved
@pedronis pedronis self-requested a review January 28, 2021 09:51
pedronis
pedronis reviewed Jan 28, 2021
View reviewed changes
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

@jhenstridge I answered to your last replies making some suggestions

anonymouse64
anonymouse64 approved these changes Feb 2, 2021
View reviewed changes
Copy link
Contributor

@anonymouse64 anonymouse64 left a comment

Choose a reason for hiding this comment

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

a couple terminology changes to use more inclusive language, but other than that lgtm, thanks for working on this

overlord/hookstate/ctlcmd/is_connected_test.go Outdated Show resolved Hide resolved
overlord/hookstate/ctlcmd/is_connected_test.go Outdated Show resolved Hide resolved
overlord/hookstate/ctlcmd/is_connected_test.go Outdated Show resolved Hide resolved
overlord/hookstate/ctlcmd/is_connected_test.go Outdated Show resolved Hide resolved
@jhenstridge
tests: disable classic checks of snapctl-is-connected-pid on Ubuntu 1…
f3c91c9 
…4.04

These tests rely on the ability to identify the snap a process ID
belongs to, which means either (a) the systemd or unified cgroup name
includes the snap name, or (b) the freezer cgroup includes the snap
name.

On Ubuntu 14.04, processes are not tracked via systemd so (a) fails.
And snap-confine does not add classic confined processes to a freezer
cgroup, so (b) fails as well.

We're still runing the parts of the test that deal with strict confined
snaps, since that functions correctly on 14.04.
@jhenstridge jhenstridge merged commit 53e00c4 into canonical:master Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexmurray alexmurray alexmurray left review comments

@zyga zyga zyga left review comments

@pedronis pedronis pedronis approved these changes

@anonymouse64 anonymouse64 anonymouse64 approved these changes

Assignees
No one assigned
Labels
Needs Samuele review Needs a review from Samuele before it can land
Projects
None yet
Milestone
No milestone
6 participants
@jhenstridge @tillkamppeter @pedronis @alexmurray @zyga @anonymouse64