Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rcgen returns incorrect SignatureAlgorithm when linked against aws-lc-rs 1.12.3 #317

Closed
GermanCoding opened this issue Feb 20, 2025 · 3 comments

Comments

@GermanCoding
Copy link

Foreword: I apologize if this is actually AWS's bug - I'm not sure where the error is, currently.

I was debugging unit test failures today, when I noticed this very odd behaviour:

Steps to reproduce

I have (test) code that does something like this:

pub fn from_pem(pem: &str) -> anyhow::Result<()> {
  let rcgen_keypair = rcgen::KeyPair::from_pem(pem).context("reading private key from pem failed")?;
  Ok(match rcgen_keypair.algorithm() {
     alg if alg == &rcgen::PKCS_ECDSA_P256_SHA256 => {
        // Just a test
        println!("This is a P-256 keypair!");
     }
     alg if alg == &rcgen::PKCS_ECDSA_P384_SHA384 => {
         println!("This is a P-384 keypair!");
     }
     _ => println!("Something else..."),
  };
  [...]
}

In addition, I have the following Cargo.toml (excerpt):

[...]
aws-lc-rs = { version = "=1.12.3", default-features = false, features = ["alloc", "ring-io", "aws-lc-sys", "prebuilt-nasm"] }
rcgen = { version = "0.13.2", default-features = false, features = ["pem", "aws_lc_rs"] }

Now, we feed this private key (this is a test key!) to from_pem:

-----BEGIN PRIVATE KEY-----
MIG2AgEAMBAGByqGSM49AgEGBSuBBAAiBIGeMIGbAgEBBDCox+o8d2IzZRUaW91Q
+5XhSTvppqz3IE6zp+t+eV7cjN+03FpjYdzI5MUoYMDvuw2hZANiAASpYDU237gY
F2L24KJSs/NlEHyXs6tKebsin6uVklyDu3WB7aS9NfKatnNF4Dm4l8fxtXU0bDMk
TJewtdXtUp5YK9kffYrWgDuhjq4X2SiUmOdYdDKzleh2ebpLokzCSxk=
-----END PRIVATE KEY-----

we get this output

This is a P-256 keypair!

but if we decode the above PEM using OpenSSL...

Private-Key: (384 bit)
priv:
    a8:c7:ea:3c:77:62:33:65:15:1a:5b:dd:50:fb:95:
    e1:49:3b:e9:a6:ac:f7:20:4e:b3:a7:eb:7e:79:5e:
    dc:8c:df:b4:dc:5a:63:61:dc:c8:e4:c5:28:60:c0:
    ef:bb:0d
pub:
    04:a9:60:35:36:df:b8:18:17:62:f6:e0:a2:52:b3:
    f3:65:10:7c:97:b3:ab:4a:79:bb:22:9f:ab:95:92:
    5c:83:bb:75:81:ed:a4:bd:35:f2:9a:b6:73:45:e0:
    39:b8:97:c7:f1:b5:75:34:6c:33:24:4c:97:b0:b5:
    d5:ed:52:9e:58:2b:d9:1f:7d:8a:d6:80:3b:a1:8e:
    ae:17:d9:28:94:98:e7:58:74:32:b3:95:e8:76:79:
    ba:4b:a2:4c:c2:4b:19
ASN1 OID: secp384r1
NIST CURVE: P-384

This is clearly a P-384 key!!!

Expected output

Interestingly, if we only make a single change in Cargo.toml, namely by downgrading aws-lc-rs to 1.12.2:

aws-lc-rs = { version = "=1.12.2", default-features = false, features = ["alloc", "ring-io", "aws-lc-sys", "prebuilt-nasm"] }
[...]

then the above script prints

This is a P-384 keypair!

which is correct.

This sounds like either a bug in aws-lc-rs, or in the interaction between aws-lc and rcgen. I'm also using aws-lc-rs directly, and there I couldn't reproduce the problem right away, which is why I'm reporting this issue to rcgen initially.

@cpu
Copy link
Member

cpu commented Feb 20, 2025

I think this was an unintended side-effect of aws/aws-lc-rs#686 (?)

I believe that previously from_private_key_der() in ec/key_pair.rs only called parse_rfc5915_private_key(), which enforces the alg's NID matches expectation post-parse.

In 686 I think it was changed so parse_rfc5915_private_key() is only called if parse_rfc5208_private_key() returns an err, and crucially that new codepath doesn't look like it examines the alg at all. Your input key looks at first glance to me like a RFC 5208 PKCS#8 private key so I think that parse succeeds and returns a P384 decoded private key without noticing that we specified the P256 algorithm ID in the call. That breaks the way rcgen is trying to determine the curve by trial and error.

I'll pull out a smaller repro and report upstream.

@cpu
Copy link
Member

cpu commented Feb 20, 2025

aws/aws-lc-rs#704

@cpu
Copy link
Member

cpu commented Feb 20, 2025

The upstream project yanked the release with the bug. Let's close this one and follow 704 for the upstream fix.

Thanks for the very detailed bug report @GermanCoding (and for Let's Debug <3))!

@cpu cpu closed this as not planned Won't fix, can't repro, duplicate, stale Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants