Repository: osbuild/osbuild
Pull request #1794 (2024-05-29 10:15:59)

Make `tailored_profile_id` required for `json` autotailoring
We have a bit of a tradeoff with this. On one hand we can can just use the `autotailor` command to override the profile id set by the user in the json tailoring file. We would just need to document this very clearly. The benefit of this is that we can maintain the explicitness of the manifest and we wouldn't have to inspect the generated tailoring XML file for the `tailoring_id`.

The downside is that the user provided `profile_id` from the json contents will get overwritten.

kingsleyzissou: See comment: https://github.com/osbuild/osbuild/pull/1793#discussion_r1617964223
thozza: > The downside is that the user provided `profile_id` from the json contents will get overwritten.

What is the actual impact of this? Would be the overridden profile_id shown somewhere and confuse the user? From what I checked, this would concern only direct osbuild stage users, not users of osbuild-composer, right? Because they can specify only the tailoring rules, but not the new profile_id.
kingsleyzissou: > > The downside is that the user provided `profile_id` from the json contents will get overwritten.
> 
> 
> 
> What is the actual impact of this? Would be the overridden profile_id shown somewhere and confuse the user? From what I checked, this would concern only direct osbuild stage users, not users of osbuild-composer, right? Because they can specify only the tailoring rules, but not the new profile_id.

So they could craft the json file and say that they want the new profile id to be `profile_foo`. Internally we would override this and rename it to `base_profile_osbuild_tailoring`. We would use this then to run the remediations.

Once the image has been built, if the user isn't aware we overwrote the profile name, they might try run a scan with the `profile_foo` rather than `base_profile_osbuild_tailoring`.
kingsleyzissou: > > > The downside is that the user provided `profile_id` from the json contents will get overwritten.
> > 
> > 
> > What is the actual impact of this? Would be the overridden profile_id shown somewhere and confuse the user? From what I checked, this would concern only direct osbuild stage users, not users of osbuild-composer, right? Because they can specify only the tailoring rules, but not the new profile_id.
> 
> So they could craft the json file and say that they want the new profile id to be `profile_foo`. Internally we would override this and rename it to `base_profile_osbuild_tailoring`. We would use this then to run the remediations.
> 
> Once the image has been built, if the user isn't aware we overwrote the profile name, they might try run a scan with the `profile_foo` rather than `base_profile_osbuild_tailoring`. Which might be unexpected for them. We would need to document this well. The alternative would be to read the profile id that the user supplied

I added a unit test in d70dfdc to show what this looks like
kingsleyzissou: Marking as draft again because it might be good to iron out the details with the compliance team and the OpenSCAP team in the office next week.
github-actions[bot]: 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.
achilleas-k: LGTM.  Minor q: https://github.com/osbuild/osbuild/pull/1794#discussion_r1672095030 was marked resolved with no response or change.
kingsleyzissou: > was marked resolved with no response or change.

Damn. I definitely changed it but must overwritten the change. Thanks, I'll fix that now
achilleas-k: Thanks!  LGTM

Patch:
diff --git a/osbuild.spec b/osbuild.spec
index f7229990..93880b17 100644
--- a/osbuild.spec
+++ b/osbuild.spec
@@ -1,7 +1,7 @@
 %global         forgeurl https://github.com/osbuild/osbuild
 %global         selinuxtype targeted
 
-Version:        123
+Version:        124
 
 %forgemeta
 
diff --git a/osbuild/__init__.py b/osbuild/__init__.py
index c9363164..a2b2c711 100644
--- a/osbuild/__init__.py
+++ b/osbuild/__init__.py
@@ -10,7 +10,7 @@ independent of osbuild but used across the osbuild codebase.
 
 from .pipeline import Manifest, Pipeline, Stage
 
