add support for signature & tbs_certificate to Certificate

[reaperhulk]

To actually verify that these signatures are correct we also need access to the tbsCertificate bytes. Then verification looks like:

        # RSA specific
        verifier = signer_public_key.verifier(
            cert.signature, padding.PKCS1v15(), cert.signature_hash_algorithm
        )
        verifier.update(tbs)
        verifier.verify()

Without tbs you can't actually verify. If we're going down this path then we need to expose tbsCertificate as well (name suggestions?).

[codecov-io]

Current coverage is 99.98%

Merging #2387 into master will not affect coverage as of 12176c7

@@            master   #2387   diff @@
======================================
  Files          123     123       
  Stmts        12744   12788    +44
  Branches      1401    1402     +1
  Methods          0       0       
======================================
+ Hit          12742   12786    +44
  Partial          2       2       
  Missed           0       0       

Review entire Coverage Diff as of 12176c7

Powered by Codecov. Updated on successful CI builds.

[danetrain]

We already have a public_bytes() method, so maybe the described method should be called tbs_bytes()?

[alex]

Does this need some words to express that you probably don't want to use this?

[etrauschke]

I'm happy you are planning to provide an interface to do the basic public_key verification but why does this have to be that complicated while having an interface to OpenSSL's X509_verify() does essentially the same thing in one call.

In general I could see the need to have several parameters you can pass to signer_public_key.verifier() but I fail to see when you would ever want to verify against a non-matching signature algorithm or padding scheme. Or is there some restriction in X509_verify() I'm missing here?

If I understand this correctly, the proposed solution has the same limitations X509_verify() has but is at least ten times harder to use (I think I would always have to copy&paste an example of this to get it right).

[reaperhulk]

@alex and others have expressed a preference for Certificate to remain a "data only" object in the recent past. The intent of exposing signature and tbs_certificate would not be for people to build their own verification, but rather to support the verification classes being proposed in #2381.

I am sympathetic to the argument that cert.verify(pub_key) might be a nicer API, especially given that it allows utilization of backend specific methods like X509_verify. I'm also not a big fan of exposing tbs_certificate, as its only use is to hash the ASN.1 bytes and do sign/verify operations, which should be handled by the library anyway. That said, I'm not sure I feel strongly enough to argue about this just yet. I think we need a complete API proposal.

[etrauschke]

If you want to keep the certificate data-only, you could still add some sort of utility function in the backend which takes a cert object and a public key and returns a verification result. This works as long as this function can get access to the underlying x509 object (which is the case with the current design).
It would still offer a straight-forward interface and wouldn't violate your greater design principles.

However, it would be pretty hard to find for users since I think that functions in the backend are not supposed to be consumed by a random user. This could also be a good thing though because it prevents people from shooting themselves in the foot accidentally, believing that this is an universally, all-encompassing verification of a given certificate.

[alex]

a) Merge conflict :-(
b) So in principle this seems reasonable, in practice it's way more "insidery" feeling than the rest of our API. Do you have suggestions on how to assauge that feeling?

[reaperhulk]

Yeah documenting these in my head tbs_certificate is almost footgun-like. "tbs_certificate is the DER bytes payload that is hashed and then signed by the private key of the certificate's issuer. This data may be used to validate a signature, but use extreme caution as certificate validation is a complex problem that involves much more than just signature checks. Users are encouraged to use thing that doesn't exist yet."

[reaperhulk]

Having thought about that a bit I'm actually more okay with exposing it with text similar to that. We can update it with the "Users are encouraged to use validation class" once we build the actual class.

[alex]

Is this the right way to expose this data? It feels a bit ad-hoc compared to public_bytes or something?

[reaperhulk]

I'm not sure what the best approach is here. tbsCertificate is a subset of the ASN1 that makes up the complete certificate object. I don't think I'd like it in public_bytes because it would be beyond confusing to require users to pass the serialization type (when 99.99% of the time you just want the whole thing) in addition to encoding.

[rowland-smith]

Blessed are the code-mergers...

Sent from my iPhone

On Nov 4, 2015, at 7:25 AM, Alex Gaynor [email protected] wrote:

Merged #2387.

—
Reply to this email directly or view it on GitHub.