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

interfaces: special-case "snapd" in sanitizeSlotReservedForOS* helpers #6844

Merged
merged 2 commits into from May 9, 2019

Conversation

stolowski
Copy link
Contributor

Special-case "snapd" in sanitizeSlotReservedForOSOrGadget and sanitizeSlotReservedForOS helpers used by most of interfaces in their sanitize* mathods, so that hotplug slots can be attached to snapd snap in core18+snapd system.

Note: no unit test for this, because these methods will be completely removed in a followup PR (because respective checks are now made by base declarations). I've verified this fixes hotplug on pi3 manually. Unfortunately removing these helpers affects ~12 interfaces directly, and ~80 tests of interfaces, so a quickfix here to make change minimal for 2.39. Followup will clean all the interfaces in master.

@stolowski stolowski added ⚠ Critical High-priority stuff (e.g. to fix master) Simple 😃 A small PR which can be reviewed quickly labels May 9, 2019
@stolowski stolowski added this to the 2.39 milestone May 9, 2019
@@ -75,15 +75,15 @@ func plugAppLabelExpr(plug *interfaces.ConnectedPlug) string {

// sanitizeSlotReservedForOS checks if slot is of type os.
func sanitizeSlotReservedForOS(iface interfaces.Interface, slot *snap.SlotInfo) error {
if slot.Snap.Type != snap.TypeOS {
if slot.Snap.Type != snap.TypeOS && slot.Snap.InstanceName() != "snapd" {
Copy link
Collaborator

@zyga zyga May 9, 2019

Choose a reason for hiding this comment

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

Should we use snap ID here instead? Though this will interfere with self-built snapd.
Is this in sync with how the policy checker does a similar verification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. If I read it correctly, policy checker indeed uses snap ID (snapIDSnapd function in policy helpers.go). Note though, policy checker is in any case effective, and this temporary quickfix is to just stop custom sanitize from choking before it's removed completely. So If security is a concern, then policy checker takes care of it anyway. I hope that makes sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The policy checker is not considered when doing --dangerous installations so perhaps using snap name here is correct.

Copy link
Collaborator

@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.

+1 based on my reasoning in the discussion inline.

@@ -75,15 +75,15 @@ func plugAppLabelExpr(plug *interfaces.ConnectedPlug) string {

// sanitizeSlotReservedForOS checks if slot is of type os.
func sanitizeSlotReservedForOS(iface interfaces.Interface, slot *snap.SlotInfo) error {
if slot.Snap.Type != snap.TypeOS {
if slot.Snap.Type != snap.TypeOS && slot.Snap.InstanceName() != "snapd" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The policy checker is not considered when doing --dangerous installations so perhaps using snap name here is correct.

@mvo5
Copy link
Contributor

mvo5 commented May 9, 2019

Looks great! Thanks! Is there an existing test we could extend to have some minimal test coverage? Or is that too difficult?

@stolowski
Copy link
Contributor Author

Looks great! Thanks! Is there an existing test we could extend to have some minimal test coverage? Or is that too difficult?

@mvo Added a minimal test now. I didn't include it initially as this code will be go away almost instantly (famous last words ;))

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test!

@stolowski stolowski added the Squash-merge Please squash this PR when merging. label May 9, 2019
@stolowski stolowski merged commit a655c27 into snapcore:master May 9, 2019
stolowski added a commit to stolowski/snapd that referenced this pull request May 9, 2019
snapcore#6844)

* Special-case "snapd" in sanitizeSlotReservedForOSOrGadget and sanitizeSlotReservedForOS helpers.

* Added a minimal test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠ Critical High-priority stuff (e.g. to fix master) Simple 😃 A small PR which can be reviewed quickly Squash-merge Please squash this PR when merging.
Projects
None yet
3 participants