Repository: conan-io/conan
Pull request: [develop2] Feature/develop2 package signing
Very early stage, to start discussion


Problem statement:
[feature] Investigate the impact of adding new files to upload/download in the export and package folders
As a mechanism to be able to add metadata to packages and recipes, signatures, etc. Is it breaking to add extra files? Can it be fixed, optin or something?

Hints from issue discussion:

Commit message: restore test

Patch to review:
diff --git a/conan/api/subapi/upload.py b/conan/api/subapi/upload.py
index 8f74806b7e6..faf0bf9fee6 100644
--- a/conan/api/subapi/upload.py
+++ b/conan/api/subapi/upload.py
@@ -3,6 +3,7 @@
 from conans.cli.conan_app import ConanApp
 from conans.client.cmd.uploader import IntegrityChecker, PackagePreparator, UploadExecutor, \
     UploadUpstreamChecker
+from conans.client.pkg_sign import PkgSignaturesPlugin
 from conans.errors import ConanException
 
 
@@ -63,6 +64,9 @@ def prepare(self, upload_bundle):
         app.load_remotes()
         preparator = PackagePreparator(app)
         preparator.prepare(upload_bundle)
+        signer = PkgSignaturesPlugin(app.cache)
+        # This might add files entries to upload_bundle with signatures
+        signer.sign(upload_bundle)
 
     @api_method
     def upload_bundle(self, upload_bundle, remote):
diff --git a/conan/cache/conan_reference_layout.py b/conan/cache/conan_reference_layout.py
index 9d5c392366d..db7b2a3933b 100644
--- a/conan/cache/conan_reference_layout.py
+++ b/conan/cache/conan_reference_layout.py
@@ -14,6 +14,7 @@
 EXPORT_FOLDER = "e"
 EXPORT_SRC_FOLDER = "es"
 DOWNLOAD_EXPORT_FOLDER = "d"
+METADATA = "metadata"
 
 
 class LayoutBase:
@@ -56,6 +57,9 @@ def export_sources(self):
     def download_export(self):
         return os.path.join(self.base_folder, DOWNLOAD_EXPORT_FOLDER)
 
+    def recipe_metadata(self):
+        return os.path.join(self.base_folder, DOWNLOAD_EXPORT_FOLDER, METADATA)
+
     def source(self):
         return os.path.join(self.base_folder, SRC_FOLDER)
 
@@ -118,7 +122,10 @@ def package(self):
         return os.path.join(self.base_folder, PACKAGES_FOLDER)
 
     def download_package(self):
-        return os.path.join(self.base_folder, "dl")
+        return os.path.join(self.base_folder, DOWNLOAD_EXPORT_FOLDER)
+
+    def package_metadata(self):
+        return os.path.join(self.base_folder, DOWNLOAD_EXPORT_FOLDER, METADATA)
 
     def package_manifests(self):
         package_folder = self.package()
diff --git a/conans/client/cmd/uploader.py b/conans/client/cmd/uploader.py
index 8d8f2735e6d..8cbe7d93576 100644
--- a/conans/client/cmd/uploader.py
+++ b/conans/client/cmd/uploader.py
@@ -12,7 +12,7 @@
 from conans.paths import (CONAN_MANIFEST, CONANFILE, EXPORT_SOURCES_TGZ_NAME,
                           EXPORT_TGZ_NAME, PACKAGE_TGZ_NAME, CONANINFO)
 from conans.util.files import (clean_dirty, is_dirty, gather_files,
-                               gzopen_without_timestamps, set_dirty_context_manager, save)
+                               gzopen_without_timestamps, set_dirty_context_manager)
 from conans.util.tracer import log_recipe_upload, log_compressed_files, log_package_upload
 
 UPLOAD_POLICY_FORCE = "force-upload"
@@ -147,6 +147,8 @@ def prepare(self, upload_bundle):
                 self._prepare_recipe(recipe, conanfile, self._app.enabled_remotes)
             if conanfile.build_policy == "always":
                 recipe.build_always = True
+                for package in recipe.packages:
+                    package.upload = False
             else:
                 recipe.build_always = False
                 for package in recipe.packages:
@@ -260,7 +262,6 @@ def __init__(self, app: ConanApp):
         self._output = ConanOutput()
 
     def upload(self, upload_data, remote):
