Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

Add skeleton for a metadata verifier #70

Merged
merged 3 commits into from
Apr 14, 2022

Conversation

rbehjati
Copy link
Contributor

@rbehjati rbehjati commented Apr 14, 2022

This is PR adds the skeleton for a metadata verifier for Amber provenances (AmberProvenanceMetadataVerifier) and calls it in ProvenanceBuildWrapper. This does not give a complete implementation for a metadata verifier, and is mainly intended to solve the problem of slow builds.

I have added a few TODO to identify the remaining work. In particular, we have to decide which fields need to be verified in the metadata verifier. The expected values should then be provided as input both to the verify command line tool, and to ProvenanceBuildWrapper, as well as the choice of the verifier (see #69).

@rbehjati rbehjati force-pushed the metadata-verifier branch 2 times, most recently from b9b1026 to a8faf69 Compare April 14, 2022 09:25
@aferr
Copy link
Contributor

aferr commented Apr 14, 2022

This looks OK. I am wondering if after this change the ProvenanceBuildWrapper is still useful, and perhaps it is best to just deprecate it. It seems like what it checks is similar to what the ProvenanceWrapper checks, so we might not need both of these.

Probably, the ProvenanceBuildWrapper should eventually be replaced with something that gets the hash from github, at which point it really would have a distinct job from ProvenanceWrapper.

We could just deprecate the current slow ProvenanceBuildWrapper until we have the github one, but the change here does solve the CI slowdown issue, so what you have here is also fine.

@rbehjati
Copy link
Contributor Author

Perhaps ProvenanceBuildWrapper and ProvenanceWrapper could be merged into a ProvenanceVerifierWrapper. But some verification is still needed, which is not done in ProvenanceWrapper.

With this change, it may seem like ProvenanceBuildWrapper is not doing any interesting verification. But as I have explained in a TODO (not exactly with this wording), ProvenanceBuildWrapper will have an instance of ProvenanceVerifier, and use that for verifying the provenance before emitting a statement.

To be more accurate, it might be better to also change hasProvenance to hasVerifiedProvenance in the auth-logic statement.

Copy link
Contributor

@mariaschett mariaschett left a comment

Choose a reason for hiding this comment

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

Thank you! :)

return fmt.Errorf("incorrect BuildType: got %s, want %v", provenance.Predicate.BuildType, common.AmberBuildTypeV1)
}

// TODO(#69): Check metadata against the expected values.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we want to indicate the todo also in the documentation of the func?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Done.

@rbehjati rbehjati merged commit 8546fc8 into project-oak:main Apr 14, 2022
@rbehjati rbehjati deleted the metadata-verifier branch May 4, 2022 10:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants