Add support for Rpm Package signing #3425

pedro-psb · 2024-02-05T22:11:13Z

This is a draft for implementing Rpm Package support in pulp_rpm.

My plan is (not necessarily in this order):

write test for service registration and validation with sample script (using rpm --addsign)
implement the RpmPackageSigningService model
write unit test for RpmTool
write functional tests with sample script (using rpm --addsign)
implement the API options and add new model fields
implement the signing call logic (I need to find the right spots to call it)
add documentation

other:

Fix tests: rpm-tool requires permission to access to /var/lib/rpm.
Currently, my workaround is to change the permission of this folder, but I believe that's not an ideal approach.
Ship empty RPM package (for signing service validation).
Investigate why the functional test fails on on azure and s3.
Don't build an empty rpm on each validate call.
Decouple key used for signing from a specific signing service, so the same signing service can use different keys.

closes #2986

ipanova · 2024-02-07T17:04:39Z

#closes is wrong ;)

pedro-psb · 2024-02-07T18:22:10Z

Ops

pedro-psb · 2024-03-19T17:45:46Z

pulp_rpm/tests/functional/api/test_package_signing.py

+    }
+    upload_task_href = rpm_package_api.create(**upload_attrs).task
+    package_href = monitor_task(upload_task_href).created_resources[2]
+    package_loc_href = rpm_package_api.read(package_href).location_href


@dralley I'm using rpm_package_api.create direcly here, and it seems to be "leaking" throughout runs (I can see the number of artifacts increasing at each run).

I'm not sure what is the recommended way to keep runs as isolated as possible. Is it using gen_object_with_cleanup? I've tried that but then I receive an empty package.

I could use feedback about the general use of fixtures here as well.

gen_object_with_cleanup is generally used with deletable resources like remotes, repositories, etc. Content isn't considered deletable, it goes through the orphan cleanup process which is indirect. Generally a test that needs completely fresh state (that is, deleting any previous content / artifacts and then re-creating them) will call orphan cleanup first and otherwise we'll just let it be.

It's perfectly OK to let the content / artifacts accumulate unless you need the test to create a new content / artifact during the test run which cannot pre-exist. Running orphan cleanup all the time unnecessarily would slow down the tests quite a bit.

Running orphan cleanup all the time unnecessarily would slow down the tests quite a bit.

Yeah, true. Thanks for the clarification

bersace · 2024-04-12T12:25:36Z

Just a question. Why do you need a new rpm package signing service ? Why core signing service can't do the job ?

pedro-psb · 2024-04-12T12:44:03Z

Just a question. Why do you need a new rpm package signing service ? Why core signing service can't do the job ?

Our signing service models relies on the implementation of a validation method, which assures a registered signing script can really perform a specific type of signing on a file. Currently, the only pulpcore signing service implementation is for detached signatures, so we need another that knows how to validate an rpm package signature, which is "attached" and requires using the rpm cli tool.

https://github.com/pulp/pulpcore/blob/9192c2bf0ccb0e0a2df595fd3efdd0980c80ff34/pulpcore/app/models/content.py#L884

bersace · 2024-04-12T14:03:44Z

@pedro-psb ok, got it. It's like pulp_deb:AptReleaseSigningService, you use it with --class ?

pedro-psb · 2024-04-12T16:08:01Z

Yes, in the pulpcore-manager command you can specify what type of signing service you are registering through the --class option. If ommited, it will use the detached one I've linked.

dralley · 2024-05-14T17:51:15Z

pulp_rpm/app/tasks/signing.py

+    package_signing_service = RpmPackageSigningService.objects.get(pk=signing_service_pk)
+    uploaded_package = PulpTemporaryFile.objects.get(pk=temporary_file_pk)
+    with NamedTemporaryFile(mode="wb", dir=".", delete=False) as final_package:
+        print("*" * 100)


debug prints

dralley · 2024-05-14T17:56:43Z

pulp_rpm/app/viewsets/package.py

+            item
+            for item in (serializer.validated_data.get(key) for key in ("upload", "repository"))
+            if item
+        ]


[serializer.validated_data.get("upload"), serializer.validated_data.get("repository")] might be simpler

dralley · 2024-05-14T17:58:50Z

pulp_rpm/tests/functional/api/test_package_signing.py

+        sign_package=True,
+    )
+    package_a_href = monitor_task(upload_response.task)
+    # import epdb;epdb.serve(port=12345)


dralley · 2024-05-14T18:01:53Z

pulp_rpm/tests/unit/test_rpm_tool.py

+    return file
+
+
+def test_get_empty_rpm_is_valid(tmp_path, monkeypatch):


Probably not necessary - either the empty rpm is there or it's not

dralley · 2024-05-16T15:12:17Z

We should document the limitations and give the feature a tech preview label

ipanova · 2024-05-27T15:14:59Z

CHANGES/2986.feature

@@ -0,0 +1 @@
+Added supported for signing RPM Packages on upload.


Consider adding a sentence that this is in tech preview

ipanova · 2024-05-27T15:19:11Z

pulp_rpm/app/models/content.py

@@ -0,0 +1,72 @@
+import tempfile


hmm, maybe having this in modes/repository.py or models/signing_service.py will be better?

I've followed pulpcore, which defines the base SigningService under content. But I agree, models/signing_service.py feels better (and its what pulp_deb does also).

ipanova · 2024-05-27T15:38:34Z

staging_docs/admin/guides/add-signing-services.md

+
+1. Create a signing script capable of signing an RPM Package.
+    - The script receives a file path as its first argument.
+    - The script should return a json-formatted. No signature is required, since its embedded.


...a json-formatted output.