-
         if upload_data.any_upload:
             self._output.info("Uploading artifacts")
         for recipe in upload_data.recipes:
diff --git a/conans/client/pkg_sign.py b/conans/client/pkg_sign.py
new file mode 100644
index 00000000000..7d954c278ce
--- /dev/null
+++ b/conans/client/pkg_sign.py
@@ -0,0 +1,41 @@
+import os
+
+from conan.cache.conan_reference_layout import METADATA
+from conans.client.loader import load_python_file
+from conans.util.files import mkdir
+
+
+class PkgSignaturesPlugin:
+    def __init__(self, cache):
+        self._cache = cache
+        signer = os.path.join(cache.plugins_path, "sign", "sign.py")
+        if os.path.isfile(signer):
+            mod, _ = load_python_file(signer)
+            # TODO: At the moment it requires both methods sign and verify, but that might be relaxed
+            self._sign = mod.sign
+            self._verify = mod.verify
+        else:
+            self._sign = self._verify = None
+
+    def sign(self, upload_data):
+        if self._sign is None:
+            return
+
+        def _sign(ref, files, folder):
+            mkdir(folder)
+            signatures = self._sign(ref, list(files.values()), folder=folder)
+            for signature in signatures:
+                files[f"{METADATA}/{signature}"] = f"{folder}/{signature}"
+
+        for recipe in upload_data.recipes:
+            if recipe.upload:
+                _sign(recipe.ref, recipe.files, self._cache.ref_layout(recipe.ref).recipe_metadata())
+            for package in recipe.packages:
+                if package.upload:
+                    _sign(package.pref, package.files,
+                          self._cache.pkg_layout(package.pref).package_metadata())
+
+    def verify(self, ref, files, folder):
+        if self._verify is None:
+            return
+        self._verify(ref, files, folder)
diff --git a/conans/client/remote_manager.py b/conans/client/remote_manager.py
index 895c8fa6ac8..b3b46d3fcea 100644
--- a/conans/client/remote_manager.py
+++ b/conans/client/remote_manager.py
@@ -5,8 +5,10 @@
 
 from requests.exceptions import ConnectionError
 
+from conan.cache.conan_reference_layout import METADATA
 from conans.cli.output import ConanOutput
 from conans.client.cache.remote_registry import Remote
+from conans.client.pkg_sign import PkgSignaturesPlugin
 from conans.errors import ConanConnectionError, ConanException, NotFoundException, \
     PackageNotFoundException
 from conans.model.info import load_binary_info
@@ -28,6 +30,7 @@ class RemoteManager(object):
     def __init__(self, cache, auth_manager, hook_manager):
         self._cache = cache
         self._auth_manager = auth_manager
+        self._signer = PkgSignaturesPlugin(cache)
 
     def check_credentials(self, remote):
         self._call_remote(remote, "check_credentials")
@@ -66,9 +69,13 @@ def get_recipe(self, ref, remote):
         ref.timestamp = ref_time
         duration = time.time() - t1
         log_recipe_download(ref, duration, remote.name, zipped_files)
-
+        # filter metadata files
+        # This could be also optimized in the download, avoiding downloading them, for performance
+        zipped_files = {k: v for k, v in zipped_files.items() if not k.startswith(METADATA)}
+        self._signer.verify(ref, zipped_files, layout.recipe_metadata())
         export_folder = layout.export()
         tgz_file = zipped_files.pop(EXPORT_TGZ_NAME, None)
+
         check_compressed_files(EXPORT_TGZ_NAME, zipped_files)
         if tgz_file:
             uncompress_file(tgz_file, export_folder)
@@ -118,7 +125,8 @@ def _get_package(self, layout, pref, remote, scoped_output):
             download_pkg_folder = layout.download_package()
             # Download files to the pkg_tgz folder, not to the final one
             zipped_files = self._call_remote(remote, "get_package", pref, download_pkg_folder)
-
+            zipped_files = {k: v for k, v in zipped_files.items() if not k.startswith(METADATA)}
+            self._signer.verify(pref, zipped_files, layout.package_metadata())
             duration = time.time() - t1
             log_package_download(pref, duration, remote, zipped_files)
 
