Repository: osbuild/osbuild
Pull request #726 (2021-07-15 08:57:17)

util/rhsm: Implement a fallback to the previous behaviour
The problem is that some deployments might not have the redhat.repo
file, yet they might have the key and certificate to access Red Hat CDN.
If that was the case, the new approach would cause a regression compared
to the previous behavior.

This patch uses the previous method if the redhat.repo file is not
found. This can still cause a regression if the file is empty or does
not contain any matching entries.

ondrejbudai: I wonder if we want to ship this in 8.5. It makes sense to ship it in 8.4.z since we don't want to regress there. For 8.5, I think it's fine to go with just the more correct behaviour that sadly doesn't work with manage_repos=0.
msehnout: > I wonder if we want to ship this in 8.5. It makes sense to ship it in 8.4.z since we don't want to regress there. For 8.5, I think it's fine to go with just the more correct behaviour that sadly doesn't work with manage_repos=0.

Yes, I'm considering the same. It does sound a bit awkward to have a different implementation in RHEL8.4.Z, but it probably makes sense.

But I think it is ok if it doesn't work with `manage_repos=0`, after all the secrets are called "rhsm" so if RHSM is not responsible for managing repos, it is only expected that Image Builder won't be able to fetch RPMs. We should have a completely separate feature for RHUI which uses HTTP headers and/or additional mechanisms which are specific to each cloud provider.
msehnout: See #727 
bcl: I think this would be useful in all versions. There are 2 other situations where the redhat.repo content won't match the urls used in the osbuild-composer repo files; one is employee subscriptions and the other is adding point release repo files so that the new distro build feature can build for other releases (eg. build RHEL 8.4 images from an 8.5 system). The problem there is that the system is subscribed, and has a key that works, but the urls aren't specifically listed in the file.
msehnout: > I think this would be useful in all versions. There are 2 other situations where the redhat.repo content won't match the urls used in the osbuild-composer repo files; one is employee subscriptions and the other is adding point release repo files so that the new distro build feature can build for other releases (eg. build RHEL 8.4 images from an 8.5 system). The problem there is that the system is subscribed, and has a key that works, but the urls aren't specifically listed in the file.

So the scenario would be that you are subscribed to a RHEL content, e.g. 8.4, using RHSM, but you are running RHEL 8.5 so RHSM doesn't create the entries in redhat.repo, but you can use the key/cert to access it? I thought this cannot be done. Doesn't it create entries in redhat.repo with `enabled=0`?

The problem with the previous approach is that it only works if you have exactly one key on the system. Otherwise it is just guessing.
bcl: > So the scenario would be that you are subscribed to a RHEL content, e.g. 8.4, using RHSM, but you are running RHEL 8.5 so RHSM doesn't create the entries in redhat.repo, but you can use the key/cert to access it? I thought this cannot be done. Doesn't it create entries in redhat.repo with `enabled=0`?

I don't know for sure, all I have is my employee subscription and the redhat.repo didn't have the specific releases in it but I could still access them via curl (and a modified .repo file that I dropped into /etc/osbuild/repositories).

> 
> The problem with the previous approach is that it only works if you have exactly one key on the system. Otherwise it is just guessing.

Right, but falling back and working most of the time is better than failing when a fallback could work. How common is it to have more than one keypair?

msehnout: Eventually we agreed this should cause no harm (fingers crossed) so it is fine to be merged to main as well as to 8.4.Z.

Patch:
diff --git a/mounts/org.osbuild.noop b/mounts/org.osbuild.noop
new file mode 100755
index 00000000..15a4bfa9
--- /dev/null
+++ b/mounts/org.osbuild.noop
@@ -0,0 +1,47 @@
+#!/usr/bin/python3
+"""
+No-op mount service
+
+Does not mount anything, but only creates an empty directory.
+Useful for testing.
+
+Host commands used: mount
+"""
+
+import os
+import sys
+from typing import Dict
+
+from osbuild import mounts
+
+
+SCHEMA = """
+"additionalProperties": false
+"""
+
+
+class NoOpMount(mounts.MountService):
+
+    def mount(self, _source: str, root: str, target: str, _options: Dict):
+
+        mountpoint = os.path.join(root, target.lstrip("/"))
+
+        os.makedirs(mountpoint, exist_ok=True)
+        self.mountpoint = mountpoint
+
+        return mountpoint
+
+    def umount(self):
+        self.mountpoint = None
+
+    def sync(self):
+        pass
+
+
+def main():
+    service = NoOpMount.from_args(sys.argv[1:])
+    service.main()
+
+
+if __name__ == '__main__':
+    main()
diff --git a/osbuild/formats/v2.py b/osbuild/formats/v2.py
index ec660c0b..43310351 100644
--- a/osbuild/formats/v2.py
+++ b/osbuild/formats/v2.py
@@ -75,8 +75,9 @@ def describe(manifest: Manifest, *, with_id=False) -> Dict:
 
     def describe_mount(mnt):
         desc = {
+            "name": mnt.name,
             "type": mnt.info.name,
-            "device": mnt.device.name,
+            "source": mnt.device.name,
             "target": mnt.target
         }
 
