OpenSCAP tailoring: add key/value rule overrides #300

kingsleyzissou · 2023-12-04T22:44:09Z

Expose a field in the blueprints to be able to override rule values
using the OpenSCAP autotailor feature. The customization exposes an
array with two values, the name of the var to overwrite and the value
to write to the var.

This depends on osbuild/osbuild#1407
Jira: https://issues.redhat.com/browse/COMPOSER-1994

thozza

Nice work. I added a few comments / ideas inline.

pkg/blueprint/customizations.go

pkg/osbuild/oscap_autotailor_stage.go

pkg/distro/rhel9/images.go

Refactor the way in which the default datastream is obtained. This will make it easier to generalise and simplify the creation of the OpenSCAP remediation & tailoring stage options. Jira: https://issues.redhat.com/browse/COMPOSER-1994

Extract the creation of the required OpenSCAP directories into the OpenSCAP package. Simplify the directory creation into a single function which returns all the required directories for both the remediation and tailoring stages. Jira: https://issues.redhat.com/browse/COMPOSER-1994

Generalise the tailoring stage options so it can be re-used by each of the distros. Before this, there was a lot of repetition for each distro when creating the stage options. This simplification also allowed some decoupling from the remediation stage options. Jira: https://issues.redhat.com/browse/COMPOSER-1994

Generalise the creation of the remediation stage options and extract the function to the `oscap` package. This function also slightly reduces the tight coupling with the `tailoring` stage options. However, this stage still depends on some of the config from the `tailoring` stage. Jira: https://issues.redhat.com/browse/COMPOSER-1994

Expose a field in the blueprints to be able to override rule values using the OpenSCAP autotailor feature. The customization exposes an array with two values, the name of the var to overwrite and the value to write to the var. Jira: https://issues.redhat.com/browse/COMPOSER-1994

Expose the overrides field in the OpenSCAP autotailor stage. This parses the input and adds the relevant fields to the osbuild json schema. Jira: https://issues.redhat.com/browse/COMPOSER-1994

Enable override rules for the `tailoring` stage. These rules allow users to specify a key/value pair for rule values that they would like to override. Jira: https://issues.redhat.com/browse/COMPOSER-1994

Update the OpenSCAP tests with the autotailor overrides Jira: https://issues.redhat.com/browse/COMPOSER-1994

thozza

Very nice rework (plus of course the added support for override values), I like it!

I added a few comments, mostly of a cosmetic nature. On top of that, I think that moving all functions that generate osbuild stage options should be moved to the osbuild package.

thozza · 2023-12-18T11:42:58Z

pkg/customizations/oscap/oscap.go

 	}
-	return defaultCentos8Datastream
+
+	return defaultRHELDatastream(d.Releasever())


Just a thought, not sure if of any value: Would it make sense to check also rhel as the prefix and maybe panic() in the default case when the distro is not explicitly supported?

thozza · 2023-12-18T11:58:49Z

pkg/customizations/oscap/stage_options.go

+	// although the osbuild stage will create this directory,
+	// it's probably better to ensure that it is created here


I had to double-check. Because the directory is technically not created here, but in the OS pipeline implementation which generates the osbuild pipeline. Directories are created before the OSCAP stage is added to the pipeline, so this is all fine in the end.

thozza · 2023-12-18T12:13:31Z

pkg/customizations/oscap/stage_options.go

+	return fmt.Sprintf("%s_osbuild_tailoring", profileID)
+}
+
+func CreateTailoringStageOptions(oscapConfig *blueprint.OpenSCAPCustomization, d distro.Distro) *osbuild.OscapAutotailorStageOptions {


I think that it would make more sense to move this to the osbuild package, instead of the oscap. We usually generate osbuild stage options from our internal abstractions in the osbuild package.

Lastly, a constructor function for stage options would IMHO feel more natural in the osbuild package.

thozza · 2023-12-18T12:19:51Z

pkg/distro/fedora/images.go

+			t.arch.distro,
+		)
+
+		if tailorConfig := osc.OpenSCAPTailorConfig; tailorConfig != nil {


Is this only to make the following two lines shorter? I must admit that I had to stop for a second to carefully read the code and understand that you are assigning a variable to another variable just to check if it is nil...

thozza · 2023-12-18T12:24:44Z

pkg/customizations/oscap/stage_options.go

@@ -64,3 +64,38 @@ func CreateTailoringStageOptions(oscapConfig *blueprint.OpenSCAPCustomization, d
 		},
 	)
 }
+
+func CreateRemediationStageOptions(


Again, this should IMO go into the osbuild package.

thozza · 2023-12-18T12:27:30Z

pkg/customizations/oscap/stage_options.go

+	if isOSTree {
+		return nil, nil, fmt.Errorf("unexpected oscap options for ostree image type")
+	}


This is IMO not the place to check if the image type is ostree or not. This is not the job if a function which generates an osbuild stage options. Instead, this should be checked on a higher-lervel in the distro implementation and we should return an error there in case one wants to use BP customizations with an image type that does not support them.

thozza · 2023-12-18T12:29:48Z

pkg/distro/fedora/images.go

-		if len(directories) > 0 {
-			osc.Directories = append(osc.Directories, directories...)
-		}
+	var directories []*fsnode.Directory


nitpick: consider renaming this to something like oscapDirectories to make it obvious what the variable is used for.

thozza · 2023-12-18T12:34:35Z

pkg/blueprint/openscap_customizations.go

+	case string:
+		ot.Var = d["var"].(string)
+	default:
+		return fmt.Errorf("TOML unmarshal: override var must be string, got %v of type %T", d["var"], d["var"])


nitpick: you could use this instead of specifying the same value twice:

return fmt.Errorf("TOML unmarshal: override var must be string, got %[1]v of type %[1]T", d["var"])

and basically in all similar cases in this file.

thozza · 2023-12-18T12:43:59Z

pkg/osbuild/oscap_autotailor_stage.go

+			continue
+		}
+
+		return fmt.Errorf("override 'value' must be an integere or a string")


would it make sense to also include the name of the override value in the error?

pkg/osbuild/oscap_autotailor_stage.go

github-actions · 2024-01-18T04:00:29Z

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

github-actions · 2024-01-25T04:00:38Z

This PR was closed because it has been stalled for 30+7 days with no activity.

kingsleyzissou requested review from thozza and supakeen December 5, 2023 14:18

kingsleyzissou force-pushed the autotailor-overrides branch from 3e15780 to 43959a9 Compare December 5, 2023 14:18

thozza reviewed Dec 6, 2023

View reviewed changes

pkg/blueprint/customizations.go Outdated Show resolved Hide resolved

pkg/osbuild/oscap_autotailor_stage.go Show resolved Hide resolved

pkg/distro/rhel9/images.go Outdated Show resolved Hide resolved

kingsleyzissou added 9 commits December 13, 2023 14:35

schutz: update osbuild commit

68fdf42

pkg/distro: generalise default datastream

2e725fc

Refactor the way in which the default datastream is obtained. This will make it easier to generalise and simplify the creation of the OpenSCAP remediation & tailoring stage options. Jira: https://issues.redhat.com/browse/COMPOSER-1994

pkg/osbuild: enable OpenSCAP tailoring overrides

fefcc4c

Expose the overrides field in the OpenSCAP autotailor stage. This parses the input and adds the relevant fields to the osbuild json schema. Jira: https://issues.redhat.com/browse/COMPOSER-1994

pkg/customizations: enable tailor overrides

be317b5

Enable override rules for the `tailoring` stage. These rules allow users to specify a key/value pair for rule values that they would like to override. Jira: https://issues.redhat.com/browse/COMPOSER-1994

test/configs: update OpenSCAP tests

3b6b310

Update the OpenSCAP tests with the autotailor overrides Jira: https://issues.redhat.com/browse/COMPOSER-1994

kingsleyzissou force-pushed the autotailor-overrides branch from 43959a9 to 3b6b310 Compare December 13, 2023 15:29

thozza reviewed Dec 18, 2023

View reviewed changes

github-actions bot added the Stale label Jan 18, 2024

github-actions bot closed this Jan 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenSCAP tailoring: add key/value rule overrides #300

OpenSCAP tailoring: add key/value rule overrides #300

kingsleyzissou commented Dec 4, 2023

thozza left a comment

thozza left a comment

thozza Dec 18, 2023

thozza Dec 18, 2023

thozza Dec 18, 2023

thozza Dec 18, 2023

thozza Dec 18, 2023

thozza Dec 18, 2023

thozza Dec 18, 2023

thozza Dec 18, 2023

thozza Dec 18, 2023

github-actions bot commented Jan 18, 2024

github-actions bot commented Jan 25, 2024

		// although the osbuild stage will create this directory,
		// it's probably better to ensure that it is created here

OpenSCAP tailoring: add key/value rule overrides #300

OpenSCAP tailoring: add key/value rule overrides #300

Conversation

kingsleyzissou commented Dec 4, 2023

thozza left a comment

Choose a reason for hiding this comment

thozza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 18, 2024

github-actions bot commented Jan 25, 2024