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

[briansmith]

In #441 (comment), @stephank suggested:

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. :)

[stephank]

Thanks for creating the issue! I'm already taking a stab at this, and will probably have something to show soon. :) (I'll make sure it's on top of #441, keeping that PR free of breaking changes.)

[briansmith]

I don't have time to write everything I'd like here, but here are three things to keep in mind off the top of my head:

1. You never have to worry about API compatibility when contributing to the ring project. We're up for improving the API at any compatibility cost unless/until otherwise documented.
2. It is OK to create RSAPublicKey, ECDSAPublicKey, etc., I suppose. But, the components should be of type &[u8] so that they support non-allocating, non-copying, non-stack-space-wasting operation.
3. I believe similar strictures are not needed for Ed25519 because an Ed25519 public key is just one component, there's only one standard way to encode it, and we already provide a way to extract it.

[cmsd2]

Do the key parts (the bignums) need to go through the same validation that from_der does to make sure ring is able to use the loaded key?

[stephank]

As @cmsd2 stated, that was the direction I was working in.

Even for Ed25519, I introduced a pub struct Ed25519PublicKey<'a>(&'a [u8]), with a from_bytes that just checks if it's 32 bytes. Ed25519 is the least interesting, but in general I've moved most checks to constructors.

RSAPublicKey contains n: bigint::Modulus, e: bigint::PublicExponent, so DER is preparsed. Though verify and the RSAKeyPair constructors still call a check_modulus method, because they have different restrictions on the size of the modulus.

So in terms of complexity, the intent is to just move things around, and not introduce any additional copying or allocation than was already the case. (But allow a bit more control, and important for us, more ways to create and inspect keys.)

I'm still working on ECDSA, but should have a PR open soon. For the curious, here's the branch as I'm working on it: master...stephank:feat/rsa-export-2

Note that I'm often rewriting history here, so I don't base any work on this yet. :)

[briansmith]

Do the key parts (the bignums) need to go through the same validation that from_der does to make sure ring is able to use the loaded key?

If I understand correctly: The public constructor for RSAPublicKey needs to do the checks that verify_rsa currently does, using the same limits that verify_rsa uses. RSAKeyPair::from_der needs to use different limits, so it needs to use a different, private, constructor. Something like:

impl<'a> RSAPublicKey<'a> {
    pub fn from_n_and_e(alg: &RSAParameters, n: &'a [u8], e: &'a [u8]) -> Self<'a> {
         from_n_and_e_private(alg.min_bits, n, e)
    }

    // Used by `from_n_and_e()` and by `RSAKeyPair::from_der()`.
    fn from_n_and_e_private(n_bits: bits::BitLength, n: &'a [u8], e: &'a [u8]) -> Self<'a> {
        // The implementation from `check_public_modulus_and_exponent` goes here.
    }
}

[briansmith]

Oops, the return type should be a Result<Self<'a>, ...>, not a bare Self<'a>, of course.

[stephank]

@briansmith Thanks for thinking along here. I'm curious to see if I've missed opportunities in the implementation. :)

I did try separate public/private constructors for RSAPublicKey, but found it confusing to have one type with two different guarantees. Instead, I kept the public key check in RSAPublicKey::verify, and the private key check in RSAKeyPair::from_der, like ring does currently.

I don't know enough about the internal representation of BIGNUM and BN_MONT_CTX to know if we can create instances of RSAPublicKey that keep references to the original data. Unless you meant constructing the bigints twice, on from_der and again on verify?

The setup I built now is to have RSAPublicKey contain the bigint types, so they can be cached and the user can free original data. The old behaviour is effectively still possible by creating RSAPublicKey lazily/on-demand.

And that's more or less what I've applied to all types of signatures. The code paths are nearly the same as they were, just shuffled around a bit.

I've finished work on the branch at master...stephank:feat/rsa-export-2 for now. It's on top of #441, so I can either push the remaining commits there, or wait until we finish that work, then create a new PR.

Significantly, I've completely removed signature::verify and VerificationAlgorithm. The API is more like signing: RSAPublicKey::verify (parameters passed on verify), ECDSAPublicKey::verify (parameters bound to key, passed at construction) and Ed25519PublicKey::verify (parameters unit struct removed).

The one concern I have myself is that signature::verify did a call to init_once. I'm not sure what paths required this to be called, and maybe we need to reinsert it elsewhere.

[briansmith]

Significantly, I've completely removed signature::verify and VerificationAlgorithm. The API is more like signing: RSAPublicKey::verify (parameters passed on verify), ECDSAPublicKey::verify (parameters bound to key, passed at construction) and Ed25519PublicKey::verify (parameters unit struct removed).

Don't do this. To understand why, try rebasing https://github.com/briansmith/webpki and https://github.com/ctz/rustls on top of your changes.

