Implement ecdsa keys parsing

[blaggacao]

closing #332

• I have no real idea what I'm doing (first time touching rust code)
• I'm remastering based on my common sense
• I'm blocked by this, so it looked like the only option at hand.
• cd rustls && cargo test --no-default-features --no-run is green, so is cd rustls && cargo test --all-features 🎉

This is intended to be a hand over, since I'm really not capable of addressing anything that goes much further than what's already in the PR.

[djc]

Unofficial review: the implementation looks good to me. I'm not sure the changes to the example code are warranted (which of PKCS8 vs EC we'd want to prioritize). Also, to get this accepted you should add some tests covering the new code. Can probably just add an example certificate and copy what tests exist for either of the existing parse functions.

[ctz]

This isn't mergable as-is, because ring cannot presently load EC keys in this format. It seems preferable to me to run a trivial, single openssl pkcs8 command to convert keys prior to giving the to rustls for use?

[blaggacao]

@ctz Hm I'm not sure I understand. I used this patch downstream and it seemed to work. But I'm also really ignorant in this topic, so that doesn't probably count.

I would prefer to get this solved at the root since there is an increasing number of tooling that provisions EC keys. Patching that tooling is an option, but it also implies maintaining a patch.

Running a "trivial" openssl command breaks atomicity and can result in race conditions where the bundle and key are rolled but the key is not getting transformed in time.

In my concrete example, SPIFFE helper is process manager that sends a reaload signal to the managed process as soon as the certificates are rolled. Having them atomically transformed with openssl command is less than trivial.

[briansmith]

I would prefer to get this solved at the root since there is an increasing number of tooling that provisions EC keys.

Why is that tooling using this weird file format? Why not send PRs to those patches upstream to switch to a standard format? It seems to me that the real bug is in the provisioning software for choosing a bad file format.

[blaggacao]

@briansmith I'm transparently relaying your questions, which I didn't have asked myself, yet: spiffe/spiffe-helper#26 Thanks! Looks like we might find something out this way.

[blaggacao]

@ctz Do you have any solution for this dead end in the back of your mind? I'm not a programmer, so forgive me if I'm not doing any more good than scratching the surface, here, while bluntly asking you for help or a solution.

Can we do something about ring so that rustls can start to support ECDSA?

[Darkspirit]

You need to convert keys of the format -----BEGIN EC PRIVATE KEY----- (EC private key in base64)
to the universal format -----BEGIN PRIVATE KEY----- (PKCS8 in base64):

https://wiki.openssl.org/index.php/Command_Line_Elliptic_Curve_Operations
$ openssl pkcs8 -topk8 -nocrypt -in before.pem -out after.pem

Then you can read the pkcs8 pem file with pkcs8_private_keys().

[blaggacao]

Not sure how to judge these suggestions. They seems elusive of parts of the problem.

But I guess I'm not going anywhere here, a pitty.

[connesc]

I would suggest to reopen this MR until support is added to ring.

@briansmith I believe this format is not as unusual as that. It's the default output format of openssl ecparam -genkey, many Go-based tool and probably others.

The current status makes any rustls-based app less usable than existing tools, such as cURL. Requiring the user to run an external command is not convenient, or even not possible in some cases (eg. openssl command not available).

[blaggacao]

@connesc I agree with you, but I don't want to get involved in any exhausting discussion about the presumed power relationship between libraries and consumers. I think you set the right tone transmitting the needs of consumers.

[olix0r]

Hi there,

I'm coming to this PR via kube-rs/kube#542 (for linkerd/linkerd2#7011 (comment)).

From reading through the issue/PR history, it's unclear to me if the takeaway is that this should not be supported at all, or that it should be supported--but in *ring* and not Rustls. Can anyone point me to discussions in *ring* about this issue?

@ctz

It seems preferable to me to run a trivial, single openssl pkcs8 command to convert keys prior to giving the to rustls for use?

@briansmith

Why is that tooling using this weird file format? Why not send PRs to those patches upstream to switch to a standard format? It seems to me that the real bug is in the provisioning software for choosing a bad file format.

I think the issue here--at least in the case we're running into this--is that Go-based tooling Just Works with these files. We've been bumping into this in the Kubernetes ecosystem where cluster configs may encode EC PRIVATE KEY credentials and then client libraries like kube-rs can't handle these keys. It's not trivial to run openssl because--as library authors, we can't rely on the existence of this tooling--and as end-users, everything works just fine until anything implemented in Rust touches the file.

Are PEM-formatted EC private keys seriously aberrant? Or are they something that we should ultimately expect *ring* to support?

[djc]

I think it would be a good start if someone wrote some code (let's say, in a separate crate for now) that can build a ring::signature::EcdsaKeyPair from an EC PRIVATE KEY file, probably with some code stolen from the rustls-pemfile crate (or based on a PR to that crate's repository). Then we can more easily review how sensitive that code is and where it might live (and failing anything else, having that code in a separate crate is probably already helpful).