-
Notifications
You must be signed in to change notification settings - Fork 40
chore(signing): verify signature on att push #2606
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
Conversation
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
migmartri
left a comment
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.
Awesome
| } | ||
|
|
||
| // verify attestation (only if chainloop is the signer) | ||
| result, err := uc.verifyBundle(ctx, rawContent) |
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.
where is the logic that makes sure the bundle is verified when keyless signing was used?
|
|
||
| func (uc *WorkflowRunUseCase) Verify(ctx context.Context, run *WorkflowRun) (*VerificationResult, error) { | ||
| func (uc *WorkflowRunUseCase) VerifyRun(ctx context.Context, run *WorkflowRun) (*VerificationResult, error) { | ||
| return uc.verifyBundle(ctx, run.Attestation.Bundle) |
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.
is there any case where run.Attestation would be empty?
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.
it seems it will not, makes sense.
| } | ||
|
|
||
| // if it's verifiable, make sure it passed | ||
| if result != nil && !result.Result { |
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.
oh, so result will be nil if can't be verified? I'd make it more explicit somehow
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.
yes in that case it would return nil, nil. I agree it should return something more explicit.
This change ensures that the attestation bundle is verified on attestation push, failing if the signature is not recognized (only when chainloop is the signer)