Implement Sigstore signing and verification of collections in Ansible repositories (refactored) #1428

mayaCostantini · 2023-04-18T13:52:50Z

Opening this PR as a refactored version of #1390

Update documentation
Tests
CI
Document discussion from Implement Sigstore signing and verification of collections in Ansible repositories #1390

mdellweg · 2023-04-18T14:19:36Z

pulp_ansible/app/models.py

@@ -275,6 +318,381 @@ class Meta:
        unique_together = ("pubkey_fingerprint", "signed_collection")


+
+class SigstoreSigningService(Content):


This should not be content.

What would be a better model to subclass?

SigningService or BaseModel

mayaCostantini · 2023-04-18T14:22:13Z

pulp_ansible/app/models.py

+    fulcio_url = models.TextField(default=PUBLIC_FULCIO_URL)
+    tuf_url = models.TextField(default=PUBLIC_TUF_URL)
+    oidc_issuer = models.TextField(default=PUBLIC_ISSUER_URL)
+    credentials_file_path = models.TextField(null=True)


Important note: this field is just here temporarily to test the signing workflow by retrieving the OIDC credentials from a file stored in plaintext on the server. This is not the approach we will be going for; I will replace it once an alternative flow to get credentials has been validated.

Note we have an encrypted field that we use for storing passwords in Pulp. You still might not want to go down this route, but it is an option: https://github.com/pulp/pulpcore/blob/main/pulpcore/app/models/fields.py#L79-L80

mdellweg · 2023-04-19T07:32:10Z

Can you please elaborate on who is signing what (ansible collections, i guess), and what it means for a content to have a valid signature?

pulp_ansible/app/management/commands/add-sigstore-signing-service.py

gerrod3 · 2023-04-20T21:59:09Z

pulp_ansible/app/management/commands/add-sigstore-signing-service.py

+        if "from_file" in options:
+            file_path = Path(options["from_file"])
+


Is this command only available when using a config file? I don't understand what this if is checking for.

pulp_ansible/app/models.py

gerrod3 · 2023-04-20T22:05:55Z

pulp_ansible/app/models.py

+    fulcio_url = models.TextField(default=PUBLIC_FULCIO_URL)
+    tuf_url = models.TextField(default=PUBLIC_TUF_URL)
+    oidc_issuer = models.TextField(default=PUBLIC_ISSUER_URL)
+    credentials_file_path = models.TextField(null=True)


Note we have an encrypted field that we use for storing passwords in Pulp. You still might not want to go down this route, but it is an option: https://github.com/pulp/pulpcore/blob/main/pulpcore/app/models/fields.py#L79-L80

pulp_ansible/app/serializers.py

pulp_ansible/app/sigstore/tasks/sigstore_signature.py

gerrod3 · 2023-04-20T23:04:15Z

pulp_ansible/app/viewsets.py

+    def sigstore_sign(self, request, pk):
+        """
+        Dispatches a Sigstore signing task.
+        This endpoint is in tech preview and can change at any time in the future.
+        """


Would it be possible to move this into the existing sign endpoint? From what I've seen it's not that different call argument wised to what we have already. If you turn the SigstoreSigningService to be of type SigningService then you can use similar logic like we have in sync and use the type of the signing service to determine which task to call.

gerrod3 · 2023-04-20T23:05:21Z

requirements.txt

@@ -6,3 +6,6 @@ Pillow>=7.0,<9.6
 pulpcore>=3.23.0,<3.25
 PyYAML>=5.4.1,<7.0
 semantic_version>=2.9,<2.11
+setuptools>=39.2,<66.2.0
+sigstore==1.1.1
+voluptuous==0.13.1


Is voluptuous still being used? Also what is setuptools being used for? I would like to keep that out the requirements.

mayaCostantini · 2023-05-30T12:11:45Z

Thanks a lot for the review @gerrod3 @mdellweg

I tried to address most comments in my last commits, the async logic in the signing task being the only thing I didn't manage to change as you wished (I tried the approach you proposed but it was not working, so I will give it a closer look).

Feel free to give it another review round. In the meantime I will continue with the missing items on my list which are secure credential storage for the OIDC client secret, updating tests and writing the corresponding signing commands in pulp-cli and get back to you when it is ready.

gerrod3

Sorry for the late review, lots of things still to discuss.

gerrod3 · 2023-06-10T05:02:08Z

pulp_ansible/app/models.py

