Add TryFrom from &[u8] and Vec<u8> to PrivateKeyDer #39

Alvenix · 2024-03-20T14:33:14Z

I am not very confident about this yet.

I am mainly using asn1 decoders to learn the differences manually.

In addition, the approach is heuristic in nature because it would be hard to implement a more complex parser.

I am planning to add more test private key files.

djc

Looks pretty good, thanks for picking this up!

I'll let @cpu and @ctz judge the specifics of the DER parsing.

djc · 2024-03-20T14:55:48Z

src/lib.rs

+
+    fn try_from(key: &'a [u8]) -> Result<Self, Self::Error> {
+        if key.len() <= 5 || key[0] != 0x30 {
+            return Err("Invalid key format");


Nit: I think we prefer lowercase error strings (also for the other instance).

djc · 2024-03-20T14:56:11Z

src/lib.rs

+    fn try_from(key: Vec<u8>) -> Result<Self, Self::Error> {
+        let private_key_der = PrivateKeyDer::try_from(&key[..])?;
+
+        match private_key_der {


Nit: would be nice to hoist the Ok() out of the match.

djc · 2024-03-20T14:57:01Z

src/lib.rs

+    type Error = &'static str;
+
+    fn try_from(key: Vec<u8>) -> Result<Self, Self::Error> {
+        let private_key_der = PrivateKeyDer::try_from(&key[..])?;


Nit: I think it would be okay to just inline the binding here? (As in, match PrivateKeyDer::try_from(&key[..])? { .. }.)

cpu · 2024-03-20T15:07:26Z

I'll let @cpu and @ctz judge the specifics of the DER parsing.

I will try to give this a review pass shortly. Likely ~tomorrow, I have some other things ahead of it.

ctz · 2024-03-20T15:18:16Z

src/lib.rs

+            skip_len += key[1] - 0x80;
+        }
+
+        match key[skip_len as usize..] {


Not a real review, but I think this panics with this path:

key[0] := 0x30 and key.len() > 5 to get us here

skip_len := 2

key[1] := 0xff, say

key[1] >= 0x81 passes

skip_len += 0xff - 0x80, ie. 129

key[129..] would panic if key is too short

I would suggest using get(skip_len as usize..) and then adding Some() around the match arms.

In the limit, this could benefit from a little fuzzing, I suppose? Shouldn't be too hard to bring up a cargo fuzz harness...

I did as you said. Maybe we should additionally limit skip length to small number. For example 4 bytes, as that would indicate 4GB payload. (If I understood it correctly)

In the limit, this could benefit from a little fuzzing, I suppose? Shouldn't be too hard to bring up a cargo fuzz harness...

If it is okay to add a more complex test, I would like to use openssl to generate large number of private keys programmatically and ensure the function chooses a correct key type.

Fuzzing to detect panics is lesser concern for me (as the function is small).

If it is okay to add a more complex test, I would like to use openssl to generate large number of private keys programmatically and ensure the function chooses a correct key type.

Depends on the complexity? I think several hundred lines of code would be worth it if it doesn't blow up test cycle times beyond a few minutes.

Fuzzing to detect panics is lesser concern for me (as the function is small).

Given that the code already tried to be pretty careful and a panic could still happen makes me a little less optimistic, I guess!

Alvenix · 2024-03-20T16:48:17Z

I tried with more keys generated programmatically and the conversion failed. Please wait for me to fix the issues before review. (I guess the function will become more complex to cover different types)

Alvenix · 2024-03-20T17:23:59Z

It seems openssl bindings somehow flipped some of the formats for RSA (use pkcs1 for pkcs8 and vice versa) or at least I hope so. I will investigate this more later. I removed the key files as I added programmatic tests which passes without changing the conversion function.

Edit:

I have opened this issue at openssl crate. It seems to me that the formats are indeed flipped somehow.

ctz · 2024-03-20T17:55:09Z

src/lib.rs

+            Some([0x02, 0x01, 0x00, // Version = 0
+            0x30, 0x0D, // Sequence of two elements, which have 13 bytes length
+            0x06, 0x09, 0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, 0x01, 0x01, // Object Identifier for RSA Encryption (PKCS #1)
+            ..]) => {
+                Ok(Self::Pkcs1(key.into()))
+            }
+
+            Some([0x02, 0x01, 0x01, // This is decoded as PKI header with value 1
+            ..]) => {
+                Ok(Self::Sec1(key.into()))
+            }
+
+            Some([0x02, 0x01, 0x00, // Version = 0
+            ..]) => {
+                Ok(Self::Pkcs8(key.into()))
+            }


At the moment PKCS#1 and PKCS#8 cases are switched. Additionally, PKCS#8 shouldn't be specific to the rsaEncryption OID. I think we should look for:

PKCS#8: sequence, integer 0, sequence (ie, Some([0x02, 0x01, 0x00, 0x30]))

PKCS#1: sequence, integer 0

SEC1: sequence, integer 1

I think a static set of sample/test files is nicer than an openssl dependency (even if it it's just a dev-dependency). It also has the advantage of being 100% reproducible.

There are some suitable files in https://github.com/rustls/rustls/tree/main/rustls/src/testdata (rsa*.der, eddsakey.der, nist*.der)

At the moment PKCS#1 and PKCS#8 cases are switched.

Oh sorry.........

I think a static set of sample/test files is nicer than an openssl dependency (even if it it's just a dev-dependency). It also has the advantage of being 100% reproducible.

Yes I could add it but I prefer to also keep the openssl tests, as I can simply increase the iterations number and try many keys of the same algorithm to check that they are valid.

Alvenix · 2024-03-20T18:46:16Z

I removed openssl test and went back to static files.

cpu

I think the general approach here is workable, but I'd like to see it written with less magic values and more information about the heuristics in use and why we think they're stable signals.

In reviewing the diff I adapted your code to be closer to what I'm thinking of. Rather than deliver a bunch of review feedback I opened a second PR based on your own with what I'm thinking: #40

WDYT?

cpu · 2024-03-21T15:12:57Z

src/lib.rs

+        // We skip the tag and the length of the sequence
+        let mut skip_len = 2;
+
+        // We skip additonal length bytes


typo here:

Suggested change

// We skip additonal length bytes

// We skip additional length bytes

src/lib.rs

Alvenix · 2024-03-21T16:57:12Z

I am closing this PR because I think the PR there is much better. We can continue discussion there.

djc reviewed Mar 20, 2024

View reviewed changes

djc requested review from cpu and ctz March 20, 2024 14:57

ctz reviewed Mar 20, 2024

View reviewed changes

Alvenix force-pushed the add_from_bytes branch 2 times, most recently from 6a8c6ea to 58c67dc Compare March 20, 2024 16:13

Alvenix force-pushed the add_from_bytes branch from 58c67dc to 562b923 Compare March 20, 2024 17:21

Alvenix force-pushed the add_from_bytes branch 2 times, most recently from e8868b9 to 037cfe1 Compare March 20, 2024 17:37

ctz reviewed Mar 20, 2024

View reviewed changes

Alvenix force-pushed the add_from_bytes branch 3 times, most recently from 5dd8225 to f53fbcc Compare March 20, 2024 18:45

Add TryFrom from &[u8] and Vec<u8> to PrivateKeyDer

b5c7b1b

Alvenix force-pushed the add_from_bytes branch from f53fbcc to b5c7b1b Compare March 20, 2024 18:47

Alvenix mentioned this pull request Mar 21, 2024

Support more private key formats when using aws_lc_rs feature rustls/rcgen#242

Merged

cpu mentioned this pull request Mar 21, 2024

Add TryFrom from &[u8] and Vec<u8> to PrivateKeyDer #40

Merged

cpu reviewed Mar 21, 2024

View reviewed changes

Alvenix closed this Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TryFrom from &[u8] and Vec<u8> to PrivateKeyDer #39

Add TryFrom from &[u8] and Vec<u8> to PrivateKeyDer #39

Alvenix commented Mar 20, 2024

djc left a comment

djc Mar 20, 2024 •

edited

Loading

djc Mar 20, 2024

djc Mar 20, 2024

cpu commented Mar 20, 2024

ctz Mar 20, 2024

djc Mar 20, 2024

Alvenix Mar 20, 2024

Alvenix Mar 20, 2024 •

edited

Loading

djc Mar 20, 2024 •

edited

Loading

Alvenix commented Mar 20, 2024

Alvenix commented Mar 20, 2024 •

edited

Loading

ctz Mar 20, 2024

ctz Mar 20, 2024

Alvenix Mar 20, 2024

Alvenix commented Mar 20, 2024

cpu left a comment

cpu Mar 21, 2024

Alvenix commented Mar 21, 2024

	// We skip additonal length bytes
	// We skip additional length bytes

Add TryFrom from &[u8] and Vec<u8> to PrivateKeyDer #39

Add TryFrom from &[u8] and Vec<u8> to PrivateKeyDer #39

Conversation

Alvenix commented Mar 20, 2024

djc left a comment

Choose a reason for hiding this comment

djc Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alvenix Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

djc Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Alvenix commented Mar 20, 2024

Alvenix commented Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alvenix commented Mar 20, 2024

cpu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alvenix commented Mar 21, 2024

djc Mar 20, 2024 •

edited

Loading

Alvenix Mar 20, 2024 •

edited

Loading

djc Mar 20, 2024 •

edited

Loading

Alvenix commented Mar 20, 2024 •

edited

Loading