-__version__ = "123"
+__version__ = "124"
 
 __all__ = [
     "Manifest",
diff --git a/setup.py b/setup.py
index 26c9f717..d8f5e27a 100644
--- a/setup.py
+++ b/setup.py
@@ -2,7 +2,7 @@ import setuptools
 
 setuptools.setup(
     name="osbuild",
-    version="123",
+    version="124",
     description="A build system for OS images",
     packages=["osbuild", "osbuild.formats", "osbuild.util"],
     license='Apache-2.0',
diff --git a/sources/org.osbuild.curl b/sources/org.osbuild.curl
index db271ff3..c858ff61 100755
--- a/sources/org.osbuild.curl
+++ b/sources/org.osbuild.curl
@@ -317,6 +317,9 @@ class CurlSource(sources.SourceService):
                 failed_urls = ",".join([itm[1]["url"] for itm in dl_pairs])
                 raise RuntimeError(f"curl: error downloading {failed_urls}: error code {return_code}")
 
+        if len(dl_pairs) > 0:
+            raise RuntimeError(f"curl: finished with return_code {return_code} but {dl_pairs} left to download")
+
     def _fetch_all_old_curl(self, amended):
         with concurrent.futures.ThreadPoolExecutor(max_workers=self.max_workers) as executor:
             for _ in executor.map(self.fetch_one, *zip(*amended)):
diff --git a/stages/org.osbuild.oscap.autotailor b/stages/org.osbuild.oscap.autotailor
index 3c33eecb..09953c04 100755
--- a/stages/org.osbuild.oscap.autotailor
+++ b/stages/org.osbuild.oscap.autotailor
@@ -14,16 +14,26 @@ def main(tree, options):
     profile = config.get("profile_id")
     new_profile = config.get("new_profile")
     tailoring_file = config.get("tailoring_file")
+    # use `new_profile` as a fallback for backwards compatibility
+    tailored_profile_id = config.get("tailored_profile_id", new_profile)
 
-    # make sure either profile & new_profile are set
-    # or tailoring_filepath is set
-    if not ((profile and new_profile) or tailoring_file):
-        raise ValueError("Either profile & new profile must be set or provide filepath to json tailoring file")
+    if new_profile is not None:
+        print("WARNING: use the `tailored_profile_id` option instead")
+
+    # this is needed now since we have renamed the `new_profile` option
+    # to `tailored_profile_id`
+    if not tailored_profile_id:
+        raise ValueError("The tailoring profile id option is a required field")
+
+    # make sure either profile or tailoring_file is set
+    if not (profile or tailoring_file):
+        raise ValueError("Either profile must be set or a filepath to json tailoring file must be provided")
 
     if tailoring_file:
         cmd = [
             "/usr/bin/autotailor",
             "--output", f"{tree}/{filepath.lstrip('/')}",
+            "--new-profile-id", tailored_profile_id,
             "--json-tailoring", f"{tree}/{tailoring_file.lstrip('/')}",
             datastream,
         ]
@@ -39,7 +49,7 @@ def main(tree, options):
     cmd = [
         "/usr/bin/autotailor",
         "--output", f"{tree}/{filepath.lstrip('/')}",
-        "--new-profile-id", new_profile
+        "--new-profile-id", tailored_profile_id,
     ]
 
     for s in selected:
diff --git a/stages/org.osbuild.oscap.autotailor.meta.json b/stages/org.osbuild.oscap.autotailor.meta.json
index 77f28c10..11091790 100644
--- a/stages/org.osbuild.oscap.autotailor.meta.json
+++ b/stages/org.osbuild.oscap.autotailor.meta.json
@@ -14,6 +14,7 @@
         "additionalProperties": false,
         "required": [
           "datastream",
+          "tailored_profile_id",
           "tailoring_file"
         ],
         "type": "object",
@@ -23,6 +24,10 @@
             "type": "string",
             "description": "The path to the datastream file"
           },
+          "tailored_profile_id": {
+            "type": "string",
+            "description": "The id of the new customized (tailored) OpenSCAP profile"
+          },
           "tailoring_file": {
             "type": "string",
             "description": "Path to the JSON file which contains tailoring options to be imported"
@@ -33,8 +38,7 @@
         "additionalProperties": false,
         "required": [
           "profile_id",
-          "datastream",
-          "new_profile"
+          "datastream"
         ],
         "type": "object",
         "description": "OpenSCAP configuration variables",
@@ -49,7 +53,11 @@
           },
           "new_profile": {
             "type": "string",
-            "description": "The name of the new customized OpenSCAP profile"
+            "description": "The id of the new customized (tailored) OpenSCAP profile"
+          },
+          "tailored_profile_id": {
+            "type": "string",
+            "description": "The id of the new customized (tailored) OpenSCAP profile"
           },
           "selected": {
             "type": "array",


Test patch:
diff --git a/sources/test/test_curl_source.py b/sources/test/test_curl_source.py
index a3fd45f0..5a591a9e 100644
--- a/sources/test/test_curl_source.py
+++ b/sources/test/test_curl_source.py
@@ -9,6 +9,7 @@ from unittest.mock import patch
 
 import pytest
 
+import osbuild.testutil
 from osbuild.testutil.net import http_serve_directory
 
 SOURCES_NAME = "org.osbuild.curl"
@@ -306,3 +307,16 @@ def test_curl_has_parallel_download(mocked_check_output, monkeypatch, sources_mo
 
     mocked_check_output.return_value = OLD_CURL_OUTPUT
     assert not sources_module.curl_has_parallel_downloads()
+
+
+# this check is only done in the "parallel=True" case
+@pytest.mark.parametrize("curl_parallel", [True], indirect=["curl_parallel"])
+def test_curl_result_is_double_checked(tmp_path, curl_parallel):
+    test_sources = make_test_sources(tmp_path, 1234, 5)
+
+    # simulate that curl returned an exit code 0 even though not all
+    # sources got downloaded
+    with osbuild.testutil.mock_command("curl", ""):
+        with pytest.raises(RuntimeError) as exp:
+            curl_parallel.fetch_all(test_sources)
+        assert re.match(r"curl: finished with return_code 0 but .* left to download", str(exp.value))
diff --git a/stages/test/test_autotailor.py b/stages/test/test_autotailor.py
index b9e90fd0..c06c42e0 100644
--- a/stages/test/test_autotailor.py
+++ b/stages/test/test_autotailor.py
@@ -1,5 +1,6 @@
 #!/usr/bin/python3
 
+import subprocess
 import sys
 from unittest.mock import call, patch
 
@@ -45,7 +46,7 @@ def fake_input_fixture():
             "config": {
                 "profile_id": "some-profile-id",
                 "datastream": "some-datastream",
-                "new_profile": "some-new-profile",
+                "tailored_profile_id": "some-new-profile",
             }
         },
     }
@@ -59,6 +60,7 @@ def fake_json_input_fixture():
             "filepath": "tailoring-output.xml",
             "config": {
                 "datastream": "some-datastream",
+                "tailored_profile_id": "some-new-profile",
                 "tailoring_file": "tailoring-file.json",
             }
         },