+    PUBLIC_OIDC_ISSUERS = (
+        "https://accounts.google.com",
+        "https://oauth2.sigstore.dev/auth",
+        "http://dex-idp:8888/auth",


Is this a real legit link? Looks weird, especially sus since it is not https.

Yes, it is stated in the official Fulcio config, probably for a locally deployed Dex server.

gerrod3 · 2023-06-10T05:04:40Z

pulp_ansible/app/models.py

+
+    name = models.CharField(db_index=True, unique=True, max_length=64)
+    rekor_url = models.TextField(default=PUBLIC_REKOR_URL)
+    rekor_root_pubkey = models.TextField(null=True)


If the default rekor_url is going to be the public url, should this rekor_root_pubkey also default to the public root pubkey? Maybe this should be accounted for in the create command.

I didn't put a default here because the privileged way of retrieving Sigstore instances public keys should be via TUF, the public key fields for Rekor and the CTLog being here just in case users need to override key retrieval by TUF.

gerrod3 · 2023-06-10T05:06:21Z

pulp_ansible/app/models.py

+    fulcio_url = models.TextField(default=PUBLIC_FULCIO_URL)
+    tuf_url = models.TextField(default=PUBLIC_TUF_URL, null=True)
+    oidc_issuer = models.TextField(default=PUBLIC_ISSUER_URL)
+    oidc_client_secret = EncryptedTextField(null=True)


Can we really allow this to be null? Can you sign something without being identified, or is this tied to the enabled_interactive field?

Yes, this could be null in case users want to authenticate interactively. But in general, I would be open to postponing the implementation of interactive signing as it poses a lot of challenges regarding Pulp tasking and needs to be implemented in the Automation Hub / Galaxy UI as well. Wdyt? Should this be planned in a future version instead?

pulp_ansible/app/models.py

gerrod3 · 2023-06-10T06:19:59Z

pulp_ansible/app/sigstore/tasks/sigstore_signature.py

+    with tempfile.NamedTemporaryFile(dir=".", delete=False, mode="w") as manifest_file:
+        manifest_file.write(checksums)
+        manifest_file.flush()
+    with open(manifest_file.name, "rb", buffering=0) as manifest_io:


Since checksums is already in-memory no need to re-write it to a file, use io.BytesIO instead: https://docs.python.org/3/library/io.html#binary-i-o

Fixed in df18426

gerrod3 · 2023-06-10T06:21:07Z

pulp_ansible/app/sigstore/tasks/sigstore_signature.py

+
+    data["data"] = signature
+    data["sigstore_x509_certificate"] = certificate
+    data["sigstore_signing_service"] = sigstore_signing_service


Suggested change

data["sigstore_signing_service"] = sigstore_signing_service

Uploaded signatures don't have a signing service.

I think they actually do:

{ "pulp_href": "/pulp/api/v3/content/ansible/content/68aeb271-691b-4910-baa8-bce859929b50/", "pulp_created": "2023-06-14T13:06:47.645521Z", "data": "MGYCMQD8J17K9G0aqKZTBPBJvL97P27fDVPp1HT/wHgxvF+h4HrxIwPYsYQG9E0aYKTzHmcCMQD3ixhAUsKdrUZTsQLEySOHVKrD61CvOeCkkk02Kgj6e+NlW99mJyCjLuoKb5v28ds=", "sigstore_x509_certificate": "-----BEGIN CERTIFICATE-----\nMIIDsTCCA1igAwIBAgIUUqFobSm73ExZkB6R2FBe8I/hxTkwCgYIKoZIzj0EAwIw\ngZcxCzAJBgNVBAYTAlVTMQswCQYDVQQIDAJNQTEPMA0GA1UEBwwGQm9zdG9uMRAw\nDgYDVQQKDAdSZWQgSGF0MRAwDgYDVQQLDAdSZWQgSGF0MRYwFAYDVQQDDA1vcGVu\nc2hpZnRhcHBzMS4wLAYJKoZIhvcNAQkBFh9vcGVuc2hpZnRhcHBzQG9wZW5zaGlm\ndGFwcHMuY29tMB4XDTIzMDYxNDEzMDY0NloXDTIzMDYxNDEzMTY0NlowADB2MBAG\nByqGSM49AgEGBSuBBAAiA2IABJk/8FHb0PA7kU5/zbbuQoFsAd1TJj+FWB1PyGtA\nVTBK4MvuXgcOKKQcI0GLeadac0HdvfZjsNitEhjobnH8+dDqvHsHOM9w+R2aZL2K\nB8dAeD9eiw5Jpa7Sov8noA94M6OCAfkwggH1MA4GA1UdDwEB/wQEAwIHgDATBgNV\nHSUEDDAKBggrBgEFBQcDAzAdBgNVHQ4EFgQUHk6ltOJgcKWkUGFpn545shIW8LIw\nHwYDVR0jBBgwFoAUwXoXJ20AjXcfuZ0Q+MfzGM5QU1cwLAYDVR0RAQH/BCIwIIEe\nbWF5YS5jb3N0YW50aW5pQHByb3Rvbm1haWwuY29tMGgGCisGAQQBg78wAQEEWmh0\ndHBzOi8va2V5Y2xvYWsta2V5Y2xvYWsuYXBwcy5vcGVuLXN2Yy1zdHMuazF3bC5w\nMS5vcGVuc2hpZnRhcHBzLmNvbS9hdXRoL3JlYWxtcy9zaWdzdG9yZTBqBgorBgEE\nAYO/MAEIBFwMWmh0dHBzOi8va2V5Y2xvYWsta2V5Y2xvYWsuYXBwcy5vcGVuLXN2\nYy1zdHMuazF3bC5wMS5vcGVuc2hpZnRhcHBzLmNvbS9hdXRoL3JlYWxtcy9zaWdz\ndG9yZTCBiQYKKwYBBAHWeQIEAgR7BHkAdwB1APlNklTuzJCFaZy/xxsoT1OgbYHg\nht07hNnZZSKIPp7GAAABiLoECfUAAAQDAEYwRAIgNuwoktic/YuF8VCSKnlhgZM4\nfxzvWtveNKTFoFaOqJ0CID6wdRqeoYJTNfgABjJyraiSRQGaa4GAf39pRNsOF3yg\nMAoGCCqGSM49BAMCA0cAMEQCIEgamVx2WHUSb7v562wGHA+4DHh+1B/6HawbIIox\n3XfgAiAG3+taNA4j8oJIiNWDNrkMbHl4eWy3YZrWDQN2SO8lTw==\n-----END CERTIFICATE-----\n", "sigstore_bundle": "{\"mediaType\": \"application/vnd.dev.sigstore.bundle+json;version=0.1\", \"verificationMaterial\": {\"x509CertificateChain\": {\"certificates\": [{\"rawBytes\": \"MIIDsTCCA1igAwIBAgIUUqFobSm73ExZkB6R2FBe8I/hxTkwCgYIKoZIzj0EAwIwgZcxCzAJBgNVBAYTAlVTMQswCQYDVQQIDAJNQTEPMA0GA1UEBwwGQm9zdG9uMRAwDgYDVQQKDAdSZWQgSGF0MRAwDgYDVQQLDAdSZWQgSGF0MRYwFAYDVQQDDA1vcGVuc2hpZnRhcHBzMS4wLAYJKoZIhvcNAQkBFh9vcGVuc2hpZnRhcHBzQG9wZW5zaGlmdGFwcHMuY29tMB4XDTIzMDYxNDEzMDY0NloXDTIzMDYxNDEzMTY0NlowADB2MBAGByqGSM49AgEGBSuBBAAiA2IABJk/8FHb0PA7kU5/zbbuQoFsAd1TJj+FWB1PyGtAVTBK4MvuXgcOKKQcI0GLeadac0HdvfZjsNitEhjobnH8+dDqvHsHOM9w+R2aZL2KB8dAeD9eiw5Jpa7Sov8noA94M6OCAfkwggH1MA4GA1UdDwEB/wQEAwIHgDATBgNVHSUEDDAKBggrBgEFBQcDAzAdBgNVHQ4EFgQUHk6ltOJgcKWkUGFpn545shIW8LIwHwYDVR0jBBgwFoAUwXoXJ20AjXcfuZ0Q+MfzGM5QU1cwLAYDVR0RAQH/BCIwIIEebWF5YS5jb3N0YW50aW5pQHByb3Rvbm1haWwuY29tMGgGCisGAQQBg78wAQEEWmh0dHBzOi8va2V5Y2xvYWsta2V5Y2xvYWsuYXBwcy5vcGVuLXN2Yy1zdHMuazF3bC5wMS5vcGVuc2hpZnRhcHBzLmNvbS9hdXRoL3JlYWxtcy9zaWdzdG9yZTBqBgorBgEEAYO/MAEIBFwMWmh0dHBzOi8va2V5Y2xvYWsta2V5Y2xvYWsuYXBwcy5vcGVuLXN2Yy1zdHMuazF3bC5wMS5vcGVuc2hpZnRhcHBzLmNvbS9hdXRoL3JlYWxtcy9zaWdzdG9yZTCBiQYKKwYBBAHWeQIEAgR7BHkAdwB1APlNklTuzJCFaZy/xxsoT1OgbYHght07hNnZZSKIPp7GAAABiLoECfUAAAQDAEYwRAIgNuwoktic/YuF8VCSKnlhgZM4fxzvWtveNKTFoFaOqJ0CID6wdRqeoYJTNfgABjJyraiSRQGaa4GAf39pRNsOF3ygMAoGCCqGSM49BAMCA0cAMEQCIEgamVx2WHUSb7v562wGHA+4DHh+1B/6HawbIIox3XfgAiAG3+taNA4j8oJIiNWDNrkMbHl4eWy3YZrWDQN2SO8lTw==\"}]}, \"tlogEntries\": [{\"logIndex\": \"349\", \"logId\": {\"keyId\": \"IFajaKdLy+sgq4+XSC5BH0KbPN8Kpcq7tYo5QMdCBpQ=\"}, \"kindVersion\": {\"kind\": \"hashedrekord\", \"version\": \"0.0.1\"}, \"integratedTime\": \"1686748007\", \"inclusionPromise\": {\"signedEntryTimestamp\": \"MEYCIQCVdYM7DhTo4ZQguWbXQD+R73UynZFtadNYjRATyBdLWwIhANPrpndVfmIf55c5Bn+dlCBGgO/IbEu8RWnxnK48jnd8\"}, \"inclusionProof\": {\"logIndex\": \"349\", \"rootHash\": \"pfsRcGU1+YTDFU2FtCoc7Ig8iv7nxypYzVn35KlIiSQ=\", \"treeSize\": \"350\", \"hashes\": [\"2Ln+aSDCopdrk9aY+DfGrwgKmkbWZuxb34NX4nCSQIg=\", \"JNyCbro9UJ9Yfhv55KCJ+vQQgv8029bretUa0ZAWFws=\", \"i+cLXIsbD4zB+OnN4JqNSuSajScXnV+UZ7B3H2o2azQ=\", \"54cKDlSFbb1IGAJLeE3wt6rWuhW4Mo7Dvu3y+UqeCwY=\", \"GUEiTpxawkRVsubS/3/DkEjy3qVFIqxwfafGliKkDTM=\", \"SfXoge+ONFcKFoRk3bbplo7ucOU9hE177NQdZxxvWZs=\"]}, \"canonicalizedBody\": \"eyJhcGlWZXJzaW9uIjoiMC4wLjEiLCJraW5kIjoiaGFzaGVkcmVrb3JkIiwic3BlYyI6eyJkYXRhIjp7Imhhc2giOnsiYWxnb3JpdGhtIjoic2hhMjU2IiwidmFsdWUiOiJlM2IwYzQ0Mjk4ZmMxYzE0OWFmYmY0Yzg5OTZmYjkyNDI3YWU0MWU0NjQ5YjkzNGNhNDk1OTkxYjc4NTJiODU1In19LCJzaWduYXR1cmUiOnsiY29udGVudCI6Ik1HWUNNUUQ4SjE3SzlHMGFxS1pUQlBCSnZMOTdQMjdmRFZQcDFIVC93SGd4dkYraDRIcnhJd1BZc1lRRzlFMGFZS1R6SG1jQ01RRDNpeGhBVXNLZHJVWlRzUUxFeVNPSFZLckQ2MUN2T2VDa2trMDJLZ2o2ZStObFc5OW1KeUNqTHVvS2I1djI4ZHM9IiwicHVibGljS2V5Ijp7ImNvbnRlbnQiOiJMUzB0TFMxQ1JVZEpUaUJEUlZKVVNVWkpRMEZVUlMwdExTMHRDazFKU1VSelZFTkRRVEZwWjBGM1NVSkJaMGxWVlhGR2IySlRiVGN6UlhoYWEwSTJVakpHUW1VNFNTOW9lRlJyZDBObldVbExiMXBKZW1vd1JVRjNTWGNLWjFwamVFTjZRVXBDWjA1V1FrRlpWRUZzVmxSTlVYTjNRMUZaUkZaUlVVbEVRVXBPVVZSRlVFMUJNRWRCTVZWRlFuZDNSMUZ0T1hwa1J6bDFUVkpCZHdwRVoxbEVWbEZSUzBSQlpGTmFWMUZuVTBkR01FMVNRWGRFWjFsRVZsRlJURVJCWkZOYVYxRm5VMGRHTUUxU1dYZEdRVmxFVmxGUlJFUkJNWFpqUjFaMUNtTXlhSEJhYmxKb1kwaENlazFUTkhkTVFWbEtTMjlhU1doMlkwNUJVV3RDUm1nNWRtTkhWblZqTW1od1dtNVNhR05JUW5wUlJ6bDNXbGMxZW1GSGJHMEtaRWRHZDJOSVRYVlpNamwwVFVJMFdFUlVTWHBOUkZsNFRrUkZlazFFV1RCT2JHOVlSRlJKZWsxRVdYaE9SRVY2VFZSWk1FNXNiM2RCUkVJeVRVSkJSd3BDZVhGSFUwMDBPVUZuUlVkQ1UzVkNRa0ZCYVVFeVNVRkNTbXN2T0VaSVlqQlFRVGRyVlRVdmVtSmlkVkZ2Um5OQlpERlVTbW9yUmxkQ01WQjVSM1JCQ2xaVVFrczBUWFoxV0dkalQwdExVV05KTUVkTVpXRmtZV013U0dSMlpscHFjMDVwZEVWb2FtOWlia2c0SzJSRWNYWkljMGhQVFRsM0sxSXlZVnBNTWtzS1FqaGtRV1ZFT1dWcGR6VktjR0UzVTI5Mk9HNXZRVGswVFRaUFEwRm1hM2RuWjBneFRVRTBSMEV4VldSRWQwVkNMM2RSUlVGM1NVaG5SRUZVUW1kT1ZncElVMVZGUkVSQlMwSm5aM0pDWjBWR1FsRmpSRUY2UVdSQ1owNVdTRkUwUlVablVWVklhelpzZEU5S1oyTkxWMnRWUjBad2JqVTBOWE5vU1ZjNFRFbDNDa2gzV1VSV1VqQnFRa0puZDBadlFWVjNXRzlZU2pJd1FXcFlZMloxV2pCUkswMW1la2ROTlZGVk1XTjNURUZaUkZaU01GSkJVVWd2UWtOSmQwbEpSV1VLWWxkR05WbFROV3BpTTA0d1dWYzFNR0ZYTlhCUlNFSjVZak5TZG1KdE1XaGhWM2QxV1RJNWRFMUhaMGREYVhOSFFWRlJRbWMzT0hkQlVVVkZWMjFvTUFwa1NFSjZUMms0ZG1FeVZqVlpNbmgyV1ZkemRHRXlWalZaTW5oMldWZHpkVmxZUW5kamVUVjJZMGRXZFV4WVRqSlplVEY2WkVoTmRXRjZSak5pUXpWM0NrMVROWFpqUjFaMVl6Sm9jRnB1VW1oalNFSjZURzFPZG1KVE9XaGtXRkp2VEROS2JGbFhlSFJqZVRsNllWZGtlbVJIT1hsYVZFSnhRbWR2Y2tKblJVVUtRVmxQTDAxQlJVbENSbmROVjIxb01HUklRbnBQYVRoMllUSldOVmt5ZUhaWlYzTjBZVEpXTlZreWVIWlpWM04xV1ZoQ2QyTjVOWFpqUjFaMVRGaE9NZ3BaZVRGNlpFaE5kV0Y2UmpOaVF6VjNUVk0xZG1OSFZuVmpNbWh3V201U2FHTklRbnBNYlU1MllsTTVhR1JZVW05TU0wcHNXVmQ0ZEdONU9YcGhWMlI2Q21SSE9YbGFWRU5DYVZGWlMwdDNXVUpDUVVoWFpWRkpSVUZuVWpkQ1NHdEJaSGRDTVVGUWJFNXJiRlIxZWtwRFJtRmFlUzk0ZUhOdlZERlBaMkpaU0djS2FIUXdOMmhPYmxwYVUwdEpVSEEzUjBGQlFVSnBURzlGUTJaVlFVRkJVVVJCUlZsM1VrRkpaMDUxZDI5cmRHbGpMMWwxUmpoV1ExTkxibXhvWjFwTk5BcG1lSHAyVjNSMlpVNUxWRVp2Um1GUGNVb3dRMGxFTm5ka1VuRmxiMWxLVkU1bVowRkNha3A1Y21GcFUxSlJSMkZoTkVkQlpqTTVjRkpPYzA5R00zbG5DazFCYjBkRFEzRkhVMDAwT1VKQlRVTkJNR05CVFVWUlEwbEZaMkZ0Vm5neVYwaFZVMkkzZGpVMk1uZEhTRUVyTkVSSWFDc3hRaTgyU0dGM1lrbEpiM2dLTTFobVowRnBRVWN6SzNSaFRrRTBhamh2U2tscFRsZEVUbkpyVFdKSWJEUmxWM2t6V1ZweVYwUlJUakpUVHpoc1ZIYzlQUW90TFMwdExVVk9SQ0JEUlZKVVNVWkpRMEZVUlMwdExTMHRDZz09In19fX0=\"}]}, \"messageSignature\": {\"messageDigest\": {\"algorithm\": \"SHA2_256\", \"digest\": \"47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=\"}, \"signature\": \"MGYCMQD8J17K9G0aqKZTBPBJvL97P27fDVPp1HT/wHgxvF+h4HrxIwPYsYQG9E0aYKTzHmcCMQD3ixhAUsKdrUZTsQLEySOHVKrD61CvOeCkkk02Kgj6e+NlW99mJyCjLuoKb5v28ds=\"}}", "signed_collection": "/pulp/api/v3/content/ansible/collection_versions/343fc505-e15b-48b9-a3f7-897532736336/", "sigstore_signing_service": "/pulp/api/v3/sigstore-signing-services/5f484e77-f5b0-4916-9680-e3bf45822804/" },

The sigstore_signing_service field is a foreign key for CollectionVersionSigstoreSignature

Why should the created signature have a reference to the signing service at all? What's the benefit of that link?
Also you can upload a signature into a system that has no signing service configured at all. So no, they don't have one.

The signatures having a reference to the signing service was originally a Galaxy request to know if they were locally created. Synced and uploaded signatures will not have an attached signing service, but I don't think they expose the upload signature endpoint in galaxy, so it actually determines if the signature was synced or locally created and can be easily used when filtering signatures.

gerrod3 · 2023-06-10T06:25:54Z

pulp_ansible/app/sigstore/tasks/sigstore_signature.py

+                async with aiofiles.tempfile.NamedTemporaryFile(
+                    dir=".", mode="w", delete=False
+                ) as manifest_file:
+                    manifest_data = await sync_to_async(_generate_checksum_manifest)(
+                        collection_version
+                    )
+                    await manifest_file.write(manifest_data)
+                async with aiofiles.open(manifest_file.name, mode="rb", buffering=0) as iofile:
+                    async with aiofiles.tempfile.NamedTemporaryFile(
+                        dir=".", mode="w", delete=False
+                    ) as manifest_content:
+                        content = await iofile.read()
+                        manifest_content.write(content)
+                    with open(manifest_content.name, "rb") as manifest_bytes:


Same two things as above with the verify code:

Can we just extract the MANIFEST.IN file instead of regenerating it?

Once we have the file in memory let's use io.BytesIO instead of rewriting it to a temp file.

Actually I realized _generate_checksum_manifest generates a string an not a stream of bytes, so I think writing to a file is necessary to make the conversion here, wdyt? I might revert df18426 in this case

Just use TextIo instead.

pulp_ansible/app/tasks/signature.py

gerrod3 · 2023-06-10T06:35:21Z

pulp_ansible/app/sigstore/issuers/keycloak.py

+            with _OAuthFlow(client_id, client_secret, self) as server:
+                # Launch web browser
+                if webbrowser.open(server.base_uri):


This code simply won't work for the signing task as 99% of the time the machine the task is running on is not accessible by the user, so the signing task will simply stall until it fails waiting for the user input.

I think I have a good idea for how to get around this problem, but it's pretty in-depth and will take me a while to write down. I would also like to get feedback from my other team-members, so let's discuss it at our next sync.

Thanks for your input during the sync, I removed the interactive signing part in ca80190

mayaCostantini · 2023-06-14T14:18:56Z

Thanks again for the thorough review @gerrod3
I tried to answer most of the comments you raised, please let me know if that works for you.

As discussed during the last sync, I will start a refactor based on sigstore-python v2 as the milestone seems not too far from being reached as of now: https://github.com/sigstore/sigstore-python/milestone/4

…rvices

…gtoreVerifyingService models

…ign keys

…issuer

… BaseModel

…ice viewsets

…Model

gerrod3 · 2023-11-06T01:47:07Z

docs/workflows/signature.rst

+- `Fulcio <https://docs.sigstore.dev/fulcio/overview/>`__
+is a free Certificate Authority used to generate ephemeral signing certificates for artifacts.
+
+- `Cosign https://docs.sigstore.dev/cosign/overview/`__


Suggested change

- `Cosign https://docs.sigstore.dev/cosign/overview/`__

- `Cosign <https://docs.sigstore.dev/cosign/overview/>`__

gerrod3 · 2023-11-06T02:03:15Z

pulp_ansible/app/models.py

+    TYPE = "collection_sigstore_signatures"
+
+    sigstore_bundle = models.JSONField(null=True)
+    sigstore_bundle_sha256_hash = models.CharField(max_length=64, unique=True, db_index=True, null=True)


Suggested change

sigstore_bundle_sha256_hash = models.CharField(max_length=64, unique=True, db_index=True, null=True)

sigstore_bundle_sha256_hash = models.CharField(max_length=64, db_index=True, null=True)

unique_together is enough. Don't forget to remove the same code from the migration file.

gerrod3 · 2023-11-06T02:05:06Z

pulp_ansible/app/serializers.py

@@ -128,6 +139,148 @@ class Meta:
        )


+class SigstoreSigningServiceSerializer(ModelSerializer):


This can only be created from the command line, correct? If so then I would change all the fields to read_only=True.

gerrod3 · 2023-11-06T02:05:26Z

pulp_ansible/app/serializers.py

+        )
+
+
+class SigstoreVerifyingServiceSerializer(ModelSerializer):


Same here as above.

gerrod3 · 2023-11-06T02:07:19Z

pulp_ansible/app/serializers.py

+        artifact_name = artifact.file.name
+        artifact_file = storage.open(artifact_name)
+
+        with tempfile.TemporaryDirectory() as tempdir:


Suggested change

with tempfile.TemporaryDirectory() as tempdir:

with tempfile.TemporaryDirectory(dir=".") as tempdir:

gerrod3 · 2023-11-06T02:36:39Z

pulp_ansible/app/sigstore/tasks/sigstore_signature.py

+    with tempfile.TemporaryDirectory() as tempdir:
+        differ = DistlibManifestChecksumFileExistenceDiffer
+        checksum = ChecksumFile(tempdir, differ=differ)
+        with tarfile.open(fileobj=artifact_file, mode="r") as tar:
+            tar.extractall(path=tempdir)
+
+            try:
+                manifest = checksum.generate_gnu_style()


What is the status of ansible-sign? I see it below as a new dependency. Do we still need to have this code or can we call ansible-sign's code instead?

gerrod3 · 2023-11-06T02:39:02Z

pulp_ansible/app/sigstore/tasks/sigstore_signature.py

+    sigstore_bundle = data.get("sigstore_bundle")
+    checksums = _generate_checksum_manifest(collection)
+
+    bundle = Bundle().from_json(sigstore_bundle) if sigstore_bundle != "null" else None


What is the status of this now that V2 is out? Can this verification code be dramatically simplified? Can the verification service be removed entirely and be replaced with a text field like GPG?

gerrod3 · 2023-11-06T02:43:39Z

pulp_ansible/app/viewsets.py

+
+
+class SigstoreSigningServiceViewSet(
+    NamedModelViewSet, mixins.RetrieveModelMixin, mixins.ListModelMixin, RolesMixin


Suggested change

NamedModelViewSet, mixins.RetrieveModelMixin, mixins.ListModelMixin, RolesMixin

NamedModelViewSet, mixins.RetrieveModelMixin, mixins.ListModelMixin

Regarding ansible-sign, I am still waiting for the sigstore code to be merged there (see ansible/ansible-sign#46), but the code should indeed be replaced when this is done. Let me get back to the ansible-sign maintainers and I will replace it.

The verification service would still be required as policies for verification must be specified with sigstore (i.e. signer's identity, OIDC issuer, etc).

I think, I said this before. There is no need for a verification service.
The design of the signing service is to protect secrets. Verification must always work without secrets. Otherwise your cryptosystem is broken. No secrets means: Everything lives in the database and is configurable via the api on the very Repository or Remote supposed to protect itself from improperly signed content.

There is no need for any secret here, the parameters specified are public and are simple text fields used by the sigstore client to verify the signature, exactly like you would use a public GPG key.

I think the term "verification service" might be misused here. It is actually more of a static configuration than a "service".

That I'm fine with. No files stored away on the workers harddrive, no bits protected by a Hardware Crypto Token. If it's more complex than a simple text field, we can even put it in a jsonfield. But it must be transparently accessible to the user.

gerrod3 · 2023-11-06T02:43:50Z

pulp_ansible/app/viewsets.py

+
+
+class SigstoreVerifyingServiceViewSet(
+    NamedModelViewSet, mixins.RetrieveModelMixin, mixins.ListModelMixin, RolesMixin


Suggested change

NamedModelViewSet, mixins.RetrieveModelMixin, mixins.ListModelMixin, RolesMixin

NamedModelViewSet, mixins.RetrieveModelMixin, mixins.ListModelMixin

gerrod3 · 2023-11-06T02:44:16Z

requirements.txt

 galaxy_importer>=0.4.5,<0.5
 GitPython>=3.1.24,<3.2
 jsonschema>=4.9,<4.20
 Pillow>=7.0,<10.2
 pulpcore>=3.25.0,<3.40
 PyYAML>=5.4.1,<7.0
 semantic_version>=2.9,<2.11
+setuptools>=39.2,<66.2.0


What is setuptools needed for?

I am not sure anymore why I added it here, so removing it.

…d open temporary directory in current path

…ionSigstoreSignatureSerializer

…eSerializer constructor and remove validate method

mayaCostantini · 2023-11-06T11:14:10Z

Thanks a lot for the review @gerrod3 ! I pushed the changes you requested, let me know if anything else comes to your mind.

gerrod3

Have some more big refactoring asks. Also, I am curious about how far along ansible-sign is and if we can use it in our verification code.

gerrod3 · 2023-11-09T20:44:53Z

pulp_ansible/app/management/commands/add-sigstore-verifying-service.py

Reading over Matthias's comments, what do you think about removing the verifying service in place of a json field on the repository? The main issue is that the verifying service can not be a management command, it needs to be created/edited through the api by non-admin users. So the question becomes whether we want to have it be a separate model in the database or have it crafted through a json field on the repository. Some questions to help us decide:

How do users set up their sigstore tooling for verifying signatures. Is their a special config file? Can we simply have users upload their file to their repository like we do for GPG?

How useful is having re-usuable/shared verifying services that can be used across many repositories?

How often do we expect users to update or add new verifying services. Is it a one and done, or something that can be expected to change each week?

it needs to be created/edited through the api by non-admin users.

I am not sure why the verifying service should be configurable by non-admin users, as its role is to filter collection versions that do not have valid signature according to a pre-defined policy, which is critical from a security point of view.
Regarding having the verifying service replaced by a JSON field, I have nothing against it except if this implies any non-admin user can configure it.

To answer your questions:

If users use a CLI such as ansible-galaxy to pull collections from a repository, they will use the command options to specify the verification policies (for example, OIDC issuer and signer's identity). The same is true for tooling from automated workflows (github actions, etc).

It would probably be useful, as private content in an organizations is likely to originate from the same few sources most of the time (however, this is just an assumption).

It would be more of a one and done or once-in-a-while action. As mentioned above, content signature sources are not so likely to change on a frequent basis.

gerrod3 · 2023-11-09T20:50:42Z

pulp_ansible/app/serializers.py

+                                for sigstore_verifying_service in sigstore_verifying_services:
+                                    verification_result = sigstore_verifying_service.sigstore_verify(manifest=io, sigstore_bundle=sigstore_bundle)


I think we should check this setting: settings.ANSIBLE_SIGNATURE_REQUIRE_VERIFICATION and if it fails all verification with this setting then we raise the failure, else just don't create the signature.

gerrod3 · 2023-11-09T20:54:40Z

pulp_ansible/app/sigstore/tasks/sigstore_signature.py

+            return manifest
+
+
+def verify_sigstore_signature_upload(data):


I think just like its counterpart, verify_signature_upload, this method should use the same logic with settings.ANSIBLE_SIGNATURE_REQUIRE_VERIFICATION and emit a warning log if there are no verifying services on the repository.

Addressed in 3b27be9

gerrod3 · 2023-11-09T20:58:38Z

pulp_ansible/app/sigstore/tasks/sigstore_signature.py

+    async def sigstore_sign_collection_versions(self, collection_versions):
+        """Signs the collection versions with Sigstore."""


Suggested change

async def sigstore_sign_collection_versions(self, collection_versions):

"""Signs the collection versions with Sigstore."""

async def sigstore_sign_collection_version(self, collection_version):

"""Signs the collection version with Sigstore."""

This should be refactored to handle just one collection version to better take advantage of async code.

gerrod3 · 2023-11-09T20:59:20Z

pulp_ansible/app/sigstore/tasks/sigstore_signature.py

+        client_secret = self.sigstore_signing_service.oidc_client_secret
+
+        identity = self.sigstore_signing_service.issuer.identity_token(
+            "sigstore", client_secret,
+        )
+
+        signing_ctx = self.sigstore_signing_service.signing_context


I would move this into init.

Addressed in e0218f0

gerrod3 · 2023-11-09T21:01:11Z

pulp_ansible/app/sigstore/tasks/sigstore_signature.py

+        with signing_ctx.signer(identity) as signer:
+            # Limits the number of subprocesses spawned/running at one time
+            async with self.semaphore:
+                async for collection_version in collection_versions:


Suggested change

async for collection_version in collection_versions:

gerrod3 · 2023-11-09T21:03:18Z

pulp_ansible/app/sigstore/tasks/sigstore_signature.py

+            await asyncio.create_task(
+                self.sigstore_sign_collection_versions(
+                    sync_to_async_iterable(new_content.iterator())
+                )
+            )


Suggested change

await asyncio.create_task(

self.sigstore_sign_collection_versions(

sync_to_async_iterable(new_content.iterator())

)

)

async for collection_version in sync_to_async_iterable(new_content.iterator()):

tasks.append(asyncio.create_task(self.sigstore_sign_collection_version(collection_version)))

await asyncio.gather(*tasks)

Addressed by 6346d5d

…mand

… verifying service could validate the signature

gerrod3 · 2023-12-06T03:06:55Z

pulp_ansible/app/management/commands/add-sigstore-verifying-service.py

@@ -26,7 +26,7 @@

 DEFAULTS = {
    "rekor-url": "https://rekor.sigstore.dev",
-    "tuf-url": "https://sigstore-tuf-root.storage.googleapis.com/",
+    "tuf-url": "https://tuf-repo-cdn.sigstore.dev/",


Is this going to be the final url?

Yes, this is the one currently used for the public Sigstore instance

gerrod3 · 2023-12-06T03:09:48Z

pulp_ansible/app/models.py

+        if self.tuf_url.endswith("/"):
+            self.tuf_url = self.tuf_url[-1]


Suggested change

if self.tuf_url.endswith("/"):

self.tuf_url = self.tuf_url[-1]

if self.tuf_url.endswith("/"):

self.tuf_url = self.tuf_url[:-1]

You could also remove the 'if' statement with self.tuf_url = self.tuf_url.rstrip("/")

gerrod3 · 2023-12-06T03:12:51Z

pulp_ansible/app/models.py

+    @property
+    def get_rekor_url(self):
+        if self.rekor_url.endswith("/"):
+            self.rekor_url = self.rekor_url[:-1]
+        return self.rekor_url
+
+    @property
+    def get_tuf_url(self):


Is this really necessary? You didn't do the same on the SigningService model.

gerrod3 · 2023-12-06T03:24:20Z

pulp_ansible/app/serializers.py

+                        elif settings.ANSIBLE_SIGNATURE_REQUIRE_VERIFICATION:
+                            raise VerificationFailureException(
+                                "Failed to verify Sigstore signature against all verifying services configured on this repository with ANSIBLE_SIGNATURE_REQUIRE_VERIFICATION enabled."
+                            )


Not sure this elif statement is on the right indentation. I was expecting it to be a part of the for loop if we don't break out of it (which I think you need to add since we don't want to save the signature twice if we have a verifier).

gerrod3 · 2023-12-06T03:29:36Z

pulp_ansible/app/sigstore/tasks/sigstore_signature.py

-            )
+            async for collection_version in sync_to_async_iterable(new_content.iterator()):
+                tasks.append(asyncio.create_task(self.sigstore_sign_collection_version(collection_version)))
+                await asyncio.gather(*tasks)


Suggested change

await asyncio.gather(*tasks)

await asyncio.gather(*tasks)

This needs to be outside the for loop else we would still be doing the tasks serially.

stale · 2024-03-05T16:24:29Z

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

stale · 2024-04-09T22:32:29Z

This pull request has been closed due to inactivity. If you feel this is in error, please reopen the pull request or file a new PR with the relevant details.

mayaCostantini marked this pull request as draft April 18, 2023 14:03

mdellweg reviewed Apr 18, 2023

View reviewed changes

mayaCostantini commented Apr 18, 2023

View reviewed changes

gerrod3 requested changes Apr 20, 2023

View reviewed changes

mayaCostantini force-pushed the repo-sigstore-sign-verify branch from 0577fa4 to 88e2cb8 Compare May 31, 2023 10:47

gerrod3 requested changes Jun 10, 2023

View reviewed changes

mayaCostantini force-pushed the repo-sigstore-sign-verify branch from 39bffb7 to 18bb3b4 Compare June 13, 2023 09:39

mayaCostantini mentioned this pull request Jun 23, 2023

Create a base SigningService class pulp/pulpcore#3952

Open

mayaCostantini added 19 commits October 23, 2023 17:53

Add management commands for creating and removing Sigstore signing se…

902be0e

…rvices

Add CollectionVersionSigstoreSignature, SigstoreSigningService and Si…

10d9d57

…gtoreVerifyingService models

Add Sigstore signing and verifying services as AnsibleRepository fore…

7cc82c7

…ign keys

Implement serializers for Sigstore signing related models

537cea4

Initialize Sigstore submodule

2362042

Define a base model for Sigstore OIDC issuers and implement Keycloak …

62a4f59

…issuer

Implement submodule for sigstore signing and verification tasks

24c98a4

Mention CollectionSigstoreSigningFirstStage as a pipeline first_stage

a7c3cb4

Add Sigstore-related exceptions

8c4810c

Update requirements.txt

ba80dca

Add viewsets for Sigstore content and sigstore_sign action on repository

7de5b57

Fix CollectionVersionSigstoreSignature import error

7f31f58

Create migrations for Sigstore-related models

fc6a24f

Fix bugs and add permissions for sigstore signing and listing signatures

030c826

Modify migrations to adapt to permission changes

0c116b7

Have SigstoreSigningService and SigstoreVerifyingService inherit from…

edd4112

… BaseModel

Change viewset parent classes for sigstore signing and verifying serv…

20f1c17

…ice viewsets

Update documentation with examples for sigstore signing and verification

dbd1ed5

Adapt sigstore-specific serializers to new model inheriting from Base…

6ae359d

…Model

mayaCostantini added 3 commits October 23, 2023 17:53

Update sigstore-python to latest stable version v2.0.1

5f6d78a

Generate migration for Sigstore feature

7646cb9

Delete old Sigstore migration

cccfd0b

mayaCostantini force-pushed the repo-sigstore-sign-verify branch from 781dfb9 to cccfd0b Compare October 23, 2023 17:56

Update signature docs

fbc6ad6

gerrod3 requested changes Nov 6, 2023

View reviewed changes

mayaCostantini added 10 commits November 6, 2023 10:03

Fix link in documentation

df63642

Remove unnecessary unique=True from CollectionVersionSigstoreSignature

e1e7a5e

Make SigstoreSigningServiceSerializer read-only

abec9a5

Make SigstoreVerifyingServiceSerializer read-only

3a5ac90

Fail if artifact or repository is not in collection validated data an…

81bb2d8

…d open temporary directory in current path

Perform secondary validation with deferred_validate in CollectionVers…

95bf169

…ionSigstoreSignatureSerializer

Make 'repository' field required in CollectionVersionSigstoreSignatur…

cae3eef

…eSerializer constructor and remove validate method

Remove RolesMixin from sigstore viewsets

daa29d7

Remove required argument from sigstore serializer fields

4b467cb

Remove setuptools from requirements.txt

f48beef

gerrod3 requested changes Nov 9, 2023

View reviewed changes

mayaCostantini added 3 commits December 1, 2023 15:10

Update default TUF metadata URL in add sigstore verifying service com…

b5792f8

…mand

Fail if settings.ANSIBLE_SIGNATURE_REQUIRE_VERIFICATION is set and no…

3b27be9

… verifying service could validate the signature

Fix signature extraction logic

3a46e8b

github-actions bot added the multi-commit label Dec 5, 2023

mayaCostantini added 3 commits December 5, 2023 13:35

Fix access_token to id_token in signing logic

c91fbc4

Move sigstore signing context to constructor in signing task

e0218f0

Take better advantage of async code in signing task

6346d5d

gerrod3 requested changes Dec 6, 2023

View reviewed changes

stale bot added the stale label Mar 5, 2024

stale bot closed this Apr 9, 2024

		@@ -275,6 +318,381 @@ class Meta:
		unique_together = ("pubkey_fingerprint", "signed_collection")



		class SigstoreSigningService(Content):

		if "from_file" in options:
		file_path = Path(options["from_file"])

	- `Cosign https://docs.sigstore.dev/cosign/overview/`__
	- `Cosign <https://docs.sigstore.dev/cosign/overview/>`__

	sigstore_bundle_sha256_hash = models.CharField(max_length=64, unique=True, db_index=True, null=True)
	sigstore_bundle_sha256_hash = models.CharField(max_length=64, db_index=True, null=True)

		@@ -128,6 +139,148 @@ class Meta:
		)


		class SigstoreSigningServiceSerializer(ModelSerializer):

	with tempfile.TemporaryDirectory() as tempdir:
	with tempfile.TemporaryDirectory(dir=".") as tempdir:



		class SigstoreSigningServiceViewSet(
		NamedModelViewSet, mixins.RetrieveModelMixin, mixins.ListModelMixin, RolesMixin



		class SigstoreVerifyingServiceViewSet(
		NamedModelViewSet, mixins.RetrieveModelMixin, mixins.ListModelMixin, RolesMixin

		for sigstore_verifying_service in sigstore_verifying_services:
		verification_result = sigstore_verifying_service.sigstore_verify(manifest=io, sigstore_bundle=sigstore_bundle)

		async def sigstore_sign_collection_versions(self, collection_versions):
		"""Signs the collection versions with Sigstore."""

		if self.tuf_url.endswith("/"):
		self.tuf_url = self.tuf_url[-1]

Implement Sigstore signing and verification of collections in Ansible repositories (refactored) #1428

Implement Sigstore signing and verification of collections in Ansible repositories (refactored) #1428

Conversation

mayaCostantini commented Apr 18, 2023 • 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

mdellweg commented Apr 19, 2023

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

mayaCostantini commented May 30, 2023

gerrod3 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

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

gerrod3 Jun 14, 2023 • 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

mayaCostantini commented Jun 14, 2023 • 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

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

mayaCostantini commented Nov 6, 2023

gerrod3 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

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

stale bot commented Mar 5, 2024

stale bot commented Apr 9, 2024

mayaCostantini commented Apr 18, 2023 •

edited

Loading

gerrod3 Jun 14, 2023 •

edited

Loading

mayaCostantini commented Jun 14, 2023 •

edited

Loading