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

Ring is unable to load ECDSA private key from a PKCS8 pem file which does not have the public key. #2133

Open
aj-bagwell opened this issue Aug 12, 2024 · 1 comment

Comments

@aj-bagwell
Copy link

When trying to load a private key and corresponding ECDSA client certificate I get an failed to parse private key as RSA, ECDSA, or EdDSA error from rustls, despite the key working fine with openssl.

After some digging it turns out it is because the private key file does not have the corresponding public key packed with it.

https://lapo.it/asn1js/#ME4CAQAwEAYHKoZIzj0CAQYFK4EEACIENzA1AgEBBDA6u5vLXwM2XYeoBzeYGVQAt7n5Vvjbtd2XDsQdk6ghFKZecMUL5h9lccg8Pwq-eqY

cat ~/ec.key
-----BEGIN PRIVATE KEY-----
ME4CAQAwEAYHKoZIzj0CAQYFK4EEACIENzA1AgEBBDA6u5vLXwM2XYeoBzeYGVQA
t7n5Vvjbtd2XDsQdk6ghFKZecMUL5h9lccg8Pwq+eqY=
-----END PRIVATE KEY-----
openssl pkey -in ~/ec.key -text -noout
Private-Key: (384 bit)
priv:
    3a:bb:9b:cb:5f:03:36:5d:87:a8:07:37:98:19:54:
    00:b7:b9:f9:56:f8:db:b5:dd:97:0e:c4:1d:93:a8:
    21:14:a6:5e:70:c5:0b:e6:1f:65:71:c8:3c:3f:0a:
    be:7a:a6
pub:
    04:55:6d:02:8c:b8:de:8a:f8:e7:99:95:5f:dd:68:
    78:9f:0c:53:da:7a:22:48:a2:2d:cd:c6:66:18:a9:
    77:da:0d:ae:6c:af:23:e6:93:9d:db:21:f7:f8:dd:
    8b:57:5a:b5:34:3d:79:d7:72:7a:81:ff:b3:90:ac:
    93:0c:64:40:d3:b7:44:36:4b:f1:19:50:c8:bc:89:
    5b:f1:8e:1d:77:14:54:eb:58:68:43:89:4c:46:9c:
    4c:3d:a8:c7:9f:bf:3a
ASN1 OID: secp384r1
NIST CURVE: P-384

There is a comment in suite_b.rs that says:

[1] publicKey. The RFC says it is optional, but we require it to be present.

The public part is not used for anything except to validate that it matches the public key derived from the private one, so I think this requirement could be relaxed.

rust playground link with minimal setup for replication:

use ring::signature::EcdsaKeyPair;
use ring::signature::ECDSA_P384_SHA384_ASN1_SIGNING;
use base64::prelude::*;
use ring::rand::SystemRandom;

fn main() {
    let der = BASE64_STANDARD.decode("ME4CAQAwEAYHKoZIzj0CAQYFK4EEACIENzA1AgEBBDA6u5vLXwM2XYeoBzeYGVQAt7n5Vvjbtd2XDsQdk6ghFKZecMUL5h9lccg8Pwq+eqY=").unwrap();
    let _key = EcdsaKeyPair::from_pkcs8(&ECDSA_P384_SHA384_ASN1_SIGNING, &der, &SystemRandom::new()).unwrap();
}
aj-bagwell added a commit to aj-bagwell/ring that referenced this issue Aug 12, 2024
PKCS8 encoded pem files have an option to have the public key in the
same file as the private key.

Not all pem files will have it, and the public key can be derived from
the private key anyway.

Fixes briansmith#2133
aj-bagwell added a commit to aj-bagwell/ring that referenced this issue Aug 12, 2024
PKCS8 encoded pem files have an option to have the public key in the
same file as the private key.

Not all pem files will have it, and the public key can be derived from
the private key anyway.

Fixes briansmith#2133

I agree to license my contributions to each file under the terms given
at the top of each file I changed.
@briansmith
Copy link
Owner

briansmith commented Sep 28, 2024

It seems fine to add the ability to parse PKCS#8 files missing the public key, I guess, but:

  • We should not change the implementation of the current API, which guarantees in its documentation that the pairwise consistency check is done. "The input must be in PKCS#8 v1 format. It must contain the public key in the ECPrivateKey structure; from_pkcs8() will verify that the public key and the private key are consistent with each other."
  • We should add a new API, and have test coverage analogous to the testing of the existing API.
  • EcdsaKeyPair::from_private_key_and_public_key needs an analogous counterpart.

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

Successfully merging a pull request may close this issue.

2 participants