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

verify-blob function should be implemented consistently with verify image function #2222

Closed
hirokuni-kitahara opened this issue Sep 5, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@hirokuni-kitahara
Copy link
Member

Description

verify-blob is now implemented in "cmd/cosign/cli/verify" package, but this is inconsistent with verify.
The core functions of verify such as VerifyImageSignature() are implemented in "pkg/cosign", and it is easy for developers to invoke cosign's verification function from their own golang projects.
However, verify-blob does not have this type of implementation, and developers need to call VerifyBlobCmd() directly. This function requires some arguments that are filepath, and this makes it hard for developers to invoke verify-blob functions from some environment such as read-only container.

The core functions of verify-blob should be inside "pkg/cosign" as well as verify image functions, and the functions should accept non-filepath arguments.
(VerifyBlobCmd() will be just a wrapper which calls the core func in "pkg/cosign" in the new implementation.)

Example

For example, something like below in "pkg/cosign" is what I am proposing in this issue.

func VerifyBlobSignature(ctx context.Context, blobBytes, sigBytes []byte, co *CheckOpts)


# reference func in pkg/cosign/verify.go
func VerifyImageSignature(ctx context.Context, sig oci.Signature, h v1.Hash, co *CheckOpts)
@asraa
Copy link
Contributor

asraa commented Sep 6, 2022

I'll be working on one change to refactor and simplify the logic, TBA.

@asraa
Copy link
Contributor

asraa commented Sep 15, 2022

Hey! We just released the security fixes and the code is refactored (and tested). If you rebase your PR (or might be easier to redo....) I think it'll be good to go!

@hirokuni-kitahara
Copy link
Member Author

Thank you very much! @asraa
I will check the latest code and will update my PR.

@asraa asraa mentioned this issue Oct 25, 2022
3 tasks
@znewman01
Copy link
Contributor

@asraa : To what extend did #2461 address this?

@asraa
Copy link
Contributor

asraa commented Nov 30, 2022

@asraa : To what extend did #2461 address this?

I think it's totally done. The one thing required is that the interface provided here is a lot cleaner than the one currently in the pkg.

Right now it's

// VerifyBlobSignature verifies a blob signature.
func VerifyBlobSignature(ctx context.Context, sig oci.Signature, co *CheckOpts) (bundleVerified bool, err error) {
	// The hash of the artifact is unused.
	return verifyInternal(ctx, sig, v1.Hash{}, verifyOCISignature, co)
}

I could switch that over to providing something like this, that was suggested in the issue, but the problem is that it doesn't capture CertChains/Bundles/etc well.

func VerifyBlobSignature(ctx context.Context, blobBytes, sigBytes []byte, co *CheckOpts)

@znewman01
Copy link
Contributor

Happy to close that out for now; I think that clean-up will happen naturally over time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants