Support Witness attestation collections #26
Conversation
|
Note that at this moment, there are no rules enforced on attestation collections because their attribute checks are still up in the air. However, it can enforce materials / products. Next up is supporting attribute checks for non-materials / products data in a collection. cc @jkjell |
adityasaky
force-pushed
the
consolidation
branch
from
bc3f6ed to
0620d38
Compare
adityasaky
requested review from
marcelamelara and
trishankatdatadog
as code owners
|
I'll wait for @jkjell to review |
adityasaky
force-pushed
the
consolidation
branch
from
0ed7108 to
9e04198
Compare
Signed-off-by: Aditya Sirish <aditya@saky.in>
adityasaky
force-pushed
the
consolidation
branch
2 times, most recently
from
8ef2732 to
dca45a2
Compare
|
cc @ChaosInTheCRD as well |
This is a bit too big and the context is missing a fair bit. Could you explain it at the next meeting? |
There was a problem hiding this comment.
Thanks for the updates @adityasaky ! I need to do another round of more detailed code review, but at a high level, I like these changes. I do think that a lot of this is going to change again with the in-toto policies stuff, but for now this is a use case we need to support.
There was a problem hiding this comment.
Thanks again @adityasaky ! The TL;DR for my review comments is that I think we should use verification of witness attestation collections as a use case for the "pluggable" inspections/policy engines we've been discussing in the policies WG. This seems like a reasonable follow on to this PR, but I'd like to hear your thoughts on how feasible it would be to implement the code in this PR as a separate module.
There was a problem hiding this comment.
I don't think we should be loading private keys into the repo, even just for testing.
There was a problem hiding this comment.
This is standard in all in-toto implementations, fwiw. Perhaps we can move it to testdata so we're following convention?
| ExpectedPredicateType string `yaml:"expectedPredicateType"` | ||
| ExpectedMaterials []string `yaml:"expectedMaterials"` | ||
| ExpectedProducts []string `yaml:"expectedProducts"` | ||
| ExpectedAttributes []Constraint `yaml:"expectedAttributes"` | ||
| ExpectedAttestors []ExpectedAttestorConstraints `yaml:"expectedAttestors"` |
There was a problem hiding this comment.
I think I'd like to eventually merge these two concepts of ExpectedPredicateType and ExpectedAttestors, right now it feels like we're customizing the attestation-verifier specifically for Witness attestation collections, and that may be ok for experimentation, but I do think we ought to be generalizing as much as we can on the long run.
| @@ -85,6 +86,5 @@ func LoadLayout(path string) (*Layout, error) { | |||
| } | |||
|
|
|||
| type AttestationIdentifier struct { | |||
| PredicateType string | |||
| Functionary string | |||
| Functionary string | |||
There was a problem hiding this comment.
Do we think we'll add other fields here again in the future? If not, a "simple" typedef may make more sense here.
|
|
||
| input, err := getActivation(statement) | ||
| // Examine collector claims in attestation collection | ||
| if step.ExpectedPredicateType == witnessattestation.CollectionType { |
There was a problem hiding this comment.
I think this special casing is really what I'm referring to when I say that we ought to be generalizing. In some sense, this code is really doing the sort of pre-processing or inspection that I think ultimately should be implemented as a separate module. We might still provide this module for Witness as part of the attestation-verifier package, but this is where I think we can start figuring out what the common APIs for inspections 2.0 .
This PR updates the schema for layouts a bit. I'll eventually break this up into separate commits.
It also adds initial support for attestation collections from Witness.