overlord/ifacestate: fix missing security setup for connected slot #2256

Closed
wants to merge 10 commits into
from

Conversation

Projects
None yet
3 participants
@albaguirre
Contributor

albaguirre commented Nov 3, 2016

Fix not setting up security for connected slot after auto-connection.

overlord/ifacestate: fix missing security setup for connected slot
Fix not setting up security for connected slot after auto-connection.
@chipaca

Thanks for this! There's a bug in the tests, and I have a question about the implementation.

interfaces/repo_test.go
@@ -1334,15 +1334,24 @@ func (s *DisconnectSnapSuite) TestNotConnected(c *C) {
c.Check(affected, HasLen, 0)
}
+func snapNamesFor(snapInfoSet map[*snap.Info]bool) []string {
+ snapNames := make([]string, len(snapInfoSet))

This comment has been minimized.

@chipaca

chipaca Nov 3, 2016

Member

this is wrong: either make it with length 0 and len(snapInfoSet) capacity (and use append as you do below), or make it with this length and use direct assignment below. This way you're ending up with a list 2× the length, with the first half full of empty strings.

@chipaca

chipaca Nov 3, 2016

Member

this is wrong: either make it with length 0 and len(snapInfoSet) capacity (and use append as you do below), or make it with this length and use direct assignment below. This way you're ending up with a list 2× the length, with the first half full of empty strings.

This comment has been minimized.

@albaguirre

albaguirre Nov 3, 2016

Contributor

Ooops you are quite right.

@albaguirre

albaguirre Nov 3, 2016

Contributor

Ooops you are quite right.

interfaces/repo.go
-// The return value is a list of names that were affected.
-func (r *Repository) DisconnectSnap(snapName string) ([]string, error) {
+// The return value is a set of snaps that were affected.
+func (r *Repository) DisconnectSnap(snapName string) (map[*snap.Info]bool, error) {

This comment has been minimized.

@chipaca

chipaca Nov 3, 2016

Member

where are you actually using the fact that you changed from a []string to a map[*snap.Info]bool? That is, what are you buying with it?

@chipaca

chipaca Nov 3, 2016

Member

where are you actually using the fact that you changed from a []string to a map[*snap.Info]bool? That is, what are you buying with it?

This comment has been minimized.

@albaguirre

albaguirre Nov 3, 2016

Contributor

So it now returns a set, so that you can add more affected snaps later (after plug-slot are connected, query connected slots and their corresponding snap infos - see 8463d86#diff-145e00c2f5e1d200010c00f39691a09cR121). That way we avoid duplicates and don't have to merge/dedup if it was just a slice

@albaguirre

albaguirre Nov 3, 2016

Contributor

So it now returns a set, so that you can add more affected snaps later (after plug-slot are connected, query connected slots and their corresponding snap infos - see 8463d86#diff-145e00c2f5e1d200010c00f39691a09cR121). That way we avoid duplicates and don't have to merge/dedup if it was just a slice

This comment has been minimized.

@chipaca

chipaca Nov 3, 2016

Member

Sorry if it's glaringly obvious to you, but where exactly do you use the info, other than to get the name?

@chipaca

chipaca Nov 3, 2016

Member

Sorry if it's glaringly obvious to you, but where exactly do you use the info, other than to get the name?

This comment has been minimized.

@albaguirre

albaguirre Nov 3, 2016

Contributor

So the returned map of affected snaps is further filled in setupProfilesForSnap in handlers.go when calling m.repo.ConnectedSnaps. Then setupAffectedSnaps uses that map - but you are correct it only uses the name from snap.Info

The basic scenario I'm trying to address is:

Scenario 1 - which is what this PR addresses

  • snap A is being installed for the first time and has plug that auto-connects to snap B slot
    • DisconnectSnap returns an empty map as there was nothing to disconnect
    • snap A plug is auto-connected to snap B slot
    • the empty map of affected snaps is passed to m.repo.ConnectedSnaps, which fills it with snapB info
    • map now contains one affected snap info (snap B's)
    • setupAffectedSnaps sets up security for snap B

Scenario 2

  • A new revision of snap A is being installed which has plug connected to snap B slot
    • DisconnectSnap returns a map with one entry (snapB info)
    • snap A plug is auto-connected to snap B slot
    • the current map of affected snaps is passed to m.repo.ConnectedSnaps
    • ConnectedSnaps inserts snapB info - it's already there so no duplication happens (avoids setting up security twice for the same snap)

The map (which is really just used a set) type could just be map[string]bool - but I thought it would be slightly more efficient to hash pointers than strings.

@albaguirre

albaguirre Nov 3, 2016

Contributor

So the returned map of affected snaps is further filled in setupProfilesForSnap in handlers.go when calling m.repo.ConnectedSnaps. Then setupAffectedSnaps uses that map - but you are correct it only uses the name from snap.Info

The basic scenario I'm trying to address is:

Scenario 1 - which is what this PR addresses

  • snap A is being installed for the first time and has plug that auto-connects to snap B slot
    • DisconnectSnap returns an empty map as there was nothing to disconnect
    • snap A plug is auto-connected to snap B slot
    • the empty map of affected snaps is passed to m.repo.ConnectedSnaps, which fills it with snapB info
    • map now contains one affected snap info (snap B's)
    • setupAffectedSnaps sets up security for snap B

Scenario 2

  • A new revision of snap A is being installed which has plug connected to snap B slot
    • DisconnectSnap returns a map with one entry (snapB info)
    • snap A plug is auto-connected to snap B slot
    • the current map of affected snaps is passed to m.repo.ConnectedSnaps
    • ConnectedSnaps inserts snapB info - it's already there so no duplication happens (avoids setting up security twice for the same snap)

The map (which is really just used a set) type could just be map[string]bool - but I thought it would be slightly more efficient to hash pointers than strings.

@albaguirre albaguirre closed this Nov 3, 2016

@albaguirre albaguirre reopened this Nov 3, 2016

albaguirre added some commits Nov 18, 2016

Use a set of strings instead of info pointers
Use a string set to describe the affected snap names - its a bit clearer.
Merge branch 'master' into fix-no-security-setup-on-slot-after-autoco…
…nnection

Conflicts:
    overlord/ifacestate/handlers.go

@albaguirre albaguirre closed this Dec 1, 2016

@albaguirre albaguirre reopened this Dec 1, 2016

@chipaca

I think it's fine now. @zyga should probably do a once-over though.

@chipaca chipaca requested a review from zyga Dec 14, 2016

@zyga

This comment has been minimized.

Show comment
Hide comment
@zyga

zyga Jan 6, 2017

Contributor

I need some time to review this but I'd like not to merge it yet.

Contributor

zyga commented Jan 6, 2017

I need some time to review this but I'd like not to merge it yet.

@zyga

This comment has been minimized.

Show comment
Hide comment
@zyga

zyga Jan 18, 2017

Contributor

Sorry for taking ages to review this.

The bug is real but I'm somewhat unsure about the solution. Let me try to cook something quickly.

Contributor

zyga commented Jan 18, 2017

Sorry for taking ages to review this.

The bug is real but I'm somewhat unsure about the solution. Let me try to cook something quickly.

@zyga

This comment has been minimized.

Show comment
Hide comment
@zyga

zyga Jan 18, 2017

Contributor

Thanks for posting this. I had a look at the root problem and came up with a slightly tweaked solution here: https://github.com/zyga/snapd/tree/fix-autoconnection-setup -- I've reused your tests but the rest is slightly different. I'll update this when a pull request is open.

EDIT: this is now picked up by #2652

Contributor

zyga commented Jan 18, 2017

Thanks for posting this. I had a look at the root problem and came up with a slightly tweaked solution here: https://github.com/zyga/snapd/tree/fix-autoconnection-setup -- I've reused your tests but the rest is slightly different. I'll update this when a pull request is open.

EDIT: this is now picked up by #2652

@zyga zyga closed this Jan 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment