snap, snap/pack: disallow pack and install of snapd, base and os with specific configure hooks #13117

ernestl · 2023-08-24T13:48:29Z

It is currently possible to pack and install snapd, base and core snaps with default-configure and configure hooks, but those hooks are ignored during installation and option setting. My proposal is to extend the snap validation check (covers packing and installation) to prevent these unintended snap/hook combinations and possible silent failure.

Spec:
Implements part of draft spec Prohibit user-defined configuration hooks for specific essential snaps

Approach:
Extend snap/validate.go hook check that was introduced in #13097 as follows:

Do not allow default-configure hook for snapd, base or os
Do not allow configure hook for snapd, base
Allow configure hook for os to prevent new errors for core or ubuntu-core with existing configure hook

Key covered paths: Snap Pack, Firstboot, Store Install, Sideload Install

JIRA: https://warthogs.atlassian.net/browse/SNAPDENG-7297
This PR builds on #13097

github-actions · 2025-01-22T15:52:16Z

Thu Feb 20 07:47:21 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13412751831

Failures:

Executing:

google:ubuntu-20.04-64:tests/main/preseed-core20

codecov · 2025-01-24T13:02:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (master@2cbce28). Learn more about missing BASE report.
Report is 245 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #13117   +/-   ##
=========================================
  Coverage          ?   77.86%           
=========================================
  Files             ?     1167           
  Lines             ?   156856           
  Branches          ?        0           
=========================================
  Hits              ?   122130           
  Misses            ?    27139           
  Partials          ?     7587

