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

Add predicate capturing results of test runs #152

Merged
merged 3 commits into from
Apr 11, 2023

Conversation

adityasaky
Copy link
Member

No description provided.

@adityasaky adityasaky force-pushed the add-test-result-attestations branch 4 times, most recently from 09b7dad to 34456cd Compare March 16, 2023 19:05
@adityasaky adityasaky marked this pull request as ready for review March 16, 2023 19:05
@adityasaky adityasaky force-pushed the add-test-result-attestations branch 4 times, most recently from f4e7335 to 51bda0e Compare March 16, 2023 19:59
@TomHennen
Copy link
Contributor

FYI one of my colleagues who has done work in this area is going to take a look and provide some comments. Stay tuned.

@adityasaky adityasaky requested a review from a team March 29, 2023 14:05
Copy link
Contributor

@TomHennen TomHennen left a comment

Choose a reason for hiding this comment

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

Thanks Aditya!

@adityasaky
Copy link
Member Author

Thanks for the reviews! I've squashed my commits now.

Copy link
Contributor

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

This looks useful. I had a few questions (inline) about this predicate specifically, and a couple of higher-level questions: one about the relationship between in-toto attestations and the broader in-toto spec, and one about inclusion of attestation framework parsing rules in each predicate.

The higher level questions can be addressed separately and should not block this PR.

spec/predicates/test-results.md Outdated Show resolved Hide resolved

The supply chain owner creates a policy that records the expected test
configurations. During verification, the policy checks that the test attestation
used the right configurations. A custom inspection may optionally parse the
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the first predicate to reference an inspection, should we be avoiding in-toto specification terms in the attestations repository (which aims to be policy engine agnostic)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's okay to mention them in use cases. I can clarify that it's with in-toto layouts / policy engine. We could similarly add a non in-toto verifier and how it'd do something...

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated with an "if using with in-toto..." conditional, lmk if this is fine.

spec/predicates/test-results.md Outdated Show resolved Hide resolved
spec/predicates/test-results.md Outdated Show resolved Hide resolved
spec/predicates/test-results.md Outdated Show resolved Hide resolved
@adityasaky adityasaky force-pushed the add-test-result-attestations branch 2 times, most recently from 489b101 to 13c9473 Compare April 4, 2023 12:56
Copy link
Contributor

@marcelamelara marcelamelara left a comment

Choose a reason for hiding this comment

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

Thanks @adityasaky !! I really like the simplicity of this predicate. I have several questions/requests for updates to improve clarity.

spec/predicates/test-results.md Outdated Show resolved Hide resolved
spec/predicates/test-results.md Outdated Show resolved Hide resolved
spec/predicates/test-results.md Outdated Show resolved Hide resolved
spec/predicates/test-results.md Outdated Show resolved Hide resolved
spec/predicates/test-results.md Outdated Show resolved Hide resolved
spec/predicates/test-results.md Outdated Show resolved Hide resolved
spec/predicates/test-results.md Outdated Show resolved Hide resolved
spec/predicates/test-results.md Outdated Show resolved Hide resolved
@adityasaky adityasaky force-pushed the add-test-result-attestations branch from 13c9473 to ec23143 Compare April 4, 2023 17:44
@adityasaky
Copy link
Member Author

Does it make more sense for the schema to be as follows? Since testRun is now required because of the configuration, we can remove that intermediate field.

{
    "_type": "https://in-toto.io/Statement/v1",
    "subject": [{...}],
    "predicateType": "https://in-toto.io/attestation/test-result/v0.1",
    "predicate": {
        "result": "pass|fail",
        "url": "<URL>",
        "configuration": ["<ResourceDescriptor>", ...],
        "passedTests": ["<TEST_NAME>", ...],
        "warnedTests": ["<TEST_NAME>", ...],
        "failedTests": ["<TEST_NAME>", ...]
    }
}

@adityasaky
Copy link
Member Author

nit: I'll reorder the fields to move required ones up. Configuration should be first since it's now required.

@adityasaky adityasaky requested a review from a team April 10, 2023 14:33
Signed-off-by: Aditya Sirish <[email protected]>
Copy link
Contributor

@marcelamelara marcelamelara left a comment

Choose a reason for hiding this comment

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

Thanks for the edits, @adityasaky . LGTM.

@marcelamelara marcelamelara merged commit 81960e6 into in-toto:main Apr 11, 2023
@adityasaky adityasaky deleted the add-test-result-attestations branch April 11, 2023 17:48
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.

5 participants