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 verification function with Opts struct #121

Closed
10 tasks done
haydentherapper opened this issue Nov 7, 2022 · 4 comments · Fixed by #181
Closed
10 tasks done

Refactor verification function with Opts struct #121

haydentherapper opened this issue Nov 7, 2022 · 4 comments · Fixed by #181
Assignees
Labels
enhancement New feature or request p0 P0 Priority Task

Comments

@haydentherapper
Copy link
Contributor

haydentherapper commented Nov 7, 2022

Description

Per RFC 3161 2.2, we need to support the following for verification:

  • Verify that the TSR's hashed message matches the digest of the artifact to be timestamped
  • Verify the TSR's ESSCertID, the certificate identifier, matches a provided TSA certificate (kind of done here, but see below for note on updating to support out-of-band provided leaf)
  • Verify the leaf certificate's subject and/or subject alternative name matches a provided subject - See https://github.com/openssl/openssl/blob/master/crypto/ts/ts_rsp_verify.c#L536
  • If embedded in the TSR, verify the TSR's leaf certificate matches a provided TSA certificate
  • Verify the TSA certificate using a CA certificate chain
  • Verify the TSA certificate and the intermediates (called "EKU chaining") all have the extended key usage set to only extended key usage - See https://github.com/sigstore/timestamp-authority/blob/main/pkg/x509/x509.go#L35 - Note that the pkcs7 library verify does not check this
  • Verify the signature of the TSR using the public key in the leaf certificate (like here)
  • Verify the OID of the TSR matches an expected OID
  • Verify the status of the TSR does not contain an error
  • Verify the nonce - Mostly important for when the response is first returned

Note that the leaf certificate may be provided out of band, as it is not a requirement that the certificate is embedded in the TSR. verifySignature from the pkcs7 library seems to expect that the certificate is present always. We'll need upstream changes I think to update verification to support TSRs without certificates.

It looks like there are more errors in the pkcs7 verification logic - for example, https://github.com/digitorus/pkcs7/blob/21b8b40e6bb42e43036336c7828a1bee77c2b1d6/verify.go#L157 assumes that all certificates stored in the pkcs7 certs struct are intermediates (so I don't think verification even works if you have a chain with an intermediate? Haven't confirmed though, please do). Upstream fixes are probably needed. I'd avoid reimplementing signature verification (complex!, so let's try to fix in upstream, and we can work off my fork for now.
EDIT: Verification works in our code because we read in all certificates into a single cert pool, and those are called the "trust store" roots - Since the leaf is in this cert pool, and the leaf is in the p7.Certificates, then verification passes because it'll find a "chain" from leaf to leaf. This is ok to use as long as we also verify the certificate chain ourselves, but I think it's worth still figuring out how to redesign this in upstream. Something like VerifyWithChain here taking a root pool, intermediate pool, optional leaf certificate, and optional EKU.

The Opts pattern we've followed from Cosign has worked well, so I'd recommend we do the same here.

type VerificationOpts struct {
  oid asn1.ObjectIdentifier
  TsaCertificate *x509.Certificate
  Intermediates *x509.CertPool
  Roots *x509.CertPool
  Nonce string
  Subject string
}

Blocked by #119

@malancas did you want to take a look at this?

@haydentherapper haydentherapper added the enhancement New feature or request label Nov 7, 2022
@haydentherapper
Copy link
Contributor Author

Also worth confirming that all verification we're doing includes what openssl does

@malancas
Copy link
Contributor

malancas commented Nov 7, 2022

@haydentherapper sure, I'll take a look.

@haydentherapper
Copy link
Contributor Author

@malancas Just checking in on issues, any updates for this one?

@malancas
Copy link
Contributor

@haydentherapper I'm making progress on this issue and am planning to post some PRs for this on Wednesday.

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

Successfully merging a pull request may close this issue.

2 participants