snap/pack: add check to disallow packing snapd, core (os) and base snaps with configure hooks

snap/info_test.go

@@ -824,15 +824,35 @@ hooks:
 func (s *infoSuite) TestReadInfoFromSnapFileCatchesInvalidImplicitHook(c *C) {
 	yaml := `name: foo
 version: 1.0`
-	snapPath := snaptest.MakeTestSnapWithFiles(c, yaml, emptyHooks("123abc"))
 
-	snapf, err := snapfile.Open(snapPath)
+	contents := [][]string{
+		{"meta/hooks/123abc", ""},
+	}
+	sideInfo := &snap.SideInfo{}
+	snapInfo := snaptest.MockSnapWithFiles(c, yaml, sideInfo, contents)
+	snapf, err := snapfile.Open(snapInfo.MountDir())
 	c.Assert(err, IsNil)
 
 	_, err = snap.ReadInfoFromSnapFile(snapf, nil)
 	c.Assert(err, ErrorMatches, ".*invalid hook name.*")
 }
 
+func (s *infoSuite) TestReadInfoFromSnapFileCatchesImplicitHookDefaultConfigureOnly(c *C) {
+	yaml := `name: foo
+version: 1.0`
+
+	contents := [][]string{
+		{"meta/hooks/default-configure", ""},
+	}
+	sideInfo := &snap.SideInfo{}
+	snapInfo := snaptest.MockSnapWithFiles(c, yaml, sideInfo, contents)
+	snapf, err := snapfile.Open(snapInfo.MountDir())
+	c.Assert(err, IsNil)
+
+	_, err = snap.ReadInfoFromSnapFile(snapf, nil)
+	c.Assert(err, ErrorMatches, "cannot specify \"default-configure\" hook without \"configure\" hook")
+}
+
 func (s *infoSuite) checkInstalledSnapAndSnapFile(c *C, instanceName, yaml string, contents string, hooks []string, checker func(c *C, info *snap.Info)) {
 	// First check installed snap
 	sideInfo := &snap.SideInfo{Revision: snap.R(42)}

snap/pack/pack.go

@@ -107,27 +107,33 @@ func CheckSkeleton(w io.Writer, sourceDir string) error {
 }
 
 func loadAndValidate(sourceDir string) (*snap.Info, error) {
-	// ensure we have valid content
+	// Parsing of snap info is duplicated in ReadInfoFromSnapFile. It is done
+	// here to retrieve the snap instance name, if available, to use in case of
+	// ReadInfoFromSnapFile error
 	yaml, err := ioutil.ReadFile(filepath.Join(sourceDir, "meta", "snap.yaml"))
 	if err != nil {
 		return nil, err
 	}
-
 	info, err := snap.InfoFromSnapYaml(yaml)
 	if err != nil {
 		return nil, err
 	}
+	instanceName := info.InstanceName()
 
-	if err := snap.Validate(info); err != nil {
-		return nil, fmt.Errorf("cannot validate snap %q: %v", info.InstanceName(), err)
+	// ReadInfoFromSnapFile covers the following required steps:
+	// 	- Read snap metadata from meta/snap.yaml
+	//      - Parse snap info from meta/snap.yaml without side info
+	//      - Add and bind implicit hooks from meta/hooks
+	//      - Validate available snap information
+	//      - Validate snapshot metadata from opional meta/snapshot.yaml
+	info, err = snap.ReadInfoFromSnapFile(snapdir.New(sourceDir), nil)
+	if err != nil {
+		return nil, fmt.Errorf("cannot validate snap %q: %v", instanceName, err)
 	}
 
 	if err := snap.ValidateContainer(snapdir.New(sourceDir), info, logger.Noticef); err != nil {
 		return nil, err
 	}
-	if _, err := snap.ReadSnapshotYamlFromSnapFile(snapdir.New(sourceDir)); err != nil {
-		return nil, err
-	}
 
 	if info.SnapType == snap.TypeGadget {
 		// TODO:UC20: optionally pass model

snap/pack/pack_test.go

@@ -122,6 +122,15 @@ func (s *packSuite) TestPackNoManifestFails(c *C) {
 	c.Assert(err, ErrorMatches, `.*/meta/snap\.yaml: no such file or directory`)
 }
 
+func (s *packSuite) TestPackInfoFromSnapYamlFails(c *C) {
+	sourceDir := makeExampleSnapSourceDir(c, `name: hello
+version: 0
+no-colon
+`)
+	_, err := pack.Snap(sourceDir, pack.Defaults)
+	c.Assert(err, ErrorMatches, `cannot parse snap.yaml: yaml: line 4: could not find expected ':'`)
+}
+
 func (s *packSuite) TestPackMissingAppFails(c *C) {
 	sourceDir := makeExampleSnapSourceDir(c, `name: hello
 version: 0
@@ -134,6 +143,102 @@ apps:
 	c.Assert(err, Equals, snap.ErrMissingPaths)
 }
 
+func (s *packSuite) TestPackDefaultConfigureWithoutConfigureError(c *C) {
+	sourceDir := makeExampleSnapSourceDir(c, `name: hello
+version: 0
+apps:
+ foo:
+  command: bin/hello-world
+`)
+	c.Assert(os.Mkdir(filepath.Join(sourceDir, "meta", "hooks"), 0755), IsNil)
+	c.Assert(ioutil.WriteFile(filepath.Join(sourceDir, "meta", "hooks", "default-configure"), []byte("#!/bin/sh"), 0755), IsNil)
+	_, err := pack.Snap(sourceDir, pack.Defaults)
+	c.Check(err, ErrorMatches, "cannot validate snap \"hello\": cannot specify \"default-configure\" hook without \"configure\" hook")
+}
+
+func (s *packSuite) TestPackConfigureHooksPermissionsError(c *C) {
+	sourceDir := makeExampleSnapSourceDir(c, `name: hello
+version: 0
+apps:
+ foo:
+  command: bin/hello-world
+`)
+	c.Assert(os.Mkdir(filepath.Join(sourceDir, "meta", "hooks"), 0755), IsNil)
+	configureHooks := []string{"configure", "default-configure"}
+	for _, hook := range configureHooks {
+		c.Assert(ioutil.WriteFile(filepath.Join(sourceDir, "meta", "hooks", hook), []byte("#!/bin/sh"), 0666), IsNil)
+		_, err := pack.Snap(sourceDir, pack.Defaults)
+		c.Check(err, ErrorMatches, "snap is unusable due to bad permissions")
+	}
+}
+
+func (s *packSuite) TestPackConfigureHooksHappy(c *C) {
+	sourceDir := makeExampleSnapSourceDir(c, `name: hello
+version: 0
+apps:
+ foo:
+  command: bin/hello-world
+`)
+	c.Assert(os.Mkdir(filepath.Join(sourceDir, "meta", "hooks"), 0755), IsNil)
+	configureHooks := []string{"configure", "default-configure"}
+	for _, hook := range configureHooks {
+		c.Assert(ioutil.WriteFile(filepath.Join(sourceDir, "meta", "hooks", hook), []byte("#!/bin/sh"), 0755), IsNil)
+		_, err := pack.Snap(sourceDir, pack.Defaults)
+		c.Assert(err, IsNil)
+	}
+}
+
+func (s *packSuite) TestPackSnapshotYamlExcludePathError(c *C) {
+	sourceDir := makeExampleSnapSourceDir(c, `name: hello
+version: 0
+apps:
+ foo:
+  command: bin/hello-world
+`)
+
+	invalidSnapshotYaml := `exclude:
+    - $SNAP_DATA/one
+    - $SNAP_UNKNOWN_DIR/two
+`
+	c.Assert(ioutil.WriteFile(filepath.Join(sourceDir, "meta", "snapshots.yaml"), []byte(invalidSnapshotYaml), 0444), IsNil)
+	_, err := pack.Snap(sourceDir, pack.Defaults)
+	c.Assert(err, ErrorMatches, "cannot validate snap \"hello\": snapshot exclude path must start with one of.*")
+}
+
+func (s *packSuite) TestPackSnapshotYamlPermissionsError(c *C) {
+	sourceDir := makeExampleSnapSourceDir(c, `name: hello
+version: 0
+apps:
+ foo:
+  command: bin/hello-world
+`)
+
+	invalidSnapshotYaml := `exclude:
+    - $SNAP_DATA/one
+    - $SNAP_COMMON/two
+`
+	c.Assert(ioutil.WriteFile(filepath.Join(sourceDir, "meta", "snapshots.yaml"), []byte(invalidSnapshotYaml), 0411), IsNil)
+	_, err := pack.Snap(sourceDir, pack.Defaults)
+	c.Assert(err, ErrorMatches, "snap is unusable due to bad permissions")
+}
+
+func (s *packSuite) TestPackSnapshotYamlHappy(c *C) {
+	sourceDir := makeExampleSnapSourceDir(c, `name: hello
+version: 0
+apps:
+ foo:
+  command: bin/hello-world
+`)
+
+	invalidSnapshotYaml := `exclude:
+    - $SNAP_DATA/one
+    - $SNAP_COMMON/two
+`
+	c.Assert(ioutil.WriteFile(filepath.Join(sourceDir, "meta", "snapshots.yaml"), []byte(invalidSnapshotYaml), 0444), IsNil)
+	_, err := pack.Snap(sourceDir, pack.Defaults)
+	c.Assert(err, IsNil)
+}
+
 func (s *packSuite) TestValidateMissingAppFailsWithErrMissingPaths(c *C) {
 	var buf bytes.Buffer
 	sourceDir := makeExampleSnapSourceDir(c, `name: hello

snap/validate.go

@@ -162,6 +162,22 @@ func ValidateLicense(license string) error {
 	return nil
 }
 
+func validateHooks(info *Info) error {
+	for _, hook := range info.Hooks {
+		if err := ValidateHook(hook); err != nil {
+			return err
+		}
+	}
+
+	hasDefaultConfigureHook := info.Hooks["default-configure"] != nil
+	hasConfigureHook := info.Hooks["configure"] != nil
+	if hasDefaultConfigureHook && !hasConfigureHook {
+		return fmt.Errorf(`cannot specify "default-configure" hook without "configure" hook`)
+	}
+
+	return nil
+}
+
 // ValidateHook validates the content of the given HookInfo
 func ValidateHook(hook *HookInfo) error {
 	if err := naming.ValidateHook(hook.Name); err != nil {
@@ -368,11 +384,9 @@ func Validate(info *Info) error {
 		}
 	}
 
-	// validate hook entries
-	for _, hook := range info.Hooks {
-		if err := ValidateHook(hook); err != nil {
-			return err
-		}
+	// Validate hook entries
+	if err := validateHooks(info); err != nil {
+		return err
 	}
 
 	// Ensure that plugs and slots have appropriate names and interface names.

snap/validate_test.go

@@ -793,6 +793,18 @@ hooks:
 	c.Check(err, ErrorMatches, `invalid hook name: "123abc"`)
 }
 
+func (s *ValidateSuite) TestIllegalHookDefaultConfigureWithoutConfigure(c *C) {
+	info, err := InfoFromSnapYaml([]byte(`name: foo
+version: 1.0
+hooks:
+  default-configure:
+`))
+	c.Assert(err, IsNil)
+
+	err = Validate(info)
+	c.Check(err, ErrorMatches, "cannot specify \"default-configure\" hook without \"configure\" hook")
+}
+
 func (s *ValidateSuite) TestPlugSlotNamesUnique(c *C) {
 	info, err := InfoFromSnapYaml([]byte(`name: snap
 version: 0