Support more private key formats when using aws_lc_rs feature

[Alvenix]

I hope this change is okay. I am not sure if I need to add tests for this? Also should I rename the function in ring-like?

[Alvenix]

Is the direction I am going with this PR okay? I added too many functions (which maybe I could reduce somehow, better rename, etc..). But I would love that, by simply enabling aws_lc_rs feature the user would get more formats supported out of box.

I am adding tests a bit later.

[djc]

Is the direction I am going with this PR okay?

I think names like _with_more_formats() do not match with what I suggested in #242 (comment), and I would probably not want to add such API.

[Alvenix]

Is the direction I am going with this PR okay?

I think names like _with_more_formats() do not match with what I suggested in #242 (comment), and I would probably not want to add such API.

Yes I understand but I want to find a way that make it easiest to the user to parse a private key of either, sec1, pkcs1, pkcs8. I feel if each format has an API specific to them. Many (at least me) would have to write a wrapper that tries them all.

What about changing the original APIs to specify pkcs8 in it is name, and keeping the generic one without? Similar to the following:

#![cfg(feature = "aws_lc_rs")]
fn from_der_and_sign_algo(key: &PrivateKeyDer<'_>)

fn from_der_and_sign_algo_pkcs8(key: &PrivatePkcs8KeyDer<'_>)

We could also not export these APIs but use them inside from_raw.

We alternatively could use the same API but make the key type differ based on enabled features. (I don't like this approach, it may confuse users)

#![cfg(feature = "aws_lc_rs")]
type SupportedPrivateKeyDer = PrivateKeyDer;

#![cfg(not(feature = "aws_lc_rs"))]
type SupportedPrivateKeyDer= PrivatePkcs8KeyDer;

If you really don't think there is a good API design with a generic solution and want a specific API for the sec1 format, I could do it as you asked like this. (But this way doesn't support for example pkcs1)

fn from_der_and_sign_algo_sec1(key: &PrivateSec1KeyDer<'_>)

[djc]

Is the direction I am going with this PR okay?

I think names like _with_more_formats() do not match with what I suggested in #242 (comment), and I would probably not want to add such API.

Yes I understand but I want to find a way that make it easiest to the user to parse a private key of either, sec1, pkcs1, pkcs8. I feel if each format has an API specific to them. Many (at least me) would have to write a wrapper that tries them all.

What about changing the original APIs to specify pkcs8 in it is name, and keeping the generic one without? Similar to the following:

#![cfg(feature = "aws_lc_rs")]
fn from_der_and_sign_algo(key: &PrivateKeyDer<'_>)

fn from_der_and_sign_algo_pkcs8(key: &PrivatePkcs8KeyDer<'_>)

We could also not export these APIs but use them inside from_raw.

If we can add tests that show that the aws-lc-rs API accepts all of PKCS#8, PKCS#1 and SEC1, I think something like this could make sense? I would move the pkcs8_ before der to make the name from_pkcs8_der_and_sign_algo().

[Alvenix]

[rustls_pemfile::private_key()] is often used to obtain a [PrivateKeyDer] from PEM
input. Alternatively, if you already have a byte slice containing DER. it can trivially be converted into [PrivatePkcs8KeyDer] using the [Into] trait.

Is there any function like rustls_pemfile::private_key that can take DER input (instead of PEM) and give PrivateKeyDer? Because I could use it in this PR. (It is the biggest remaining issue)

[djc]

Look at the From impls for PrivateKeyDer?

[cpu]

I would move the pkcs8_ before der to make the name from_pkcs8_der_and_sign_algo().

I think it makes sense to make this change ahead of the next release so we can land this branch as a follow-up without needing any breaking API changes: #249

[Alvenix]

Look at the From impls for PrivateKeyDer?

The ones here? All these require knowing the key type. (e.g: converting first to pkcs8 for example). I meant something that parses &[u8] slice and provide PrivateKeyDer without knowing the type:

\\ For pem
let key = include_bytes!("private_key.pem");
let mut key = BufReader::new(key);
let key: PrivateKeyDer<'_> = rustls_pemfile::private_key(&mut key).unwrap().unwrap();

\\ for der?
let key = include_bytes!("private_key.der");
\\ ...

If there is no way to convert currently, is it okay for the functions to take a byte slice directly? Or should I try to add a function which perform the conversion? (in this crate or some other rustls crate)

#![cfg(feature = "aws_lc_rs")]
fn from_der_and_sign_algo(key: &[u8]);

instead of

#![cfg(feature = "aws_lc_rs")]
fn from_der_and_sign_algo(key: &PrivateKeyDer<'_>);

[djc]

We discussed this on Discord and we think it might make sense to have TryFrom<&[u8]> and TryFrom<Vec<u8>> implementations for PrivateKeyDer (in rustls-pki-types) that do some minimal DER parsing to determine whether the passed in DER is consistent with PKCS#1, PKCS#8 or SEC1 or error out otherwise. Would you be willing to take that on?

[Alvenix]

We discussed this on Discord and we think it might make sense to have TryFrom<&[u8]> and TryFrom<Vec<u8>> implementations for PrivateKeyDer (in rustls-pki-types) that do some minimal DER parsing to determine whether the passed in DER is consistent with PKCS#1, PKCS#8 or SEC1 or error out otherwise. Would you be willing to take that on?

Yes, I will try to do so.

[Alvenix]

I rebased this on prefer-aws branch. I patched this to use changes in pki-types. These changes are temporary until both are ready.

The added test show aws-lc-rs work with the mentioned formats.

[djc]

This is looking great modulo some nits!

[cpu]

This looks good to me, thank you. I think once we sort out the pki-types side (rustls/pki-types#40) and publish a release this would be ready to go.

[cpu]

I think once we sort out the pki-types side (rustls/pki-types#40) and publish a release

This was done and pki-types 1.4.0 has the required change.

@Alvenix When you have a chance can you update your branch to take that release and drop the patch? Thanks!

[Alvenix]

I think once we sort out the pki-types side (rustls/pki-types#40) and publish a release

This was done and pki-types 1.4.0 has the required change.

@Alvenix When you have a chance can you update your branch to take that release and drop the patch? Thanks!

I updated it, but PR 252 should be merged before. So I can rebase on main.

[cpu]

Thanks!

PR #252 should be merged before. So I can rebase on main.

Agreed, I think we're just waiting on est31 to +1 it.

[djc]

#252 has been merged. If you can rebase soon this can be released at the same time.

[est31]

Can we maybe offer the new name everywhere, on ring-only builds being a wrapper to the pkcs8 version?

[Alvenix]

Can we maybe offer the new name everywhere, on ring-only builds being a wrapper to the pkcs8 version?

I have done the requested changes in WIP: to squash after review which I will squash later after review is done.

The third commit is temporary, it contains a patch to pki-types to fix a bug.

[djc]

Great work! @est31 good suggestion -- @Alvenix sorry if I led you in the wrong direction before, I think this is a better outcome.

[djc]

LGTM, great work!

[djc]

My other question is whether it still makes sense to expose from_pkcs8_der_and_sign_algo() now that we have the more general method?

[cpu]

Thanks!

[Alvenix]

I did the requested changes, rebased and squashed all commits.

[est31]

whether it still makes sense to expose from_pkcs8_der_and_sign_algo() now that we have the more general method?

There might be instances where you only want to support pkcs8 because some standard or so requires it to be pkcs8. Validation plus loading with something more permissive is prone to exploitable bugs where you trick the validator then get to the loading stage and get treated differently.

I'm not sure it's enough justification to have all parsing functions be present both for pkcs8 and general formats though, maybe we just have the lowest level

[djc]

There might be instances where you only want to support pkcs8 because some standard or so requires it to be pkcs8. Validation plus loading with something more permissive is prone to exploitable bugs where you trick the validator then get to the loading stage and get treated differently.

If you really want PKCS#8 you can try_from() PrivateKeyDer::Pkcs8(_), right? I think this API still facilitates that.