@@ -90,7 +92,7 @@ def test_oscap_autotailor_overrides_smoke(mock_subprocess_run, fake_input, stage
                     "value": 50,
                 },
             ]
-        }, "{'profile_id': 'some-profile-id', 'datastream': 'some-datastream', 'new_profile': 'some-new-profile',"
+        }, "{'profile_id': 'some-profile-id', 'datastream': 'some-datastream', 'tailored_profile_id': 'some-new-profile',"
             + " 'overrides': [{'no': 'var', 'value': 50}]} is not valid under any of the given schemas"),
         ({
             "overrides": [
@@ -99,7 +101,7 @@ def test_oscap_autotailor_overrides_smoke(mock_subprocess_run, fake_input, stage
                     "var": "some",
                 },
             ]
-        }, "{'profile_id': 'some-profile-id', 'datastream': 'some-datastream', 'new_profile': 'some-new-profile',"
+        }, "{'profile_id': 'some-profile-id', 'datastream': 'some-datastream', 'tailored_profile_id': 'some-new-profile',"
             + " 'overrides': [{'no': 'value', 'var': 'some'}]} is not valid under any of the given schemas"),
         ({
             "overrides": [
@@ -108,7 +110,7 @@ def test_oscap_autotailor_overrides_smoke(mock_subprocess_run, fake_input, stage
                     "value": {"some": "object"},
                 },
             ]
-        }, "{'profile_id': 'some-profile-id', 'datastream': 'some-datastream', 'new_profile': 'some-new-profile',"
+        }, "{'profile_id': 'some-profile-id', 'datastream': 'some-datastream', 'tailored_profile_id': 'some-new-profile',"
             + " 'overrides': [{'var': 'ssh_idle_timeout_value', 'value': {'some': 'object'}}]} is not valid under any of the given schemas"),
     ],
 )
