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

refactor: share logic between verify-blob, verify, and verify-attestation #2461

Merged
merged 5 commits into from
Nov 18, 2022

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Nov 15, 2022

Signed-off-by: Asra Ali asraa@google.com

  • Cleanup: Unifies the logic around blob verification and OCI
  • Cleanup: Updates the OCI logic to check for the timestamp using the bundle (@hectorj2f you can also add a condition to replace validityTime with the TSA)
  • Security: Fixes an issue at HEAD: with COSIGN_EXPERIMENTAL removed, OCI validation succeeds when there is no bundle provided in offline mode, EVEN IF the certificate was expired (basically we skip the cert expiration check if there is no call to Rekor or a bundle).

Follow-ups:

  • Fix the security issue when using TSA

Summary

Release Note

Documentation

pkg/cosign/verify.go Outdated Show resolved Hide resolved
pkg/cosign/verify.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Going to just move the blob verifies to the package. That will further simplify.

@hectorj2f
Copy link
Contributor

@asraa I opened a PR adding support for the blob verification for TSA and/or Rekor here: #2464

@asraa
Copy link
Contributor Author

asraa commented Nov 16, 2022

@asraa I opened a PR adding support for the blob verification for TSA and/or Rekor here: #2464

The signing makes sense, but can we use this refactored shared logic for verification?

@hectorj2f
Copy link
Contributor

These changes look good to me.

but can we use this refactored shared logic for verification?

This depends on our choice for blob signing. If we choose to generate two bundles. How do we expect the verification to work ?

@asraa
Copy link
Contributor Author

asraa commented Nov 16, 2022

If we choose to generate two bundles. How do we expect the verification to work ?

