Exporting all components of RSA and ED25519 secret keys #582

P-E-Meunier · 2017-10-18T20:21:17Z

Solves #579 for my RSA keys.
This also makes it possible to derive a public key from a secret key (which is also something I really needed for RSA keys).

This also makes it possible to derive a public key from a secret key.

coveralls · 2017-10-19T03:01:03Z

Coverage decreased (-0.7%) to 94.813% when pulling e892a5c on P-E-Meunier:master into 33784df on briansmith:master.

coveralls · 2017-10-19T03:01:03Z

Coverage decreased (-0.8%) to 94.754% when pulling e892a5c on P-E-Meunier:master into 33784df on briansmith:master.

coveralls · 2017-10-19T03:01:03Z

Coverage decreased (-0.8%) to 94.754% when pulling e892a5c on P-E-Meunier:master into 33784df on briansmith:master.

P-E-Meunier · 2017-10-19T09:10:54Z

Success! I just implemented an ssh-agent client, might add a server later today.

coveralls · 2017-10-19T09:37:06Z

Coverage decreased (-0.7%) to 94.813% when pulling e892a5c on P-E-Meunier:master into 33784df on briansmith:master.

P-E-Meunier · 2017-10-19T23:19:12Z

Ok, I'm almost done with the agent server. It works fine for Ed25519 keys, and the only thing missing now is to generate dP and dQ from d, p and q using the bigint module. Is there an easy answer?

briansmith · 2017-10-20T01:47:44Z

Ok, I'm almost done with the agent server. It works fine for Ed25519 keys, and the only thing missing now is to generate dP and dQ from d, p and q using the bigint module. Is there an easy answer?

They're already in the input. Notice in the code these two lines:

let _dP = der::positive_integer(input)?;
let _dQ = der::positive_integer(input)?;

You just need to put dP and dQ into the RSAPrivateKeyComponents structure.

briansmith · 2017-10-20T01:49:34Z

src/ec/curve25519/ed25519.rs

