From 298c65c21370ec02b53e74c8e58533c640696185 Mon Sep 17 00:00:00 2001 From: Philip Meulengracht Date: Mon, 23 May 2022 09:23:27 +0200 Subject: [PATCH 1/8] dirs: add directory entry for systemd root dir --- dirs/dirs.go | 1 + 1 file changed, 1 insertion(+) diff --git a/dirs/dirs.go b/dirs/dirs.go index 2a83f879850..188c0c5ea2f 100644 --- a/dirs/dirs.go +++ b/dirs/dirs.go @@ -99,6 +99,7 @@ var ( SnapRuntimeServicesDir string SnapUserServicesDir string SnapSystemdConfDir string + SnapSystemdDir string SnapDesktopFilesDir string SnapDesktopIconsDir string SnapPolkitPolicyDir string From cc01bbdd9fdcde749abaf9bf24e0426fde2beeac Mon Sep 17 00:00:00 2001 From: Philip Meulengracht Date: Mon, 23 May 2022 09:24:04 +0200 Subject: [PATCH 2/8] overlord/ifacestate: add code to handle journal quota groups, which adds the correct mount layout in confinement options when a snap has a quota group with a journal quota set --- overlord/ifacestate/handlers.go | 87 +++++++++++++++++++++++++++------ overlord/ifacestate/helpers.go | 2 +- 2 files changed, 73 insertions(+), 16 deletions(-) diff --git a/overlord/ifacestate/handlers.go b/overlord/ifacestate/handlers.go index e23209e1d6b..26aac1c43bd 100644 --- a/overlord/ifacestate/handlers.go +++ b/overlord/ifacestate/handlers.go @@ -22,6 +22,7 @@ package ifacestate import ( "errors" "fmt" + "path" "reflect" "sort" "strings" @@ -29,26 +30,82 @@ import ( "gopkg.in/tomb.v2" + "github.com/snapcore/snapd/dirs" "github.com/snapcore/snapd/i18n" "github.com/snapcore/snapd/interfaces" "github.com/snapcore/snapd/interfaces/hotplug" "github.com/snapcore/snapd/logger" "github.com/snapcore/snapd/overlord/hookstate" + "github.com/snapcore/snapd/overlord/servicestate" "github.com/snapcore/snapd/overlord/snapstate" "github.com/snapcore/snapd/overlord/state" "github.com/snapcore/snapd/snap" + "github.com/snapcore/snapd/snap/quota" "github.com/snapcore/snapd/timings" ) var snapstateFinishRestart = snapstate.FinishRestart +func getQuotaGroup(st *state.State, snapName string) *quota.Group { + allGrps, err := servicestate.AllQuotas(st) + if err != nil { + return nil + } + + for _, grp := range allGrps { + for _, name := range grp.Snaps { + if name == snapName { + return grp + } + } + } + return nil +} + +func addJournalQuotaLayout(quotaGroup *quota.Group, layouts *[]snap.Layout) error { + if quotaGroup.JournalLimit == nil { + return nil + } + + // We need to bind mount the journal namespace folder ontop of + // the journal folder + // /etc/systemd/journal. -> /etc/systemd/journal + journalLayout := snap.Layout{ + Bind: path.Join(dirs.SnapSystemdDir, fmt.Sprintf("journal.snap-%s", quotaGroup.Name)), + Path: path.Join(dirs.SnapSystemdDir, "journal"), + Mode: 0755, + } + *layouts = append(*layouts, journalLayout) + return nil +} + +func getExtraLayouts(st *state.State, snapName string) ([]snap.Layout, error) { + var extraLayouts []snap.Layout + if quotaGrp := getQuotaGroup(st, snapName); quotaGrp != nil { + if err := addJournalQuotaLayout(quotaGrp, &extraLayouts); err != nil { + return extraLayouts, err + } + } + + return extraLayouts, nil +} + // confinementOptions returns interfaces.ConfinementOptions from snapstate.Flags. -func confinementOptions(flags snapstate.Flags) interfaces.ConfinementOptions { +func confinementOptions(flags snapstate.Flags, extraLayouts []snap.Layout) interfaces.ConfinementOptions { return interfaces.ConfinementOptions{ - DevMode: flags.DevMode, - JailMode: flags.JailMode, - Classic: flags.Classic, + DevMode: flags.DevMode, + JailMode: flags.JailMode, + Classic: flags.Classic, + ExtraLayouts: extraLayouts, + } +} + +func buildConfinementOptions(st *state.State, snapName string, flags snapstate.Flags) interfaces.ConfinementOptions { + extraLayouts, err := getExtraLayouts(st, snapName) + if err != nil { + logger.Noticef("cannot get extra mount layouts of snap %q: %s", snapName, err) } + return confinementOptions(flags, extraLayouts) } func (m *InterfaceManager) setupAffectedSnaps(task *state.Task, affectingSnap string, affectedSnaps []string, tm timings.Measurer) error { @@ -72,7 +129,7 @@ func (m *InterfaceManager) setupAffectedSnaps(task *state.Task, affectingSnap st if err := addImplicitSlots(st, affectedSnapInfo); err != nil { return err } - opts := confinementOptions(snapst.Flags) + opts := buildConfinementOptions(st, affectedSnapInfo.SnapName(), snapst.Flags) if err := m.setupSnapSecurity(task, affectedSnapInfo, opts, tm); err != nil { return err } @@ -115,7 +172,7 @@ func (m *InterfaceManager) doSetupProfiles(task *state.Task, tomb *tomb.Tomb) er return nil } - opts := confinementOptions(snapsup.Flags) + opts := buildConfinementOptions(task.State(), snapInfo.SnapName(), snapsup.Flags) return m.setupProfilesForSnap(task, tomb, snapInfo, opts, perfTimings) } @@ -206,7 +263,7 @@ func (m *InterfaceManager) setupProfilesForSnap(task *state.Task, _ *tomb.Tomb, return err } affectedSnaps = append(affectedSnaps, snapInfo) - confinementOpts = append(confinementOpts, confinementOptions(snapst.Flags)) + confinementOpts = append(confinementOpts, buildConfinementOptions(st, name, snapst.Flags)) } return m.setupSecurityByBackend(task, affectedSnaps, confinementOpts, tm) @@ -297,7 +354,7 @@ func (m *InterfaceManager) undoSetupProfiles(task *state.Task, tomb *tomb.Tomb) if err != nil { return err } - opts := confinementOptions(snapst.Flags) + opts := buildConfinementOptions(task.State(), snapName, snapst.Flags) return m.setupProfilesForSnap(task, tomb, snapInfo, opts, perfTimings) } } @@ -499,12 +556,12 @@ func (m *InterfaceManager) doConnect(task *state.Task, _ *tomb.Tomb) (err error) }() if !delayedSetupProfiles { - slotOpts := confinementOptions(slotSnapst.Flags) + slotOpts := buildConfinementOptions(st, slotRef.Snap, slotSnapst.Flags) if err := m.setupSnapSecurity(task, slot.Snap, slotOpts, perfTimings); err != nil { return err } - plugOpts := confinementOptions(plugSnapst.Flags) + plugOpts := buildConfinementOptions(st, plugRef.Name, plugSnapst.Flags) if err := m.setupSnapSecurity(task, plug.Snap, plugOpts, perfTimings); err != nil { return err } @@ -605,7 +662,7 @@ func (m *InterfaceManager) doDisconnect(task *state.Task, _ *tomb.Tomb) error { if err != nil { return err } - opts := confinementOptions(snapst.Flags) + opts := buildConfinementOptions(st, snapInfo.SnapName(), snapst.Flags) if err := m.setupSnapSecurity(task, snapInfo, opts, perfTimings); err != nil { return err } @@ -711,11 +768,11 @@ func (m *InterfaceManager) undoDisconnect(task *state.Task, _ *tomb.Tomb) error return err } - slotOpts := confinementOptions(slotSnapst.Flags) + slotOpts := buildConfinementOptions(st, connRef.SlotRef.Snap, slotSnapst.Flags) if err := m.setupSnapSecurity(task, slot.Snap, slotOpts, perfTimings); err != nil { return err } - plugOpts := confinementOptions(plugSnapst.Flags) + plugOpts := buildConfinementOptions(st, connRef.PlugRef.Snap, plugSnapst.Flags) if err := m.setupSnapSecurity(task, plug.Snap, plugOpts, perfTimings); err != nil { return err } @@ -794,11 +851,11 @@ func (m *InterfaceManager) undoConnect(task *state.Task, _ *tomb.Tomb) error { if err != nil { return err } - slotOpts := confinementOptions(slotSnapst.Flags) + slotOpts := buildConfinementOptions(st, slotRef.Snap, slotSnapst.Flags) if err := m.setupSnapSecurity(task, slot.Snap, slotOpts, perfTimings); err != nil { return err } - plugOpts := confinementOptions(plugSnapst.Flags) + plugOpts := buildConfinementOptions(st, plugRef.Snap, plugSnapst.Flags) if err := m.setupSnapSecurity(task, plug.Snap, plugOpts, perfTimings); err != nil { return err } diff --git a/overlord/ifacestate/helpers.go b/overlord/ifacestate/helpers.go index 725e5cdcc70..fc5d3c68cd0 100644 --- a/overlord/ifacestate/helpers.go +++ b/overlord/ifacestate/helpers.go @@ -177,7 +177,7 @@ func (m *InterfaceManager) regenerateAllSecurityProfiles(tm timings.Measurer) er if err := snapstate.Get(m.state, snapName, &snapst); err != nil { logger.Noticef("cannot get state of snap %q: %s", snapName, err) } - return confinementOptions(snapst.Flags) + return buildConfinementOptions(m.state, snapName, snapst.Flags) } // For each backend: From e2fa16e55192d9e67f182a67d80fec5af2aeecfd Mon Sep 17 00:00:00 2001 From: Philip Meulengracht Date: Tue, 7 Jun 2022 12:18:17 +0200 Subject: [PATCH 3/8] dirs: fix rebase --- dirs/dirs.go | 1 - 1 file changed, 1 deletion(-) diff --git a/dirs/dirs.go b/dirs/dirs.go index 188c0c5ea2f..2a83f879850 100644 --- a/dirs/dirs.go +++ b/dirs/dirs.go @@ -99,7 +99,6 @@ var ( SnapRuntimeServicesDir string SnapUserServicesDir string SnapSystemdConfDir string - SnapSystemdDir string SnapDesktopFilesDir string SnapDesktopIconsDir string SnapPolkitPolicyDir string From 9fd2e81bcfeed92d312e046749e74022ecce76e0 Mon Sep 17 00:00:00 2001 From: Philip Meulengracht Date: Tue, 7 Jun 2022 13:13:06 +0200 Subject: [PATCH 4/8] overlord/ifacestate: update docs, add unit test, reuse SnapServiceOptions in servicestate instead of looking up quota group manually --- overlord/ifacestate/export_test.go | 1 + overlord/ifacestate/handlers.go | 65 +++++++------- overlord/ifacestate/handlers_test.go | 123 ++++++++++++++++++++++----- overlord/ifacestate/helpers.go | 2 +- 4 files changed, 135 insertions(+), 56 deletions(-) diff --git a/overlord/ifacestate/export_test.go b/overlord/ifacestate/export_test.go index 9f6eefb131e..ae147661e34 100644 --- a/overlord/ifacestate/export_test.go +++ b/overlord/ifacestate/export_test.go @@ -61,6 +61,7 @@ var ( BatchConnectTasks = batchConnectTasks FirstTaskAfterBootWhenPreseeding = firstTaskAfterBootWhenPreseeding + BuildConfinementOptions = buildConfinementOptions ) type ConnectOpts = connectOpts diff --git a/overlord/ifacestate/handlers.go b/overlord/ifacestate/handlers.go index 26aac1c43bd..f05b6b4c7c9 100644 --- a/overlord/ifacestate/handlers.go +++ b/overlord/ifacestate/handlers.go @@ -22,6 +22,7 @@ package ifacestate import ( "errors" "fmt" + "log" "path" "reflect" "sort" @@ -46,27 +47,16 @@ import ( var snapstateFinishRestart = snapstate.FinishRestart -func getQuotaGroup(st *state.State, snapName string) *quota.Group { - allGrps, err := servicestate.AllQuotas(st) - if err != nil { - return nil - } - - for _, grp := range allGrps { - for _, name := range grp.Snaps { - if name == snapName { - return grp - } - } - } - return nil -} - +// addJournalQuotaLayout handles the addition of a journal quota bind mount +// in case the snap has a journal quota. This mimicks what systemd does for +// services with log namespaces. func addJournalQuotaLayout(quotaGroup *quota.Group, layouts *[]snap.Layout) error { + log.Println("3.1") if quotaGroup.JournalLimit == nil { return nil } + log.Println("3.2") // We need to bind mount the journal namespace folder ontop of // the journal folder // /etc/systemd/journal. -> /etc/systemd/journal @@ -79,10 +69,18 @@ func addJournalQuotaLayout(quotaGroup *quota.Group, layouts *[]snap.Layout) erro return nil } -func getExtraLayouts(st *state.State, snapName string) ([]snap.Layout, error) { +// getExtraLayouts helper function to dynamically calculate the extra mount layouts for +// a snap instance. These are the layouts which can change during the lifetime of a snap +// like for instance mimicking systemd journal namespace mount layouts. +func getExtraLayouts(st *state.State, snapInstanceName string) ([]snap.Layout, error) { + snapOpts, err := servicestate.SnapServiceOptions(st, snapInstanceName, nil) + if err != nil { + return nil, err + } + var extraLayouts []snap.Layout - if quotaGrp := getQuotaGroup(st, snapName); quotaGrp != nil { - if err := addJournalQuotaLayout(quotaGrp, &extraLayouts); err != nil { + if snapOpts.QuotaGroup != nil { + if err := addJournalQuotaLayout(snapOpts.QuotaGroup, &extraLayouts); err != nil { return extraLayouts, err } } @@ -100,10 +98,11 @@ func confinementOptions(flags snapstate.Flags, extraLayouts []snap.Layout) inter } } -func buildConfinementOptions(st *state.State, snapName string, flags snapstate.Flags) interfaces.ConfinementOptions { - extraLayouts, err := getExtraLayouts(st, snapName) +func buildConfinementOptions(st *state.State, snapInstanceName string, flags snapstate.Flags) interfaces.ConfinementOptions { + extraLayouts, err := getExtraLayouts(st, snapInstanceName) if err != nil { - logger.Noticef("cannot get extra mount layouts of snap %q: %s", snapName, err) + logger.Noticef("cannot get extra mount layouts of snap %q: %s", snapInstanceName, err) + log.Printf("cannot get extra mount layouts of snap %q: %s", snapInstanceName, err) } return confinementOptions(flags, extraLayouts) } @@ -129,7 +128,7 @@ func (m *InterfaceManager) setupAffectedSnaps(task *state.Task, affectingSnap st if err := addImplicitSlots(st, affectedSnapInfo); err != nil { return err } - opts := buildConfinementOptions(st, affectedSnapInfo.SnapName(), snapst.Flags) + opts := buildConfinementOptions(st, affectedSnapInfo.InstanceName(), snapst.Flags) if err := m.setupSnapSecurity(task, affectedSnapInfo, opts, tm); err != nil { return err } @@ -172,7 +171,7 @@ func (m *InterfaceManager) doSetupProfiles(task *state.Task, tomb *tomb.Tomb) er return nil } - opts := buildConfinementOptions(task.State(), snapInfo.SnapName(), snapsup.Flags) + opts := buildConfinementOptions(task.State(), snapInfo.InstanceName(), snapsup.Flags) return m.setupProfilesForSnap(task, tomb, snapInfo, opts, perfTimings) } @@ -263,7 +262,7 @@ func (m *InterfaceManager) setupProfilesForSnap(task *state.Task, _ *tomb.Tomb, return err } affectedSnaps = append(affectedSnaps, snapInfo) - confinementOpts = append(confinementOpts, buildConfinementOptions(st, name, snapst.Flags)) + confinementOpts = append(confinementOpts, buildConfinementOptions(st, snapInfo.InstanceName(), snapst.Flags)) } return m.setupSecurityByBackend(task, affectedSnaps, confinementOpts, tm) @@ -354,7 +353,7 @@ func (m *InterfaceManager) undoSetupProfiles(task *state.Task, tomb *tomb.Tomb) if err != nil { return err } - opts := buildConfinementOptions(task.State(), snapName, snapst.Flags) + opts := buildConfinementOptions(task.State(), snapInfo.InstanceName(), snapst.Flags) return m.setupProfilesForSnap(task, tomb, snapInfo, opts, perfTimings) } } @@ -556,12 +555,12 @@ func (m *InterfaceManager) doConnect(task *state.Task, _ *tomb.Tomb) (err error) }() if !delayedSetupProfiles { - slotOpts := buildConfinementOptions(st, slotRef.Snap, slotSnapst.Flags) + slotOpts := buildConfinementOptions(st, slotSnapst.InstanceName(), slotSnapst.Flags) if err := m.setupSnapSecurity(task, slot.Snap, slotOpts, perfTimings); err != nil { return err } - plugOpts := buildConfinementOptions(st, plugRef.Name, plugSnapst.Flags) + plugOpts := buildConfinementOptions(st, plugSnapst.InstanceName(), plugSnapst.Flags) if err := m.setupSnapSecurity(task, plug.Snap, plugOpts, perfTimings); err != nil { return err } @@ -662,7 +661,7 @@ func (m *InterfaceManager) doDisconnect(task *state.Task, _ *tomb.Tomb) error { if err != nil { return err } - opts := buildConfinementOptions(st, snapInfo.SnapName(), snapst.Flags) + opts := buildConfinementOptions(st, snapInfo.InstanceName(), snapst.Flags) if err := m.setupSnapSecurity(task, snapInfo, opts, perfTimings); err != nil { return err } @@ -768,11 +767,11 @@ func (m *InterfaceManager) undoDisconnect(task *state.Task, _ *tomb.Tomb) error return err } - slotOpts := buildConfinementOptions(st, connRef.SlotRef.Snap, slotSnapst.Flags) + slotOpts := buildConfinementOptions(st, slotSnapst.InstanceName(), slotSnapst.Flags) if err := m.setupSnapSecurity(task, slot.Snap, slotOpts, perfTimings); err != nil { return err } - plugOpts := buildConfinementOptions(st, connRef.PlugRef.Snap, plugSnapst.Flags) + plugOpts := buildConfinementOptions(st, plugSnapst.InstanceName(), plugSnapst.Flags) if err := m.setupSnapSecurity(task, plug.Snap, plugOpts, perfTimings); err != nil { return err } @@ -851,11 +850,11 @@ func (m *InterfaceManager) undoConnect(task *state.Task, _ *tomb.Tomb) error { if err != nil { return err } - slotOpts := buildConfinementOptions(st, slotRef.Snap, slotSnapst.Flags) + slotOpts := buildConfinementOptions(st, slotSnapst.InstanceName(), slotSnapst.Flags) if err := m.setupSnapSecurity(task, slot.Snap, slotOpts, perfTimings); err != nil { return err } - plugOpts := buildConfinementOptions(st, plugRef.Snap, plugSnapst.Flags) + plugOpts := buildConfinementOptions(st, plugSnapst.InstanceName(), plugSnapst.Flags) if err := m.setupSnapSecurity(task, plug.Snap, plugOpts, perfTimings); err != nil { return err } diff --git a/overlord/ifacestate/handlers_test.go b/overlord/ifacestate/handlers_test.go index b87ba029832..b1179be9c90 100644 --- a/overlord/ifacestate/handlers_test.go +++ b/overlord/ifacestate/handlers_test.go @@ -20,25 +20,49 @@ package ifacestate_test import ( + "path" + . "gopkg.in/check.v1" + "github.com/snapcore/snapd/dirs" + "github.com/snapcore/snapd/overlord/configstate/config" "github.com/snapcore/snapd/overlord/ifacestate" + "github.com/snapcore/snapd/overlord/servicestate/servicestatetest" + "github.com/snapcore/snapd/overlord/snapstate" "github.com/snapcore/snapd/overlord/state" + "github.com/snapcore/snapd/snap" + "github.com/snapcore/snapd/snap/quota" + "github.com/snapcore/snapd/snap/snaptest" ) -type handlersSuite struct{} +const snapAyaml = `name: snap-a +type: app +base: base-snap-a +` + +type handlersSuite struct { + st *state.State +} var _ = Suite(&handlersSuite{}) +func (s *handlersSuite) SetUpTest(c *C) { + s.st = state.New(nil) + dirs.SetRootDir(c.MkDir()) +} + +func (s *handlersSuite) TearDownTest(c *C) { + dirs.SetRootDir("") +} + func (s *handlersSuite) TestInSameChangeWaitChain(c *C) { - st := state.New(nil) - st.Lock() - defer st.Unlock() + s.st.Lock() + defer s.st.Unlock() // no wait chain (yet) - startT := st.NewTask("start", "...start") - intermediateT := st.NewTask("intermediateT", "...intermediateT") - searchT := st.NewTask("searchT", "...searchT") + startT := s.st.NewTask("start", "...start") + intermediateT := s.st.NewTask("intermediateT", "...intermediateT") + searchT := s.st.NewTask("searchT", "...searchT") c.Check(ifacestate.InSameChangeWaitChain(startT, searchT), Equals, false) // add (indirect) wait chain @@ -48,16 +72,15 @@ func (s *handlersSuite) TestInSameChangeWaitChain(c *C) { } func (s *handlersSuite) TestInSameChangeWaitChainDifferentChanges(c *C) { - st := state.New(nil) - st.Lock() - defer st.Unlock() + s.st.Lock() + defer s.st.Unlock() - t1 := st.NewTask("t1", "...") - chg1 := st.NewChange("chg1", "...") + t1 := s.st.NewTask("t1", "...") + chg1 := s.st.NewChange("chg1", "...") chg1.AddTask(t1) - t2 := st.NewTask("t2", "...") - chg2 := st.NewChange("chg2", "...") + t2 := s.st.NewTask("t2", "...") + chg2 := s.st.NewChange("chg2", "...") chg2.AddTask(t2) // add a cross change wait chain @@ -66,23 +89,79 @@ func (s *handlersSuite) TestInSameChangeWaitChainDifferentChanges(c *C) { } func (s *handlersSuite) TestInSameChangeWaitChainWithCycles(c *C) { - st := state.New(nil) - st.Lock() - defer st.Unlock() + s.st.Lock() + defer s.st.Unlock() // cycles like this are unexpected in practice but are easier to test than // the exponential paths situation that e.g. seed changes present. - startT := st.NewTask("start", "...start") - task1 := st.NewTask("task1", "...") + startT := s.st.NewTask("start", "...start") + task1 := s.st.NewTask("task1", "...") task1.WaitFor(startT) - task2 := st.NewTask("task2", "...") + task2 := s.st.NewTask("task2", "...") task2.WaitFor(task1) - task3 := st.NewTask("task3", "...") + task3 := s.st.NewTask("task3", "...") task3.WaitFor(task2) startT.WaitFor(task2) startT.WaitFor(task3) - unrelated := st.NewTask("unrelated", "...") + unrelated := s.st.NewTask("unrelated", "...") c.Check(ifacestate.InSameChangeWaitChain(startT, unrelated), Equals, false) } + +func mockInstalledSnap(c *C, st *state.State, snapYaml string) *snap.Info { + snapInfo := snaptest.MockSnap(c, snapYaml, &snap.SideInfo{ + Revision: snap.R(1), + }) + + snapName := snapInfo.SnapName() + si := &snap.SideInfo{RealName: snapName, SnapID: snapName + "-id", Revision: snap.R(1)} + snapstate.Set(st, snapName, &snapstate.SnapState{ + Active: true, + Sequence: []*snap.SideInfo{si}, + Current: si.Revision, + SnapType: string(snapInfo.Type()), + }) + return snapInfo +} + +func (s *handlersSuite) TestBuildConfinementOptions(c *C) { + // Mock installed snap + s.st.Lock() + defer s.st.Unlock() + + snapInfo := mockInstalledSnap(c, s.st, snapAyaml) + flags := snapstate.Flags{} + opts := ifacestate.BuildConfinementOptions(s.st, snapInfo.InstanceName(), snapstate.Flags{}) + + c.Check(len(opts.ExtraLayouts), Equals, 0) + c.Check(opts.Classic, Equals, flags.Classic) + c.Check(opts.DevMode, Equals, flags.DevMode) + c.Check(opts.JailMode, Equals, flags.JailMode) +} + +func (s *handlersSuite) TestBuildConfinementOptionsWithLogNamespace(c *C) { + // Mock installed snap + s.st.Lock() + defer s.st.Unlock() + + tr := config.NewTransaction(s.st) + tr.Set("core", "experimental.quota-groups", true) + tr.Commit() + + snapInfo := mockInstalledSnap(c, s.st, snapAyaml) + + // Create a new quota group with a journal quota + err := servicestatetest.MockQuotaInState(s.st, "foo", "", []string{snapInfo.InstanceName()}, quota.NewResourcesBuilder().WithJournalNamespace().Build()) + c.Assert(err, IsNil) + + flags := snapstate.Flags{} + opts := ifacestate.BuildConfinementOptions(s.st, snapInfo.InstanceName(), snapstate.Flags{}) + + c.Assert(len(opts.ExtraLayouts), Equals, 1) + c.Check(opts.ExtraLayouts[0].Bind, Equals, path.Join(dirs.SnapSystemdDir, "journal.snap-foo")) + c.Check(opts.ExtraLayouts[0].Path, Equals, path.Join(dirs.SnapSystemdDir, "journal")) + c.Check(opts.Classic, Equals, flags.Classic) + c.Check(opts.DevMode, Equals, flags.DevMode) + c.Check(opts.JailMode, Equals, flags.JailMode) +} diff --git a/overlord/ifacestate/helpers.go b/overlord/ifacestate/helpers.go index fc5d3c68cd0..9d3d545c4aa 100644 --- a/overlord/ifacestate/helpers.go +++ b/overlord/ifacestate/helpers.go @@ -177,7 +177,7 @@ func (m *InterfaceManager) regenerateAllSecurityProfiles(tm timings.Measurer) er if err := snapstate.Get(m.state, snapName, &snapst); err != nil { logger.Noticef("cannot get state of snap %q: %s", snapName, err) } - return buildConfinementOptions(m.state, snapName, snapst.Flags) + return buildConfinementOptions(m.state, snapst.InstanceName(), snapst.Flags) } // For each backend: From 00ceecd3c8c372da0f1e276ec58e2513e40c3a78 Mon Sep 17 00:00:00 2001 From: Philip Meulengracht Date: Tue, 7 Jun 2022 13:24:46 +0200 Subject: [PATCH 5/8] overlord/ifacestate: remove debugging --- overlord/ifacestate/handlers.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/overlord/ifacestate/handlers.go b/overlord/ifacestate/handlers.go index f05b6b4c7c9..b5350775021 100644 --- a/overlord/ifacestate/handlers.go +++ b/overlord/ifacestate/handlers.go @@ -22,7 +22,6 @@ package ifacestate import ( "errors" "fmt" - "log" "path" "reflect" "sort" @@ -51,12 +50,10 @@ var snapstateFinishRestart = snapstate.FinishRestart // in case the snap has a journal quota. This mimicks what systemd does for // services with log namespaces. func addJournalQuotaLayout(quotaGroup *quota.Group, layouts *[]snap.Layout) error { - log.Println("3.1") if quotaGroup.JournalLimit == nil { return nil } - log.Println("3.2") // We need to bind mount the journal namespace folder ontop of // the journal folder // /etc/systemd/journal. -> /etc/systemd/journal @@ -102,7 +99,6 @@ func buildConfinementOptions(st *state.State, snapInstanceName string, flags sna extraLayouts, err := getExtraLayouts(st, snapInstanceName) if err != nil { logger.Noticef("cannot get extra mount layouts of snap %q: %s", snapInstanceName, err) - log.Printf("cannot get extra mount layouts of snap %q: %s", snapInstanceName, err) } return confinementOptions(flags, extraLayouts) } From c22b7d355f85fea5e5d7815afb8a26594a534f4e Mon Sep 17 00:00:00 2001 From: Philip Meulengracht Date: Tue, 7 Jun 2022 13:54:56 +0200 Subject: [PATCH 6/8] overlord/ifacestate: cleanup comments a bit --- overlord/ifacestate/handlers.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/overlord/ifacestate/handlers.go b/overlord/ifacestate/handlers.go index b5350775021..0da91fae8e6 100644 --- a/overlord/ifacestate/handlers.go +++ b/overlord/ifacestate/handlers.go @@ -47,15 +47,13 @@ import ( var snapstateFinishRestart = snapstate.FinishRestart // addJournalQuotaLayout handles the addition of a journal quota bind mount -// in case the snap has a journal quota. This mimicks what systemd does for -// services with log namespaces. +// to mimicks what systemd does for services with log namespaces. func addJournalQuotaLayout(quotaGroup *quota.Group, layouts *[]snap.Layout) error { if quotaGroup.JournalLimit == nil { return nil } - // We need to bind mount the journal namespace folder ontop of - // the journal folder + // bind mount the journal namespace folder ontop of the journal folder // /etc/systemd/journal. -> /etc/systemd/journal journalLayout := snap.Layout{ Bind: path.Join(dirs.SnapSystemdDir, fmt.Sprintf("journal.snap-%s", quotaGroup.Name)), From d31a32fdf07e38c2ba79c6324f2e9c9b3711f892 Mon Sep 17 00:00:00 2001 From: Philip Meulengracht Date: Tue, 7 Jun 2022 19:38:53 +0200 Subject: [PATCH 7/8] overlord/ifacestate: review comments code cleanup, include error handling --- overlord/ifacestate/handlers.go | 99 ++++++++++++++++++---------- overlord/ifacestate/handlers_test.go | 8 +-- overlord/ifacestate/helpers.go | 6 +- 3 files changed, 74 insertions(+), 39 deletions(-) diff --git a/overlord/ifacestate/handlers.go b/overlord/ifacestate/handlers.go index 0da91fae8e6..a974e550067 100644 --- a/overlord/ifacestate/handlers.go +++ b/overlord/ifacestate/handlers.go @@ -46,22 +46,21 @@ import ( var snapstateFinishRestart = snapstate.FinishRestart -// addJournalQuotaLayout handles the addition of a journal quota bind mount -// to mimicks what systemd does for services with log namespaces. -func addJournalQuotaLayout(quotaGroup *quota.Group, layouts *[]snap.Layout) error { +// getJournalQuotaLayout returns the necessary journal quota mount layouts +// to mimick what systemd does for services with log namespaces. +func getJournalQuotaLayout(quotaGroup *quota.Group) []snap.Layout { if quotaGroup.JournalLimit == nil { - return nil + return []snap.Layout{} } - // bind mount the journal namespace folder ontop of the journal folder + // bind mount the journal namespace folder on top of the journal folder // /etc/systemd/journal. -> /etc/systemd/journal - journalLayout := snap.Layout{ + layouts := []snap.Layout{{ Bind: path.Join(dirs.SnapSystemdDir, fmt.Sprintf("journal.snap-%s", quotaGroup.Name)), Path: path.Join(dirs.SnapSystemdDir, "journal"), Mode: 0755, - } - *layouts = append(*layouts, journalLayout) - return nil + }} + return layouts } // getExtraLayouts helper function to dynamically calculate the extra mount layouts for @@ -75,30 +74,24 @@ func getExtraLayouts(st *state.State, snapInstanceName string) ([]snap.Layout, e var extraLayouts []snap.Layout if snapOpts.QuotaGroup != nil { - if err := addJournalQuotaLayout(snapOpts.QuotaGroup, &extraLayouts); err != nil { - return extraLayouts, err - } + extraLayouts = append(extraLayouts, getJournalQuotaLayout(snapOpts.QuotaGroup)...) } return extraLayouts, nil } -// confinementOptions returns interfaces.ConfinementOptions from snapstate.Flags. -func confinementOptions(flags snapstate.Flags, extraLayouts []snap.Layout) interfaces.ConfinementOptions { +func buildConfinementOptions(st *state.State, snapInstanceName string, flags snapstate.Flags) (interfaces.ConfinementOptions, error) { + extraLayouts, err := getExtraLayouts(st, snapInstanceName) + if err != nil { + return interfaces.ConfinementOptions{}, fmt.Errorf("cannot get extra mount layouts of snap %q: %s", snapInstanceName, err) + } + return interfaces.ConfinementOptions{ DevMode: flags.DevMode, JailMode: flags.JailMode, Classic: flags.Classic, ExtraLayouts: extraLayouts, - } -} - -func buildConfinementOptions(st *state.State, snapInstanceName string, flags snapstate.Flags) interfaces.ConfinementOptions { - extraLayouts, err := getExtraLayouts(st, snapInstanceName) - if err != nil { - logger.Noticef("cannot get extra mount layouts of snap %q: %s", snapInstanceName, err) - } - return confinementOptions(flags, extraLayouts) + }, nil } func (m *InterfaceManager) setupAffectedSnaps(task *state.Task, affectingSnap string, affectedSnaps []string, tm timings.Measurer) error { @@ -122,7 +115,10 @@ func (m *InterfaceManager) setupAffectedSnaps(task *state.Task, affectingSnap st if err := addImplicitSlots(st, affectedSnapInfo); err != nil { return err } - opts := buildConfinementOptions(st, affectedSnapInfo.InstanceName(), snapst.Flags) + opts, err := buildConfinementOptions(st, affectedSnapInfo.InstanceName(), snapst.Flags) + if err != nil { + return err + } if err := m.setupSnapSecurity(task, affectedSnapInfo, opts, tm); err != nil { return err } @@ -165,7 +161,10 @@ func (m *InterfaceManager) doSetupProfiles(task *state.Task, tomb *tomb.Tomb) er return nil } - opts := buildConfinementOptions(task.State(), snapInfo.InstanceName(), snapsup.Flags) + opts, err := buildConfinementOptions(task.State(), snapInfo.InstanceName(), snapsup.Flags) + if err != nil { + return err + } return m.setupProfilesForSnap(task, tomb, snapInfo, opts, perfTimings) } @@ -255,8 +254,13 @@ func (m *InterfaceManager) setupProfilesForSnap(task *state.Task, _ *tomb.Tomb, if err := addImplicitSlots(st, snapInfo); err != nil { return err } + opts, err := buildConfinementOptions(st, snapInfo.InstanceName(), snapst.Flags) + if err != nil { + return err + } + affectedSnaps = append(affectedSnaps, snapInfo) - confinementOpts = append(confinementOpts, buildConfinementOptions(st, snapInfo.InstanceName(), snapst.Flags)) + confinementOpts = append(confinementOpts, opts) } return m.setupSecurityByBackend(task, affectedSnaps, confinementOpts, tm) @@ -347,7 +351,10 @@ func (m *InterfaceManager) undoSetupProfiles(task *state.Task, tomb *tomb.Tomb) if err != nil { return err } - opts := buildConfinementOptions(task.State(), snapInfo.InstanceName(), snapst.Flags) + opts, err := buildConfinementOptions(task.State(), snapInfo.InstanceName(), snapst.Flags) + if err != nil { + return err + } return m.setupProfilesForSnap(task, tomb, snapInfo, opts, perfTimings) } } @@ -549,12 +556,18 @@ func (m *InterfaceManager) doConnect(task *state.Task, _ *tomb.Tomb) (err error) }() if !delayedSetupProfiles { - slotOpts := buildConfinementOptions(st, slotSnapst.InstanceName(), slotSnapst.Flags) + slotOpts, err := buildConfinementOptions(st, slotSnapst.InstanceName(), slotSnapst.Flags) + if err != nil { + return err + } if err := m.setupSnapSecurity(task, slot.Snap, slotOpts, perfTimings); err != nil { return err } - plugOpts := buildConfinementOptions(st, plugSnapst.InstanceName(), plugSnapst.Flags) + plugOpts, err := buildConfinementOptions(st, plugSnapst.InstanceName(), plugSnapst.Flags) + if err != nil { + return err + } if err := m.setupSnapSecurity(task, plug.Snap, plugOpts, perfTimings); err != nil { return err } @@ -655,7 +668,10 @@ func (m *InterfaceManager) doDisconnect(task *state.Task, _ *tomb.Tomb) error { if err != nil { return err } - opts := buildConfinementOptions(st, snapInfo.InstanceName(), snapst.Flags) + opts, err := buildConfinementOptions(st, snapInfo.InstanceName(), snapst.Flags) + if err != nil { + return err + } if err := m.setupSnapSecurity(task, snapInfo, opts, perfTimings); err != nil { return err } @@ -761,11 +777,18 @@ func (m *InterfaceManager) undoDisconnect(task *state.Task, _ *tomb.Tomb) error return err } - slotOpts := buildConfinementOptions(st, slotSnapst.InstanceName(), slotSnapst.Flags) + slotOpts, err := buildConfinementOptions(st, slotSnapst.InstanceName(), slotSnapst.Flags) + if err != nil { + return err + } if err := m.setupSnapSecurity(task, slot.Snap, slotOpts, perfTimings); err != nil { return err } - plugOpts := buildConfinementOptions(st, plugSnapst.InstanceName(), plugSnapst.Flags) + + plugOpts, err := buildConfinementOptions(st, plugSnapst.InstanceName(), plugSnapst.Flags) + if err != nil { + return err + } if err := m.setupSnapSecurity(task, plug.Snap, plugOpts, perfTimings); err != nil { return err } @@ -844,11 +867,19 @@ func (m *InterfaceManager) undoConnect(task *state.Task, _ *tomb.Tomb) error { if err != nil { return err } - slotOpts := buildConfinementOptions(st, slotSnapst.InstanceName(), slotSnapst.Flags) + + slotOpts, err := buildConfinementOptions(st, slotSnapst.InstanceName(), slotSnapst.Flags) + if err != nil { + return err + } if err := m.setupSnapSecurity(task, slot.Snap, slotOpts, perfTimings); err != nil { return err } - plugOpts := buildConfinementOptions(st, plugSnapst.InstanceName(), plugSnapst.Flags) + + plugOpts, err := buildConfinementOptions(st, plugSnapst.InstanceName(), plugSnapst.Flags) + if err != nil { + return err + } if err := m.setupSnapSecurity(task, plug.Snap, plugOpts, perfTimings); err != nil { return err } diff --git a/overlord/ifacestate/handlers_test.go b/overlord/ifacestate/handlers_test.go index b1179be9c90..59c8869299b 100644 --- a/overlord/ifacestate/handlers_test.go +++ b/overlord/ifacestate/handlers_test.go @@ -126,14 +126,14 @@ func mockInstalledSnap(c *C, st *state.State, snapYaml string) *snap.Info { } func (s *handlersSuite) TestBuildConfinementOptions(c *C) { - // Mock installed snap s.st.Lock() defer s.st.Unlock() snapInfo := mockInstalledSnap(c, s.st, snapAyaml) flags := snapstate.Flags{} - opts := ifacestate.BuildConfinementOptions(s.st, snapInfo.InstanceName(), snapstate.Flags{}) + opts, err := ifacestate.BuildConfinementOptions(s.st, snapInfo.InstanceName(), snapstate.Flags{}) + c.Check(err, IsNil) c.Check(len(opts.ExtraLayouts), Equals, 0) c.Check(opts.Classic, Equals, flags.Classic) c.Check(opts.DevMode, Equals, flags.DevMode) @@ -141,7 +141,6 @@ func (s *handlersSuite) TestBuildConfinementOptions(c *C) { } func (s *handlersSuite) TestBuildConfinementOptionsWithLogNamespace(c *C) { - // Mock installed snap s.st.Lock() defer s.st.Unlock() @@ -156,8 +155,9 @@ func (s *handlersSuite) TestBuildConfinementOptionsWithLogNamespace(c *C) { c.Assert(err, IsNil) flags := snapstate.Flags{} - opts := ifacestate.BuildConfinementOptions(s.st, snapInfo.InstanceName(), snapstate.Flags{}) + opts, err := ifacestate.BuildConfinementOptions(s.st, snapInfo.InstanceName(), snapstate.Flags{}) + c.Check(err, IsNil) c.Assert(len(opts.ExtraLayouts), Equals, 1) c.Check(opts.ExtraLayouts[0].Bind, Equals, path.Join(dirs.SnapSystemdDir, "journal.snap-foo")) c.Check(opts.ExtraLayouts[0].Path, Equals, path.Join(dirs.SnapSystemdDir, "journal")) diff --git a/overlord/ifacestate/helpers.go b/overlord/ifacestate/helpers.go index 9d3d545c4aa..008e7eb1708 100644 --- a/overlord/ifacestate/helpers.go +++ b/overlord/ifacestate/helpers.go @@ -177,7 +177,11 @@ func (m *InterfaceManager) regenerateAllSecurityProfiles(tm timings.Measurer) er if err := snapstate.Get(m.state, snapName, &snapst); err != nil { logger.Noticef("cannot get state of snap %q: %s", snapName, err) } - return buildConfinementOptions(m.state, snapst.InstanceName(), snapst.Flags) + opts, err := buildConfinementOptions(m.state, snapst.InstanceName(), snapst.Flags) + if err != nil { + logger.Noticef("cannot get confinement options for snap %q: %s", snapName, err) + } + return opts } // For each backend: From 2b28dda312e87baa1e803186f4aa90ab4f859de7 Mon Sep 17 00:00:00 2001 From: Philip Meulengracht Date: Wed, 8 Jun 2022 11:41:28 +0200 Subject: [PATCH 8/8] overlord/ifacestate: review feedback code cleanup --- overlord/ifacestate/handlers.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/overlord/ifacestate/handlers.go b/overlord/ifacestate/handlers.go index a974e550067..dd2d01e123e 100644 --- a/overlord/ifacestate/handlers.go +++ b/overlord/ifacestate/handlers.go @@ -46,11 +46,11 @@ import ( var snapstateFinishRestart = snapstate.FinishRestart -// getJournalQuotaLayout returns the necessary journal quota mount layouts +// journalQuotaLayout returns the necessary journal quota mount layouts // to mimick what systemd does for services with log namespaces. -func getJournalQuotaLayout(quotaGroup *quota.Group) []snap.Layout { +func journalQuotaLayout(quotaGroup *quota.Group) []snap.Layout { if quotaGroup.JournalLimit == nil { - return []snap.Layout{} + return nil } // bind mount the journal namespace folder on top of the journal folder @@ -74,7 +74,7 @@ func getExtraLayouts(st *state.State, snapInstanceName string) ([]snap.Layout, e var extraLayouts []snap.Layout if snapOpts.QuotaGroup != nil { - extraLayouts = append(extraLayouts, getJournalQuotaLayout(snapOpts.QuotaGroup)...) + extraLayouts = append(extraLayouts, journalQuotaLayout(snapOpts.QuotaGroup)...) } return extraLayouts, nil