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

Resolve ambiguity in ResourceDescriptor annotations #875

Closed
Tracked by #900
MarkLodato opened this issue Jun 6, 2023 · 4 comments · Fixed by #892
Closed
Tracked by #900

Resolve ambiguity in ResourceDescriptor annotations #875

MarkLodato opened this issue Jun 6, 2023 · 4 comments · Fixed by #892
Assignees

Comments

@MarkLodato
Copy link
Member

From #870:

Problem

The v1.0 provenance spec is currently in conflict with itself because it defines ResourceDescriptor twice:

  • The body of the spec indirectly defines ResourceDescriptor by linking to the in-toto v1 definition, which only allows annotations of JSON type object, i.e. map<string, google.protobuf.Struct>.
  • The summary (cue and proto) define of ResourceDescriptor directly, allowing annotations of any JSON type (object, array, string, number, boolean), i.e. map<string, google.protobuf.Value>.

This can manifest itself if a producer generates a non-object type but the consumer only accepts non-object type.

If the spec had said that only one was canonical and the other was informative, we could fix this by just updating the informative one. But we have no such language, thus the spec is ambiguous.

Fixing this is backwards incompatible because it is changing a type.

  • Making it more restrictive (i.e. only object type) means that existing implementations that generate any type will fail.
  • Making it less restrictive (i.e. any JSON type) means that existing implementations that only accept object type will fail.

Thus technically this should require a major version bump according to Semantic Versioning. However, a major version bump is fairly disruptive. We believe it will likely cause confusion among consumers, who likely will not notice this. (There are not many implementations of SLSA v1.0 yet.)

An alternative solution is to keep our definition and update in-toto v1 to be more permissive, as per in-toto/attestation#242.

Decision

In the 2023-06-05 spec meeting meeting, we decided to handle this with a one-off minor version bump because a major version bump is too disruptive. So we won't merge this PR as-is but instead will create a v1.1 branch. As part of that, we will also address future inconsistency by (1) clearly marking one format canonical in the event of a conflict and (2) linking to an immutable version of the in-toto spec to avoid potential changes in in-toto. (We could also avoid conflict by only defining it once, or not inlining the ResourceDescriptor, but that makes the spec harder to read by requiring readers to bounce between two different docs.)

Note: If in-toto decides to change to match SLSA (in-toto/attestation#242) we don't need a version bump, but we will still do (1) and (2) above.

@marcelamelara
Copy link
Contributor

On the in-toto end, some implementers are a bit confused about two proto definitions for SLSA provenance. So, given the decision to have a provenance definition hosted by in-toto used to generate bindings, I also think the SLSA spec/docs should be explicit about which proto definition is the authoritative one for implementers.

@chuangw6
Copy link

chuangw6 commented Jun 15, 2023

On the in-toto end, some implementers are a bit confused about two proto definitions for SLSA provenance. So, given the decision to have a provenance definition hosted by in-toto used to generate bindings, I also think the SLSA spec/docs should be explicit about which proto definition is the authoritative one for implementers.

Concur with @marcelamelara!

From the discussion in the intoto project, sounds like nobody is opposed to map<string, value>. Once the change is landed in the proto in intoto project, I guess SLSA should be good to clarify this in official 1.0 spec and doc without having to bump to 1.1?

@MarkLodato
Copy link
Member Author

This has now been resolved on the in-toto side.

The remaining work on SLSA side is to address future incompatibilities by stating which is canonical.

@MarkLodato MarkLodato changed the title Release v1.1 with fix for ambiguity in ResourceDescriptor annotations Resolve ambiguity in ResourceDescriptor annotations Jun 28, 2023
@joshuagl
Copy link
Member

I've filed #892 to address the remaining clarifications following the upstream in-toto/attestation change.

joshuagl added a commit that referenced this issue Aug 24, 2023
As discussed in the [2023-06-05 spec
meeting](https://docs.google.com/document/d/1kMP62o3KI0IqjPRSNtUqADodBqpEL_wlL1PEOsl6u20/edit#heading=h.4svh1796azw9)
clarify the format of ResourceDescriptor and make clear that the schema
are only informative, with the specification text (whether our text or
the linked in-toto/attestation spec) being the canonical definition.

- Update protobuf and cue definitions to match the in-toto attestations
specification.
- Emphasise that the specification text, not the schema, is the
authoritative specification should they be in conflict.
- Link to an immutable version of the in-toto specification to avoid
potential changes in in-toto resulting in SemVer breaking changes to
SLSA's formats.

Fixes: #875

Signed-off-by: Joshua Lock <joshua.lock@uk.verizon.com>
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Issue triage Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants