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

Fix BYO-root with intermediate to fetch intermediates from annotation #2244

Merged
merged 1 commit into from Sep 20, 2022

Conversation

haydentherapper
Copy link
Contributor

This fixes a regression where intermediates, when not present in the intermediate certificate pool, would be fetched from the OCI chain annotation.

Ref #1554, which includes more details

Signed-off-by: Hayden Blauzvern hblauzvern@google.com

Summary

Release Note

Fixed bug where intermediate certificates were not automatically read from the OCI chain annotation

Documentation

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2022

Codecov Report

Merging #2244 (7ad31ee) into main (c1322bc) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2244      +/-   ##
==========================================
+ Coverage   28.57%   28.60%   +0.02%     
==========================================
  Files         131      131              
  Lines        7852     7855       +3     
==========================================
+ Hits         2244     2247       +3     
  Misses       5302     5302              
  Partials      306      306              
Impacted Files Coverage Δ
...ernal/pkg/cosign/fulcio/fulcioroots/fulcioroots.go 57.77% <100.00%> (+3.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gmarks-ntap
Copy link

I tried these changes with my sample workflow. Unfortunately I now get a panic when attempting to verify my test image. I'm not sure I fully described my test scenario, so that might help to know those details.

// environment description

  • I generated an RSA 3072 private key with openssl
  • I created a CSR from this private key
  • I signed this CSR with a fake CA that I created for testing (I created a root and an intermediate CA and used the intermediate CA to sign the cert)
  • I imported my openssl generated key to cosign (cosign import-key-pair ...)
  • I signed my image using the imported key and attached my cert and cert-chain files during signing
    cosign sign --key import-cosign.key --cert cosign-client.cert.pem --cert-chain ca-chain.cert.pem <image>

// Using cosign without these changes the verify fails as previously described
COSIGN_EXPERIMENTAL=1 cosign verify <image>
Error: no matching signatures:
x509: certificate signed by unknown authority
main.go:52: error during command execution: no matching signatures:
x509: certificate signed by unknown authority

// When testing with cosign built from this PR I get the following panic

panic: interface conversion: interface {} is *rsa.PublicKey, not *ecdsa.PublicKey

goroutine 1 [running]:
github.com/sigstore/cosign/pkg/cosign.validateAndUnpackCert(0xc0009d2e11?, 0x6?)
        /root/develop/github/hayden-cosign/pkg/cosign/verify.go:140 +0x1d0
github.com/sigstore/cosign/pkg/cosign.verifySignatures.func1(0xc0009b3ad0, {0x2abb810, 0xc000128000}, {{0x25a894d?, 0x7ffdfda7b5ff?}, {0xc00055c740?, 0xc000c60400?}}, 0xc0009b3817, {0x2aca290, 0xc0009f6fc0})
        /root/develop/github/hayden-cosign/pkg/cosign/verify.go:318 +0x9f
github.com/sigstore/cosign/pkg/cosign.verifySignatures({0x2abb810, 0xc000128000}, {0x2aca650?, 0xc0009c5b90?}, {{0x25a894d?, 0x1b7c500?}, {0xc00055c740?, 0x1a?}}, 0x7ffdfda7b5ff?)
        /root/develop/github/hayden-cosign/pkg/cosign/verify.go:353 +0x252
github.com/sigstore/cosign/pkg/cosign.VerifyImageSignatures({0x2abb810, 0xc000128000}, {0x2abf480, 0xc00090a9b0}, 0xc0009b3ad0)
        /root/develop/github/hayden-cosign/pkg/cosign/verify.go:249 +0x13c
github.com/sigstore/cosign/cmd/cosign/cli/verify.(*VerifyCommand).Exec(0xc0009b3ba0, {0x2abb810, 0xc000128000}, {0xc00053ebc0, 0x1, 0x4b003d?})
        /root/develop/github/hayden-cosign/cmd/cosign/cli/verify/verify.go:165 +0x832
github.com/sigstore/cosign/cmd/cosign/cli.Verify.func1(0xc000952a00, {0xc00053ebc0, 0x1, 0x1})
        /root/develop/github/hayden-cosign/cmd/cosign/cli/verify.go:106 +0x24b
github.com/spf13/cobra.(*Command).execute(0xc000952a00, {0xc00053eb80, 0x1, 0x1})
        /root/go/pkg/mod/github.com/spf13/cobra@v1.3.0/command.go:856 +0x67c
github.com/spf13/cobra.(*Command).ExecuteC(0xc000430000)
        /root/go/pkg/mod/github.com/spf13/cobra@v1.3.0/command.go:974 +0x3bd
github.com/spf13/cobra.(*Command).Execute(...)
        /root/go/pkg/mod/github.com/spf13/cobra@v1.3.0/command.go:902
main.main()
        /root/develop/github/hayden-cosign/cmd/cosign/main.go:45 +0x4c```

@haydentherapper
Copy link
Contributor Author

What version of Cosign are you on? We've made some changes recently that should add support for more key types.

@gmarks-ntap
Copy link

What version of Cosign are you on? We've made some changes recently that should add support for more key types.

The original cosign version was v1.9.0. That's what I used to sign with.

The version that got the panic I built from cloning your fork. But I just realized that I didn't switch branches before building so I didn't build the right code. I just switched to the fix-byo-root branch and rebuilt. Now I don't get the panic, I do get the tuf warning messages, but it still appears to error.

This is with a build of cosign from your branch

... many more warning lines removed
tuf: warning using deprecated ecdsa hex-encoded keys
tuf: warning using deprecated ecdsa hex-encoded keys
Error: no matching signatures:
x509: certificate signed by unknown authority
main.go:62: error during command execution: no matching signatures:
x509: certificate signed by unknown authority

@haydentherapper
Copy link
Contributor Author

The warnings can be ignored, we're fixing that in a downstream library now.

Lemme see if I can replicate this issue with the commands you specified. To confirm, you're passing no other flags to verify correct? This would mean that Cosign pulls the certificate/chain from the OCI annotations (which should have been set on sign)

@gmarks-ntap
Copy link

To confirm, you're passing no other flags to verify correct?

Correct, no other flags are used.

@znewman01
Copy link
Contributor

  • I generated an RSA 3072 private key with openssl
  • I created a CSR from this private key
  • I signed this CSR with a fake CA that I created for testing (I created a root and an intermediate CA and used the intermediate CA to sign the cert)
  • I imported my openssl generated key to cosign (cosign import-key-pair ...)
  • I signed my image using the imported key and attached my cert and cert-chain files during signing
    cosign sign --key import-cosign.key --cert cosign-client.cert.pem --cert-chain ca-chain.cert.pem <image>
    [...]
    This is with a build of cosign from your branch
cosign verify
[...]
x509: certificate signed by unknown authority

Have you set SIGSTORE_ROOT_FILE? If not, Cosign doesn't know about the CA, so when it fetches the cert and chain embedded in the OCI registry, it has no trust root to verify against. (Sorry if that's been established in some other context and I'm making you repeat yourself.)

Copy link
Contributor

@znewman01 znewman01 left a comment

Choose a reason for hiding this comment

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

It might be nice to have a test for the original issue, too. I need to be convinced that making the intermediatePool to be nil solves the problem 😄

@haydentherapper
Copy link
Contributor Author

@znewman01 I'll add more tests!

@gmarks-ntap Without reproducing this yet, I'm pretty certain Zach is correct, that the issue is not specifying SIGSTORE_ROOT_FILE. When you're not providing any additional flags like certificate-chain, and you've provided the experimental flag, Cosign will fetch the Fulcio verification chain from the TUF root. To override this, you either need to create your own TUF root (see our blog) or use SIGSTORE_ROOT_FILE, which should point to a file with your PEM-encoded root.

@gmarks-ntap
Copy link

When I try to verify using COSIGN_EXPERIMENTAL=1 SIGSTORE_ROOT_FILE=... I get the following error.

Error: no matching signatures:
signature not found in transparency log
main.go:62: error during command execution: no matching signatures:
signature not found in transparency log

I get the same results whether SIGSTORE_ROOT_FILE is set to the CA root certificate only or a chain file with my intermediate cert first followed by the root CA cert.

I haven't yet tried setting up my own TUF root. The blog looks like a good guide, but I will hold of on trying that unless you think that really will provide useful information beyond using SIGSTORE_ROOT_FILE env variable.

@haydentherapper
Copy link
Contributor Author

This is likely because the experimental flag wasn’t set on signing, so no entry was created in the transparency log. if you don’t set it on verify (or you do set it on sign and verify), it should work

This fixes a regression where intermediates, when not present in the
intermediate certificate pool, would be fetched from the OCI chain
annotation.

Ref sigstore#1554, which includes more details

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@haydentherapper
Copy link
Contributor Author

@znewman01 Was there another test you wanted here? I think this test should be testing the original issue, where the specified root file is missing an intermediate

@gmarks-ntap
Copy link

This is likely because the experimental flag wasn’t set on signing, so no entry was created in the transparency log. if you don’t set it on verify (or you do set it on sign and verify), it should work

Yep. I pushed a new image and signed it while using the experimental env variable. Once I did that I'm able to validate my image with the 'attached' cert and cert-chain using the verify command without specifying any parameters to the cosign verify command.

Interestingly, I can verify with cosign version 1.9.0 as well as the cosign built from this branch. So it appears, at least for my case, my issue was really just that I didn't use experimental mode when signing.

Additionally, I tried setting SIGSTORE_ROOT_FILE to both by root CA certificate as well as my CA chain file. Cosign was able to validate the image with either file specified, with both cosign versions I tried. I'm not sure if that is expected or not but just some extra data.

@haydentherapper
Copy link
Contributor Author

Additionally, I tried setting SIGSTORE_ROOT_FILE to both by root CA certificate as well as my CA chain file

This is working as intended. If you don't specify the intermediates in SIGSTORE_ROOT_FILE, they are automatically fetched from the annotation. There was a regression between 1.9 and this fix that broke this behavior.

@dlorenc dlorenc merged commit 63fb755 into sigstore:main Sep 20, 2022
@github-actions github-actions bot added this to the v1.13.0 milestone Sep 20, 2022
@haydentherapper haydentherapper deleted the fix-byo-root branch September 20, 2022 01:10
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.

None yet

5 participants