+/// for signing, see [Ed25519KeyPair](./Ed25519KeyPair.html) instead.
+pub struct Ed25519KeyPairComponents {
+    /// The 32-byte private part of this pair, i.e. the seed.
+    pub private_key: [u8; 32],


This should be called seed.

briansmith · 2017-10-20T01:50:04Z

src/ec/curve25519/ed25519.rs

+    /// `Ed25519KeyPair::from_pkcs8_maybe_unchecked()` instead.
+    pub fn from_pkcs8(input: untrusted::Input)
+                      -> Result<Ed25519KeyPairComponents, error::Unspecified> {
+        let (seed, public_key) = unwrap_pkcs8(pkcs8::Version::V2Only, input)?;


Once you have seed and public_key, you can return Ed25519KeyPairComponents { seed, public_key }. In particular, you don't need to do any further validation.

briansmith · 2017-10-20T01:50:46Z

src/ec/curve25519/ed25519.rs

+    /// public key). This also detects any corruption of the public or private
+    /// key.
+    pub fn from_seed_and_public_key(seed: untrusted::Input,
+                                    public_key: untrusted::Input)


If you already have the seed and the public key, then you already had the components you needed so there's nothing to do. Thus, this function isn't necessary in Ed25519KeyPairComponents.

Actually, I need it, because keys from SSH key files are not always in PKCS#8, and I still need to get their components.

Sorry, I don't understand. There's already a public pub fn from_seed_and_public_key() function in Ed25519KeyPair so you can just use it directly without using Ed25519KeyPairComponents at all, right?

briansmith · 2017-10-20T01:52:02Z

src/ec/curve25519/ed25519.rs

+    /// Since the public key is not given, the public key will be computed from
+    /// the private key. It is not possible to detect misuse or corruption of
+    /// the private key since the public key isn't given as input.
+    pub fn from_seed_unchecked(seed: untrusted::Input)


Are you actually using this function in your key server? I suspect you're not, if not, let's remove it. Otherwise, I am curious why you need it, as I suspect you shouldn't need it.

briansmith

This looks like it's on the right track. Besides the changes I mentioned elsewhere, the constructors for RSAKeyPair and Ed25519KeyPair need to be rewritten in terms of RSAKeyPairComponents and Ed25519KeyPairComponents. For example, RSAKeyPair::from_pkcs8() should be something like RSAKeyPairComponents::from_pkcs8(input).,map(Self::from_components) where

/// Note: not public.
fn from_components<'a>(components: RSAKeyPairComponents<'a> -> Result<Self, error::Unspecified> {
    // Most of the code that's currently in `from_der()`
}

briansmith · 2017-10-20T01:52:32Z

src/ec/curve25519/ed25519.rs

+    }
+
+    /// Returns a reference to the little-endian-encoded public key bytes.
+    pub fn public_key_bytes(&'a self) -> &'a [u8] {


This function isn't needed because self.public_key is public.

briansmith · 2017-10-20T01:53:08Z

src/rsa/signing.rs

+    /// Prime factor of n (big endian).
+    pub p: untrusted::Input<'a>,
+    /// Prime factor of n (big endian).
+    pub q: untrusted::Input<'a>,


This is missing dP and dQ. Also, make sure these are all in the same order as they're given in the RFC.

briansmith · 2017-10-20T01:55:33Z

src/signature.rs

@@ -302,14 +302,15 @@ pub use ec::curve25519::ed25519::{
    ED25519,

    Ed25519KeyPair,
+    Ed25519KeyPairComponents,


This should be exposed from ring::signature::primitives not from ring::signature.

briansmith · 2017-10-20T01:55:56Z

src/signature.rs

    ED25519_PKCS8_V2_LEN,
    ED25519_PUBLIC_KEY_LEN,
 };

 pub use pkcs8::PKCS8Document;

 #[cfg(all(feature = "rsa_signing", feature = "use_heap"))]
-pub use rsa::signing::{RSAKeyPair, RSASigningState};
+pub use rsa::signing::{RSAKeyPair, RSAKeyPairComponents, RSASigningState};


Ditto; this should be exposed from ring::signature::primitives, not ring::signature.

P-E-Meunier · 2017-10-20T07:20:49Z

They're already in the input. Notice in the code these two lines:

Sorry, I was imprecise. They are not always in the input in all cases. Sure, in PKCS#8, they are, but they are not stored by the SSH agent.

Oh wait, there's a "comment" field in the agent. Nevermind.

Thanks for the rest of the review!

P-E-Meunier · 2017-10-20T08:30:47Z

Ok, this works, but my server is only be able to exchange RSA keys with my client (not with OpenSSH). I really need a way to compute dP and dQ from the other parameters.

…onstructors.

coveralls · 2017-10-20T17:48:42Z

Coverage decreased (-0.6%) to 94.993% when pulling 8c36772 on P-E-Meunier:master into 33784df on briansmith:master.

coveralls · 2017-10-21T00:30:25Z

Coverage decreased (-0.6%) to 94.993% when pulling 8c36772 on P-E-Meunier:master into 33784df on briansmith:master.

P-E-Meunier · 2017-10-21T05:18:32Z

By the way, I just fixed my problem by using the num-bigint crate. I now have a client and server for SSH-agent (the only missing feature is smartcards).

coveralls · 2017-10-25T15:57:07Z

Coverage decreased (-0.6%) to 94.946% when pulling ba228e1 on P-E-Meunier:master into 33784df on briansmith:master.

briansmith · 2017-11-04T00:37:22Z

src/ec/curve25519/ed25519.rs

@@ -228,6 +228,13 @@ impl<'a> Ed25519KeyPairComponents {
        Self::from_seed_unchecked(seed)
    }

+    /// Constructs an `Ed25519KeyPair` from these components.
+    pub fn to_key_pair(&self) -> Result<Ed25519KeyPair, error::Unspecified> {


I specifically want to avoid having a to_key_pair function here, so let's remove this commit. Users can implement this logic themselves if they want.

I specifically want to avoid having a to_key_pair function here, so let's remove this commit. Users can implement this logic themselves if they want.

Never mind this, I misunderstood and I've changed my mind. Ignore the previous comment.

briansmith · 2017-11-04T01:12:42Z

Sorry, I started to review this and then realized it's going to take too much time for me to do today. Ignore my earlier comments. I'll think through what I want to do here and return to the review another day.

briansmith · 2017-12-28T01:01:57Z

Thanks for submitting this. I understand it's not needed by the original submitter any longer so I'm closing this. I think we'll add this functionality one way or another soon.

P-E-Meunier added 2 commits October 18, 2017 22:11

Exporting all components of RSA secret keys

8ea2360

This also makes it possible to derive a public key from a secret key.

Exporting the components of Ed25519 keypairs.

e892a5c

P-E-Meunier changed the title ~~Exporting all components of RSA secret keys~~ Exporting all components of RSA and ED25519 secret keys Oct 18, 2017

briansmith reviewed Oct 20, 2017

View reviewed changes

briansmith requested changes Oct 20, 2017

View reviewed changes

P-E-Meunier added 2 commits October 20, 2017 14:39

Module signatures, and using the "components" structures in keypair c…

80210b2

…onstructors.

Removing useless constructors for Ed25519KeyPairComponents

8c36772

Add a .to_key_pair() method to Ed25519 key pair components

ba228e1

This was referenced Oct 25, 2017

Separate DER-handling code from the main crate #586

Closed

Support AES-encrypted DER-encoded private keys #577

Closed

briansmith reviewed Nov 4, 2017

View reviewed changes

jprider63 mentioned this pull request Dec 7, 2017

Exporting components of secret keys #579

Open

briansmith closed this Dec 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exporting all components of RSA and ED25519 secret keys #582

Exporting all components of RSA and ED25519 secret keys #582

P-E-Meunier commented Oct 18, 2017

coveralls commented Oct 19, 2017 •

edited

Loading

coveralls commented Oct 19, 2017

coveralls commented Oct 19, 2017

P-E-Meunier commented Oct 19, 2017

coveralls commented Oct 19, 2017 •

edited

Loading

P-E-Meunier commented Oct 19, 2017

briansmith commented Oct 20, 2017

briansmith Oct 20, 2017

briansmith Oct 20, 2017

briansmith Oct 20, 2017 •

edited

Loading

P-E-Meunier Oct 20, 2017

briansmith Nov 4, 2017

briansmith Oct 20, 2017

briansmith left a comment

briansmith Oct 20, 2017

briansmith Oct 20, 2017

briansmith Oct 20, 2017

briansmith Oct 20, 2017

P-E-Meunier commented Oct 20, 2017 •

edited

Loading

P-E-Meunier commented Oct 20, 2017

coveralls commented Oct 20, 2017 •

edited

Loading

coveralls commented Oct 21, 2017 •

edited

Loading

P-E-Meunier commented Oct 21, 2017

coveralls commented Oct 25, 2017 •

edited

Loading

briansmith Nov 4, 2017

briansmith Nov 4, 2017

briansmith commented Nov 4, 2017

briansmith commented Dec 28, 2017

Exporting all components of RSA and ED25519 secret keys #582

Exporting all components of RSA and ED25519 secret keys #582

Conversation

P-E-Meunier commented Oct 18, 2017

coveralls commented Oct 19, 2017 • edited Loading

coveralls commented Oct 19, 2017

coveralls commented Oct 19, 2017

P-E-Meunier commented Oct 19, 2017

coveralls commented Oct 19, 2017 • edited Loading

P-E-Meunier commented Oct 19, 2017

briansmith commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansmith Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

P-E-Meunier commented Oct 20, 2017 • edited Loading

P-E-Meunier commented Oct 20, 2017

coveralls commented Oct 20, 2017 • edited Loading

coveralls commented Oct 21, 2017 • edited Loading

P-E-Meunier commented Oct 21, 2017

coveralls commented Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansmith commented Nov 4, 2017

briansmith commented Dec 28, 2017

coveralls commented Oct 19, 2017 •

edited

Loading

coveralls commented Oct 19, 2017 •

edited

Loading

briansmith Oct 20, 2017 •

edited

Loading

P-E-Meunier commented Oct 20, 2017 •

edited

Loading

coveralls commented Oct 20, 2017 •

edited

Loading

coveralls commented Oct 21, 2017 •

edited

Loading

coveralls commented Oct 25, 2017 •

edited

Loading