ipanova · 2024-05-27T15:39:44Z

staging_docs/admin/guides/add-signing-services.md

+1. Register it with `pulpcore-manager add-signing-service`.
+    - The `--class` should be `rpm:RpmPackageSigningService`.
+    - The key provided here serves only for validating the script.
+      The signing fingerprint are provided dynamically, as [on upload signing](site:pulp_rpm/docs/user/guides/06-sign-packages/#on-upload).


ipanova · 2024-05-27T15:41:17Z

staging_docs/user/guides/06-sign-packages.md

+### Example
+
+```bash
+# Create a Repository w/ required porams


s/porams/params

ipanova · 2024-05-27T15:47:30Z

staging_docs/user/guides/06-sign-packages.md

+pulp rpm content upload \
+  --repository ${REPOSITORY} \
+  --file ${FILE} \
+  --sign-package True


Do I read this correctly that one need to specify on upload per package whether to sign it or not? Meaning that in repo there could be mixture of signed/unsigned content?
Have we considered to automatically sign on upload if the package signing service was defined on the repo?In the similar fashion as we do with metadata signing service?

Yes, you understand correctly.
I remember we have discussed the possibility of doing something similar to this, I'm not sure why I didn't commit to this design.

I'll do like that, really looks more consistent with the usecase we want to support.

What this does: - Create RpmPackageSigningService - Create RpmTool utility - Add fields to Repository model - Add branch on Package upload to sign the package with the associated SigningService and fingerprint. - Add docs Closes pulp#2986

[noissue]

github-actions bot added the multi-commit Added when a PR consists of more than one commit label Feb 6, 2024

pedro-psb force-pushed the add-rpm-sign-support branch 5 times, most recently from 5807490 to d3f921c Compare March 4, 2024 16:14

pedro-psb force-pushed the add-rpm-sign-support branch 4 times, most recently from 9cdd600 to 9299ed4 Compare March 14, 2024 20:34

pedro-psb commented Mar 19, 2024

View reviewed changes

pedro-psb force-pushed the add-rpm-sign-support branch 6 times, most recently from ef49b8b to d833e55 Compare April 25, 2024 20:04

pedro-psb force-pushed the add-rpm-sign-support branch from fe2849f to 220629e Compare May 6, 2024 21:43

dralley reviewed May 14, 2024

View reviewed changes

pedro-psb force-pushed the add-rpm-sign-support branch from 220629e to 0956ff6 Compare May 15, 2024 21:58

pedro-psb force-pushed the add-rpm-sign-support branch from 0956ff6 to 9de4d87 Compare May 22, 2024 21:37

github-actions bot removed the multi-commit Added when a PR consists of more than one commit label May 22, 2024

pedro-psb force-pushed the add-rpm-sign-support branch 3 times, most recently from a1b4c25 to af6855c Compare May 22, 2024 21:46

pedro-psb marked this pull request as ready for review May 22, 2024 21:47

pedro-psb force-pushed the add-rpm-sign-support branch from af6855c to 0b3505e Compare May 23, 2024 13:35

ipanova reviewed May 27, 2024

View reviewed changes

github-actions bot added the multi-commit Added when a PR consists of more than one commit label May 28, 2024

pedro-psb force-pushed the add-rpm-sign-support branch from e1915dc to 2492b1c Compare May 28, 2024 21:26

pedro-psb added 3 commits May 29, 2024 13:06

Add support for signing packages on upload

c22fbe0

What this does: - Create RpmPackageSigningService - Create RpmTool utility - Add fields to Repository model - Add branch on Package upload to sign the package with the associated SigningService and fingerprint. - Add docs Closes pulp#2986

Review: Fix typos and misc

b3a21d1

[noissue]

Review: use presence of SigningService at Repo as trigger

243c56f

[noissue]

pedro-psb force-pushed the add-rpm-sign-support branch from 2492b1c to 243c56f Compare May 29, 2024 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Rpm Package signing #3425

Add support for Rpm Package signing #3425

pedro-psb commented Feb 5, 2024 •

edited

ipanova commented Feb 7, 2024

pedro-psb commented Feb 7, 2024

pedro-psb Mar 19, 2024

dralley Mar 19, 2024 •

edited

pedro-psb Mar 19, 2024

bersace commented Apr 12, 2024

pedro-psb commented Apr 12, 2024 •

edited

bersace commented Apr 12, 2024

pedro-psb commented Apr 12, 2024

dralley May 14, 2024

dralley May 14, 2024

dralley May 14, 2024

dralley May 14, 2024

dralley commented May 16, 2024

ipanova May 27, 2024

ipanova May 27, 2024

pedro-psb May 27, 2024

ipanova May 27, 2024

ipanova May 27, 2024

ipanova May 27, 2024

ipanova May 27, 2024

pedro-psb May 27, 2024

		return file


		def test_get_empty_rpm_is_valid(tmp_path, monkeypatch):

		@@ -0,0 +1 @@
		Added supported for signing RPM Packages on upload.

Add support for Rpm Package signing #3425

Are you sure you want to change the base?

Add support for Rpm Package signing #3425

Conversation

pedro-psb commented Feb 5, 2024 • edited

ipanova commented Feb 7, 2024

pedro-psb commented Feb 7, 2024

Choose a reason for hiding this comment

dralley Mar 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bersace commented Apr 12, 2024

pedro-psb commented Apr 12, 2024 • edited

bersace commented Apr 12, 2024

pedro-psb commented Apr 12, 2024

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

dralley commented May 16, 2024

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

pedro-psb commented Feb 5, 2024 •

edited

dralley Mar 19, 2024 •

edited

pedro-psb commented Apr 12, 2024 •

edited