diff --git a/conans/test/integration/test_pkg_signing.py b/conans/test/integration/test_pkg_signing.py
new file mode 100644
index 00000000000..ca29712faee
--- /dev/null
+++ b/conans/test/integration/test_pkg_signing.py
@@ -0,0 +1,43 @@
+import os
+import textwrap
+
+from conans.test.assets.genconanfile import GenConanfile
+from conans.test.utils.tools import TestClient
+from conans.util.files import save
+
+
+def test_pkg_sign():
+    c = TestClient(default_server_user=True)
+    c.save({"conanfile.py": GenConanfile("pkg", "0.1").with_exports("export/*")
+            .with_exports_sources("export_sources/*").with_package_file("myfile", "mycontents!"),
+            "export/file1.txt": "file1!",
+            "export_sources/file2.txt": "file2!"})
+    signer = textwrap.dedent(r"""
+        import os
+
+        def sign(ref, files, folder):
+            print("Signing ref: ", ref)
+            print("Signing files: ", files)
+            signature = os.path.join(folder, "signature.asc")
+            open(signature, "w").write("\n".join(files))
+            return ["signature.asc"]
+
+        def verify(ref, files, folder):
+            print("Verifying ref: ", ref)
+            print("Verifying files: ", files)
+            signature = os.path.join(folder, "signature.asc")
+            contents = open(signature).read()
+            print("verifying contents", contents)
+            for f in files:
+                assert f in contents
+        """)
+    save(os.path.join(c.cache.plugins_path, "sign", "sign.py"), signer)
+    c.run("create .")
+    c.run("upload * -r=default -c")
+    assert "Signing ref:  pkg/0.1" in c.out
+    assert "Signing ref:  pkg/0.1:da39a3ee5e6b4b0d3255bfef95601890afd80709" in c.out
+    c.run("remove * -f")
+    c.run("install --requires=pkg/0.1")
+    print(c.out)
+    assert "Verifying ref:  pkg/0.1" in c.out
+    assert "Verifying ref:  pkg/0.1:da39a3ee5e6b4b0d3255bfef95601890afd80709" in c.out


Code review comments:
diff_hunk: @@ -0,0 +1,43 @@
+import os
+import textwrap
+
+from conans.test.assets.genconanfile import GenConanfile
+from conans.test.utils.tools import TestClient
+from conans.util.files import save
+
+
+def test_pkg_sign():
+    c = TestClient(default_server_user=True)
+    c.save({"conanfile.py": GenConanfile("pkg", "0.1").with_exports("export/*")
+            .with_exports_sources("export_sources/*").with_package_file("myfile", "mycontents!"),
+            "export/file1.txt": "file1!",
+            "export_sources/file2.txt": "file2!"})
+    signer = textwrap.dedent(r"""
+        import os
+
+        def sign(ref, files, folder):
+            print("Signing ref: ", ref)
+            print("Signing files: ", files)
+            signature = os.path.join(folder, "signature.asc")
+            open(signature, "w").write("\n".join(files))
+            return ["signature.asc"]
+
+        def verify(ref, files, folder):
path: conans/test/integration/test_pkg_signing.py
text: @author:
``files`` is the list of files, excluding already the metadata, and they are absolute paths, while ``folder`` is the ``download/metatada`` folder

@user1:
This interface looks indeed weird. I don't like the `folder` argument. I might prefer a `metadata_files` with the absolute paths, but I see that it would be confusing to introduce the `metadata` concept when the plugin is only about signing. But maybe this plugin could always write the returned files to `metadata/signing`, and then pass the `signed_files` instead of `folder`.

@author:
It can be named ``metadata_folder`` instead of ``folder``? If passed a list of files, then users would need to iterate or find their wanted files in that list, instead of ``os.path.join(metadata_folder, "myfilename")``?

Other alternative is just passing the parent folder (this is the "download" one), which will contain:
- "donwnload" folder
   - conanfile.py
   - conanmanifest.txt
   - conan_sources.tgz
   - metadata
      - signature.asc
     
And then let the plugin itself should know the layout and relative ``metadata/signature.asc`` path?

@user1:
>  If passed a list of files, then users would need to iterate or find their wanted files in that list

That is why I suggested a subfolder in the metadata for the signing, not only metadata. So everything in that folder "should" be of interest.