Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ITE-11: Support attribute constraints in layouts #50
ITE-11: Support attribute constraints in layouts #50
Changes from all commits
e2ecfe2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking for clarification: the in-toto spec for the Steps format specifies these fields as
expected_materials
andexpected_products
.I haven't seen any ITEs or spec updates that change this, but ITE-10 and this ITE-11 (as well as the project at https://github.com/in-toto/attestation-verifier) seem to use this
camelCase
for these same fields.Is it the case that:
camelCase
, but that's just for convenience, and actual serialization should remainsnake_case
camelCase
for the standard naming of fields in the layout schema, but the spec just hasn't been updatedWe are using our own in-toto implementation for verification and are attempting to adhere to the spec as much as possible/practical, but are having a hard time determining whether there is a correct/canonical serialization format based on these inconsistencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me personally, it was mostly 1, though it's something to consider and nail down for compatibility / consistency. Happy to change these back to snake case unless there's interest in using this breaking change to also change the serialization format. I don't feel strongly one way or another.
FWIW, I think we should probably reject 2 if it does come up; it's tempting to treat this as an implementation detail but we should aim for maximum compatibility of layouts and corresponding tooling. @jkjell we should coordinate this, if applicable, wrt witness policies too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A raw list of CEL expressions might be a bit limiting. If something fails, it's difficult to present a clear error message of what went wrong. You might want to have a field for error messages. (Similarly, giving each an optional name might make it more readable.)
For example:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#55
This is really great! I'm going to work on this and support it in attestation-verifier, but without blocking merging this ITE as draft as it's been an open PR for some time now.