Add parameter ecdsa_deterministic_signing to x509 sign methods

[emlun]

Hi! This is a feature I implemented for myself in order to generate reproducible test vectors for W3C Web Authentication (see also the pull request). The feature already exists in the ECDSA layer, so this patch just plumbs it through to the X.509 layer. I named the parameter ecdsa_deterministic_signing to mirror the corresponding deterministic_signing parameter in the ec module, and the ecdsa_ prefix mirroring its sibling parameter rsa_padding.

I tried to get this to work in PKCS7SignatureBuilder.add_signer too, since its sign_and_serialize Rust implementation also has calls that need a new false argument, but didn't manage to get PKCS7SignatureBuilder.sign() to produce reproducible outputs. I think one reason for that is that sign_and_serialize in Rust internally sets the signing time to now(), but making that injectable as a parameter didn't seem to suffice to make the result deterministic. My experiments are available on my wip/pkcs7-sign-ecdsa-deterministic branch in case there's interest in it.

[reaperhulk]

Let's separate whether we want this from the specifics of this implementation. I'm not aware of any security reason to not allow deterministic and we can (mostly) reuse existing implementation. While it's a niche requirement and for leaf certificates you can get the same deterministic behavior with ed25519 keys I think I'm fine with supporting it.

I do wonder if we want to extend the signing APIs this way or if we want to revisit them entirely. It sure would be nice if we were passing the ECDSA(hash, deterministic) structure rather than what we've got now... @alex do you have an opinion?

[alex]

Hmm, on the one hand, I don't love the dangling bool.

On the other hand, passing a full ECDSA() object duplicates the hash algorithm, and then we gotta check that they don't disagree I guess? And we can't just replace the hash with the ECDSA object, because the RSA padding doesn't carry a hash and there's no object that represents both. We kind of failed at symmetry here.

So all the solutions, short of redo signing, feel wrong.

[reaperhulk]

Yeah it seems like we would need to either do some comical overloading of sign such that it can take entirely different arguments in addition to the existing ones, define a new sign method that has a better signature but is functionally identical except for this one feature, or embrace our fate.

[emlun]

Thanks for the replies! Please let me know if you'd like me to do anything here, otherwise I'll wait for y'all to decide how you want to proceed. 🙂

[emlun]

Also for the record, it occurred to me that this can be (albeit awkwardly) worked around by manually re-signing the tbs_certificate_bytes with the CA key. For example like this, using the asn1 library to decode and re-encode the ASN.1 wrapping:

def to_deterministic_cert(cert, ca_key):
    """
    Workaround for python-cryptography X509CertificateBuilder not supporting deterministic ECDSA signing.
    This function takes a certificate and re-signs it using deterministic ECDSA.
    """
    assert False, "DO NOT copy-paste this code without understanding what it does!"
    dec = asn1.Decoder()
    dec.start(cert.public_bytes(serialization.Encoding.DER))
    tag, value = dec.read()
    sig = ca_key.sign(cert.tbs_certificate_bytes, ec.ECDSA(hashes.SHA256(), deterministic_signing=True))
    enc = asn1.Encoder()
    enc.start()
    enc.write(sig, asn1.Numbers.BitString)
    sig_der = enc.output()
    der_cert = cert.public_bytes(serialization.Encoding.DER)
    len_diff = len(sig) - len(value[2])
    assert der_cert[1] == 0x82
    assert der_cert[3] + len_diff >= 0
    assert der_cert[3] + len_diff <= 255
    der_newcert = der_cert[0:3] + bytes([der_cert[3] + len_diff]) + der_cert[4:-(len(value[2])+3)] + sig_der
    newcert = x509.load_der_x509_certificate(der_newcert, default_backend())
    return newcert

It's awkward, fragile and probably full of dragons, so don't use this for actual applications. But I did confirm that this produces outputs that are byte-for-byte identical to the equivalent use of this ecdsa_deterministic_signing feature. 🙂

[reaperhulk]

Doing that is actually quite safe, albeit disgusting 😄

I don't think I have a better suggestion than the kwarg bool so I guess let's move forward with that. This needs a rebase and appears to have a bit of missing coverage (you can see the lines in the all-green job). I also suspect you can make the bool passing simpler and not need some of the into_bound/into_any contortions.

[alex]

There's also a conflict in the changlog, sorry!

[alex]

implemented in #12887

[emlun]

Cool! Thanks!