@@ -85,10 +86,10 @@ def describe(manifest: Manifest, *, with_id=False) -> Dict:
         return desc
 
     def describe_mounts(mounts: Dict):
-        desc = {
-            name: describe_mount(mnt)
-            for name, mnt in mounts.items()
-        }
+        desc = [
+            describe_mount(mnt)
+            for mnt in mounts.values()
+        ]
         return desc
 
     def describe_stage(s: Stage):
@@ -214,10 +215,15 @@ def load_input(name: str, description: Dict, index: Index, stage: Stage, manifes
         ip.add_reference(r, desc)
 
 
-def load_mount(name: str, description: Dict, index: Index, stage: Stage):
+def load_mount(description: Dict, index: Index, stage: Stage):
     mount_type = description["type"]
     info = index.get_module_info("Mount", mount_type)
 
+    name = description["name"]
+
+    if name in stage.mounts:
+        raise ValueError(f"Duplicated mount '{name}'")
+
     source = description["source"]
     target = description["target"]
 
@@ -245,9 +251,9 @@ def load_stage(description: Dict, index: Index, pipeline: Pipeline, manifest: Ma
     for name, desc in ips.items():
         load_input(name, desc, index, stage, manifest, source_refs)
 
-    mounts = description.get("mounts", {})
-    for name, desc in mounts.items():
-        load_mount(name, desc, index, stage)
+    mounts = description.get("mounts", [])
+    for mount in mounts:
+        load_mount(mount, index, stage)
 
     return stage
 
@@ -394,6 +400,10 @@ def validate(manifest: Dict, index: Index) -> ValidationResult:
     def validate_stage_modules(klass, stage, path):
         group = ModuleInfo.MODULES[klass]
         items = stage.get(group, {})
+
+        if isinstance(items, list):
+            items = {i["name"]: i for i in items}
+
         for name, mod in items.items():
             validate_module(mod, klass, path + [group, name])
 
diff --git a/osbuild/util/rhsm.py b/osbuild/util/rhsm.py
index 987a175e..21a2d502 100644
--- a/osbuild/util/rhsm.py
+++ b/osbuild/util/rhsm.py
@@ -5,18 +5,54 @@ with the subscriptions attached to the host machine.
 """
 
 import configparser
+import contextlib
+import glob
+import os
 import re
 
 
 class Subscriptions:
     def __init__(self, repositories):
         self.repositories = repositories
+        # These are used as a fallback if the repositories don't
+        # contain secrets for a requested URL.
+        self.secrets = None
+
+    def get_fallback_rhsm_secrets(self):
+        rhsm_secrets = {
+            'ssl_ca_cert': "/etc/rhsm/ca/redhat-uep.pem",
+            'ssl_client_key': "",
+            'ssl_client_cert': ""
+        }
+
+        keys = glob.glob("/etc/pki/entitlement/*-key.pem")
+        for key in keys:
+            # The key and cert have the same prefix
+            cert = key.rstrip("-key.pem") + ".pem"
+            # The key is only valid if it has a matching cert
+            if os.path.exists(cert):
+                rhsm_secrets['ssl_client_key'] = key
+                rhsm_secrets['ssl_client_cert'] = cert
+                # Once the dictionary is complete, assign it to the object
+                self.secrets = rhsm_secrets
+
+        raise RuntimeError("no matching rhsm key and cert")
 
     @classmethod
     def from_host_system(cls):
         """Read redhat.repo file and process the list of repositories in there."""
-        with open("/etc/yum.repos.d/redhat.repo", "r") as fp:
-            return cls.parse_repo_file(fp)
+        ret = cls(None)
+        with contextlib.suppress(FileNotFoundError):
+            with open("/etc/yum.repos.d/redhat.repo", "r") as fp:
+                ret = cls.parse_repo_file(fp)
+
+        with contextlib.suppress(RuntimeError):
+            ret.get_fallback_rhsm_secrets()
+
+        if not ret.repositories and not ret.secrets:
+            raise RuntimeError("No RHSM secrets found on this host.")
+
+        return ret
 
     @staticmethod
     def _process_baseurl(input_url):
@@ -56,6 +92,7 @@ class Subscriptions:
         return cls(repositories)
 
     def get_secrets(self, url):
+        # Try to find a matching URL from redhat.repo file first
         for parameters in self.repositories.values():
             if parameters["matchurl"].match(url) is not None:
                 return {
@@ -64,4 +101,8 @@ class Subscriptions:
                     "ssl_client_cert": parameters["sslclientcert"]
                 }
 
+        # In case there is no matching URL, try the fallback
+        if self.secrets:
+            return self.secrets
+
         raise RuntimeError(f"There are no RHSM secret associated with {url}")
diff --git a/schemas/osbuild2.json b/schemas/osbuild2.json
index d7e7beb0..5261b751 100644
--- a/schemas/osbuild2.json
+++ b/schemas/osbuild2.json
@@ -26,7 +26,7 @@
     "device": {
       "title": "Device for a stage",
       "additionalProperties": false,
-      "required": ["type", "options"],
+      "required": ["type"],
       "properties": {
         "type": { "type": "string" },
         "options": {
@@ -63,16 +63,16 @@
 
     "mounts": {
       "title": "Collection of mount points for a stage",
-      "additionalProperties": {
-        "$ref": "#/definitions/mount"
-      }
+      "type": "array",
+      "items": { "$ref": "#/definitions/mount"}
     },
 
     "mount": {
       "title": "Mount point for a stage",
       "additionalProperties": false,
-      "required": ["type", "source", "target"],
+      "required": ["name", "type", "source", "target"],
       "properties": {
+        "name": { "type": "string" },
         "type": { "type": "string" },
         "source": {
           "type": "string"
diff --git a/stages/org.osbuild.copy b/stages/org.osbuild.copy
index 15e3cd7d..19330968 100755
--- a/stages/org.osbuild.copy
+++ b/stages/org.osbuild.copy
@@ -64,8 +64,7 @@ SCHEMA_2 = r"""
   "additionalProperties": true
 },
 "mounts": {
-  "type": "object",
-  "additionalProperties": true
+  "type": "array"
 },
 "inputs": {
   "type": "object",
diff --git a/stages/org.osbuild.noop b/stages/org.osbuild.noop
index 4b4d1ea8..e68c72da 100755
--- a/stages/org.osbuild.noop
+++ b/stages/org.osbuild.noop
@@ -17,8 +17,14 @@ SCHEMA_2 = """
 "options": {
   "additionalProperties": true
 },
+"devices": {
+  "additionalProperties": true
+},
 "inputs": {
   "additionalProperties": true
+},
+"mounts": {
+  "additionalProperties": true
 }
 """
 
diff --git a/stages/org.osbuild.zipl.inst b/stages/org.osbuild.zipl.inst
index d8bacff6..f5c9721d 100755
--- a/stages/org.osbuild.zipl.inst
+++ b/stages/org.osbuild.zipl.inst
@@ -47,15 +47,8 @@ SCHEMA_2 = r"""
   }
 },
 "mounts": {
-  "type": "object",
-  "additionalProperties": true,
-  "required": ["root"],
-  "properties": {
-    "root": {
-      "type": "object",
-      "additionalProperties": true
-    }
-  }
+  "type": "array",
+  "minItems": 1
 }
 """
 


Test patch:
diff --git a/test/data/manifests/fedora-ostree-bootiso.json b/test/data/manifests/fedora-ostree-bootiso.json
index 3f43bb97..3df7747a 100644
--- a/test/data/manifests/fedora-ostree-bootiso.json
+++ b/test/data/manifests/fedora-ostree-bootiso.json
@@ -1763,13 +1763,14 @@
               }
             }
           },
