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: add support for rsapkcs15 keys #851

Merged
merged 1 commit into from
Oct 15, 2021
Merged

Conversation

rchincha
Copy link
Contributor

@rchincha rchincha commented Oct 6, 2021

currently cosign only supports ecdsa keys for signature verification.

Signed-off-by: Ramkumar Chinchani [email protected]

@dlorenc
Copy link
Member

dlorenc commented Oct 6, 2021

Thanks!

@dlorenc
Copy link
Member

dlorenc commented Oct 6, 2021

I think this is safe to add on verify, but I'm still not sure adding direct support for customizing algorithms on key-generation and signatures is a great idea.

@rchincha
Copy link
Contributor Author

rchincha commented Oct 6, 2021

I think this is safe to add on verify, but I'm still not sure adding direct support for customizing algorithms on key-generation and signatures is a great idea.

ecdsa-sha256 and rsa-sha256 on verify-side would cover most use cases, since signing keys are likely to be those, possibly in a vault or some external tool. That said, would wait till I can add a few test cases.

@dekkagaijin
Copy link
Member

dekkagaijin commented Oct 6, 2021

As Dan said, unless there's a concrete use-case that's being blocked by limiting support to ECDSA-SHA256 in cosign itself, we err on the side of limiting complexity (and therefore choices).

@rchincha
Copy link
Contributor Author

rchincha commented Oct 6, 2021

As Dan said, unless there's a concrete use-case that's being blocked by limiting support to ECDSA-SHA256 in cosign itself, we err on the side of limiting complexity (and therefore choices).

That the signing keys are RSA-2k? And cosign is not the one signing? [1]

[1] https://github.com/sigstore/cosign/blob/main/EXAMPLES.md

@dekkagaijin
Copy link
Member

The examples also mention the way to download signatures and verify with a separate tool, e.g.:

$ cosign download signature us-central1-docker.pkg.dev/dlorenc-vmtest2/test/taskrun > signatures.json
# There could be multiple signatures, let's pretend it's the last one.
# Extract the payload and signature, base64 decoding them.
$ cat signatures.json | tail -1 | jq -r .Payload | base64 -D > payload.txt
$ cat signatures.json | tail -1 | jq -r .Base64Signature | base64 -D > signature.txt
# Verify in openssl
$ openssl dgst -sha256 -verify pubkey.pem -signature signature.txt payload.txt

@rchincha
Copy link
Contributor Author

rchincha commented Oct 6, 2021

The examples also mention the way to download signatures and verify with a separate tool, e.g.:

$ cosign download signature us-central1-docker.pkg.dev/dlorenc-vmtest2/test/taskrun > signatures.json
# There could be multiple signatures, let's pretend it's the last one.
# Extract the payload and signature, base64 decoding them.
$ cat signatures.json | tail -1 | jq -r .Payload | base64 -D > payload.txt
$ cat signatures.json | tail -1 | jq -r .Base64Signature | base64 -D > signature.txt
# Verify in openssl
$ openssl dgst -sha256 -verify pubkey.pem -signature signature.txt payload.txt

Sure, in which case the role cosign plays is rather limited - push and pull the signature layer. If so, that's fine, good to know.

@dlorenc
Copy link
Member

dlorenc commented Oct 6, 2021

Sorry - yes this is fine in verify!

@@ -105,6 +106,18 @@ func PemToECDSAKey(pemBytes []byte) (*ecdsa.PublicKey, error) {
return ecdsaPub, nil
}

func PemToRSAKey(pemBytes []byte) (*rsa.PublicKey, error) {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of merging these functions so we can do a type assertion switch instead of trying them in order as error fallbacks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at top of sigstore/sigstore project for helper apis. Just thinking about this some more, to support a full verify list, more hash funcs may need to be checked against if done "automatically" without cmdline arg hints. For example, cosign assumes sha256, but sha512 is another possibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR using sigstore/sigstore helpers.
Caveat is we (continue to) assume SHA256.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the SHA256 assumption is a bit inherent to cosign's usage with registries. OCI registries are currently very coupled to SHA256, so this isn't too high of a priority to solve here.

@rchincha rchincha force-pushed the rsa-verify branch 3 times, most recently from 3b8c2c4 to 86dc942 Compare October 6, 2021 17:46
Signed-off-by: Ramkumar Chinchani <[email protected]>
@dlorenc dlorenc merged commit c041930 into sigstore:main Oct 15, 2021
@github-actions github-actions bot added this to the v1.3.0 milestone Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants