From bc1623aa6b13e0dc80e0bd032f474809ff0ae11f Mon Sep 17 00:00:00 2001 From: Achilleas Koutsou Date: Wed, 4 Dec 2024 11:31:05 +0100 Subject: [PATCH 1/4] distro/fedora: don't return early from FIPS check in checkOptions() When the check was first added [1] it was the last in the function and was written to immediately return. New checks were added after the check, without changing the early return, which means invalid configurations were not caught. Append to warnings instead of returning. [1] 665a128f049c62b14285498f753a8c5fdbaa646d --- pkg/distro/fedora/imagetype.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/distro/fedora/imagetype.go b/pkg/distro/fedora/imagetype.go index 85558315f8..b250af5e1d 100644 --- a/pkg/distro/fedora/imagetype.go +++ b/pkg/distro/fedora/imagetype.go @@ -290,6 +290,8 @@ func (t *imageType) Manifest(bp *blueprint.Blueprint, func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOptions) ([]string, error) { customizations := bp.Customizations + var warnings []string + if !t.rpmOstree && options.OSTree != nil { return nil, fmt.Errorf("OSTree is not supported for %q", t.Name()) } @@ -448,8 +450,7 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp } if customizations.GetFIPS() && !common.IsBuildHostFIPSEnabled() { - w := fmt.Sprintln(common.FIPSEnabledImageWarning) - return []string{w}, nil + warnings = append(warnings, fmt.Sprintln(common.FIPSEnabledImageWarning)) } instCust, err := customizations.GetInstaller() @@ -474,5 +475,5 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp } } - return nil, nil + return warnings, nil } From 80adea940fe14c886817c4a9aaeeb07722da454c Mon Sep 17 00:00:00 2001 From: Achilleas Koutsou Date: Wed, 4 Dec 2024 11:37:37 +0100 Subject: [PATCH 2/4] distro/fedora: always return warnings from checkOptions() Always return the warnings list from checkOptions() so that future modifications or reshuffling of the checks doesn't drop any warnings collected before the return. --- pkg/distro/fedora/imagetype.go | 58 +++++++++++++++++----------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/pkg/distro/fedora/imagetype.go b/pkg/distro/fedora/imagetype.go index b250af5e1d..6f2a52e032 100644 --- a/pkg/distro/fedora/imagetype.go +++ b/pkg/distro/fedora/imagetype.go @@ -293,31 +293,31 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp var warnings []string if !t.rpmOstree && options.OSTree != nil { - return nil, fmt.Errorf("OSTree is not supported for %q", t.Name()) + return warnings, fmt.Errorf("OSTree is not supported for %q", t.Name()) } // we do not support embedding containers on ostree-derived images, only on commits themselves if len(bp.Containers) > 0 && t.rpmOstree && (t.name != "iot-commit" && t.name != "iot-container") { - return nil, fmt.Errorf("embedding containers is not supported for %s on %s", t.name, t.arch.distro.name) + return warnings, fmt.Errorf("embedding containers is not supported for %s on %s", t.name, t.arch.distro.name) } if options.OSTree != nil { if err := options.OSTree.Validate(); err != nil { - return nil, err + return warnings, err } } if t.bootISO && t.rpmOstree { // ostree-based ISOs require a URL from which to pull a payload commit if options.OSTree == nil || options.OSTree.URL == "" { - return nil, fmt.Errorf("boot ISO image type %q requires specifying a URL from which to retrieve the OSTree commit", t.name) + return warnings, fmt.Errorf("boot ISO image type %q requires specifying a URL from which to retrieve the OSTree commit", t.name) } } if t.name == "iot-raw-image" || t.name == "iot-qcow2-image" { allowed := []string{"User", "Group", "Directories", "Files", "Services", "FIPS"} if err := customizations.CheckAllowed(allowed...); err != nil { - return nil, fmt.Errorf(distro.UnsupportedCustomizationError, t.name, strings.Join(allowed, ", ")) + return warnings, fmt.Errorf(distro.UnsupportedCustomizationError, t.name, strings.Join(allowed, ", ")) } // TODO: consider additional checks, such as those in "edge-simplified-installer" in RHEL distros } @@ -328,16 +328,16 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp if t.name == "iot-simplified-installer" { allowed := []string{"InstallationDevice", "FDO", "Ignition", "Kernel", "User", "Group", "FIPS"} if err := customizations.CheckAllowed(allowed...); err != nil { - return nil, fmt.Errorf(distro.UnsupportedCustomizationError, t.name, strings.Join(allowed, ", ")) + return warnings, fmt.Errorf(distro.UnsupportedCustomizationError, t.name, strings.Join(allowed, ", ")) } if customizations.GetInstallationDevice() == "" { - return nil, fmt.Errorf("boot ISO image type %q requires specifying an installation device to install to", t.name) + return warnings, fmt.Errorf("boot ISO image type %q requires specifying an installation device to install to", t.name) } // FDO is optional, but when specified has some restrictions if customizations.GetFDO() != nil { if customizations.GetFDO().ManufacturingServerURL == "" { - return nil, fmt.Errorf("boot ISO image type %q requires specifying FDO.ManufacturingServerURL configuration to install to when using FDO", t.name) + return warnings, fmt.Errorf("boot ISO image type %q requires specifying FDO.ManufacturingServerURL configuration to install to when using FDO", t.name) } var diunSet int if customizations.GetFDO().DiunPubKeyHash != "" { @@ -350,54 +350,54 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp diunSet++ } if diunSet != 1 { - return nil, fmt.Errorf("boot ISO image type %q requires specifying one of [FDO.DiunPubKeyHash,FDO.DiunPubKeyInsecure,FDO.DiunPubKeyRootCerts] configuration to install to when using FDO", t.name) + return warnings, fmt.Errorf("boot ISO image type %q requires specifying one of [FDO.DiunPubKeyHash,FDO.DiunPubKeyInsecure,FDO.DiunPubKeyRootCerts] configuration to install to when using FDO", t.name) } } // ignition is optional, we might be using FDO if customizations.GetIgnition() != nil { if customizations.GetIgnition().Embedded != nil && customizations.GetIgnition().FirstBoot != nil { - return nil, fmt.Errorf("both ignition embedded and firstboot configurations found") + return warnings, fmt.Errorf("both ignition embedded and firstboot configurations found") } if customizations.GetIgnition().FirstBoot != nil && customizations.GetIgnition().FirstBoot.ProvisioningURL == "" { - return nil, fmt.Errorf("ignition.firstboot requires a provisioning url") + return warnings, fmt.Errorf("ignition.firstboot requires a provisioning url") } } } else if t.name == "iot-installer" || t.name == "image-installer" { // "Installer" is actually not allowed for image-installer right now, but this is checked at the end allowed := []string{"User", "Group", "FIPS", "Installer", "Timezone", "Locale"} if err := customizations.CheckAllowed(allowed...); err != nil { - return nil, fmt.Errorf(distro.UnsupportedCustomizationError, t.name, strings.Join(allowed, ", ")) + return warnings, fmt.Errorf(distro.UnsupportedCustomizationError, t.name, strings.Join(allowed, ", ")) } } else if t.name == "live-installer" { allowed := []string{"Installer"} if err := customizations.CheckAllowed(allowed...); err != nil { - return nil, fmt.Errorf(distro.NoCustomizationsAllowedError, t.name) + return warnings, fmt.Errorf(distro.NoCustomizationsAllowedError, t.name) } } } if kernelOpts := customizations.GetKernel(); kernelOpts.Append != "" && t.rpmOstree { - return nil, fmt.Errorf("kernel boot parameter customizations are not supported for ostree types") + return warnings, fmt.Errorf("kernel boot parameter customizations are not supported for ostree types") } mountpoints := customizations.GetFilesystems() partitioning, err := customizations.GetPartitioning() if err != nil { - return nil, err + return warnings, err } if (len(mountpoints) > 0 || partitioning != nil) && t.rpmOstree { - return nil, fmt.Errorf("Custom mountpoints and partitioning are not supported for ostree types") + return warnings, fmt.Errorf("Custom mountpoints and partitioning are not supported for ostree types") } if len(mountpoints) > 0 && partitioning != nil { - return nil, fmt.Errorf("partitioning customizations cannot be used with custom filesystems (mountpoints)") + return warnings, fmt.Errorf("partitioning customizations cannot be used with custom filesystems (mountpoints)") } if err := blueprint.CheckMountpointsPolicy(mountpoints, policies.MountpointPolicies); err != nil { - return nil, err + return warnings, err } if err := blueprint.CheckDiskMountpointsPolicy(partitioning, policies.MountpointPolicies); err != nil { - return nil, err + return warnings, err } if err := partitioning.ValidateLayoutConstraints(); err != nil { return nil, err @@ -406,13 +406,13 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp if osc := customizations.GetOpenSCAP(); osc != nil { supported := oscap.IsProfileAllowed(osc.ProfileID, oscapProfileAllowList) if !supported { - return nil, fmt.Errorf("OpenSCAP unsupported profile: %s", osc.ProfileID) + return warnings, fmt.Errorf("OpenSCAP unsupported profile: %s", osc.ProfileID) } if t.rpmOstree { - return nil, fmt.Errorf("OpenSCAP customizations are not supported for ostree types") + return warnings, fmt.Errorf("OpenSCAP customizations are not supported for ostree types") } if osc.ProfileID == "" { - return nil, fmt.Errorf("OpenSCAP profile cannot be empty") + return warnings, fmt.Errorf("OpenSCAP profile cannot be empty") } } @@ -422,7 +422,7 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp err = blueprint.ValidateDirFileCustomizations(dc, fc) if err != nil { - return nil, err + return warnings, err } dcp := policies.CustomDirectoriesPolicies @@ -435,18 +435,18 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp err = blueprint.CheckDirectoryCustomizationsPolicy(dc, dcp) if err != nil { - return nil, err + return warnings, err } err = blueprint.CheckFileCustomizationsPolicy(fc, fcp) if err != nil { - return nil, err + return warnings, err } // check if repository customizations are valid _, err = customizations.GetRepositories() if err != nil { - return nil, err + return warnings, err } if customizations.GetFIPS() && !common.IsBuildHostFIPSEnabled() { @@ -455,12 +455,12 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp instCust, err := customizations.GetInstaller() if err != nil { - return nil, err + return warnings, err } if instCust != nil { // only supported by the Anaconda installer if slices.Index([]string{"iot-installer"}, t.name) == -1 { - return nil, fmt.Errorf("installer customizations are not supported for %q", t.Name()) + return warnings, fmt.Errorf("installer customizations are not supported for %q", t.Name()) } // NOTE: the image type check is redundant with the check above, but @@ -471,7 +471,7 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp instCust.Kickstart != nil && len(instCust.Kickstart.Contents) > 0 && (customizations.GetUsers() != nil || customizations.GetGroups() != nil) { - return nil, fmt.Errorf("iot-installer installer.kickstart.contents are not supported in combination with users or groups") + return warnings, fmt.Errorf("iot-installer installer.kickstart.contents are not supported in combination with users or groups") } } From 811bc5fa344a10334dae206d4e94436b5591bcdc Mon Sep 17 00:00:00 2001 From: Achilleas Koutsou Date: Wed, 4 Dec 2024 11:39:44 +0100 Subject: [PATCH 3/4] distro/rhel: always return warnings from checkOptions() Always return the warnings list from checkOptions() so that future modifications or reshuffling of the checks doesn't drop any warnings collected before the return. --- pkg/distro/rhel/rhel8/options.go | 4 ++-- pkg/distro/rhel/rhel9/options.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/distro/rhel/rhel8/options.go b/pkg/distro/rhel/rhel8/options.go index 5a075b78cf..8f0494fb9c 100644 --- a/pkg/distro/rhel/rhel8/options.go +++ b/pkg/distro/rhel/rhel8/options.go @@ -29,14 +29,14 @@ func checkOptions(t *rhel.ImageType, bp *blueprint.Blueprint, options distro.Ima if options.OSTree != nil { if err := options.OSTree.Validate(); err != nil { - return nil, err + return warnings, err } } if t.BootISO && t.RPMOSTree { // ostree-based ISOs require a URL from which to pull a payload commit if options.OSTree == nil || options.OSTree.URL == "" { - return nil, fmt.Errorf("boot ISO image type %q requires specifying a URL from which to retrieve the OSTree commit", t.Name()) + return warnings, fmt.Errorf("boot ISO image type %q requires specifying a URL from which to retrieve the OSTree commit", t.Name()) } if t.Name() == "edge-simplified-installer" { diff --git a/pkg/distro/rhel/rhel9/options.go b/pkg/distro/rhel/rhel9/options.go index 4bd10ac076..6a5bd90e12 100644 --- a/pkg/distro/rhel/rhel9/options.go +++ b/pkg/distro/rhel/rhel9/options.go @@ -31,14 +31,14 @@ func checkOptions(t *rhel.ImageType, bp *blueprint.Blueprint, options distro.Ima if options.OSTree != nil { if err := options.OSTree.Validate(); err != nil { - return nil, err + return warnings, err } } if t.BootISO && t.RPMOSTree { // ostree-based ISOs require a URL from which to pull a payload commit if options.OSTree == nil || options.OSTree.URL == "" { - return nil, fmt.Errorf("boot ISO image type %q requires specifying a URL from which to retrieve the OSTree commit", t.Name()) + return warnings, fmt.Errorf("boot ISO image type %q requires specifying a URL from which to retrieve the OSTree commit", t.Name()) } if t.Name() == "edge-simplified-installer" { From db697f2d7785f7e13ef1f8bde16f9d6ad671fb39 Mon Sep 17 00:00:00 2001 From: Achilleas Koutsou Date: Wed, 4 Dec 2024 11:42:17 +0100 Subject: [PATCH 4/4] distro/rhel: don't print warnings in checkOptions() When collecting warnings in checkOptions() functions, don't `log.Print()` the warning. Let the caller decide whether to print them, suppress them, make them errors, format them, etc. --- pkg/distro/rhel/rhel10/options.go | 5 +---- pkg/distro/rhel/rhel8/options.go | 10 ++-------- pkg/distro/rhel/rhel9/options.go | 13 +++---------- 3 files changed, 6 insertions(+), 22 deletions(-) diff --git a/pkg/distro/rhel/rhel10/options.go b/pkg/distro/rhel/rhel10/options.go index 60550c41ed..059830a439 100644 --- a/pkg/distro/rhel/rhel10/options.go +++ b/pkg/distro/rhel/rhel10/options.go @@ -2,7 +2,6 @@ package rhel10 import ( "fmt" - "log" "slices" @@ -91,9 +90,7 @@ func checkOptions(t *rhel.ImageType, bp *blueprint.Blueprint, options distro.Ima } if customizations.GetFIPS() && !common.IsBuildHostFIPSEnabled() { - w := fmt.Sprintln(common.FIPSEnabledImageWarning) - log.Print(w) - warnings = append(warnings, w) + warnings = append(warnings, fmt.Sprintln(common.FIPSEnabledImageWarning)) } instCust, err := customizations.GetInstaller() diff --git a/pkg/distro/rhel/rhel8/options.go b/pkg/distro/rhel/rhel8/options.go index 8f0494fb9c..6e9d3e04dd 100644 --- a/pkg/distro/rhel/rhel8/options.go +++ b/pkg/distro/rhel/rhel8/options.go @@ -2,7 +2,6 @@ package rhel8 import ( "fmt" - "log" "strings" "slices" @@ -91,14 +90,10 @@ func checkOptions(t *rhel.ImageType, bp *blueprint.Blueprint, options distro.Ima // TODO(edge): directly error if these options are provided when rhel-9.5's time arrives if t.Name() == "edge-commit" || t.Name() == "edge-container" { if customizations.GetUsers() != nil { - w := fmt.Sprintf("Please note that user customizations on %q image type are deprecated and will be removed in the near future\n", t.Name()) - log.Print(w) - warnings = append(warnings, w) + warnings = append(warnings, fmt.Sprintf("Please note that user customizations on %q image type are deprecated and will be removed in the near future\n", t.Name())) } if customizations.GetGroups() != nil { - w := fmt.Sprintf("Please note that group customizations on %q image type are deprecated and will be removed in the near future\n", t.Name()) - log.Print(w) - warnings = append(warnings, w) + warnings = append(warnings, fmt.Sprintf("Please note that group customizations on %q image type are deprecated and will be removed in the near future\n", t.Name())) } } @@ -185,7 +180,6 @@ func checkOptions(t *rhel.ImageType, bp *blueprint.Blueprint, options distro.Ima if customizations.GetFIPS() && !common.IsBuildHostFIPSEnabled() { w := fmt.Sprintln(common.FIPSEnabledImageWarning) - log.Print(w) warnings = append(warnings, w) } diff --git a/pkg/distro/rhel/rhel9/options.go b/pkg/distro/rhel/rhel9/options.go index 6a5bd90e12..1e11753db5 100644 --- a/pkg/distro/rhel/rhel9/options.go +++ b/pkg/distro/rhel/rhel9/options.go @@ -2,7 +2,6 @@ package rhel9 import ( "fmt" - "log" "strings" "slices" @@ -104,14 +103,10 @@ func checkOptions(t *rhel.ImageType, bp *blueprint.Blueprint, options distro.Ima // TODO(edge): directly error if these options are provided when rhel-9.5's time arrives if t.Name() == "edge-commit" || t.Name() == "edge-container" { if customizations.GetUsers() != nil { - w := fmt.Sprintf("Please note that user customizations on %q image type are deprecated and will be removed in the near future\n", t.Name()) - log.Print(w) - warnings = append(warnings, w) + warnings = append(warnings, fmt.Sprintf("Please note that user customizations on %q image type are deprecated and will be removed in the near future\n", t.Name())) } if customizations.GetGroups() != nil { - w := fmt.Sprintf("Please note that group customizations on %q image type are deprecated and will be removed in the near future\n", t.Name()) - log.Print(w) - warnings = append(warnings, w) + warnings = append(warnings, fmt.Sprintf("Please note that group customizations on %q image type are deprecated and will be removed in the near future\n", t.Name())) } } @@ -203,9 +198,7 @@ func checkOptions(t *rhel.ImageType, bp *blueprint.Blueprint, options distro.Ima } if customizations.GetFIPS() && !common.IsBuildHostFIPSEnabled() { - w := fmt.Sprintln(common.FIPSEnabledImageWarning) - log.Print(w) - warnings = append(warnings, w) + warnings = append(warnings, fmt.Sprintln(common.FIPSEnabledImageWarning)) } instCust, err := customizations.GetInstaller()