-          "mounts": {
-            "root": {
+          "mounts": [
+            {
+              "name": "root",
               "type": "org.osbuild.ext4",
               "source": "root",
               "target": "/"
             }
-          }
+          ]
         }
       ]
     },
@@ -1935,13 +1936,14 @@
               }
             }
           },
-          "mounts": {
-            "efi": {
+          "mounts": [
+            {
+              "name": "efi",
               "type": "org.osbuild.fat",
               "source": "efi",
               "target": "/"
             }
-          }
+          ]
         },
         {
           "type": "org.osbuild.copy",
diff --git a/test/data/manifests/fedora-ostree-bootiso.mpp.json b/test/data/manifests/fedora-ostree-bootiso.mpp.json
index b2c55024..732eefe8 100644
--- a/test/data/manifests/fedora-ostree-bootiso.mpp.json
+++ b/test/data/manifests/fedora-ostree-bootiso.mpp.json
@@ -534,13 +534,14 @@
               }
             }
           },
-          "mounts": {
-            "root": {
+          "mounts": [
+            {
+              "name": "root",
               "type": "org.osbuild.ext4",
               "source": "root",
               "target": "/"
             }
-          }
+          ]
         }
       ]
     },
@@ -720,13 +721,14 @@
               }
             }
           },
-          "mounts": {
-            "efi": {
+          "mounts": [
+            {
+              "name": "efi",
               "type": "org.osbuild.fat",
               "source": "efi",
               "target": "/"
             }
-          }
+          ]
         },
         {
           "type": "org.osbuild.copy",
diff --git a/test/data/manifests/fedora-ostree-image.json b/test/data/manifests/fedora-ostree-image.json
index c1ecb768..18c9e20b 100644
--- a/test/data/manifests/fedora-ostree-image.json
+++ b/test/data/manifests/fedora-ostree-image.json
@@ -1080,23 +1080,26 @@
               }
             }
           },
-          "mounts": {
-            "root": {
+          "mounts": [
+            {
+              "name": "root",
               "type": "org.osbuild.xfs",
               "source": "root",
               "target": "/"
             },
-            "boot": {
+            {
+              "name": "boot",
               "type": "org.osbuild.ext4",
               "source": "boot",
               "target": "/boot"
             },
-            "efi": {
+            {
+              "name": "efi",
               "type": "org.osbuild.fat",
               "source": "efi",
               "target": "/boot/efi"
             }
-          }
+          ]
         },
         {
           "type": "org.osbuild.grub2.inst",
diff --git a/test/data/manifests/fedora-ostree-image.mpp.json b/test/data/manifests/fedora-ostree-image.mpp.json
index 00ad6a60..4b8309c5 100644
--- a/test/data/manifests/fedora-ostree-image.mpp.json
+++ b/test/data/manifests/fedora-ostree-image.mpp.json
@@ -534,23 +534,26 @@
               }
             }
           },
-          "mounts": {
-            "root": {
+          "mounts": [
+            {
+              "name": "root",
               "type": "org.osbuild.xfs",
               "source": "root",
               "target": "/"
             },
-            "boot": {
+            {
+              "name": "boot",
               "type": "org.osbuild.ext4",
               "source": "boot",
               "target": "/boot"
             },
-            "efi": {
+            {
+              "name": "efi",
               "type": "org.osbuild.fat",
               "source": "efi",
               "target": "/boot/efi"
             }
-          }
+          ]
         },
         {
           "type": "org.osbuild.grub2.inst",
diff --git a/test/mod/test_fmt_v2.py b/test/mod/test_fmt_v2.py
index 6e5c8aaf..43df8fad 100644
--- a/test/mod/test_fmt_v2.py
+++ b/test/mod/test_fmt_v2.py
@@ -2,6 +2,7 @@
 # Tests specific for version 2 of the format
 #
 
+import copy
 import os
 import unittest
 
@@ -65,7 +66,35 @@ BASIC_PIPELINE = {
                                 "name:tree": {}
                             }
                         }
-                    }
+                    },
+                    "devices": {
+                        "root": {
+                            "type": "org.osbuild.loopback",
+                            "options": {
+                                "filename": "empty.img"
+                            }
+                        },
+                        "boot": {
+                            "type": "org.osbuild.loopback",
+                            "options": {
+                                "filename": "empty.img"
+                            }
+                        },
+                    },
+                    "mounts": [
+                        {
+                            "name": "root",
+                            "type": "org.osbuild.noop",
+                            "source": "root",
+                            "target": "/",
+                        },
+                        {
+                            "name": "boot",
+                            "type": "org.osbuild.noop",
+                            "source": "boot",
+                            "target": "/boot",
+                        }
+                    ]
                 }
             ]
         }