I think the same as OCI. Here's a flow:

  1. Verify the cryptographic signature on the artifact using the public key verifier
  2. If a Rekor bundle is provided or required, verify it and extract the integrated time.
  3. If a TSA bundle is provided or requried, verify it and extract the TSA time. This will take precedence over the Rekor time (since it's verifiable)
  4. If a certificate is used, check the certificate against the time (either Rekor or TSA, but TSA if both are provided)

@hectorj2f
Copy link
Contributor

Okay, let's get these changes in main.

@asraa
Copy link
Contributor Author

asraa commented Nov 16, 2022

Just updated to fix the tests. I had to do two things:

  • OCI does not compare certificates/pubkeys in the bundle, so our blob tests doing that with a tampered bundle don't fail as expected
  • Another tests fails because of verification doesn't REQUIRE a TLOG check if it's not needed (i.e. for public keys). I can change that. Please let me know.

@asraa asraa marked this pull request as ready for review November 16, 2022 17:42
@hectorj2f
Copy link
Contributor

@asraa Please, rebase your branch. I have added some changes to use a flag skip-tlog-verify as proposed. It also supports both validations tsa and rekor. Let me know if you find any issue so I can help.

@asraa
Copy link
Contributor Author

asraa commented Nov 16, 2022

SG, just fixed the public key comparisons for OCI as well.

Copy link
Contributor Author

@asraa asraa left a comment

Choose a reason for hiding this comment

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

@hectorj2f @priyawadhwa @znewman01 @kpk47 i would appreciate a review!

Some of the verification logic is now clobbered with a few PRs merged into main, and I will put out a follow-up & more tests (particularly: if you skip TLOG verify and use a TSA you also end up skipping the short-lived expiry validation now)

@asraa asraa changed the title draft: share logic between blob verification & OCI refactor: share logic between verify-blob, verify, and verify-attestation Nov 16, 2022
signature string
key []byte
cert *x509.Certificate
bundlePath string
// If online lookups to Rekor are enabled
experimental bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of these tests set the experimental flag, but we've begun to remove it - should we remove it from the tests too? (Separate refactor prolly)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll add in a separate PR: it actually does nothing now :)

hectorj2f
hectorj2f previously approved these changes Nov 16, 2022
Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

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

LGTM, we have simplified the verification logic a lot.

Signed-off-by: Asra Ali <asraa@google.com>

fix all remaining issues

Signed-off-by: Asra Ali <asraa@google.com>

fix all tests

Signed-off-by: Asra Ali <asraa@google.com>

fix lint

Signed-off-by: Asra Ali <asraa@google.com>

fix lint

Signed-off-by: Asra Ali <asraa@google.com>

lint

Signed-off-by: Asra Ali <asraa@google.com>

fix test

Signed-off-by: Asra Ali <asraa@google.com>
@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2022

Codecov Report

Merging #2461 (c21521d) into main (ba5997b) will decrease coverage by 0.64%.
The diff coverage is 35.77%.

@@            Coverage Diff             @@
##             main    #2461      +/-   ##
==========================================
- Coverage   30.96%   30.32%   -0.65%     
==========================================
  Files         139      139              
  Lines        8635     8475     -160     
==========================================
- Hits         2674     2570     -104     
+ Misses       5600     5552      -48     
+ Partials      361      353       -8     
Impacted Files Coverage Δ
cmd/cosign/cli/options/verify.go 0.00% <0.00%> (ø)
cmd/cosign/cli/sign/sign.go 15.46% <0.00%> (ø)
cmd/cosign/cli/verify.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify/verify.go 19.48% <0.00%> (+1.33%) ⬆️
pkg/cosign/verify.go 36.20% <28.36%> (+0.09%) ⬆️
cmd/cosign/cli/verify/verify_blob.go 50.77% <55.88%> (-2.13%) ⬇️
cmd/cosign/cli/sign.go 0.00% <0.00%> (ø)

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

Signed-off-by: Asra Ali <asraa@google.com>

fix

Signed-off-by: Asra Ali <asraa@google.com>
hectorj2f
hectorj2f previously approved these changes Nov 16, 2022
pkg/cosign/verify.go Show resolved Hide resolved
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.

(I owe you a more detailed pass, just a couple things that jumped out at first)


// Require a certificate/key OR a local bundle file that has the cert.
if options.NOf(c.KeyRef, c.CertRef, c.Sk, c.BundlePath) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be != 1?

Oh, I guess not because BundlePath isn't really a full "bundle" yet. That's annoying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeahhhh. This is super annoying because in sign-blob, you can produce a bundle path (to store the sig) INDEPENDENT of the tlog upload. If you don't upload to the tlog, then the bundle will ONLY contain the signature (and NOT the public key).

This was tested in the e2e tests... so I had to modify this. Basically: in some cases you legit pass in a --key AND a --bundle that stores the signature.

cmd/cosign/cli/verify/verify_blob.go Outdated Show resolved Hide resolved
if co.RootCerts == nil {
co.RootCerts = x509.NewCertPool()
}
co.RootCerts.AddCert(chain[len(chain)-1])
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that if we pass a chain, we automatically trust it?

That feels wrong to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Before the refactoring, this was a way to pass in a chain as an alternative to fetching from the TUF root. Without a chain, we'd default to the TUF metadata, otherwise we'd use the chain passed in.

One bit of feedback is there's a lot of places we're checking chain != nil or c.CertChain != "", so it's a little hard to quickly see exactly where we're getting the trust root from.

Copy link
Contributor

@haydentherapper haydentherapper Nov 17, 2022

Choose a reason for hiding this comment

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

To Zack's point, there are three ways you could pass a root in (this is also all applicable to intermediates):

  • SIGSTORE_ROOT_FILE, which will get read in in fulcio.GetRoots
  • tuf (either the sigstore tuf root or a self-hosted one), read in in fulcio.GetRoots
  • Passing in the cert chain here

I'm not opposed if we want to force the user to set SIGSTORE_ROOT_FILE, as this supports both roots and intermediates (added in #1749). I don't recall why this was added this way - likely just a product of iterative development and some overlapping features haha.

That said, I'm also not opposed to supporting the chain flag too. It really is no different than SIGSTORE_ROOT_FILE, but if it makes things easier, we could drop it. Just wanna make sure we don't lose any supported features.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah: this is because in the BLOB case you pass in the cert chain, so you DO trust it "out of band".
In the OCI case, the cert chain is provided on the registry. This was the only way I could get parity on the behavior. the cert chain passed here does represent the full trust chain.

Copy link
Contributor Author

@asraa asraa Nov 17, 2022

Choose a reason for hiding this comment

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

One bit of feedback is there's a lot of places we're checking chain != nil or c.CertChain != "", so it's a little hard to quickly see exactly where we're getting the trust root from.

Just realized you mentioned this so I went and checked. There's only one place that c.CertChain is used, and that's to populated chain -- after that everything is populated from chain. Cleaned that up.

If we treat --cert-chain as a separate option rather than checking when --certificate is provided OR if --bundle is provided, I just check independently once. Then I add as the annotation when a certificate is used (whether populated from --certificate or --bundle)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LMK if there's anything else here, or we can resolve.

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed #2472 as a follow up

}
}
opts = append(opts, static.WithCertChain(certPEM, chainPEM))
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if chain is not set, in the case of using the default TUF chain here? chainPEM will not be set, is that an issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, in

if len(chain) > 0 {
, it's fine if the chain isn't set because we set the checkopts later. But TBH, I forget why this code was there, to read in the intermediates from the chain

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the purpose was that the roots were loading in earlier, and if you have an intermediate in your chain, we'd automatically populate an intermediate pool. Had to review 7402eb4 where I added this

Copy link
Contributor

@haydentherapper haydentherapper Nov 17, 2022

Choose a reason for hiding this comment

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

I bet this was added originally because we didn't have the intermediate in the TUF root, so the idea was that we'd fetch the intermediate from the chain automatically (and we treated the intermediate as "untrusted", just a part of chain building)

(Note to myself, write better PR messages)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the purpose was that the roots were loading in earlier, and if you have an intermediate in your chain, we'd automatically populate an intermediate pool. Had to review 7402eb4 where I added this

Exactly. Before we actually reset intermediates, but in using blobs, we actually wanted to preserve the TUF roots that were populated already.

Signed-off-by: Asra Ali <asraa@google.com>
@asraa asraa dismissed stale reviews from haydentherapper and hectorj2f via 89fdc26 November 17, 2022 15:38
Signed-off-by: Asra Ali <asraa@google.com>
@asraa
Copy link
Contributor Author

asraa commented Nov 17, 2022

@haydentherapper @znewman01 lemme know if you have time to do another pass! since it's working around the verification logic, would love to have it in. I have a follow-up issue on cleaning up the verification main method.

Signed-off-by: Asra Ali <asraa@google.com>
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.

6 participants