Add a new field that holds the value of the public key to SigningService #1012

lubosmj · 2020-11-06T17:00:20Z

closes #7700

pulpbot · 2020-11-06T17:04:32Z

Attached issue: https://pulp.plan.io/issues/7700

lubosmj · 2020-11-06T18:19:24Z

In order to utilize this change in pulp_rpm, the following has to be done:

diff --git a/pulp_rpm/app/tasks/publishing.py b/pulp_rpm/app/tasks/publishing.py
index 86cc91e4..061febc1 100644
--- a/pulp_rpm/app/tasks/publishing.py
+++ b/pulp_rpm/app/tasks/publishing.py
@@ -5,9 +5,11 @@ import shutil
 from tempfile import NamedTemporaryFile
 
 import createrepo_c as cr
+import tempfile
 import libcomps
 
 from django.core.files import File
+from django.core.files.base import ContentFile
 from django.core.files.storage import default_storage as storage
 from django.db.models import Q
 
@@ -527,12 +529,16 @@ def create_repomd_xml(content, publication, checksum_types, extra_repomdrecords,
             file=File(open(sign_results['signature'], 'rb'))
         )
 
-        # publish a public key required for further verification
-        PublishedMetadata.create_from_file(
-            relative_path=os.path.join(repodata_path, os.path.basename(sign_results['key'])),
-            publication=publication,
-            file=File(open(sign_results['key'], 'rb'))
-        )
+        with tempfile.NamedTemporaryFile() as tf:
+            tf.write(signing_service.public_key.encode("utf-8"))
+            tf.flush()
+
+            # publish a public key required for further verification
+            PublishedMetadata.create_from_file(
+                relative_path=os.path.join(repodata_path, os.path.basename("public.key")),
+                publication=publication,
+                file=File(open(tf.name, "rb")),
+            )
     else:
         PublishedMetadata.create_from_file(
             relative_path=os.path.join(repodata_path, os.path.basename(repomd_path)),

Everything should work without modifying the plugin as well, because of the compatibility policy.

mdellweg · 2020-11-09T09:52:17Z

pulpcore/app/serializers/content.py

@@ -260,10 +260,13 @@ class SigningServiceSerializer(base.ModelSerializer):

    pulp_href = base.IdentityField(view_name="signing-services-detail")
    name = serializers.CharField(help_text=_("A unique name used to recognize a script."))
+    public_key = serializers.CharField(


would it be possible, that we save the fingerprint of that key to the database too, and report that in the serializer?

This would be good, but also we could add this later?

It may be beneficial to do it now, to not have to "calculate" it in an upcoming migration.

mdellweg · 2020-11-09T09:58:21Z

pulpcore/app/models/content.py

@@ -598,6 +602,9 @@ def sign(self, filename):
        except json.JSONDecodeError:
            raise RuntimeError("The signing service script did not return valid JSON!")

+        if self.public_key:
+            return_value["key"] = self.public_key


WRT the deprecation policy, i believe we need to be a bit more careful here.
The old interface to the plugin returned a filename in return_value["key"], so we should do the same by writing the value of this db field to such a file. I have no idea, how we can warn about the deprecation when accessing that field other than subclassing the return_value dict however. In that case we can make the whole "write to file" process lazy.

The "new" interface could be return_value["public_key"].

With this comment I was thinking we would not be ever getting the public_key from the script anymore.

bmbouter · 2020-11-10T16:13:50Z

CHANGES/7700.feature

@@ -0,0 +1 @@
+Added a new field used for storing the value of the public key in SigningService.


Can the field name be added here please

bmbouter · 2020-11-10T16:15:32Z

pulpcore/app/models/content.py


    """

    name = models.TextField(db_index=True, unique=True)
+    public_key = models.TextField(default="")


I imagined this would be a required field. All SigningServices use PKI which use public keys is what I was thinking.

I think this is a migration thing. What about existing signing_services?
IIRC django won't even let you add a required field without providing a default.

With 3.9 signing services are immutable so I think we've already gotten to the point where users will need to make them new.

So yes to required. Can we also not default it to anything and provide a more verbose default only for the migration?

We could provide a migration which sets the value at migration time. It could sidestep the hook by calling https://rsinger86.github.io/django-lifecycle/advanced/#suppressing-hooked-methods in the data migration itself. What do you think about that idea?

I was imaging a migration that has 3 parts:

Add the field but make it optional

Perform the data migration populating the field

SQL update to make that field required (now that all existing data also has it). This last part would "match" the model code marking the field as required.

I do not quite understand what are you trying to achieve with those 3 parts, @bmbouter. How do you want to populate the data from an old SigningService object if there was not any public_key present back then? It will contain the same data like before. Do you want to extract the value of the public key by executing a signing script and manually reading the value of return_value["key"] within the migration?

I do not quite understand what are you trying to achieve with those 3 parts, @bmbouter. How do you want to populate the data from an old SigningService object if there was not any public_key present back then? It will contain the same data like before. Do you want to extract the value of the public key by executing a signing script and manually reading the value of return_value["key"] within the migration?

That's what I was thinking yes. If the migration fails, I think it should probably put in dummy data and log to the user an error and print the error to stderr as well so it's shown in the foreground too. We can't have the migraitons "fail to apply"

@mdellweg what do you think about ^?

Agreed, we must make the migration apply cleanly at any cost.
Trying to extract the public key is a convenience we offer the user, but it could fail for various reasons (revoked/expired keys is probably just one of them).
So i think, we should make the failing units unusable somehow to force the user to reinstanciate them. Having dummy data in the key field breaks the promise we give our plugins IMHO.

@mdellweg agreed. In terms of making the failing units unusable, I was thinking it would play out like:

The migration logs them and puts an empty string in the public_key field.

Then the SQL alter to make public_key required can be applied without error and the transaction can finish.

The code expects the public_key data to be accurate and so in cases where it's just '' the code will receive a RuntimeError. Or perhaps it could even check if its '' and gracefully refuse. Either way is ok with me.

bmbouter · 2020-11-10T16:16:07Z

pulpcore/app/models/content.py

-            A unique name describing the script (or executable) used for signing.
+            A unique name describing a script (or executable) used for signing.
+        public_key (models.TextField):
+            An absolute path to a public key.


This should be updated to store the key itself, not a path.

bmbouter · 2020-11-10T16:24:35Z

pulpcore/app/models/content.py

@@ -619,6 +626,15 @@ def save(self, *args, **kwargs):
        Save a signing service to the database (unless it fails to validate).
        """
        self.validate()
+
+        if not self.public_key:


I imagine all new SigningServices created will require users to put in a public key with this change. I don't think this needs to go through the deprecation cycle since plugin writers had a plugin key returned to them before and they still will. The deprecation policy I didn't imagine applied to the "scripts", users upgrading will need to take action at that time to recreate their SigningServices. This is my take at least.

This is about saving new SigningServices? Yes, they should be up to date. And we should not allow to create deprecated stuff in the database.
I would expect to see this warning whenever existing preexisting ones were used.

bmbouter · 2020-11-20T19:41:30Z

pulpcore/app/models/content.py


    """

    name = models.TextField(db_index=True, unique=True)
+    public_key = models.TextField(default="")


I don't expect this to have a default. It's a required field so it can't have one.

bmbouter · 2020-11-20T19:51:24Z

pulpcore/app/models/content.py

-                                "Fingerprints of a provided public key and a verified public key "
-                                "are not equal. The signing script is probably not valid."
-                            )
+                    if self.public_key:


Similarly here, I was thinking with this comment we are always getting the key from the database and not from the signing script ever.

bmbouter · 2020-11-20T19:51:41Z

pulpcore/app/models/content.py

+                        )
+                    elif verified.pubkey_fingerprint != import_result.fingerprints[0]:
+                        raise RuntimeError(
+                            "Fingerprints of the provided public key and the verified public "


This should be i8ln with gettext.

lubosmj · 2020-11-27T15:09:38Z

I addressed the requested changes. Please take a look at the PR again, @mdellweg and @bmbouter. I tested the PR on pulp_rpm (with the changes from the comment #1012 (comment) and without them) and everything seems to be working.

bmbouter · 2020-12-01T20:27:14Z

CHANGES/plugin_api/7700.deprecation

@@ -0,0 +1,2 @@
+Access to the path of the public key of a signing service was deprecated. The value of the public
+key is now directly accessible via the model instance as SigningService.public_key.


Suggested change

key is now directly accessible via the model instance as SigningService.public_key.

key is now expected to be saved in the model instance as ``SigningService.public_key``.

mdellweg

I don't have any major concerns with the current state. Just some ideas.
Also the deprecation window has probably moved a version.

mdellweg · 2020-12-08T16:20:08Z

docs/workflows/signed-metadata.rst

-                   "pulp_href": "/pulp/api/v3/signing-services/601feba7-a5d9-4f0a-919b-77be52fad0f7/",
-                   "script": "/var/lib/pulp/scripts/sign-metadata.sh"
+                   "public_key": "-----BEGIN PGP PUBLIC KEY BLOCK-----\n\nmQENBF+TO3EBCACzzRET1+eYET1vDQttMRGLjvFCXxLmp6YrXGJMNleVxuFAxQi4\nweQhryzfSOT3tTR/dCE6s2j7lhtPfYC9UV9AoEjFx6V6QBFlQzlwxscXwuPlhDiQ\nPJ9bCtyV3VKpMvsQWUY6DqAjLpxuiC3CMZo4+RBjQ93cpBNkKdCFtRpJ8CCOI2QT\ntd6Vd0IE6V60WxUFmYj56n1k656d0h+lyg40RPhj/ilZw+ExzEuFIS5mSo5YI/qA\nNN7F6Yc1cc4Deu92/3rYbSkdo53erLquZgoMIVAHfqTXf0bZjVMs16r3ps2HSLL9\nlTkCKAuX/20lZl7nvsGDVIPVWudbmLOueH+1ABEBAAG0GWFkbWluIDxhZG1pbkBl\neGFtcGxlLmNvbT6JAU4EEwEIADgWIQRM+EtRneU34wGht7MuTrtOtE1jPQUCX5M7\ncQIbAwULCQgHAgYVCgkICwIEFgIDAQIeAQIXgAAKCRAuTrtOtE1jPQb4B/4htLJL\narD+t/JH4GZFtfLi/QjF7NNKHOkbgf7SgcX5qztJ4J9wp0KwAvM3EaSM+tNgBJqW\nHZ0gxkkqOJ1hq1n+QCoVgQinbmzzmI5jvkK/E1prw+ZQ9RAg/+qYQ8WrYPrTRv8O\nXpOlzNCiTwiq1pTN6OlKaNpfWsCizg0SSdqYqsvJTCypHOYkzvfbLRnvxoq4uq3i\nBmcVuTNCLX0Hx9vuyf3PclpWQpku9eqid4cFg1+bpnJh5NP6YYYuKIFP6LyQ1D2e\nb1onARLOjvtCmejIiDVsYZLemvQoFZHmi6Hp0YoBW8tG/pr+lhsjY+Bewh8qXG5N\nGREMJ4Q+UBoi0nYruQENBF+TO3EBCADTjteYvfsONlybWY6bbsZ6gCulSXval25S\nW6fWkT1k0UZF2nttL1lpv/uFd5oW7T8yIFeY9R9B6mCOJxXYsomIeu9rrYZMBTJ6\nEH/Irg/WnJsLPXnkXKCcNU7WgoSmZHPkr+f9Qj0xRM57PNgFDacD+kMU9ohfbzQM\nX2xfX/9e8bDG1n5IbNoGQl48+V94b3Pjrbh1dvu/7purQFGamwBq8+ZkhU3WJfBH\n3xOpN5ykLcy4czbbbUng6Yz2LjX3hpJEKCMGb33rVikPWTcvBRcXJ+JA3mhFNqCp\nbhW7JmzrWHc3sVHmHIltETsdbR01bbvk/YKngUj+j1IklupPP3ujABEBAAGJATYE\nGAEIACAWIQRM+EtRneU34wGht7MuTrtOtE1jPQUCX5M7cQIbDAAKCRAuTrtOtE1j\nPS4sB/0TMbRmuA0c4d/E+fuvRtJJ0I+LF7f6YzqI65wFoGC6oYTT5bC4nFPyELLz\n8jTaCg5ltrIqOVmWIU/4A1QpnswbF6DluuaJL5N/cxJlDGZNClX4Iny3GwJAH9S3\n8VuEuAt8ysJsh3cDok9npFycGUx5/YLsl7fVK+VuIMs2Gw8MKicYtTgI6viePWDU\nIBQNUOinJbOiQZSG3O/2026AFtAQAtozywbkZVt1OLxy6B5h9XbXzDTi8r4LrFfi\nryw3hYeiO7BDNFngi5+WalloExjOj/O8987EZXRJplaFgG/tAycpcyJryzqPfK+B\n26RvEar3jOq1fyzovqSphw2djH2t\n=mlZX\n-----END PGP PUBLIC KEY BLOCK-----\n",
+                   "pulp_created": "2020-11-06T15:42:20.645197Z",


Should we shorten this in the docs with -----BEGIN PGP PUBLIC KEY BLOCK-----\n\n [...] \n-----END PGP PUBLIC KEY BLOCK-----\n?
I don't see much value for the reader to see the raw key material.

mdellweg · 2020-12-08T16:25:07Z

pulpcore/app/migrations/0053_add_public_key.py

+                except RuntimeError:
+                    warnings.warn(
+                        "The public key migration for '{}' failed. Consider creating a new signing "
+                        "service instead.".format(service.name),


If this migration failed, will the resource be non functional? We should add this to the warning.

mdellweg · 2020-12-08T16:28:17Z

pulpcore/app/migrations/0053_add_public_key.py

+                    )
+                else:
+                    with open(sign_results['key'], 'rb') as key:
+                        service.public_key = key.read()


Should we add another try-warn safety layer here, too?

mdellweg · 2020-12-08T16:30:37Z

pulpcore/app/models/content.py

+            "deprecated as of the next release (3.10.0). Use 'signing_service.public_key' instead."
+        )
+        warnings.warn(msg, DeprecationWarning)
+


Ideally this warning will only be triggered when someone accesses this key in the dict.
Is it worth subclassing dict for this?

mdellweg · 2020-12-08T16:33:42Z

pulpcore/app/serializers/content.py

@@ -260,10 +260,13 @@ class SigningServiceSerializer(base.ModelSerializer):

    pulp_href = base.IdentityField(view_name="signing-services-detail")
    name = serializers.CharField(help_text=_("A unique name used to recognize a script."))
+    public_key = serializers.CharField(


It may be beneficial to do it now, to not have to "calculate" it in an upcoming migration.

lubosmj · 2020-12-11T16:48:55Z

@mdellweg, fingerprints are now calculated during the migration. Have a look.

pulpcore/app/migrations/0053_add_public_key.py

mdellweg · 2020-12-15T09:22:49Z

pulpcore/app/models/content.py

+                # raise the deprecation warning when a plugin writer tries to access the public key
+                warnings.warn(self.msg, DeprecationWarning)
+
+            return super().__getitem__(item)


This is exactly what i meant.
I feel like i created a monster. ;)

mdellweg · 2020-12-15T09:23:50Z

pulpcore/app/migrations/0053_add_public_key.py

+            service.public_key = public_key
+
+            import_result = gpg.import_keys(public_key)
+            service.pubkey_fingerprint = import_result.fingerprints[0]


mdellweg · 2020-12-16T08:58:10Z

pulpcore/app/migrations/0053_add_public_key.py

+            name='pubkey_fingerprint',
+            field=models.TextField(default=''),
+        ),
+        migrations.RunPython(migrate_public_key_values),


Can you please provide a down migration here?
It' probably a noop.

https://docs.djangoproject.com/en/3.1/ref/migration-operations/#runpython

I do not quite understand what you want me to do. What is the down migration?

Were you referring to the reverse_code parameter?

Yes, exactly. And it is probably an empty lambda expression, but it allows to rollback migrations.

I added migrations.RunPython.noop (https://docs.djangoproject.com/en/2.2/ref/migration-operations/#django.db.migrations.operations.RunPython.noop).

mdellweg

I believe, this now catches all the exceptions we could encounter.

For the sake of simplicity i'd rather have a single try-ecxept-warn-pass around the best-effort migration of every single singing service. But i'm not going to block this PR there.

Still a second pair of eyes on the db-migration would be good. @bmbouter

mdellweg · 2020-12-23T09:22:13Z

pulpcore/app/migrations/0053_add_public_key.py

+        "The public key migration for '{}' failed. Consider creating a new signing "
+        "service instead and try to run the migration again. The resource will not "
+        "be valid.".format(service.name),


You cannot "run the migration again".
How about:

Suggested change

"The public key migration for '{}' failed. Consider creating a new signing "

"service instead and try to run the migration again. The resource will not "

"be valid.".format(service.name),

"The public key migration for '{}' failed. The resource will not "

"be valid. Consider creating a new signing service instead."

"".format(service.name),

mdellweg · 2020-12-23T09:22:34Z

pulpcore/app/migrations/0053_add_public_key.py

+                    pass
+                else:
+                    update_model_fields(sign_results, service, gpg)
+


There's a newline missing.

mdellweg · 2020-12-23T09:30:35Z

pulpcore/app/migrations/0053_add_public_key.py

+        migrations.AlterField(
+            model_name='signingservice',
+            name='pubkey_fingerprint',
+            field=models.TextField(),


What happens if two signing services failed to migrate?
Is it correct that this is not a problem (?) because:

the fields are not unique

the database allows the fields to stay empty (but the services proved to be unusable anyway)

removing the default makes the field required for new services

Remember: As long as the database is in a "supported state" (including misconfigured singing services) the migration must pass without exposing an uncaught exception. And i heard, we do not allow migrations to be touched later.

bmbouter · 2021-01-25T20:22:45Z

pulpcore/app/models/content.py

+                _(
+                    "The public key needs to be specified during the new instance creation. Other "
+                    "ways of providing the public key are considered deprecated as of the release "
+                    "(3.9.0)."


This needs to show 3.10.0 I think right?

Also all the 3.10.0 strings in this PR can just say 3.10 I think.

bmbouter · 2021-01-25T20:23:36Z

pulpcore/app/models/content.py

    script = models.TextField()

+    class _WarningsDict(dict):


Please move this class definition to the module level. It's ok to still call it _WarningsDict though.

bmbouter

@lubosmj this is excellent. I'm requesting a few small changes and then I'll lgtm.

Please fix the version deprecation strings to be 3.10 and move the _WarningsDict to the module level and we'll be done!

closes #7700

bmbouter

Thank you @lubosmj ! This is excellent! I really appreciate all the effort you've put into this.

lubosmj force-pushed the signing-service-publickey-imm-7700 branch 2 times, most recently from a09d83e to abed7ec Compare November 6, 2020 17:21

lubosmj marked this pull request as ready for review November 6, 2020 18:16

lubosmj force-pushed the signing-service-publickey-imm-7700 branch from abed7ec to abeeb95 Compare November 6, 2020 18:34

mdellweg requested changes Nov 9, 2020

View reviewed changes

bmbouter reviewed Nov 10, 2020

View reviewed changes

bmbouter reviewed Nov 20, 2020

View reviewed changes

lubosmj force-pushed the signing-service-publickey-imm-7700 branch 4 times, most recently from 3491e39 to 9bedd89 Compare November 27, 2020 15:04

lubosmj requested review from bmbouter and mdellweg November 27, 2020 15:09

bmbouter reviewed Dec 1, 2020

View reviewed changes

lubosmj force-pushed the signing-service-publickey-imm-7700 branch from 9bedd89 to 85b960c Compare December 4, 2020 10:41

lubosmj requested a review from bmbouter December 4, 2020 10:41

lubosmj force-pushed the signing-service-publickey-imm-7700 branch 2 times, most recently from b7c4d37 to 8075425 Compare December 5, 2020 11:12

mdellweg reviewed Dec 8, 2020

View reviewed changes

lubosmj force-pushed the signing-service-publickey-imm-7700 branch 2 times, most recently from dd79841 to ada1682 Compare December 11, 2020 16:40

lubosmj requested a review from mdellweg December 11, 2020 16:48

lubosmj force-pushed the signing-service-publickey-imm-7700 branch 5 times, most recently from 5ce9f36 to 07cff53 Compare December 15, 2020 08:28

mdellweg requested changes Dec 15, 2020

View reviewed changes

mdellweg reviewed Dec 16, 2020

View reviewed changes

lubosmj force-pushed the signing-service-publickey-imm-7700 branch from 07cff53 to f9f7d3d Compare December 22, 2020 15:17

mdellweg approved these changes Dec 23, 2020

View reviewed changes

lubosmj force-pushed the signing-service-publickey-imm-7700 branch from f9f7d3d to 4c5ad04 Compare January 25, 2021 15:40

bmbouter reviewed Jan 25, 2021

View reviewed changes

bmbouter requested changes Jan 25, 2021

View reviewed changes

lubosmj force-pushed the signing-service-publickey-imm-7700 branch 4 times, most recently from 236a1f1 to ca7d8df Compare January 26, 2021 16:12

Add a new field that holds the value of the public key to SigningService

8927369

closes #7700

lubosmj force-pushed the signing-service-publickey-imm-7700 branch from ca7d8df to 8927369 Compare January 26, 2021 16:35

bmbouter approved these changes Jan 26, 2021

View reviewed changes

bmbouter merged commit 2e03660 into pulp:master Jan 26, 2021

quba42 mentioned this pull request Mar 16, 2021

Update AptReleaseSigningService validation pulp/pulp_deb#252

Merged

		@@ -0,0 +1 @@
		Added a new field used for storing the value of the public key in SigningService.

		@@ -0,0 +1,2 @@
		Access to the path of the public key of a signing service was deprecated. The value of the public
		key is now directly accessible via the model instance as SigningService.public_key.

	key is now directly accessible via the model instance as SigningService.public_key.
	key is now expected to be saved in the model instance as ``SigningService.public_key``.

Add a new field that holds the value of the public key to SigningService #1012

Add a new field that holds the value of the public key to SigningService #1012

Conversation

lubosmj commented Nov 6, 2020

pulpbot commented Nov 6, 2020

lubosmj commented Nov 6, 2020 • edited Loading

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

bmbouter Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter Nov 10, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lubosmj commented Nov 27, 2020

bmbouter Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

mdellweg left a 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

lubosmj commented Dec 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdellweg Dec 16, 2020 • edited Loading

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

lubosmj Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

mdellweg left a 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

bmbouter left a comment

Choose a reason for hiding this comment

bmbouter left a comment

Choose a reason for hiding this comment

lubosmj commented Nov 6, 2020 •

edited

Loading

bmbouter Nov 10, 2020 •

edited

Loading

bmbouter Nov 10, 2020 •

edited

Loading

bmbouter Dec 1, 2020 •

edited

Loading

mdellweg Dec 16, 2020 •

edited

Loading

lubosmj Dec 22, 2020 •

edited

Loading