Stop using public key sent from Autograph #1890
Comments
|
we query it - so if they remove from their API we break. |
2009: Verify with x5u instead of public_key r=jaredkerim a=glasserc This is meant to address #1890, at least partially. I tried to re-use the existing signature verification logic but I had to jump through a bunch of hoops to get the data back into the form the pubkey check uses. (See commit comments for details.) The test is equally ugly -- this doesn't actually ensure that data is in the right format or anything, it's basically just a restatement of the implementation as a test. @mythmon, how did you get the data for the existing pubkey test? Maybe I can do something similar to generate a real x5u. With this change, I was able to `update_recipe_signatures`, `update_product_details`, and `update_actions` without crashes. Co-authored-by: Ethan Glasser-Camp <[email protected]>
|
I'm not sure what to do with this issue. We currently now verify signatures using Besides this, there's one other outstanding concern with Autograph in dev, which is that certs are served as The stated purpose of this issue is to stop using the public-key field of the Autograph response, but it seems to me that for old recipes, that may be the only way we have available to validate them (since |
2027: Encode only the public key information r=jaredkerim a=glasserc Refs #1890. Fixes #2021. In #2009 I hacked up generation of a public key by serializing a complete certificate. However, `fastecdsa` expects only a public key, which is just one part of the certificate. That the code in #2009 ever worked at all is accidental and almost miraculous. It seems that `fastecdsa` doesn't really do full parsing of the ASN.1 in its input -- it just kind of does a "good enough" job (probably for speed?) and plucks the last vaguely-parametric-looking things out of the file. This means that depending on the other contents of the certificate, it might actually grab the correct `x` and `y` and function correctly. But it can also grab other bit strings and fail catastrophically. I was able to reproduce this failure by running the contract tests against stage. This patch fixes the tests. However, we've come to this point because I committed code that merely "seemed to work", so I wanted to be much more thorough in my work this time. The cert that we have is an X.509 certificate, described in [RFC 5280](https://tools.ietf.org/html/rfc5280). RFC 7468 [explains](https://tools.ietf.org/html/rfc7468#section-13) that a PEM-encoded public key corresponds to the subject public key information field from RFC 5280, so retrieving this field from the cert and serializing it is correct. Just to confirm, we can also see that `fastecdsa` specifies in https://github.com/AntonKueltz/fastecdsa/blob/master/fastecdsa/encoding/pem.py#L109 that it accepts elliptic-curve key data in [RFC 5480](https://tools.ietf.org/html/rfc5480) format. This RFC is the one that adds a format for encoding elliptic-curve keys to RFC 3279, which is the RFC referred to by RFC 5280 as defining the methods for encoding public key materials in X.509 certificates. So again, the public key data from the certificate should be the format that this library expects. /cc @mozbhearsum @jvehent Co-authored-by: Ethan Glasser-Camp <[email protected]>
2017: Fix contract tests r=jaredkerim a=glasserc Refs #1890. Update the contract tests to pass x5us instead of public keys. Co-authored-by: Ethan Glasser-Camp <[email protected]>
2017: Fix contract tests r=jaredkerim a=glasserc Refs #1890. Update the contract tests to pass x5us instead of public keys. Co-authored-by: Ethan Glasser-Camp <[email protected]>
Currently, Normandy stores and validates the
public_keyfield returned by autograph when signing objects. This is not used by Normandy Client when verifying recipe signatures. Instead the x5u is used to retrieve a certificate chain, which is a more sound verification process.The server on the other hand is attempting to validate these signatures, which isn't good. To quote @jvehent on slack:
We should remove the public key field from our database, and ignore it if Autograph sends it. We should stop validating it, and stop sending it to Normandy Client. None of this is a security problem, it is just not an optimal way of handling certificates and signatures.
The text was updated successfully, but these errors were encountered: