pkg/oscap: refactor the oscap configs

The oscap tailoring and remediation configs were too tightly coupled with the osbuild stage implementations. These configs can be refactored further by making the `TailoringConfig` a pointer on the `RemediationConfig`, reversing the dependency of the configs. These changes should reduce the complexity of the OpenSCAP stage options and hopefully reduce some of the confusion too.
say-paul · Aug 1, 2024 · 7cc2db4 · 7cc2db4
1 parent 1a6d42f
commit 7cc2db4
Show file tree

Hide file tree

Showing 7 changed files with 77 additions and 107 deletions.
diff --git a/pkg/customizations/oscap/oscap.go b/pkg/customizations/oscap/oscap.go
@@ -50,70 +50,52 @@ const (
 type RemediationConfig struct {
 	Datastream         string
 	ProfileID          string
-	TailoringPath      string
 	CompressionEnabled bool
+	TailoringConfig    *TailoringConfig
 }
 
 type TailoringConfig struct {
-	RemediationConfig
 	TailoredProfileID string
 	JSONFilepath      string
+	TailoringPath     string
 	Selected          []string
 	Unselected        []string
 }
 
-func newTailoringConfigs(profileID string, tc blueprint.OpenSCAPTailoringCustomizations, remediationConfig RemediationConfig) (*RemediationConfig, *TailoringConfig, error) {
-	// the tailoring config needs to know about the original base profile id,
-	// but the remediation config needs to know the updated profile id.
-	remediationConfig.ProfileID = fmt.Sprintf("%s_osbuild_tailoring", remediationConfig.ProfileID)
-	remediationConfig.TailoringPath = filepath.Join(DataDir, "tailoring.xml")
-
-	tailoringConfig := &TailoringConfig{
-		RemediationConfig: RemediationConfig{
-			ProfileID:     profileID,
-			Datastream:    remediationConfig.Datastream,
-			TailoringPath: remediationConfig.TailoringPath,
-		},
+func (rc *RemediationConfig) addTailoringConfigs(tc blueprint.OpenSCAPTailoringCustomizations) (*RemediationConfig, error) {
+	rc.TailoringConfig = &TailoringConfig{
+		TailoredProfileID: fmt.Sprintf("%s_osbuild_tailoring", rc.ProfileID),
 		Selected:          tc.Selected,
 		Unselected:        tc.Unselected,
-		TailoredProfileID: remediationConfig.ProfileID,
+		TailoringPath:     filepath.Join(DataDir, "tailoring.xml"),
 	}
 
-	return &remediationConfig, tailoringConfig, nil
+	return rc, nil
 }
 
-func newJsonConfigs(profileID string, json blueprint.OpenSCAPJSONTailoringCustomizations, remediationConfig RemediationConfig) (*RemediationConfig, *TailoringConfig, error) {
+func (rc *RemediationConfig) addJsonConfigs(json blueprint.OpenSCAPJSONTailoringCustomizations) (*RemediationConfig, error) {
 	if json.Filepath == "" {
-		return nil, nil, fmt.Errorf("Filepath to an JSON tailoring file is required")
+		return nil, fmt.Errorf("Filepath to an JSON tailoring file is required")
 	}
 
 	if json.ProfileID == "" {
-		return nil, nil, fmt.Errorf("Tailoring profile ID is required for an JSON tailoring file")
+		return nil, fmt.Errorf("Tailoring profile ID is required for an JSON tailoring file")
 	}
 
-	// the tailoring config needs to know about the original base profile id,
-	// but the remediation config needs to know the updated profile id.
-	remediationConfig.ProfileID = json.ProfileID
-	remediationConfig.TailoringPath = filepath.Join(DataDir, "tailoring.xml")
-
-	tailoringConfig := &TailoringConfig{
-		RemediationConfig: RemediationConfig{
-			ProfileID:     profileID,
-			Datastream:    remediationConfig.Datastream,
-			TailoringPath: remediationConfig.TailoringPath,
-		},
+	rc.TailoringConfig = &TailoringConfig{
 		JSONFilepath:      json.Filepath,
 		TailoredProfileID: json.ProfileID,
+		TailoringPath:     filepath.Join(DataDir, "tailoring.xml"),
 	}
 
-	return &remediationConfig, tailoringConfig, nil
+	return rc, nil
 }
 
-func NewConfigs(oscapConfig blueprint.OpenSCAPCustomization, defaultDatastream *string) (*RemediationConfig, *TailoringConfig, error) {
+func NewConfigs(oscapConfig blueprint.OpenSCAPCustomization, defaultDatastream *string) (*RemediationConfig, error) {
 	var datastream = oscapConfig.DataStream
 	if datastream == "" {
 		if defaultDatastream == nil {
-			return nil, nil, fmt.Errorf("No OSCAP datastream specified and the distro does not have any default set")
+			return nil, fmt.Errorf("No OSCAP datastream specified and the distro does not have any default set")
 		}
 		datastream = *defaultDatastream
 	}
@@ -129,13 +111,13 @@ func NewConfigs(oscapConfig blueprint.OpenSCAPCustomization, defaultDatastream *
 
 	switch {
 	case tc != nil && json != nil:
-		return nil, nil, fmt.Errorf("Multiple tailoring types set, only one type can be chosen (JSON/Override rules)")
+		return nil, fmt.Errorf("Multiple tailoring types set, only one type can be chosen (JSON/Override rules)")
 	case tc != nil:
-		return newTailoringConfigs(oscapConfig.ProfileID, *tc, remediationConfig)
+		return remediationConfig.addTailoringConfigs(*tc)
 	case json != nil:
-		return newJsonConfigs(oscapConfig.ProfileID, *json, remediationConfig)
+		return remediationConfig.addJsonConfigs(*json)
 	default:
-		return &remediationConfig, nil, nil
+		return &remediationConfig, nil
 	}
 }
 

diff --git a/pkg/customizations/oscap/oscap_test.go b/pkg/customizations/oscap/oscap_test.go
@@ -11,11 +11,10 @@ import (
 
 func TestOscapConfigGeneration(t *testing.T) {
 	tests := []struct {
-		name                string
-		options             blueprint.OpenSCAPCustomization
-		expectedRemediation *RemediationConfig
-		expectedTailoring   *TailoringConfig
-		err                 error
+		name     string
+		options  blueprint.OpenSCAPCustomization
+		expected *RemediationConfig
+		err      error
 	}{
 		{
 			name:    "no-datastream",
@@ -59,20 +58,15 @@ func TestOscapConfigGeneration(t *testing.T) {
 					ProfileID: "some-tailored-id",
 				},
 			},
-			expectedRemediation: &RemediationConfig{
+			expected: &RemediationConfig{
 				Datastream:         "datastream",
-				ProfileID:          "some-tailored-id",
-				TailoringPath:      filepath.Join(DataDir, "tailoring.xml"),
+				ProfileID:          "some-profile-id",
 				CompressionEnabled: true,
-			},
-			expectedTailoring: &TailoringConfig{
-				RemediationConfig: RemediationConfig{
-					Datastream:    "datastream",
-					ProfileID:     "some-profile-id",
-					TailoringPath: filepath.Join(DataDir, "tailoring.xml"),
+				TailoringConfig: &TailoringConfig{
+					TailoringPath:     filepath.Join(DataDir, "tailoring.xml"),
+					JSONFilepath:      "/some/filepath.json",
+					TailoredProfileID: "some-tailored-id",
 				},
-				JSONFilepath:      "/some/filepath.json",
-				TailoredProfileID: "some-tailored-id",
 			},
 			err: nil,
 		},
@@ -86,37 +80,30 @@ func TestOscapConfigGeneration(t *testing.T) {
 					Unselected: []string{"two", "four"},
 				},
 			},
-			expectedRemediation: &RemediationConfig{
+			expected: &RemediationConfig{
 				Datastream:         "datastream",
-				ProfileID:          "some-profile-id_osbuild_tailoring",
-				TailoringPath:      filepath.Join(DataDir, "tailoring.xml"),
+				ProfileID:          "some-profile-id",
 				CompressionEnabled: true,
-			},
-			expectedTailoring: &TailoringConfig{
-				RemediationConfig: RemediationConfig{
-					Datastream:    "datastream",
-					ProfileID:     "some-profile-id",
-					TailoringPath: filepath.Join(DataDir, "tailoring.xml"),
+				TailoringConfig: &TailoringConfig{
+					TailoringPath:     filepath.Join(DataDir, "tailoring.xml"),
+					TailoredProfileID: "some-profile-id_osbuild_tailoring",
+					Selected:          []string{"one", "three"},
+					Unselected:        []string{"two", "four"},
 				},
-				TailoredProfileID: "some-profile-id_osbuild_tailoring",
-				Selected:          []string{"one", "three"},
-				Unselected:        []string{"two", "four"},
 			},
 			err: nil,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			remediationConfig, tailoringConfig, err := NewConfigs(tt.options, nil)
+			remediationConfig, err := NewConfigs(tt.options, nil)
 			if tt.err != nil {
 				assert.NotNil(t, err)
 				assert.EqualValues(t, tt.err, err)
 				assert.Nil(t, remediationConfig)
-				assert.Nil(t, tailoringConfig)
 			} else {
 				assert.NoError(t, err)
-				assert.EqualValues(t, tt.expectedRemediation, remediationConfig)
-				assert.EqualValues(t, tt.expectedTailoring, tailoringConfig)
+				assert.EqualValues(t, tt.expected, remediationConfig)
 			}
 		})
 	}

diff --git a/pkg/distro/fedora/images.go b/pkg/distro/fedora/images.go
@@ -194,12 +194,11 @@ func osCustomizations(
 		}
 		osc.Directories = append(osc.Directories, oscapDataNode)
 
-		remediationConfig, tailoringConfig, err := oscap.NewConfigs(*oscapConfig, imageConfig.DefaultOSCAPDatastream)
+		remediationConfig, err := oscap.NewConfigs(*oscapConfig, imageConfig.DefaultOSCAPDatastream)
 		if err != nil {
 			panic(fmt.Errorf("error creating OpenSCAP configs: %w", err))
 		}
 
-		osc.OpenSCAPTailorConfig = tailoringConfig
 		osc.OpenSCAPRemediationConfig = remediationConfig
 	}
 

diff --git a/pkg/distro/rhel/images.go b/pkg/distro/rhel/images.go
@@ -215,12 +215,11 @@ func osCustomizations(
 		}
 		osc.Directories = append(osc.Directories, oscapDataNode)
 
-		remediationConfig, tailoringConfig, err := oscap.NewConfigs(*oscapConfig, imageConfig.DefaultOSCAPDatastream)
+		remediationConfig, err := oscap.NewConfigs(*oscapConfig, imageConfig.DefaultOSCAPDatastream)
 		if err != nil {
 			panic(fmt.Errorf("error creating OpenSCAP configs: %w", err))
 		}
 
-		osc.OpenSCAPTailorConfig = tailoringConfig
 		osc.OpenSCAPRemediationConfig = remediationConfig
 	}
 

diff --git a/pkg/manifest/os.go b/pkg/manifest/os.go
@@ -129,7 +129,6 @@ type OSCustomizations struct {
 	ContainersStorage   *string
 
 	// OpenSCAP config
-	OpenSCAPTailorConfig      *oscap.TailoringConfig
 	OpenSCAPRemediationConfig *oscap.RemediationConfig
 
 	Subscription *subscription.ImageOptions
@@ -324,7 +323,7 @@ func (p *OS) getBuildPackages(distro Distro) []string {
 		packages = append(packages, "skopeo")
 	}
 
-	if p.OpenSCAPTailorConfig != nil {
+	if p.OpenSCAPRemediationConfig != nil && p.OpenSCAPRemediationConfig.TailoringConfig != nil {
 		packages = append(packages, "openscap-utils")
 	}
 
@@ -807,22 +806,15 @@ func (p *OS) serialize() osbuild.Pipeline {
 		}
 	}
 
-	if p.OpenSCAPTailorConfig != nil {
-		if p.OpenSCAPRemediationConfig == nil {
-			// This is a programming error, since it doesn't make sense
-			// to have tailoring configs without openscap config.
-			panic(fmt.Errorf("OpenSCAP autotailoring cannot be set if no OpenSCAP config has been provided"))
-		}
-
-		tailoringStageOpts := osbuild.NewOscapAutotailorStageOptions(p.OpenSCAPTailorConfig)
-		pipeline.AddStage(osbuild.NewOscapAutotailorStage(tailoringStageOpts))
-	}
-
 	// NOTE: We need to run the OpenSCAP stages as the last stage before SELinux
 	// since the remediation may change file permissions and other aspects of the
 	// hardened image
-	if p.OpenSCAPRemediationConfig != nil {
-		remediationStageOpts := osbuild.NewOscapRemediationStageOptions(oscap.DataDir, p.OpenSCAPRemediationConfig)
+	if remediationConfig := p.OpenSCAPRemediationConfig; remediationConfig != nil {
+		if remediationConfig.TailoringConfig != nil {
+			tailoringStageOpts := osbuild.NewOscapAutotailorStageOptions(remediationConfig)
+			pipeline.AddStage(osbuild.NewOscapAutotailorStage(tailoringStageOpts))
+		}
+		remediationStageOpts := osbuild.NewOscapRemediationStageOptions(oscap.DataDir, remediationConfig)
 		pipeline.AddStage(osbuild.NewOscapRemediationStage(remediationStageOpts))
 	}
 

diff --git a/pkg/osbuild/oscap_autotailor_stage.go b/pkg/osbuild/oscap_autotailor_stage.go
@@ -73,36 +73,41 @@ func NewOscapAutotailorStage(options *OscapAutotailorStageOptions) *Stage {
 	}
 }
 
-func NewOscapAutotailorStageOptions(options *oscap.TailoringConfig) *OscapAutotailorStageOptions {
+func NewOscapAutotailorStageOptions(options *oscap.RemediationConfig) *OscapAutotailorStageOptions {
 	if options == nil {
 		return nil
 	}
 
+	tailoringConfig := options.TailoringConfig
+	if tailoringConfig == nil {
+		return nil
+	}
+
 	// TODO: don't panic! unfortunately this would involve quite
 	// a big refactor and we still need to be a bit defensive here
-	if options.RemediationConfig.TailoringPath == "" {
+	if tailoringConfig.TailoringPath == "" {
 		panic(fmt.Errorf("The tailoring path for the OpenSCAP remediation config cannot be empty, this is a programming error"))
 	}
 
-	if options.JSONFilepath != "" {
+	if tailoringConfig.JSONFilepath != "" {
 		return &OscapAutotailorStageOptions{
-			Filepath: options.RemediationConfig.TailoringPath,
+			Filepath: tailoringConfig.TailoringPath,
 			Config: AutotailorJSONConfig{
-				TailoredProfileID: options.TailoredProfileID,
-				Datastream:        options.RemediationConfig.Datastream,
-				TailoringFile:     options.JSONFilepath,
+				Datastream:        options.Datastream,
+				TailoredProfileID: tailoringConfig.TailoredProfileID,
+				TailoringFile:     tailoringConfig.JSONFilepath,
 			},
 		}
 	}
 
 	return &OscapAutotailorStageOptions{
-		Filepath: options.RemediationConfig.TailoringPath,
+		Filepath: tailoringConfig.TailoringPath,
 		Config: AutotailorKeyValueConfig{
-			NewProfile: options.TailoredProfileID,
-			Datastream: options.RemediationConfig.Datastream,
-			ProfileID:  options.RemediationConfig.ProfileID,
-			Selected:   options.Selected,
-			Unselected: options.Unselected,
+			Datastream: options.Datastream,
+			ProfileID:  options.ProfileID,
+			NewProfile: tailoringConfig.TailoredProfileID,
+			Selected:   tailoringConfig.Selected,
+			Unselected: tailoringConfig.Unselected,
 		},
 	}
 }
diff --git a/pkg/osbuild/oscap_remediation_stage.go b/pkg/osbuild/oscap_remediation_stage.go
@@ -81,13 +81,19 @@ func NewOscapRemediationStageOptions(dataDir string, options *oscap.RemediationC
 		return nil
 	}
 
+	config := OscapConfig{
+		ProfileID:   options.ProfileID,
+		Datastream:  options.Datastream,
+		Compression: options.CompressionEnabled,
+	}
+
+	if tc := options.TailoringConfig; tc != nil {
+		config.ProfileID = tc.TailoredProfileID
+		config.Tailoring = tc.TailoringPath
+	}
+
 	return &OscapRemediationStageOptions{
 		DataDir: dataDir,
-		Config: OscapConfig{
-			ProfileID:   options.ProfileID,
-			Datastream:  options.Datastream,
-			Tailoring:   options.TailoringPath,
-			Compression: options.CompressionEnabled,
-		},
+		Config:  config,
 	}
 }