Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
overlord/ifacestate: fix setup-profiles to use new snap revision for setup #1049
Conversation
|
The underlying issue here felt strange when this logic was first introduced, and seeing this bug so early on seems to show that it is indeed quite dangerous logic. We're storing out-of-date information inside the repository, and then trusting it to be right at arbitrary points in time, despite everything else the system is doing. As such, let's please fix this logic more aggressively: let's stop returning side infos from DisconnectSnap, and return their names instead. This will prevent that data from being misused. Also, please never trust the |
niemeyer
added
the
Reviewed
label
Apr 20, 2016
zyga
added some commits
Apr 25, 2016
pedronis
reviewed
Apr 25, 2016
| } | ||
| + sort.Strings(result) |
zyga
Apr 25, 2016
Contributor
Yes, otherwise the order of Setup() calls is not deterministic and one of the tests catches that.
pedronis
reviewed
Apr 25, 2016
| @@ -300,6 +300,23 @@ func Info(s *state.State, name string, revision int) (*snap.Info, error) { | ||
| return nil, fmt.Errorf("cannot find snap %q at revision %d", name, revision) | ||
| } | ||
| +// Current returns the information about the current revision of a snap with the given name. | ||
| +func Current(s *state.State, name string) (*snap.Info, error) { |
pedronis
Apr 25, 2016
Contributor
this can be simplified a bit:
if err != nil && err != state.ErrNoState {
return nil, err
}
if snapst.Current() == nil {
return nil, fmt.Errorf("cannot find snap %q", name)
}
...
pedronis
reviewed
Apr 25, 2016
| + if err != nil { | ||
| + return err | ||
| + } | ||
| + snap.AddImplicitSlots(snapInfo) |
pedronis
Apr 25, 2016
Contributor
this looks strange? if it's always needed shouldn't something in snap do this instead when we make Infos? otherwise shouldn't there be an helper in ifacestate that gets the slots from an info and fills in the implicit ones then?
zyga
Apr 25, 2016
Contributor
This is still a temporary hack as we should not need this (we should publish this data in the OS snap normally). The helper itself would be weird as AddImplicitSlots is the helper, we access the info objects in various ways each time.
I'd much rather remove this function entirely with the next wave of OS snap updates
zyga
Apr 25, 2016
Contributor
Or, for a more realistic approach, I'd like to keep this as-is and have a separate branch that ensures we just call AddImplicitSlots in one place in snapstate.
pedronis
removed
the
Reviewed
label
Apr 25, 2016
|
yeap, |
|
as mentioned some of the integration test failures seem to point to real issues |
|
@pedronis fixed |
zyga commentedApr 20, 2016
This patch fixes a bug in setup-profiles where on snap upgrade, the
security would be still set up with the older revision of the snap.
This was caused by DisconnectSnap returning the list of snaps
(represented as snap.Info's) that are affected by the disconnect
operation. Those infos were subsequently used to setup the security of
each snap. This didn't work because snap.Info encodes the Revision which
changes on each upgrade.
Note that this bug is not a security vulnerability as apparmor uses the
revision in the actual profile so applications from both revisions are
simply unavailable.
Fixes: https://bugs.launchpad.net/snappy/+bug/1572463
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com