[briansmith]

I don't know enough about the internal representation of BIGNUM and BN_MONT_CTX to know if we can create instances of RSAPublicKey that keep references to the original data. Unless you meant constructing the bigints twice, on from_der and again on verify?

The internal representation of BIGNUM and BN_MONT_CTX is a little-endian array of words (32-bit on 32-bit systems, 64-bit on 64-bit systems), not a big-endian array of bytes. Basically, your proposed changes to the RSAKeyPair API can't work, for this reason.

[briansmith]

Basically, your proposed changes to the RSAKeyPair API can't work, for this reason.

Oh, sorry, I misread a previous comment of yours. I need to check if it works reasonable given the memory usage requirements we have.

[briansmith]

Hi, in order for me to look at your code changes, I need you to submit them as PRs where all commits have the contributor statement (see https://github.com/briansmith/ring#contributing) at the end.

One thing to keep in mind is that ring is optimized for the case where keys and signatures are encoded in the formats that it supports natively, which is why we have ring::signature::verify with that API. I want keep the existing API (perhaps with improvements that benefit users of that API) and add the minimum necessary to allow people to implement JOSE stuff in their own crates.

[stephank]

Alright, that all makes sense, and I've taken a few wrong directions here and there.

In light of that, I've pushed one extra commit to #441, adding just the RSAPublicKey API on top of the existing API. With that, I think #441 should solve this issue at least for RSA.

The remaining stuff is of no real benefit, I think. I actually didn't think about how to export/import ECDSA keys. The work I did before allowed for just 'caching' the parse_compressed_point step, but no real component access.

I also added the missing contributor statements to the commits.

Thanks again for taking the time to review and provide feedback. :)

[briansmith]

Alright, that all makes sense, and I've taken a few wrong directions here and there.

I do think all your changes make sense. It's just that there are competing concerns regarding memory (stack) usage and other concerns that they have to be weighed against.

Are you working on JWT? If so, maybe we should have a discussion with the (several) other people who are working on JWT support to come to an agreement on exactly what we need to change in ring, and also perhaps find a way for people to work together on a common JWT library based on ring.

[stephank]

I do think all your changes make sense. It's just that there are competing concerns regarding memory (stack) usage and other concerns that they have to be weighed against.

Well, I certainly wrecked important API in the webpki context. 😁 I also do see the value in preventing access to private components. It'd be better to have something along the lines of Ed25519KeyPairBytes for RSA and ECDSA eventually.

Are you working on JWT? If so, maybe we should have a discussion with the (several) other people who are working on JWT support to come to an agreement on exactly what we need to change in ring, and also perhaps find a way for people to work together on a common JWT library based on ring.

Yes, I work on Portier, which is both an OpenID Connect relying party and identity provider. We currently have our own bare-bones JWT implementation that does just RS256.

I have an in-progress (but working!) branch of Portier on top of #441: portier/portier-broker#130

I see you already pinged some JWT people. Would it make more sense to have that discussion in Keats/jsonwebtoken#13 and #441? This thread has gotten a bit lengthy in a different direction already.

[briansmith]

@stephank Please save your branch where you did your bigger refactoring somewhere safe. I actually am considering doing something like that, but I don't have time to work on it until after April, and I'm not 100% sure I want to commit to doing it.

[lawliet89]

Are you working on JWT? If so, maybe we should have a discussion with the (several) other people who are working on JWT support to come to an agreement on exactly what we need to change in ring, and also perhaps find a way for people to work together on a common JWT library based on ring.

Sorry to dig up an old thread. Adding my two cents since I have been working on a JOSE library (https://github.com/lawliet89/biscuit) that was based off https://github.com/Keats/rust-jwt initially, but I have had to change the API surface almost completely to add support for JWE, among other things.

So far, I have managed to whip up some semblance of the JWK representation, and as ring currently does not support asymmetric encryption (and AES GCM only requires an octet sequence), it works fine for content and key encryption for JWE. If/when ring adds support for, say, RSAES-PKCS1-v1_5 or RSA_OAEP, then the API suggested in this thread would be useful.

On the other hand, ring and my library supports signing/verifying signatures via RSA, and because there is no way to convert between the JWK representation and the format that ring requires, the library is currently using some weird Secret enum that I would like to remove and use a JWK instead.

[briansmith]

Amusingly, I implemented some of this, but in trust-dns: https://github.com/bluejekyll/trust-dns/blob/master/client/src/rr/dnssec/rsa_public_key.rs and https://github.com/bluejekyll/trust-dns/blob/master/client/src/rr/dnssec/rsa_public_key.rs. It seems like a good idea to copy/move that stuff into ring.

[kinosang]

Any update?