Allow exporting public components of `RSAKeyPair`. #441

stephank · 2017-02-01T20:51:14Z

In Portier, we load RSA private keys once, leveraging the DER parsing in Ring. However, we need to then serve the public keys as JWK to clients. These additions allow us to grab the public components of an RSAKeyPair to build the JWK.

I agree to license my contributions to each file under the terms given at the top of each file I changed.

stephank · 2017-02-02T13:47:37Z

That is one huge CI setup. :)

Thinking some more about just what's in this PR, I have 2 questions myself:

Is the export_ naming ok? I considered read_, but it also sounds weird. Something else?
While simpler, I think it's probably inconsistent and less useful to expose the public exponent as an u64? Now considering treating it like the modulus, adding public_exponent_len and export_public_exponent instead.

For the opposite direction (loading JWK), I want to propose an API change. This can be a separate PR just fine. My Rust is still not as fluent as I'd like it to be, but I think this should be possible:

Formalize the (n, e) pair we pass around as a public struct RSAPublicKey. (Some of the src/rsa/rsa.rs methods could probably be moved to this type.)
Replace n and e in RSAKeyPair, with an RSAPublicKey field.
Move current public component RSAKeyPair accessors to RSAPublicKey. (Can drop the public_ prefix.)
Provide an accessor for &RSAPublicKey on RSAKeyPair.
Provide bigint accessors on RSAKeyPair for the private components, similar to what this PR does for the public components. (For completeness)
Provide constructors for DER and BE bigint byte arrays.
Add a public key type field to the trait VerificationAlgorithm, and a verify_with_key method as an alternative to verify, which operates on an instance of that type.
Apply same to Ed25519. Hopefully smooth. :)

Does that sound good?

briansmith

Thanks for contributing this.

Please make the requested changes. Also, please add the contributor's statement to the end of your commit messages; see https://github.com/briansmith/ring#contributing.

briansmith · 2017-02-02T21:15:33Z

src/rsa/signing.rs

+    /// Exports the public modulus as big endian to the output array.
+    ///
+    /// The array should be of at least the size returned by
+    /// `public_modulus_len`.


I suggest this (with fixed line wrapping to 80 characters):

/// Extracts the public modulus. /// /// `out` must be exactly `public_modulus_len()` bytes long. It /// will be filled with the big-endian-encoded bytes of the /// public key, without any padding.

briansmith · 2017-02-02T21:19:02Z

src/rsa/signing.rs

+    /// `public_modulus_len`.
+    pub fn export_public_modulus(&self, out: &mut [u8])
+                    -> Result<(), error::Unspecified> {
+        self.n.fill_be_bytes(out)


fill_be_bytes() zero-pads the result. I think that JWT and similar things don't want zero-padded output, so there needs to be a check that out is exactly the right length. This check would make this symmetric with the ring::signature::verify_rsa() and other APIs exposed by ring.

Also, let's add unit tests that verify that we reject 1-byte-too-long and 1-byte-too-short out, and accept correct-length out.

Shouldn't we leave this to the user? The user must allocate enough space somehow based on public_modulus_len. If there's not enough space, fill_be_bytes errors, if there's more space, it's padded, but that may be the user's intent. Doesn't verify_rsa also accept padded input?

I'll look into the unit tests!

Here's an excerpt from the documentation for verify_rsa:

/// `n` is the public key modulus and `e` is the public key exponent. Both are /// interpreted as unsigned big-endian encoded values. Both must be positive /// and neither may have any leading zeros.

These requirements come directly from JWA https://tools.ietf.org/html/rfc7518#section-6.3.1.1 (for n):

Note that implementers have found that some cryptographic libraries prefix an extra zero-valued octet to the modulus representations they return, for instance, returning 257 octets for a 2048-bit key, rather than 256. Implementations using such libraries will need to take care to omit the extra octet from the base64url-encoded representation.

and (for e):

For instance, when representing the value 65537, the octet sequence to be base64url-encoded MUST consist of the three octets [1, 0, 1];

Thus supporting extra zero padding doesn't do anybody any good, AFAICT.

briansmith · 2017-02-02T21:22:45Z

src/rsa/signing.rs

+
+    /// Return the public exponent.
+    pub fn public_exponent(&self) -> u64 {
+        self.e.value()


Maybe it's better to output the exponent as a big-endian byte slice, like the modulus? That would be symmetric with ring::signature::verify_rsa and export_public_modulus(), and it is probably what all the callers need anyway.

briansmith · 2017-02-02T21:28:16Z

src/rsa/bigint.rs

+            GFp_BN_bn2bin_padded(out.as_mut_ptr(), out.len(),
+                                 self.as_ref())
+        })
+    }


Let's move this unsafe implementation of fill_be_bytes() to Nonnegative, and then make the Modulus and Elem implementations be simple wrappers around it.

I'm not sure how to do this. Modulus contains a BN_MONT_CTX, which owns a BIGNUM, but Nonnegative also wants ownership of its BIGNUM.

bit_length has the same issue. I can try to add both to BIGNUM, then make the others wrappers.

briansmith · 2017-02-02T21:37:15Z

src/rsa/bigint.rs

@@ -218,6 +218,21 @@ pub struct Modulus<M> {
    ring: PhantomData<M>,
 }

+impl<M> Modulus<M> {


Please change this to (wrapping at 80 characters):

/// Exporting information about the value is only allowed for the public /// modulus `N`, not for private moduli. impl Modulus<super::N> {

Why would we not allow this? JWK also has a private key mapping: https://tools.ietf.org/html/rfc7517#appendix-A.2

Why would we not allow this? JWK also has a private key mapping: https://tools.ietf.org/html/rfc7517#appendix-A.2

In ring, when you construct a private or symmetric key object, there's no way to get the key material out. This way, you can safely pass an &RSAKeyPair or &OpeningKey to a function and know that that function won't ever leak the key.

If you look at Ed25519KeyPair::generate_serializable, you can see that when we generate a key pair, we also output the bytes needed to serialize the key, along with the generated key pair. When we add RSA key generation to ring, we'll do it the same way.

briansmith · 2017-02-02T21:45:47Z

Is the export_ naming ok? I considered read_, but it also sounds weird. Something else?

It isn't perfect but I don't have a better suggestion either.

While simpler, I think it's probably inconsistent and less useful to expose the public exponent as an u64? Now considering treating it like the modulus, adding public_exponent_len and export_public_exponent instead.

I agree.

For the opposite direction (loading JWK), I want to propose an API change.

Your suggestion is now tracked in #445. Let's discuss it there. I'll need some time to process it.

briansmith · 2017-02-02T21:57:43Z

This would fix #299.

coveralls · 2017-02-03T20:30:54Z

Changes Unknown when pulling 6a27497181f8097be144131baa6d5c4201bfad25 on stephank:feat/rsa-export into ** on briansmith:master**.

stephank · 2017-02-04T10:17:45Z

I've addressed feedback as far as I could.

I don't see a reason why we wouldn't allow exporting the private components too, though I'm also not sure what that API would look like. Doing the same for all RSA private parts would add a slew of methods. But I don't have anything else.

coveralls · 2017-02-04T13:14:29Z

Coverage increased (+0.01%) to 93.538% when pulling b7b00d6db6545e9ac0eab59cce86a4d6c7f1a2ad on stephank:feat/rsa-export into cb57b1e on briansmith:master.

coveralls · 2017-02-04T16:16:47Z

Coverage increased (+0.02%) to 93.547% when pulling abec71d on stephank:feat/rsa-export into cb57b1e on briansmith:master.

coveralls · 2017-02-04T20:46:13Z

Coverage increased (+0.02%) to 93.547% when pulling abec71d on stephank:feat/rsa-export into cb57b1e on briansmith:master.

Only reason this wasn't possible because verification needed the number of bits. Added an implementation of `bit_length` to `Modulus` to make this possible. I agree to license my contributions to each file under the terms given at the top of each file I changed.

I agree to license my contributions to each file under the terms given at the top of each file I changed.

Used internally in place of passing around an `(n, e)` pair, and as part of `RSAKeyPair`, but also part of the public API. Some of the `src/rsa/rsa.rs` functions and `RSAKeyPair` methods pertaining to the public key components have been moved to `RSAPublicKey` methods. Parts of `check_public_modulus_and_exponent` are now found in the `RSAPublicKey` constructors, while the `n_min_bits` and `n_max_bits` checks are still in a separate `check_modulus` method. (The parameters differ between verification and signing.) I agree to license my contributions to each file under the terms given at the top of each file I changed.

briansmith · 2018-06-02T20:55:29Z

@stephank Sorry for the long delay on this.

I independently did some foundational work for this. For example, I added the equivalent of the RSAPublicKey type here:

ring/src/rsa/verification.rs

Lines 24 to 25 in 0c0f7c4

    
           #[derive(Debug)] 
        
           pub struct Key {

Now there are a lot of conflicts. I'm going to close this PR since there's an issue open for getting RSA public keys from RSA private keys and I think we should explore a new approach to resolving that.

stephank · 2018-06-03T07:03:17Z

@briansmith No worries! Thanks for all the effort you're putting into ring & co, and sorry I couldn't keep up. :)

There are two ways to configure an X509_STORE_CTX after X509_STORE_CTX_init. One can either modify the already initialized X509_VERIFY_PARAM or replace it. Modifying the existing one is more common. Replacing it actually misses some defaults. (See issue #441 for details.) In preparation for actually being able to test changes to the default, switch tests to that model. In doing so, no longer need to explicitly configure the depth and can test that default. (Though we should write tests for the depth at some point.) Bug: 439, 441 Change-Id: I254a82585d70d44eb94920f604891ebfbff4af4c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49745 Commit-Queue: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]> Reviewed-by: Adam Langley <[email protected]>

stephank mentioned this pull request Feb 1, 2017

[WIP] Update dependencies. portier/portier-broker#130

Closed

3 tasks

briansmith requested changes Feb 2, 2017

View reviewed changes

briansmith mentioned this pull request Feb 2, 2017

New API for verifying public key signatures using keys constructed by components #445

Open

stephank force-pushed the feat/rsa-export branch from 6a27497 to 13a4458 Compare February 4, 2017 10:09

stephank force-pushed the feat/rsa-export branch from 13a4458 to b7b00d6 Compare February 4, 2017 12:37

stephank force-pushed the feat/rsa-export branch from b7b00d6 to abec71d Compare February 4, 2017 14:21

Stéphan Kochen added 3 commits February 4, 2017 23:31

Allow exporting public components of RSAKeyPair.

4335850

I agree to license my contributions to each file under the terms given at the top of each file I changed.

stephank force-pushed the feat/rsa-export branch from abec71d to 15f5e6b Compare February 4, 2017 22:32

stephank mentioned this pull request Feb 5, 2017

forked. Have features, want to contribute. Keats/jsonwebtoken#13

Closed

P-E-Meunier mentioned this pull request May 20, 2017

Export all components of all keys (secret and public) #545

Closed

briansmith closed this Jun 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow exporting public components of `RSAKeyPair`. #441

Allow exporting public components of `RSAKeyPair`. #441

stephank commented Feb 1, 2017 •

edited

Loading

stephank commented Feb 2, 2017

briansmith left a comment

briansmith Feb 2, 2017

briansmith Feb 2, 2017

stephank Feb 3, 2017

briansmith Feb 3, 2017

briansmith Feb 2, 2017

briansmith Feb 2, 2017

stephank Feb 4, 2017

briansmith Feb 2, 2017

stephank Feb 4, 2017

briansmith Feb 4, 2017

briansmith commented Feb 2, 2017

briansmith commented Feb 2, 2017

coveralls commented Feb 3, 2017 •

edited

Loading

stephank commented Feb 4, 2017

coveralls commented Feb 4, 2017 •

edited

Loading

coveralls commented Feb 4, 2017 •

edited

Loading

coveralls commented Feb 4, 2017 •

edited

Loading

briansmith commented Jun 2, 2018

stephank commented Jun 3, 2018

Allow exporting public components of RSAKeyPair. #441

Allow exporting public components of RSAKeyPair. #441

Conversation

stephank commented Feb 1, 2017 • edited Loading

stephank commented Feb 2, 2017

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

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

Choose a reason for hiding this comment

briansmith commented Feb 2, 2017

briansmith commented Feb 2, 2017

coveralls commented Feb 3, 2017 • edited Loading

stephank commented Feb 4, 2017

coveralls commented Feb 4, 2017 • edited Loading

coveralls commented Feb 4, 2017 • edited Loading

coveralls commented Feb 4, 2017 • edited Loading

briansmith commented Jun 2, 2018

stephank commented Jun 3, 2018

Allow exporting public components of `RSAKeyPair`. #441

Allow exporting public components of `RSAKeyPair`. #441

stephank commented Feb 1, 2017 •

edited

Loading

coveralls commented Feb 3, 2017 •

edited

Loading

coveralls commented Feb 4, 2017 •

edited

Loading

coveralls commented Feb 4, 2017 •

edited

Loading

coveralls commented Feb 4, 2017 •

edited

Loading