@@ -128,5 +130,84 @@ def test_oscap_autotailor_json_smoke(mock_subprocess_run, fake_json_input, stage
 
     assert mock_subprocess_run.call_args_list == [
         call(["/usr/bin/autotailor", "--output", "/some/sysroot/tailoring-output.xml",
+              "--new-profile-id", "some-new-profile",
               "--json-tailoring", "/some/sysroot/tailoring-file.json", "some-datastream"],
              encoding='utf8', stdout=sys.stderr, check=True)]
+
+
+@pytest.mark.parametrize(
+    "test_data,expected_err",
+    [
+        ({}, "{} is not valid under any of the given schemas"),
+        ({
+            "tailored_profile_id": "some-new-profile"
+        }, "{'tailored_profile_id': 'some-new-profile'}"
+            + " is not valid under any of the given schemas"),
+        ({
+            "datastream": "some-datastream",
+            "tailored_profile_id": "some-new-profile"
+        }, "{'datastream': 'some-datastream', 'tailored_profile_id': 'some-new-profile'}"
+            + " is not valid under any of the given schemas"),
+        ({
+            "datastream": "some-datastream",
+            "tailoring_file": "/some/tailoring/file.json"
+        }, "{'datastream': 'some-datastream', 'tailoring_file': '/some/tailoring/file.json'}"
+            + " is not valid under any of the given schemas"),
+    ],
+)
+@pytest.mark.parametrize("stage_schema", ["1"], indirect=True)
+def test_schema_validation_oscap_json_autotailor(fake_json_input, stage_schema, test_data, expected_err):
+    fake_json_input["options"]["config"] = test_data
+    res = stage_schema.validate(fake_json_input)
+    assert res.valid is False
+    testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
+
+
+JSON_TAILORING = """{
+  "profiles": [
+    {
+      "id": "some-profile-id",
+      "base_profile_id": "some-profile-id",
+      "rules": {
+        "some-rule": {
+          "evaluate": true,
+          "severity": "high"
+        }
+      },
+      "variables": {
+        "some-variable": {
+          "value": 600
+        }
+      }
+    }
+  ]
+}
+"""
+
+
+@pytest.mark.parametrize(
+    "expected_profile",
+    [
+        ("xccdf_org.ssgproject.content_profile_some-new-profile"),
+        ("xccdf_org.ssgproject.content_profile_some-other-profile")
+    ]
+)
+@pytest.mark.skipif(not testutil.has_executable("autotailor"), reason="no autotailor executable")
+def test_oscap_autotailor_json_profile_override(fake_json_input, stage_module, expected_profile, tmp_path):
+    options = fake_json_input["options"]
+    options["config"]["tailored_profile_id"] = expected_profile
+
+    results_file = tmp_path / options["filepath"]
+    tailoring_file = tmp_path / options["config"]["tailoring_file"]
+    tailoring_file.write_text(JSON_TAILORING)
+
+    stage_module.main(str(tmp_path), options)
+
+    result = subprocess.run(
+        ["oscap", "info", "--profiles", results_file],
+        stdout=subprocess.PIPE,
+        check=True,
+        text=True,
+    )
+
+    assert f"Id: {expected_profile}\n" in result.stdout
diff --git a/test/data/stages/oscap.autotailor-json/b.json b/test/data/stages/oscap.autotailor-json/b.json
index fbd0c5b1..13ad9fbe 100644
--- a/test/data/stages/oscap.autotailor-json/b.json
+++ b/test/data/stages/oscap.autotailor-json/b.json
@@ -902,6 +902,7 @@
             "filepath": "/usr/share/osbuild/oscap/tailoring.xml",
             "config": {
               "datastream": "/usr/share/xml/scap/ssg/content/ssg-fedora-ds.xml",
+              "tailored_profile_id": "ospp_osbuild_tailoring",
               "tailoring_file": "/json-tailoring.json"
             }
           }
diff --git a/test/data/stages/oscap.autotailor-json/b.mpp.yaml b/test/data/stages/oscap.autotailor-json/b.mpp.yaml
index 9497f806..95aeefcc 100644
--- a/test/data/stages/oscap.autotailor-json/b.mpp.yaml
+++ b/test/data/stages/oscap.autotailor-json/b.mpp.yaml
@@ -76,4 +76,5 @@ pipelines:
           filepath: /usr/share/osbuild/oscap/tailoring.xml
           config:
             datastream: /usr/share/xml/scap/ssg/content/ssg-fedora-ds.xml
+            tailored_profile_id: "ospp_osbuild_tailoring"
             tailoring_file: /json-tailoring.json
diff --git a/test/data/stages/oscap.autotailor/b.json b/test/data/stages/oscap.autotailor/b.json
index b8cebf04..c5714cc4 100644
--- a/test/data/stages/oscap.autotailor/b.json
+++ b/test/data/stages/oscap.autotailor/b.json
@@ -464,7 +464,7 @@
             "config": {
               "datastream": "/usr/share/xml/scap/ssg/content/ssg-fedora-ds.xml",
               "profile_id": "xccdf_org.ssgproject.content_profile_ospp",
-              "new_profile": "xccdf_org.ssgproject.content_profile_ospp_osbuild_tailoring",
+              "tailored_profile_id": "xccdf_org.ssgproject.content_profile_ospp_osbuild_tailoring",
               "selected": [
                 "partition_for_var_log"
               ],
diff --git a/test/data/stages/oscap.autotailor/b.mpp.yaml b/test/data/stages/oscap.autotailor/b.mpp.yaml
index 5360f50c..f2564f55 100644
--- a/test/data/stages/oscap.autotailor/b.mpp.yaml
+++ b/test/data/stages/oscap.autotailor/b.mpp.yaml
@@ -21,7 +21,7 @@ pipelines:
           config:
             datastream: /usr/share/xml/scap/ssg/content/ssg-fedora-ds.xml
             profile_id: xccdf_org.ssgproject.content_profile_ospp
-            new_profile: xccdf_org.ssgproject.content_profile_ospp_osbuild_tailoring
+            tailored_profile_id: xccdf_org.ssgproject.content_profile_ospp_osbuild_tailoring
             selected:
               - partition_for_var_log
             unselected: