Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ArtC: Add data.fileInformation.integrityProtection #320

Merged
merged 2 commits into from Oct 27, 2022

Conversation

magnusbaeck
Copy link
Member

@magnusbaeck magnusbaeck commented Oct 10, 2022

Applicable Issues

Fixes #290

Description of the Change

This new (optional) member allows publishers to include the checksum of the artifact's file(s) when the artifact is announced, allowing consumers to verify that the file they eventually download is the original file.

Alternate Designs

None.

Benefits

Makes it possible to catch deliberate and non-deliberate tampering of artifact files. Should be combined with signed events but has value on its own.

Possible Drawbacks

None. It's an optional member.

Sign-off

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or

(b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or

(c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it.

(d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved.

Signed-off-by: Magnus Bäck <magnus.back@axis.com>

This new member allows publishers to include the checksum of the
artifact's file(s) when the artifact is announced, allowing consumers
to verify that the file they eventually download is the original file.
@magnusbaeck magnusbaeck changed the title ArtC: Add integrityProtection member ArtC: Add data.fileInformation.integrityProtection Oct 17, 2022
@magnusbaeck
Copy link
Member Author

Here's a diff between ArtC 3.2.0 and 3.3.0:

--- definitions/EiffelArtifactCreatedEvent/3.2.0.yml	2022-10-17 13:43:03.630580027 +0200
+++ definitions/EiffelArtifactCreatedEvent/3.3.0.yml	2022-10-17 15:06:29.528555037 +0200
@@ -52,6 +52,35 @@
               type: array
               items:
                 type: string
+            integrityProtection:
+              _description: An optional object containing a digest of
+                the file's contents, i.e. a checksum, computed using
+                the specified algorithm.
+              type: object
+              properties:
+                alg:
+                  _description: The cryptographic algorithm used to compute
+                    the digest of the file's contents.
+                  _format: One of the hash algorithms listed in section 1 of
+                    [NIST FIPS 180-4](https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.180-4.pdf),
+                    excluding "SHA-1".
+                  type: string
+                  enum:
+                    - SHA-224
+                    - SHA-256
+                    - SHA-384
+                    - SHA-512
+                    - SHA-512/224
+                    - SHA-512/256
+                digest:
+                  _description: The digest of the file contents.
+                  _format: A lowercase string of hexadecimal digits.
+                  type: string
+                  pattern: ^[0-9a-f]+$
+              required:
+                - alg
+                - digest
+              additionalProperties: false
           required:
             - name
           additionalProperties: false
@@ -183,6 +212,9 @@
       types:
         - EiffelArtifactCreatedEvent
 _history:
+  - version: 3.3.0
+    introduced_in: No edition set
+    changes: Added data.fileInformation.integrityProtection member (see [Issue 290](https://github.com/eiffel-community/eiffel/issues/290)).
   - version: 3.2.0
     introduced_in: 'No edition set'
     changes: Add schema URL to the meta object (see [Issue 280](https://github.com/eiffel-community/eiffel/issues/280)).
@@ -212,3 +244,5 @@
     url: ../examples/events/EiffelArtifactCreatedEvent/backend.json
   - title: Dependent example
     url: ../examples/events/EiffelArtifactCreatedEvent/dependent.json
+  - title: Checksum example
+    url: ../examples/events/EiffelArtifactCreatedEvent/checksum.json
--- schemas/EiffelArtifactCreatedEvent/3.2.0.json	2022-10-17 15:06:50.904495149 +0200
+++ schemas/EiffelArtifactCreatedEvent/3.3.0.json	2022-10-17 15:06:50.972494958 +0200
@@ -18,9 +18,9 @@
         "version": {
           "type": "string",
           "enum": [
-            "3.2.0"
+            "3.3.0"
           ],
-          "default": "3.2.0"
+          "default": "3.3.0"
         },
         "time": {
           "type": "integer"
@@ -149,6 +149,31 @@
                 "items": {
                   "type": "string"
                 }
+              },
+              "integrityProtection": {
+                "type": "object",
+                "properties": {
+                  "alg": {
+                    "type": "string",
+                    "enum": [
+                      "SHA-224",
+                      "SHA-256",
+                      "SHA-384",
+                      "SHA-512",
+                      "SHA-512/224",
+                      "SHA-512/256"
+                    ]
+                  },
+                  "digest": {
+                    "type": "string",
+                    "pattern": "^[0-9a-f]+$"
+                  }
+                },
+                "required": [
+                  "alg",
+                  "digest"
+                ],
+                "additionalProperties": false
               }
             },
             "required": [

@magnusbaeck magnusbaeck marked this pull request as ready for review October 17, 2022 14:03
@magnusbaeck magnusbaeck requested a review from a team as a code owner October 17, 2022 14:03
t-persson
t-persson previously approved these changes Oct 20, 2022
@e-backmark-ericsson
Copy link
Member

e-backmark-ericsson commented Oct 27, 2022

You provide quite a good argumentation in the issue text, regarding the relation to the meta.security.integrityProtection. I think it would be valuable to document that also in the spec. Would you be able to add a section about "Integrity of Referenced Items" or similar, to eiffel-syntax-and-usage/security.md for example?

Also, there is a huge buzz nowadays in open source communities around sigstore and cosign for signing things like artifacts. I believe that we eventually should support such signing utilities and that will probably affect this integrityProtection property. Is it worth it to include such signed signatures now, or should we take that later, with the risk of getting deprecated properties here?

@magnusbaeck
Copy link
Member Author

You provide quite a good argumentation in the issue text, regarding the relation to the meta.security.integrityProtection. I think it would be valuable to document that also in the spec. Would you be able to add a section about "Integrity of Referenced Items" or similar, to eiffel-syntax-and-usage/security.md for example?

Good idea. It's easy to forget the non-reference documentation when making protocol changes.

Also, there is a huge buzz nowadays in open source communities around sigstore and cosign for signing things like artifacts. I believe that we eventually should support such signing utilities and that will probably affect this integrityProtection property. Is it worth it to include such signed signatures now, or should we take that later, with the risk of getting deprecated properties here?

Cosign appears to be targeting OCI images only. That doesn't preclude its use for ArtC entirely but would limit the scope. I don't feel I understand Sigstore and friends well enough to be able get them into the protocol. I can't even tell whether it makes sense to have ArtC support Sigstore unless combined with meta.security signing, or what Sigstore really buys us if we have the hash proposed in this PR and meta.security signing is used.

IOW I suggest moving forward with what we have. It's better than nothing and should be quite secure if combined with meta.security.

@e-backmark-ericsson
Copy link
Member

This relates to the old issue #289

@magnusbaeck
Copy link
Member Author

Would you be able to add a section about "Integrity of Referenced Items" or similar, to eiffel-syntax-and-usage/security.md for example?

Done.

@magnusbaeck magnusbaeck merged commit aafb9ec into eiffel-community:master Oct 27, 2022
@magnusbaeck magnusbaeck deleted the artc-checksum branch October 27, 2022 17:49
@magnusbaeck magnusbaeck added the protocol All protocol changes label Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement protocol All protocol changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add checksum to ArtC's data.fileInformation objects
3 participants