search

mozilla-services / python-autograph-utils

A library to simplify use of Autograph
Other
5 stars 4 forks source link

Chain of trust #5

Closed glasserc closed 4 years ago

glasserc commented 4 years ago

This is a feature that's been missing as far as I can tell from all of the extant server-side implementations of Autograph signature verification. It's implemented using the code from https://cryptography.io/en/latest/x509/reference/#cryptography.x509.Certificate.tbs_certificate_bytes. It seems to work on the test certificate chain I've been using in the tests. However, the docs note:

Note: This only verifies that the certificate was signed with the private key associated with the public key provided and does not perform any of the other checks needed for secure certificate validation. Additionally, this example will only work for RSA public keys with PKCS1v15 signatures, and so it can’t be used for general purpose signature verification.

@jvehent: What are the "other checks needed for secure certificate validation"? This is the last one I thought I had to implement but I'm worried I'm missing something else. Also, should I be adding code to try to figure out what signature algorithm is used on every cert in the chain? The test chain is RSA + SHA384 throughout but maybe that's something you foresee changing.

glasserc commented 4 years ago

https://github.com/alex/x509-validator/blob/master/validator.py demonstrates how this might look if we need to support multiple different signature algorithms. It's not that difficult to add them.

https://opendev.org/x/cursive/src/branch/master/cursive/verifiers.py is another sort of demonstration, although it's a little heavier because it explicitly feeds the hash itself.

glasserc commented 4 years ago

OK, I think this is due for another round of review. In addition to being rebased onto master, I also added support for ECDSA cert keys, and implemented the checks @jvehent said. Edited to add: The cert chain I had pulled from dev did not have the name constraints that Julien was talking about, so I also pulled in a cert chain from stage. This chain happens to have ECDSA keys already, so that's why I implemented support for them. Now some tests rely on stage and others on dev; is it worth cleaning this up?

glasserc commented 4 years ago

I have commits that address this feedback but am unable to update the PR because of https://github.com/mozilla-services/github-management/issues/40.

glasserc commented 4 years ago

OK, I think I have addressed your feedback @leplatrem. @jvehent ?

leplatrem commented 4 years ago

Shall we release a preview version on Pypi so that we can start working on integrating this library?

glasserc commented 4 years ago

I was hoping to hear from Julien but yeah, I guess we should get moving on this. I'll merge it and cut a release.

jvehent commented 4 years ago

Sorry folks, haven't had a chance to review. Code looked good last I checked so I'd say just ship it :)

On Mon, Nov 11, 2019, 19:21 Ethan Glasser-Camp notifications@github.com wrote:

Merged #5 https://github.com/mozilla-services/python-autograph-utils/pull/5 into master.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mozilla-services/python-autograph-utils/pull/5?email_source=notifications&email_token=AADFPAI4JGNGDM2TOX5VEPDQTHZKJA5CNFSM4JFETF62YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOUZEAYHA#event-2789739548, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADFPALBJP7PIIGZ2DOGDQLQTHZKJANCNFSM4JFETF6Q .