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.

Sign up for GitHub

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

Jump to bottom

fix: fix cert chain validation for verify-blob in non-experimental mode #2256

Merged
merged 5 commits into from Sep 16, 2022
Merged

fix: fix cert chain validation for verify-blob in non-experimental mode #2256

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7f50741
fix: fix cert chain validation
asraa Sep 15, 2022
e641243
one more test
asraa Sep 15, 2022
1405355
add one more test
asraa Sep 15, 2022
0a2e6e7
address comments
asraa Sep 15, 2022
4f1ae75
update
asraa Sep 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 20 additions & 2 deletions cmd/cosign/cli/verify/verify_blob.go
View file
Open in desktop
Expand Up @@ -107,7 +107,7 @@ func VerifyBlobCmd(ctx context.Context, ko options.KeyOpts, certRef, certEmail,
co.RekorClient = rekorClient
}
}
if certRef == "" || options.EnableExperimental() {
if options.EnableExperimental() {
co.RootCerts, err = fulcio.GetRoots()
if err != nil {
return fmt.Errorf("getting Fulcio roots: %w", err)
Expand Down Expand Up @@ -145,6 +145,14 @@ func VerifyBlobCmd(ctx context.Context, ko options.KeyOpts, certRef, certEmail,
return err
}
if certChain == "" {
co.RootCerts, err = fulcio.GetRoots()
asraa marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return fmt.Errorf("getting Fulcio roots: %w", err)
}
co.IntermediateCerts, err = fulcio.GetIntermediates()
if err != nil {
return fmt.Errorf("getting Fulcio intermediates: %w", err)
}
co.SigVerifier, err = cosign.ValidateAndUnpackCert(cert, co)
if err != nil {
return err
Expand Down Expand Up @@ -178,10 +186,20 @@ func VerifyBlobCmd(ctx context.Context, ko options.KeyOpts, certRef, certEmail,
// check if cert is actually a public key
co.SigVerifier, err = sigs.LoadPublicKeyRaw(certBytes, crypto.SHA256)
} else {
if certChain == "" {
co.RootCerts, err = fulcio.GetRoots()
if err != nil {
return fmt.Errorf("getting Fulcio roots: %w", err)
}
co.IntermediateCerts, err = fulcio.GetIntermediates()
if err != nil {
return fmt.Errorf("getting Fulcio intermediates: %w", err)
}
}
co.SigVerifier, err = cosign.ValidateAndUnpackCert(cert, co)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we handle else for when the cert chain is set? Otherwise, the certificate wouldn't validate if it's been signed by a non-fulcio pki

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lemme see if I can also add a test here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm...I think it's pretty unexpected that --certificate-chain would mean "accept whatever cert was at the root of the chain."

This is part of why I think an explicit --certificate-root flag would be a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do document the the chain is the intermediates and root -

cosign/cmd/cosign/cli/options/certificate.go

Lines 67 to 70 in 04f509c

"path to a list of CA certificates in PEM format which will be needed "+
"when building the certificate chain for the signing certificate. "+
"Must start with the parent intermediate CA certificate of the "+
"signing certificate and end with the root certificate")

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, let's. It's a bit unclear to me what should be the right behavior here, especially if I provide both a sigstore root and certificate chain.

Copy link
Contributor

Choose a reason for hiding this comment

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

The behavior here should match 1.11.x (the more significant change is around checking expiration)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If someone could help me update the release notes to close this out, that'd be great. I added one in. I'm not sure if anything breaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm I don't see anything under "Release Notes" in the PR description.

Maybe:

The v1.12.0 release introduced a regression: when COSIGN_EXPERIMENTAL was not set, cosign verify-blob would check a --certificate (without a --certificate-chain provided) against the operating system root CA bundle. In this release, Cosign checks the certificate against Fulcio's CA root instead (restoring the earlier behavior).

I think we should have a summary of the current behavior (as opposed to a diff, in the release note) somewhere too; maybe that's what we can hash out in #2254

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I put it in the summary instead. Moved, but feel free to edit.

Sg, I'll add a longer explanation

}
if err != nil {
return err
return fmt.Errorf("loading verifier from bundle: %w", err)
}
bundle = b.Bundle
// No certificate is provided: search by artifact sha in the TLOG.
Expand Down
124 changes: 122 additions & 2 deletions cmd/cosign/cli/verify/verify_blob_test.go
View file
Open in desktop
Expand Up @@ -434,7 +434,6 @@ func TestVerifyBlob(t *testing.T) {
experimental: false,
shouldErr: true,
},

{
name: "valid signature with expired certificate - experimental good rekor lookup",
blob: blobBytes,
Expand All @@ -446,7 +445,6 @@ func TestVerifyBlob(t *testing.T) {
expiredLeafPem, true),
shouldErr: false,
},

{
name: "valid signature with expired certificate - experimental bad rekor integrated time",
blob: blobBytes,
Expand Down Expand Up @@ -957,6 +955,128 @@ func TestVerifyBlobCmdWithBundle(t *testing.T) {
t.Fatalf("expected error with mismatched issuer, got %v", err)
}
})
t.Run("Intermediate root not explicit in non-experimental mode", func(t *testing.T) {
asraa marked this conversation as resolved.
Show resolved Hide resolved
identity := "hello@foo.com"
issuer := "issuer"
leafCert, _, leafPemCert, signer := keyless.genLeafCert(t, identity, issuer)

// Create blob
blob := "someblob"

// Sign blob with private key
sig, err := signer.SignMessage(bytes.NewReader([]byte(blob)))
if err != nil {
t.Fatal(err)
}

// Create bundle
entry := genRekorEntry(t, hashedrekord.KIND, hashedrekord.New().DefaultVersion(), []byte(blob), leafPemCert, sig)
b := createBundle(t, sig, leafPemCert, keyless.rekorLogID, leafCert.NotBefore.Unix()+1, entry)
b.Bundle.SignedEntryTimestamp = keyless.rekorSignPayload(t, b.Bundle.Payload)
bundlePath := writeBundleFile(t, keyless.td, b, "bundle.json")
blobPath := writeBlobFile(t, keyless.td, blob, "blob.txt")
certPath := writeBlobFile(t, keyless.td, string(leafPemCert), "cert.pem")

// Verify command
err = VerifyBlobCmd(context.Background(),
options.KeyOpts{BundlePath: bundlePath},
certPath, /*certRef*/
identity, /*certEmail*/
issuer, /*certOidcIssuer*/
"", /*certChain*/ // Chain is fetched from TUF/SIGSTORE_ROOT_FILE
"", /*sigRef*/ // Sig is fetched from bundle
blobPath, /*blobRef*/
// GitHub identity flags start
"", "", "", "", "",
// GitHub identity flags end
false /*enforceSCT*/)
if err != nil {
t.Fatalf("expected success without specifying the intermediates, got %v", err)
}
})
}

func TestVerifyBlobCmdInvalidRootCA(t *testing.T) {
keyless := newKeylessStack(t)
// Change the keyless stack.
_ = newKeylessStack(t)
t.Run("Invalid certificate root explicit certRef", func(t *testing.T) {
identity := "hello@foo.com"
issuer := "issuer"
leafCert, _, leafPemCert, signer := keyless.genLeafCert(t, identity, issuer)

// Create blob
blob := "someblob"

// Sign blob with private key
sig, err := signer.SignMessage(bytes.NewReader([]byte(blob)))
if err != nil {
t.Fatal(err)
}

// Create bundle
entry := genRekorEntry(t, hashedrekord.KIND, hashedrekord.New().DefaultVersion(), []byte(blob), leafPemCert, sig)
b := createBundle(t, sig, leafPemCert, keyless.rekorLogID, leafCert.NotBefore.Unix()+1, entry)
b.Bundle.SignedEntryTimestamp = keyless.rekorSignPayload(t, b.Bundle.Payload)
bundlePath := writeBundleFile(t, keyless.td, b, "bundle.json")
blobPath := writeBlobFile(t, keyless.td, blob, "blob.txt")
certPath := writeBlobFile(t, keyless.td, string(leafPemCert), "cert.pem")

// Verify command
err = VerifyBlobCmd(context.Background(),
options.KeyOpts{BundlePath: bundlePath},
certPath, /*certRef*/
identity, /*certEmail*/
issuer, /*certOidcIssuer*/
"", /*certChain*/ // Chain is fetched from TUF/SIGSTORE_ROOT_FILE
"", /*sigRef*/ // Sig is fetched from bundle
blobPath, /*blobRef*/
// GitHub identity flags start
"", "", "", "", "",
// GitHub identity flags end
false /*enforceSCT*/)
if err == nil || !strings.Contains(err.Error(), "certificate signed by unknown authority") {
t.Fatalf("expected error with invalid root CA, got %v", err)
}
})
t.Run("Invalid certificate root explicit bundle", func(t *testing.T) {
asraa marked this conversation as resolved.
Show resolved Hide resolved
identity := "hello@foo.com"
issuer := "issuer"
leafCert, _, leafPemCert, signer := keyless.genLeafCert(t, identity, issuer)

// Create blob
blob := "someblob"

// Sign blob with private key
sig, err := signer.SignMessage(bytes.NewReader([]byte(blob)))
if err != nil {
t.Fatal(err)
}

// Create bundle
entry := genRekorEntry(t, hashedrekord.KIND, hashedrekord.New().DefaultVersion(), []byte(blob), leafPemCert, sig)
b := createBundle(t, sig, leafPemCert, keyless.rekorLogID, leafCert.NotBefore.Unix()+1, entry)
b.Bundle.SignedEntryTimestamp = keyless.rekorSignPayload(t, b.Bundle.Payload)
bundlePath := writeBundleFile(t, keyless.td, b, "bundle.json")
blobPath := writeBlobFile(t, keyless.td, blob, "blob.txt")

// Verify command
err = VerifyBlobCmd(context.Background(),
options.KeyOpts{BundlePath: bundlePath},
"", /*certRef*/ // Fetched from bundle
identity, /*certEmail*/
issuer, /*certOidcIssuer*/
"", /*certChain*/ // Chain is fetched from TUF/SIGSTORE_ROOT_FILE
"", /*sigRef*/ // Sig is fetched from bundle
blobPath, /*blobRef*/
// GitHub identity flags start
"", "", "", "", "",
// GitHub identity flags end
false /*enforceSCT*/)
if err == nil || !strings.Contains(err.Error(), "certificate signed by unknown authority") {
t.Fatalf("expected error with invalid root CA, got %v", err)
}
})
}

type keylessStack struct {
Expand Down