-
Notifications
You must be signed in to change notification settings - Fork 50
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
feat: Create interface for verifiers #187
Conversation
First question: on top of this, but the problem is that blob verification and container verification have different APIs. Container verification takes an image reference, whereas blobs require the provenance bytes. Either we have some sort of struct with type information capturing the object to verify, or we have two possibilities for each builder |
We can create a different API, as you say: VerifyImage(), VerifyArtifact() for the interface. Would that work? I'm thinking of having a function called HasSupport(branch | tag | trigger | etc), as well. We can discuss this further in a follow-up PR, though. |
That SGTM In addition, can we expose a SLSA Level |
Yes we can. Level 3 is a little subjective, so I was hesitant to do that. But maybe that's a good way to get the ball rolling. |
@@ -41,21 +44,25 @@ func main() { | |||
os.Exit(1) | |||
} | |||
|
|||
var ptag, pversiontag *string | |||
var pbuilderID, ptag, pversiontag *string | |||
|
|||
if isFlagPassed("tag") { | |||
ptag = &tag | |||
} | |||
if isFlagPassed("versioned-tag") { |
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.
comment from @ianlewis:
aside: I'm curious. Remind me again why these flags are copied to local variables? Also, why we need a special isFlagPassed function?
reply from @laurentsimon
I don't think I found a way to have an option be a pointer to a string, and for the pointer to be nil if the user does not provide the option. If that's possible, I can update the code.
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.
Yeah, I believe StringVar
is as close as you can get, and the best that can be done is use a default empty string. but problem is that empty string doesn't necessarily mean no value.
In this case we can document that? empty tags passed in mean no tag verification.
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.
done. Added a comment.
"github.com/slsa-framework/slsa-verifier/verifiers/internal/gha" | ||
) | ||
|
||
func Verify(ctx context.Context, |
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.
TODO: support VerifyWorkflowIdentity(workflow, nil, source)
/cc @ianlewis
trustedBuilderRepository + "/.github/workflows/generator_generic_slsa3.yml": true, | ||
trustedBuilderRepository + "/.github/workflows/builder_go_slsa3.yml": true, | ||
} | ||
|
||
// VerifyWorkflowIdentity verifies the signing certificate information | ||
func VerifyWorkflowIdentity(id *WorkflowIdentity, source string) error { | ||
func VerifyWorkflowIdentity(id *WorkflowIdentity, builderOpts *options.BuilderOpts, source string) (string, error) { |
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.
TBD but we may want to move this under the Verifier type and have the signature take in the PublicKeyOrCertificate.
In the case of GCB/other keys, they will have different "identity" guarantees in the public key or cert.
Fine for now since this is just a refactor. It won't break more API to add a new method to the Verifier type
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.
Let me try to make a small change move this function under builderOpt.
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.
mhh, will leave as-is for now since VerifyWorkflowIdentity()
is called within the main entry point VerifyArtifact()
, so there's no need to expose it directly in this PR. Can refactor more in follow-up PRs
* Combine common checks * fix buildType check * move e2e_this_file * Add environment checks
This PR supersedes #184.
This PR attempts to create a better framework to support new builders. It starts simple:
register/register.go
:register.RegisterVerifier()
: seeverifiers/internal/gcb/verifier.go
andverifiers/internal/gha/verifier.go
.v.Match(builderID)
. Seeverifiers/verifier.go
verifiers/verifier.go
. I'm doing this to avoid breaking previous version, but we can change this behavior later. We could also check whether builder supports particular repositories: GCB supports GitLab, whereas GHA builders do not.I also moved errors to their own
errors
folder.Everything else is moving files around and renaming calls to account for files that have been moved around.