Flag	Coverage Δ
unittests	`77.86% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

miguelpires

lgtm

ZeyadYasser

LGTM

pedronis

some comments

overlord/snapstate/check_snap.go

snap/validate.go

…core)

pedronis

thanks for the changes, something seems a bit off with the error message building logic though, otoh made a different suggestion

pedronis · 2025-02-13T09:37:02Z

snap/validate.go

-	}
-
-	if info.SnapType == TypeSnapd || info.SnapType == TypeBase || (info.SnapType == TypeOS && info.InstanceName() != "core") {
+	if info.SnapType == TypeSnapd || info.SnapType == TypeBase || info.SnapType == TypeOS {
 		var hookNames strings.Builder
 		if hasDefaultConfigureHook {
 			hookNames.WriteString(`"default-configure"`)
 			if hasConfigureHook {


this now needs to become the same check as below, no? but then I'm unsure using a strings.Builder is the best approach, wouldn't a []string and using Join with " or " be simpler?

thanks, corrected and simplified this and corresponding test

…nfigure and configure

pedronis

thank you, @ernestl this has changed a bit since. maybe you can get one of the original reviewers to do a quick 2nd pass?

miguelpires

Looks good, just some comments on testing

snap/pack/pack_test.go

miguelpires · 2025-02-17T15:32:08Z

snap/pack/pack_test.go

+		configureHooks := []string{"configure"}
+		c.Assert(os.Mkdir(filepath.Join(sourceDir, "meta", "hooks"), 0755), IsNil)
+		for _, hook := range configureHooks {
+			c.Assert(os.WriteFile(filepath.Join(sourceDir, "meta", "hooks", hook), []byte("#!/bin/sh"), 0755), IsNil)
+		}


Suggested change

configureHooks := []string{"configure"}

c.Assert(os.Mkdir(filepath.Join(sourceDir, "meta", "hooks"), 0755), IsNil)

for _, hook := range configureHooks {

c.Assert(os.WriteFile(filepath.Join(sourceDir, "meta", "hooks", hook), []byte("#!/bin/sh"), 0755), IsNil)

}

c.Assert(os.Mkdir(filepath.Join(sourceDir, "meta", "hooks"), 0755), IsNil)

c.Assert(os.WriteFile(filepath.Join(sourceDir, "meta", "hooks", "configure"), []byte("#!/bin/sh"), 0755), IsNil)

thanks, improved

snap/pack/pack_test.go

miguelpires

looks good, thanks for the clarification

ernestl force-pushed the SNAPDENG-7297_Prohibit_Configuration_Hooks_For_Core_Base_Snapd branch 3 times, most recently from 203749a to b490ccc Compare August 25, 2023 15:31

ernestl mentioned this pull request Aug 28, 2023

o/devicestate, o/snapstate: move firstboot configure task after essential snap installs #13127

Draft

ernestl force-pushed the SNAPDENG-7297_Prohibit_Configuration_Hooks_For_Core_Base_Snapd branch from b490ccc to c10b5b3 Compare January 22, 2025 15:39

ernestl force-pushed the SNAPDENG-7297_Prohibit_Configuration_Hooks_For_Core_Base_Snapd branch from c10b5b3 to b6a8698 Compare January 24, 2025 12:44

ernestl closed this Jan 24, 2025

ernestl reopened this Jan 24, 2025

ernestl force-pushed the SNAPDENG-7297_Prohibit_Configuration_Hooks_For_Core_Base_Snapd branch 2 times, most recently from 7990b70 to 6e7c4f6 Compare January 25, 2025 06:04

ernestl changed the title ~~snap/pack: disallow packing snapd, os and base snaps with default-/configure hooks~~ snap/pack: disallow pack, install and sideload of snapd, os and base with default-/configure hooks Jan 25, 2025

ernestl changed the title ~~snap/pack: disallow pack, install and sideload of snapd, os and base with default-/configure hooks~~ snap/pack: disallow pack and install of snapd, os and base with default-/configure hooks Jan 25, 2025

ernestl changed the title ~~snap/pack: disallow pack and install of snapd, os and base with default-/configure hooks~~ snap/pack: disallow pack and install of snapd, base and core with default-/configure hooks Jan 25, 2025

ernestl changed the title ~~snap/pack: disallow pack and install of snapd, base and core with default-/configure hooks~~ snap/pack: disallow pack and install of snapd, base and core with configure hooks Jan 25, 2025

ernestl changed the title ~~snap/pack: disallow pack and install of snapd, base and core with configure hooks~~ snap/pack: disallow pack and install of snapd, base and os with configure hooks Jan 25, 2025

ernestl force-pushed the SNAPDENG-7297_Prohibit_Configuration_Hooks_For_Core_Base_Snapd branch 3 times, most recently from 4d8c591 to 4d38e1d Compare January 27, 2025 07:51

ernestl added this to the 2.70 milestone Jan 27, 2025

ernestl requested a review from pedronis January 27, 2025 09:18

ernestl added the Needs Samuele review Needs a review from Samuele before it can land label Jan 27, 2025

ernestl marked this pull request as ready for review January 27, 2025 09:57

ernestl requested review from miguelpires and ZeyadYasser January 27, 2025 10:01

ernestl closed this Jan 27, 2025

ernestl reopened this Jan 27, 2025

disallow pack/install of snapd, base and os snaps with configure hooks

7a4f5ec

ernestl force-pushed the SNAPDENG-7297_Prohibit_Configuration_Hooks_For_Core_Base_Snapd branch from 4d38e1d to 7a4f5ec Compare January 27, 2025 10:18

miguelpires approved these changes Jan 28, 2025

View reviewed changes

ZeyadYasser approved these changes Jan 30, 2025

View reviewed changes

pedronis requested changes Feb 3, 2025

View reviewed changes

overlord/snapstate/check_snap.go Outdated Show resolved Hide resolved

snap/validate.go Outdated Show resolved Hide resolved

ernestl added 2 commits February 5, 2025 11:51

overlord/snapstate: keep CheckSnap configure hook validation

4eaf1b3

snap, snap/pack: allow configure hook for all OS snaps (core, ubuntu-…

0719ad3

…core)

ernestl changed the title ~~snap/pack: disallow pack and install of snapd, base and os with configure hooks~~ snap, snap/pack: disallow pack and install of snapd and base with configure and os with default-configure hook Feb 5, 2025

ernestl changed the title ~~snap, snap/pack: disallow pack and install of snapd and base with configure and os with default-configure hook~~ snap, snap/pack: disallow pack and install of snapd, base and os with specific configure hooks Feb 5, 2025

ernestl added the Squash-merge Please squash this PR when merging. label Feb 5, 2025

ernestl requested a review from pedronis February 5, 2025 11:31

pedronis requested changes Feb 13, 2025

View reviewed changes

snap: fixed hook validation and test for OS snap with both default-co…

f59a40c

…nfigure and configure

ernestl requested a review from pedronis February 13, 2025 20:46

pedronis approved these changes Feb 14, 2025

View reviewed changes

ernestl requested a review from miguelpires February 17, 2025 10:45

miguelpires reviewed Feb 17, 2025

View reviewed changes

miguelpires approved these changes Feb 19, 2025

View reviewed changes

snap/pack: improve unit tests

6835853

ernestl merged commit 699b1b4 into canonical:master Feb 20, 2025
63 of 64 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

snap, snap/pack: disallow pack and install of snapd, base and os with specific configure hooks #13117

snap, snap/pack: disallow pack and install of snapd, base and os with specific configure hooks #13117

ernestl commented Aug 24, 2023 •

edited

Loading

github-actions bot commented Jan 22, 2025 •

edited

Loading

codecov bot commented Jan 24, 2025 •

edited

Loading

miguelpires left a comment

ZeyadYasser left a comment

pedronis left a comment

pedronis left a comment

pedronis Feb 13, 2025

ernestl Feb 13, 2025

pedronis left a comment

miguelpires left a comment

miguelpires Feb 17, 2025

ernestl Feb 19, 2025

miguelpires left a comment

snap, snap/pack: disallow pack and install of snapd, base and os with specific configure hooks #13117

snap, snap/pack: disallow pack and install of snapd, base and os with specific configure hooks #13117

Conversation

ernestl commented Aug 24, 2023 • edited Loading

github-actions bot commented Jan 22, 2025 • edited Loading

Failures:

Executing:

codecov bot commented Jan 24, 2025 • edited Loading

Codecov Report

miguelpires left a comment

Choose a reason for hiding this comment

ZeyadYasser left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

pedronis Feb 13, 2025

Choose a reason for hiding this comment

ernestl Feb 13, 2025

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

miguelpires left a comment

Choose a reason for hiding this comment

miguelpires Feb 17, 2025

Choose a reason for hiding this comment

ernestl Feb 19, 2025

Choose a reason for hiding this comment

miguelpires left a comment

Choose a reason for hiding this comment

ernestl commented Aug 24, 2023 •

edited

Loading

github-actions bot commented Jan 22, 2025 •

edited

Loading

codecov bot commented Jan 24, 2025 •

edited

Loading