slsa-framework / slsa-verifier

Verify provenance from SLSA compliant builders
Apache License 2.0
226 stars 48 forks source link

Add attestation store option #724

Closed laurentsimon closed 8 months ago

laurentsimon commented 10 months ago

Following https://github.com/slsa-framework/slsa-github-generator/pull/2962, we need to expose this functionality. /cc @saisatishkarra

saisatishkarra commented 10 months ago

@laurentsimon I am looking forward to a direction to add an explicit option to the verifier (name) and code details to get started. Can you also clarify if the verifier already works with the COSIGN_REPOSITORY env variable without the explicit CLI option?

laurentsimon commented 10 months ago

I think the slsa-verifier should work with COSIGN_REPOSITORY, since we're just calling the cosign APIs. So setting the env variable in slsa-verifier should work.

Here are the places to add an --provenance-registry:

  1. https://github.com/slsa-framework/slsa-verifier/blob/main/cli/slsa-verifier/verify.go#L78 to add a CLI option
  2. Add to add a ProvenanceRegistry field
  3. Update this function https://github.com/slsa-framework/slsa-verifier/blob/main/verifiers/internal/gha/verifier.go#L245 and set the env variable with the provenance registry, if the field is set

If we can add a test, that'd be great... maybe in https://github.com/slsa-framework/slsa-verifier/blob/main/cli/slsa-verifier/main_regression_test.go. Not sure where the best place is for the test yet :)

Let me know if this makes sense or not. Thanks again for your help!

laurentsimon commented 9 months ago

See https://github.com/slsa-framework/slsa-github-generator/issues/3024#issuecomment-1863129448

saisatishkarra commented 8 months ago

@laurentsimon can you elaborate on the following:

laurentsimon commented 8 months ago

Hi

Can we use the option name as --provenance-repository instead of --provenance-registry since it is a full image repository and not to be confused with only the registry which can be ghcr.io / docker.io/xxx etc?

ah, you caught me unaware here. So you're saying that COSIGN_REPOSITORY is a full image name, I did not realize that. I thought the registry changed only. I think we'll need a --provenance-repository then, you're right. I was thinking that we could re-use provenance-path (like for blobs), but that seems confusing and feels like filesystem paths.

Should we completely deprecate using the COSIGN_REPOSITORY env and only allow --provenance-registry ?

yes.

Make input --provenance-registry as Optional field

yes

Does this answer all the questions?

Thanks again for all your contributions, really appreciated!

saisatishkarra commented 8 months ago

@laurentsimon Here is the PR https://github.com/slsa-framework/slsa-verifier/pull/736 for the added option.

I have NOT deprecated the COSIGN_REPOSITORY environment and instead took the overriding approach to prioritize user input --provenance-repository when both are simultaneously set. I felt this keeps slsa-verifier generically consistent with the default UX experience of cosign user as per their docs. LMK what you think and merge if everything looks good.

Looking forward for a release / tag with this functionality.

laurentsimon commented 8 months ago

@laurentsimon Here is the PR #736 for the added option.

I have NOT deprecated the COSIGN_REPOSITORY environment and instead took the overriding approach to prioritize user input --provenance-repository when both are simultaneously set. I felt this keeps slsa-verifier generically consistent with the default UX experience of cosign user as per their docs. LMK what you think and merge if everything looks good.

The fact that we use cosign is an implementation detail that need / should not be exposed to client. I think we should remove this option. In fact, we're in the process of moving to sigstore-go instead of cosign

--provenance-repository

That made me realize we call this option --provenance-registry in the slsa-github-generator. I think we should update the option name too? Wdut?