allow multiple key types

[kevinmeziere]

Addresses #901

[davepacheco]

@kevinmeziere it looks like this is failing a few CI checks.

@ahl can you take a look at this one? It seems okay but I think you've been in this area more recently.

[kevinmeziere]

Should be cleaned up now. not really sure what to make of the the failing 'cargo fmt -- --check' as I cannot reproduce locally.

[ahl]

please see my suggestion; would it be easy enough to add a test to validate this new behavior?

[ahl]

rustls_pemfile::private_key docs say "Yields the first PEM section describing a private key (of any type), or an error if a problem occurs while trying to read PEM sections."

Github isn't letting me make this suggestion for the full span, but from 521-529 I'd suggest doing:

        let private_key = rustls_pemfile::private_key(&mut key_reader)
            .map_err(|err| {
                io_error(format!("failed to load private key: {err}"))
            })?
            .ok_or_else(|| io_error("no private key found".into()))?;

[ahl]

@kevinmeziere checking in on this; how would you like to proceed?

[kevinmeziere]

Sorry for the slowness... I have taken your changes (which work nicely) and have yet to make tests. I hope to get to that this week.

[ahl]

Sorry for the slowness... I have taken your changes (which work nicely) and have yet to make tests. I hope to get to that this week.

No apology needed! Just checking in. Thank you for the submission. With regard to tests, I don't want to create an unnecessary hurdle... and I don't want functionality you're depending on to regress! If testing is too onerous, let us know!

[kevinmeziere]

After spending sometime poking around at this it is not clear to me that it is possible to have rcgen create an ec key pair. It would seem this is necessary to do in (or a duplicate of) tests/common/mod.rs:62. I do not have a good enough grasp of cryptography to be comfortable to try and implement a new test with a different certificate generating library. I see a couple ways forward:
1 - Leave untested. The existing test do check for a pkcs8 pair, so it would just be additional key types that are not tested.
2 - Withdraw. For my own purposes I would be ok managing my own local patch.
3 - Create some terrible test that either has a cert file that it loads from a file or shells out to create a pair and loads. Seems terrible.

Please let me know if you have some input on this.

[ahl]

I think leaving it untested would be fine. No need to withdraw it -- this is a good fix. I think some static data in a file or thunked into a string might not be so horrible? Let me know what you think. Thanks.

[kevinmeziere]

I was mainly concerned that the certificate had to be valid for rustls to load it. Also because we would be loading the certificate it would be testing more than just "can dropshot load cert type x", so potentially changes in rustls in the future would case fails that have nothing to do with what we are "testing". In the other test dropshot uses "localhost" so that seems like a fine assumption, also we could just set the expiration to 20 years from now. If thats ok with you I'll write it up.

[ahl]

I was mainly concerned that the certificate had to be valid for rustls to load it. Also because we would be loading the certificate it would be testing more than just "can dropshot load cert type x", so potentially changes in rustls in the future would case fails that have nothing to do with what we are "testing". In the other test dropshot uses "localhost" so that seems like a fine assumption, also we could just set the expiration to 20 years from now. If thats ok with you I'll write it up.

I think that's fine. A comment on the test that's like: "welcome to the year 2044. You're still using dropshot? Presumably from your Martian colony? Was 2038 a big deal... or meh? Anyhow, here's how you renew what we ancients called 'a cert'"

[ahl]

Can you update the PR with the code suggestions above if those indeed seem like the right approach? We can figure out testing later.