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

Allow duplicate values for slsa_provenance_v0.2 schema #1534

Merged

Conversation

yashvardhannanavati
Copy link
Contributor

When processing multi-arch images, it is possible for the image index and the index manifests to have the same name. This change enables that. It is also in compliance with
https://slsa.dev/spec/v0.2/provenance#schema

@lcarva
Copy link
Member

lcarva commented Apr 17, 2024

/ok-to-test

Copy link
Member

@lcarva lcarva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@zregvart, could you chime in here as you were pretty involved in adding this schema.

@lcarva
Copy link
Member

lcarva commented Apr 17, 2024

As an example, Chains created a SLSA Provenance with this for subject:

[
  {
    "name": "quay.io/redhat-user-workloads-stage/ynanavat-tenant/bootc-image-builder/bootc-image-builder",
    "digest": {
      "sha256": "df93e3976eb26199f3eaaa06cc4cd83f9d6663491aabaf73f67018cbf094dbc8"
    }
  },
  {
    "name": "quay.io/redhat-user-workloads-stage/ynanavat-tenant/bootc-image-builder/bootc-image-builder",
    "digest": {
      "sha256": "91a66493a17b922553468c24ce4d719fa22db3cabac3064c91f8ad9774ff0e91"
    }
  },
  {
    "name": "quay.io/redhat-user-workloads-stage/ynanavat-tenant/bootc-image-builder/bootc-image-builder",
    "digest": {
      "sha256": "929e3d5f989c811078e38d38ccd0708cdc6c18d06064d2a23bddb61b861e3ec2"
    }
  }
]

There's also nothing in the spec that mentions the name should be unique. In fact, it states this:

// Output file; name is "" to indicate "not important".
"subject": [{"name": "
", "digest": {"sha256": "5678..."}}],

That implies the name could be repeated, e.g. _, if there are multiple subjects where the name is not important.

Copy link

codecov bot commented Apr 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.59%. Comparing base (73f5fb0) to head (476042e).
Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1534   +/-   ##
=======================================
  Coverage   80.59%   80.59%           
=======================================
  Files          66       66           
  Lines        4761     4761           
=======================================
  Hits         3837     3837           
  Misses        924      924           
Flag Coverage Δ
generative 80.59% <ø> (ø)
integration 80.59% <ø> (ø)
unit 80.59% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@lcarva
Copy link
Member

lcarva commented Apr 17, 2024

/ok-to-test

@lcarva
Copy link
Member

lcarva commented Apr 17, 2024

/retest

When processing multi-arch images, it is possible for the image
index and the index manifests to have the same name. This change
enables that. It is also in compliance with
https://slsa.dev/spec/v0.2/provenance#schema

Signed-off-by: Yashvardhan Nanavati <yashn@bu.edu>
@yashvardhannanavati
Copy link
Contributor Author

/retest

2 similar comments
@simonbaird
Copy link
Member

/retest

@yashvardhannanavati
Copy link
Contributor Author

/retest

@zregvart
Copy link
Member

This tells me that whatever is generating the SLSA provenance is not doing it in conformance with the specification:

subject[*].name string, required

Identifier to distinguish this artifact from others within the subject.

The semantics are up to the producer and consumer. Because consumers evaluate the name against a policy, the name SHOULD be stable between attestations. If the name is not meaningful, use "_". For example, a [SLSA Provenance](https://slsa.dev/provenance) attestation might use the name to specify output filename, expecting the consumer to only considers entries with a particular name. Alternatively, a vulnerability scan attestation might use the name "_" because the results apply regardless of what the artifact is named.

MUST be non-empty and unique within subject.

@lcarva
Copy link
Member

lcarva commented Apr 18, 2024

This tells me that whatever is generating the SLSA provenance is not doing it in conformance with the specification:

subject[*].name string, required

Identifier to distinguish this artifact from others within the subject.

The semantics are up to the producer and consumer. Because consumers evaluate the name against a policy, the name SHOULD be stable between attestations. If the name is not meaningful, use "_". For example, a [SLSA Provenance](https://slsa.dev/provenance) attestation might use the name to specify output filename, expecting the consumer to only considers entries with a particular name. Alternatively, a vulnerability scan attestation might use the name "_" because the results apply regardless of what the artifact is named.

MUST be non-empty and unique within subject.

Ah! I didn't check the in-toto spec which actually better defines that field. Well the culprit here is Chains. I also think that a unique name is somewhat shortsighted when it comes to images. Filed an issue on the spec: in-toto/attestation#348

Regardless of the outcome there, I think we should merge this. We don't get any value from enforcing this restricting. Arguably, the uniqueness check also doesn't capture the special meaning of _.

@lcarva
Copy link
Member

lcarva commented Apr 18, 2024

/retest

@lcarva
Copy link
Member

lcarva commented Apr 18, 2024

Filed EC-558 for the long-term solution to this problem.

We discussed this and agreed to go ahead with the merge.

@lcarva lcarva merged commit e0dc295 into enterprise-contract:main Apr 18, 2024
12 checks passed
zregvart added a commit to zregvart/ec-cli that referenced this pull request Apr 22, 2024
The github.com/qri-io/jsonschema seems to suffer from thread safety
issues, see[1]. This replaces that implementation with
github.com/santhosh-tekuri/jsonschema/v5.

Note that the `uniqueKeys` functionality that has been disabled in enterprise-contract#1534
has not been ported and since it is now dead code has been removed.

[1] https://github.com/enterprise-contract/ec-cli/actions/runs/8777708975/job/24083046629?pr=1544#step:6:5121
zregvart added a commit to zregvart/ec-cli that referenced this pull request Apr 23, 2024
The github.com/qri-io/jsonschema seems to suffer from thread safety
issues, see[1]. This replaces that implementation with
github.com/santhosh-tekuri/jsonschema/v5.

Note that the `uniqueKeys` functionality that has been disabled in enterprise-contract#1534
has not been ported and since it is now dead code has been removed.

[1] https://github.com/enterprise-contract/ec-cli/actions/runs/8777708975/job/24083046629?pr=1544#step:6:5121
zregvart added a commit to zregvart/ec-cli that referenced this pull request Apr 25, 2024
The github.com/qri-io/jsonschema seems to suffer from thread safety
issues, see[1]. This replaces that implementation with
github.com/santhosh-tekuri/jsonschema/v5.

Note that the `uniqueKeys` functionality that has been disabled in enterprise-contract#1534
has not been ported and since it is now dead code has been removed.

[1] https://github.com/enterprise-contract/ec-cli/actions/runs/8777708975/job/24083046629?pr=1544#step:6:5121
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants