From 48a701c29fccc540e6850a3af8cae83271377770 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 12 Aug 2024 11:33:32 +0200 Subject: [PATCH 1/3] pathpolicy: tweak error messages and add test Trivial tweak for the errors returned from PathPolicies.Check() so that they are a bit more user friendly. See also https://github.com/osbuild/bootc-image-builder/pull/601 --- pkg/pathpolicy/path_policy.go | 7 +++---- pkg/pathpolicy/path_policy_test.go | 10 ++++++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/pkg/pathpolicy/path_policy.go b/pkg/pathpolicy/path_policy.go index 423a5751b1..0efd2ee531 100644 --- a/pkg/pathpolicy/path_policy.go +++ b/pkg/pathpolicy/path_policy.go @@ -26,15 +26,14 @@ func NewPathPolicies(entries map[string]PathPolicy) *PathPolicies { // Check a given path against the PathPolicies func (pol *PathPolicies) Check(fsPath string) error { - // Quickly check we have a path and it is absolute if fsPath == "" || fsPath[0] != '/' { - return fmt.Errorf("path must be absolute") + return fmt.Errorf("path %q must be absolute", fsPath) } // ensure that only clean paths are valid if fsPath != path.Clean(fsPath) { - return fmt.Errorf("path must be canonical") + return fmt.Errorf("path %q must be canonical", fsPath) } node, left := pol.Lookup(fsPath) @@ -46,7 +45,7 @@ func (pol *PathPolicies) Check(fsPath string) error { // 1) path is explicitly not allowed or // 2) a subpath was match but an explicit match is required if policy.Deny || (policy.Exact && len(left) > 0) { - return fmt.Errorf("path '%s ' is not allowed", fsPath) + return fmt.Errorf("path %q is not allowed", fsPath) } // exact match or recursive path allowed diff --git a/pkg/pathpolicy/path_policy_test.go b/pkg/pathpolicy/path_policy_test.go index 28dec75d1b..e2e5a046af 100644 --- a/pkg/pathpolicy/path_policy_test.go +++ b/pkg/pathpolicy/path_policy_test.go @@ -47,3 +47,13 @@ func TestPathPolicyCheck(t *testing.T) { } } } + +func TestPathPolicyErrors(t *testing.T) { + policy := NewPathPolicies(map[string]PathPolicy{ + "/": {Exact: true}, + }) + + assert.EqualError(t, policy.Check("/foo"), `path "/foo" is not allowed`) + assert.EqualError(t, policy.Check("./"), `path "./" must be absolute`) + assert.EqualError(t, policy.Check("/boot/"), `path "/boot/" must be canonical`) +} From b6ea2792f873b60633f2602a11814663738a626f Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 12 Aug 2024 12:44:15 +0200 Subject: [PATCH 2/3] blueprint: improve error message for invalid mountpoint customizations Small tweak to the way the error from CheckMountpointsPolicy is constructed to include the details from the pathpolicy errors. Thanks to Achilleas --- pkg/blueprint/filesystem_customizations.go | 12 ++++----- .../filesystem_customizations_test.go | 26 +++++++++++++++++++ 2 files changed, 32 insertions(+), 6 deletions(-) create mode 100644 pkg/blueprint/filesystem_customizations_test.go diff --git a/pkg/blueprint/filesystem_customizations.go b/pkg/blueprint/filesystem_customizations.go index ad6959a717..829f72e952 100644 --- a/pkg/blueprint/filesystem_customizations.go +++ b/pkg/blueprint/filesystem_customizations.go @@ -2,6 +2,7 @@ package blueprint import ( "encoding/json" + "errors" "fmt" "github.com/osbuild/images/internal/common" @@ -73,16 +74,15 @@ func (fsc *FilesystemCustomization) UnmarshalJSON(data []byte) error { // CheckMountpointsPolicy checks if the mountpoints are allowed by the policy func CheckMountpointsPolicy(mountpoints []FilesystemCustomization, mountpointAllowList *pathpolicy.PathPolicies) error { - invalidMountpoints := []string{} + var errs []error for _, m := range mountpoints { - err := mountpointAllowList.Check(m.Mountpoint) - if err != nil { - invalidMountpoints = append(invalidMountpoints, m.Mountpoint) + if err := mountpointAllowList.Check(m.Mountpoint); err != nil { + errs = append(errs, err) } } - if len(invalidMountpoints) > 0 { - return fmt.Errorf("The following custom mountpoints are not supported %+q", invalidMountpoints) + if len(errs) > 0 { + return fmt.Errorf("The following errors occurred while setting up custom mountpoints:\n%w", errors.Join(errs...)) } return nil diff --git a/pkg/blueprint/filesystem_customizations_test.go b/pkg/blueprint/filesystem_customizations_test.go new file mode 100644 index 0000000000..daf043457e --- /dev/null +++ b/pkg/blueprint/filesystem_customizations_test.go @@ -0,0 +1,26 @@ +package blueprint + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/osbuild/images/pkg/pathpolicy" +) + +func TestCheckMountpointsPolicy(t *testing.T) { + policy := pathpolicy.NewPathPolicies(map[string]pathpolicy.PathPolicy{ + "/": {Exact: true}, + }) + + mps := []FilesystemCustomization{ + {Mountpoint: "/foo"}, + {Mountpoint: "/boot/"}, + } + + expectedErr := `The following errors occurred while setting up custom mountpoints: +path "/foo" is not allowed +path "/boot/" must be canonical` + err := CheckMountpointsPolicy(mps, policy) + assert.EqualError(t, err, expectedErr) +} From 249495dd302c8b571246b21ee5dba0e843344573 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 12 Aug 2024 14:13:21 +0200 Subject: [PATCH 3/3] distro: update blueprint tests for new error msg --- pkg/distro/fedora/distro_test.go | 4 ++-- pkg/distro/rhel/rhel10/distro_test.go | 4 ++-- pkg/distro/rhel/rhel7/distro_test.go | 4 ++-- pkg/distro/rhel/rhel8/distro_test.go | 4 ++-- pkg/distro/rhel/rhel9/distro_test.go | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/distro/fedora/distro_test.go b/pkg/distro/fedora/distro_test.go index 6b3a2e19d9..cd50693408 100644 --- a/pkg/distro/fedora/distro_test.go +++ b/pkg/distro/fedora/distro_test.go @@ -748,7 +748,7 @@ func TestDistro_CustomFileSystemManifestError(t *testing.T) { } else if imgTypeName == "live-installer" { assert.EqualError(t, err, fmt.Sprintf(distro.NoCustomizationsAllowedError, imgTypeName)) } else { - assert.EqualError(t, err, "The following custom mountpoints are not supported [\"/etc\"]") + assert.EqualError(t, err, "The following errors occurred while setting up custom mountpoints:\npath \"/etc\" is not allowed") } } } @@ -896,7 +896,7 @@ func TestDistro_DirtyMountpointsNotAllowed(t *testing.T) { } else if imgTypeName == "live-installer" { assert.EqualError(t, err, fmt.Sprintf(distro.NoCustomizationsAllowedError, imgTypeName)) } else { - assert.EqualError(t, err, "The following custom mountpoints are not supported [\"//\" \"/var//\" \"/var//log/audit/\"]") + assert.EqualError(t, err, "The following errors occurred while setting up custom mountpoints:\npath \"//\" must be canonical\npath \"/var//\" must be canonical\npath \"/var//log/audit/\" must be canonical") } } } diff --git a/pkg/distro/rhel/rhel10/distro_test.go b/pkg/distro/rhel/rhel10/distro_test.go index bf86b03977..b000dbe27b 100644 --- a/pkg/distro/rhel/rhel10/distro_test.go +++ b/pkg/distro/rhel/rhel10/distro_test.go @@ -418,7 +418,7 @@ func TestDistro_CustomFileSystemManifestError(t *testing.T) { for _, imgTypeName := range arch.ListImageTypes() { imgType, _ := arch.GetImageType(imgTypeName) _, _, err := imgType.Manifest(&bp, distro.ImageOptions{}, nil, 0) - assert.EqualError(t, err, "The following custom mountpoints are not supported [\"/etc\"]") + assert.EqualError(t, err, "The following errors occurred while setting up custom mountpoints:\npath \"/etc\" is not allowed") } } } @@ -538,7 +538,7 @@ func TestDistro_DirtyMountpointsNotAllowed(t *testing.T) { for _, imgTypeName := range arch.ListImageTypes() { imgType, _ := arch.GetImageType(imgTypeName) _, _, err := imgType.Manifest(&bp, distro.ImageOptions{}, nil, 0) - assert.EqualError(t, err, "The following custom mountpoints are not supported [\"//\" \"/var//\" \"/var//log/audit/\"]") + assert.EqualError(t, err, "The following errors occurred while setting up custom mountpoints:\npath \"//\" must be canonical\npath \"/var//\" must be canonical\npath \"/var//log/audit/\" must be canonical") } } } diff --git a/pkg/distro/rhel/rhel7/distro_test.go b/pkg/distro/rhel/rhel7/distro_test.go index 5f8f0bc153..fe96b5d35c 100644 --- a/pkg/distro/rhel/rhel7/distro_test.go +++ b/pkg/distro/rhel/rhel7/distro_test.go @@ -296,7 +296,7 @@ func TestDistro_CustomFileSystemManifestError(t *testing.T) { for _, imgTypeName := range arch.ListImageTypes() { imgType, _ := arch.GetImageType(imgTypeName) _, _, err := imgType.Manifest(&bp, distro.ImageOptions{}, nil, 0) - assert.EqualError(t, err, "The following custom mountpoints are not supported [\"/etc\"]") + assert.EqualError(t, err, "The following errors occurred while setting up custom mountpoints:\npath \"/etc\" is not allowed") } } } @@ -408,7 +408,7 @@ func TestDistro_DirtyMountpointsNotAllowed(t *testing.T) { for _, imgTypeName := range arch.ListImageTypes() { imgType, _ := arch.GetImageType(imgTypeName) _, _, err := imgType.Manifest(&bp, distro.ImageOptions{}, nil, 0) - assert.EqualError(t, err, "The following custom mountpoints are not supported [\"//\" \"/var//\" \"/var//log/audit/\"]") + assert.EqualError(t, err, "The following errors occurred while setting up custom mountpoints:\npath \"//\" must be canonical\npath \"/var//\" must be canonical\npath \"/var//log/audit/\" must be canonical") } } } diff --git a/pkg/distro/rhel/rhel8/distro_test.go b/pkg/distro/rhel/rhel8/distro_test.go index 04c821ffd1..b459a40fdc 100644 --- a/pkg/distro/rhel/rhel8/distro_test.go +++ b/pkg/distro/rhel/rhel8/distro_test.go @@ -682,7 +682,7 @@ func TestDistro_CustomFileSystemManifestError(t *testing.T) { } else if unsupported[imgTypeName] { assert.Error(t, err) } else { - assert.EqualError(t, err, "The following custom mountpoints are not supported [\"/etc\"]") + assert.EqualError(t, err, "The following errors occurred while setting up custom mountpoints:\npath \"/etc\" is not allowed") } } } @@ -842,7 +842,7 @@ func TestDistro_DirtyMountpointsNotAllowed(t *testing.T) { if unsupported[imgTypeName] { assert.Error(t, err) } else { - assert.EqualError(t, err, "The following custom mountpoints are not supported [\"//\" \"/var//\" \"/var//log/audit/\"]") + assert.EqualError(t, err, "The following errors occurred while setting up custom mountpoints:\npath \"//\" must be canonical\npath \"/var//\" must be canonical\npath \"/var//log/audit/\" must be canonical") } } } diff --git a/pkg/distro/rhel/rhel9/distro_test.go b/pkg/distro/rhel/rhel9/distro_test.go index 4c4df734d9..cb6c70a64a 100644 --- a/pkg/distro/rhel/rhel9/distro_test.go +++ b/pkg/distro/rhel/rhel9/distro_test.go @@ -676,7 +676,7 @@ func TestDistro_CustomFileSystemManifestError(t *testing.T) { } else if imgTypeName == "edge-installer" || imgTypeName == "edge-simplified-installer" || imgTypeName == "edge-raw-image" || imgTypeName == "edge-ami" || imgTypeName == "edge-vsphere" { continue } else { - assert.EqualError(t, err, "The following custom mountpoints are not supported [\"/etc\"]") + assert.EqualError(t, err, "The following errors occurred while setting up custom mountpoints:\npath \"/etc\" is not allowed") } } } @@ -806,7 +806,7 @@ func TestDistro_DirtyMountpointsNotAllowed(t *testing.T) { if strings.HasPrefix(imgTypeName, "edge-") { continue } else { - assert.EqualError(t, err, "The following custom mountpoints are not supported [\"//\" \"/var//\" \"/var//log/audit/\"]") + assert.EqualError(t, err, "The following errors occurred while setting up custom mountpoints:\npath \"//\" must be canonical\npath \"/var//\" must be canonical\npath \"/var//log/audit/\" must be canonical") } } }