@@ -75,35 +104,36 @@ BASIC_PIPELINE = {
 BAD_SHA = "sha256:15a654d32efaa75b5df3e2481939d0393fe1746696cc858ca094ccf8b76073cd"
 
 BAD_REF_PIPELINE = {
-  "version": "2",
-  "sources": {
-    "org.osbuild.curl": {
-      "items": {
-        "sha256:c540ca8c5e21ba5f063286c94a088af2aac0b15bc40df6fd562d40154c10f4a1": "",
-      }
-    }
-  },
-  "pipelines": [
-    {
-      "name": "build",
-      "stages": [
-        {
-          "type": "org.osbuild.rpm",
-          "inputs": {
-            "packages": {
-              "type": "org.osbuild.files",
-              "origin": "org.osbuild.source",
-              "references": {
-                BAD_SHA: {}
-              }
+    "version": "2",
+    "sources": {
+        "org.osbuild.curl": {
+            "items": {
+                "sha256:c540ca8c5e21ba5f063286c94a088af2aac0b15bc40df6fd562d40154c10f4a1": "",
             }
-          }
         }
-      ]
-    }
-  ]
+    },
+    "pipelines": [
+        {
+            "name": "build",
+            "stages": [
+                {
+                    "type": "org.osbuild.rpm",
+                    "inputs": {
+                        "packages": {
+                            "type": "org.osbuild.files",
+                            "origin": "org.osbuild.source",
+                            "references": {
+                                BAD_SHA: {}
+                            }
+                        }
+                    }
+                }
+            ]
+        }
+    ]
 }
 
+
 class TestFormatV1(unittest.TestCase):
     def setUp(self):
         self.index = osbuild.meta.Index(os.curdir)
@@ -191,4 +221,88 @@ class TestFormatV1(unittest.TestCase):
             fmt.load(desc, self.index)
 
         self.assertTrue(str(ex.exception).find(BAD_SHA) > -1,
-            "The unknown source reference is not included in the exception")
+                        "The unknown source reference is not included in the exception")
+
+    def test_mounts(self):
+        BASE = {
+            "version": "2",
+
+            "pipelines": [
+                {
+                    "name": "test",
+                    "runner": "org.osbuild.linux",
+                    "stages": [
+                        {
+                            "type": "org.osbuild.noop",
+                            "options": {"zero": 0},
+                            "devices": {
+                                "root": {
+                                    "type": "org.osbuild.loopback",
+                                    "options": {
+                                        "filename": "empty.img"
+                                    }
+                                }
+                            },
+                            "mounts": []
+                        }
+                    ]
+                }
+            ]
+        }
+
+        # verify the device
+        pipeline = copy.deepcopy(BASE)
+        stage = pipeline["pipelines"][0]["stages"][0]
+        mounts = stage["mounts"]
+
+        mounts.extend([{
+            "name": "root",
+            "type": "org.osbuild.noop",
+            "source": "root",
+            "target": "/",
+        }])
+
+        manifest, _ = self.load_manifest(pipeline)
+        self.assertIsNotNone(manifest)
+        test = manifest["test"]
+        self.assertIsNotNone(test)
+        stage = test.stages[0]
+        root = stage.mounts["root"]
+        self.assertIsNotNone(root)
+        self.assertIsNotNone(root.device)
+        self.assertEqual(root.device.name, "root")
+
+        # duplicated mount
+        pipeline = copy.deepcopy(BASE)
+        stage = pipeline["pipelines"][0]["stages"][0]
+        mounts = stage["mounts"]
+
+        mounts.extend([{
+            "name": "root",
+            "type": "org.osbuild.noop",
+            "source": "root",
+            "target": "/",
+        }, {
+            "name": "root",
+            "type": "org.osbuild.noop",
+            "source": "root",
+            "target": "/",
+        }])
+
+        with self.assertRaises(ValueError):
+            self.load_manifest(pipeline)
+
+        # mount without a device
+        pipeline = copy.deepcopy(BASE)
+        stage = pipeline["pipelines"][0]["stages"][0]
+        mounts = stage["mounts"]
+
+        mounts.extend([{
+            "name": "boot",
+            "type": "org.osbuild.noop",
+            "source": "boot",
+            "target": "/boot",
+        }])
+
+        with self.assertRaises(ValueError):
+            self.load_manifest(pipeline)