Expand ResourceDescriptor `annotations` to allow any type?
Closed this issue · 11 comments
The ResourceDescriptor annotations field is currently map<string, object> which means that it's not possible to have an annotation of type string, number, boolean, or array. For example, you cannot do:
"annotations": {
"canonicalSource": "https://..."
}Instead you have to do:
"annotations": {
"canonicalSource": { "value": "https://..." }
}Is there a reason for this restriction? If not, is it possible to remove it (replacing google.protobuf.Struct with google.protobuf.Value)? Not sure if this would require a version bump.
Background: slsa-framework/slsa#870. In SLSA, we copied the definition of ResourceDescriptor but used Value, not realizing this restriction was in place. So now the SLSA spec disagrees with in-toto. Since it's not clear to me why the restriction exists in the first place, I wanted to check here to see if it's worth making in-toto match SLSA.
Is the reason for extensibility? For example, if you decide to change the format of the canonicalSource, you can add a new field with the new semantics without having to define a new extension?
I don't recall a reason for this restriction. I think the original suggestion was that it just be object. I believe I suggested map<string, object> as an improvement. I never thought of map<string, value>.
If I'm not mistaken, Value supports embedding objects as well so it shouldn't be backwards incompatible. I think it makes sense to support non-object annotations.
+1 to switching to map<string, value> for annotations. I believe making the field a map was in part for better extensibility, but I think that's orthogonal to switching map values from object to Value type.
This sounds fine to us. Not sure if we'll have time to do it soon. Happy to review PRs!
in-toto-golang has defined the annotation as map[string]interface{} which allows pointing to any type i.e. string, number, boolean, or array.
If map<string, value> is intended in intoto, IMHO it's just a matter of clarification for the annotation field in the doc https://github.com/in-toto/attestation/blob/main/spec/v1/resource_descriptor.md#fields? And it might be not too late to clarify this since there are not many implementations of SLSA v1.0 yet.
@chuangw6 I think we discussed moving the Provenance def in in-toto-golang to using the proto generated schema in this repo. I imagine that'd apply to resource descriptors too.
@adityasaky ack. It would be really helpful if we can have the proto definition for the whole slsa spec and its individual fields i.e. resourceDescriptor in a single place so that we can have a single source of truth, which helps avoid discrepancy in future.
Currently,
- slsa defines the proto for the statement and resource descriptor in https://github.com/slsa-framework/slsa/blob/main/docs/provenance/schema/v1/provenance.proto.
- intoto also has the proto defined here https://github.com/in-toto/attestation/tree/main/protos/in_toto_attestation/v1.
We need consensus on where this single place should be from both intoto and slsa sides.
@chuangw6 Where the provenance proto lives is a discussion that we already had and resolved: slsa-framework/slsa#855 The proto on the SLSA repo is for reference only, and not authoritative.
If there are further requirements, this is probably the time to clarify them.
Thanks for the pointer @marcelamelara.
Just saw @joshuagl is assigned to this issue. Do we have a ETA for fixing this in the proto on this repo?
Once this is fixed in in-toto, I think SLSA should be good to clarify this in the spec and reference the authoritative&correct proto in this intoto repo.
Thank you all!