Implement decode_rsa, verify_rsa for using public key components instead of DER key. #69

AaronFriel · 2019-01-03T06:45:56Z

This addresses #68 and enables supporting JWTs from JSON Web Key sets where the JWK set exposes the (n, e) tuple.

Keats

Thanks for the PR!
Is it possible to get the modulus/exponent from a .pem file? I am guessing most people will have a .pem file so it would be nice to have it work automagically.

I am a bit busy with Tera and Zola currently but am planning to work on this crate afterwards (along with #66) so it will be merged at that time.

src/crypto.rs

AaronFriel · 2019-01-05T19:37:10Z

@Keats, the use case is not wider file format support than .DER. This is about handling RFC 7517 - JSON Web Key (JWK). Here is Google's JWKS document, served from https://www.googleapis.com/oauth2/v3/certs:

{
  "keys": [
    {
      "kid": "7978a91347261a291bd71dcab4a464be7d279666",
      "e": "AQAB",
      "kty": "RSA",
      "alg": "RS256",
      "n": "sFlU5LpHUtYIm7B27iiu7c4ZPZk7ULUNmFdMVsTmYJxJqQBKUIKU9ozwF6TlUsECmYUMLpQhX_iHuaZRcpG2YiG7jbmi9HMlonIXX7uUe7PIf8rNHhveX_VI7ZpwPTnab3_7ciy_o8ZFde6KNltkx_DLRO6hXf6z6ow1APFIIriaNlF8niz5cy0fPIv0e_Z2p13Sz3mnAACjBKZGPw2X9GWh5XpRoDEQBcibXpeLuA7ti8zLZuH-9ybXOoou699fr4QHFxUkcd_8fFqmzO5PKnlOnJZ0gtuXCCYYc9XPX-WSqlqbGNMZy0Giu2wHbNbeWdepkgVlGuJonTnMx4gLuQ",
      "use": "sig"
    },
    {
      "kid": "8aad66bdefc1b43d8db27e65e2e2ef301879d3e8",
      "e": "AQAB",
      "kty": "RSA",
      "alg": "RS256",
      "n": "vvAaaSpfr934Qx0ioFiWsopq7UCfLNn0zjYVbq4bvUcGSXU9kowYmQArR7WlIkjk1moffla0UV75QRaQPATva1oD5xQnnW-20haeMWTSsMgUHoN0Np9AD8ffPz-DfMJBOHIo4REL1BFFS33HSZgPl0hxJ-5UScqr4lW1JMy5XGeRho30dnmKTpakU1Oc35hFYKSea_O2SXfmbqiAkWlWkilEzgHq4pzVWiDZe4ZgfMdD4vqkSNrO_PkBFBT1mnBJztQ1h4v1jvUW-zeYYwIcPTaOX-xOTiGH9uQkcNPpe5pBrIZJqR5VNrDl_bJOmvVlhhXZSn4fkxA8kyQcZXGaTw",
      "use": "sig"
    }
  ]
}

We only have access to a base64url encoded modulus and exponent. In order for me to use this library with JWKS, I need to be able to decode tokens using the modulus and exponent tuple.

…ead of DER key. Signed-off-by: Aaron Friel <aaron@twochairs.com>

jamwaffles · 2019-01-12T12:49:05Z

Another use case that I'm after is taking an RSA private key file from a Google Cloud service account JSON definition. It would be great to see RSA support added instead of having to extract the key using jq and converting it to a DER file. Does this PR address using PEM keys with the encode() method?

AaronFriel · 2019-01-30T18:00:56Z

@Keats I addressed your requested changes.

Keats · 2019-02-04T17:49:36Z

To be sure I don't forget anything, RSA keys can be set:

in DER format: currently the case
with (n, e) format: this PR
in PKCS 8: Attempt to fallback to pkcs8 if der fails #74

@jamwaffles I believe #74 fixes your issue no? Looks like it's for Google Cloud services as well

AaronFriel · 2019-02-07T05:35:30Z

No, PKCS #8 is typically base64 encoded in the standard .pem format:

That's RFC 5208.

This is to handle RFC 7517.

AaronFriel · 2019-02-18T19:32:11Z

@Keats Is there a blocker in this? This library isn't usable in a JWKS environment (like those configured by Istio, or Google's JWKS infrastructure, etc.) without the ability to decode based solely on the exponent and modulus.

Keats · 2019-02-19T08:57:46Z

Not anything blocking this PR in particular, but the next version is blocked on #76 to avoid having to publish multiple major versions

Terkwood · 2019-02-19T10:08:11Z

Nicely done. Thanks to all contributors on this line of work, and the library itself! 🤸‍♂️

…

On Tue, Feb 19, 2019, 03:58 Vincent Prouillet ***@***.***> wrote: Not anything blocking this PR in particular, but the next version is blocked on #76 <#76> to avoid having to publish multiple major versions — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#69 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AlDziMeGsQA4Tk-VigSmmvGB3aYvTYjgks5vO7zLgaJpZM4Znofq> .

AaronFriel · 2019-02-26T18:48:53Z

@Keats this should be semver compatible as a "minor" bump, barring API changes right?

Keats · 2019-02-26T19:23:51Z

It would work as a minor bump but then if the plan in #76 goes ahead would be removed very soon after.
Hopefully I can find some time this weekend to work on #76 to have a clearer idea

manifest · 2019-03-05T20:01:17Z

@Keats We don't want to pass a PEM encoded or JWK encoded text representation of a binary key as an argument of jsonwebtoken::decode function and decode the key each time the function is called, right?

manifest · 2019-03-05T20:06:09Z

I'm trying to understand if this feature somehow relates or blocks the feature with ES256 support.

Keats · 2019-03-08T22:34:34Z

We don't want to pass a PEM encoded or JWK encoded text representation of a binary key as an argument of jsonwebtoken::decode function and decode the key each time the function is called, right?

Have a look at the first post of #76, I think providing a function for each usecase is going to be easier in the end as we can see it's going to be too messy trying to make everything fit in a single function as we can already see.

manifest · 2019-03-09T03:27:04Z

Have a look at the first post of #76

Ok, I see. Having decoder types seems as a good idea.

Keats · 2019-03-22T08:38:12Z

Can someone look at the conflicts with after the ES support has been merged in next?

mann-ed · 2019-03-30T01:04:58Z

@AaronFriel and @Keats I am using Aaron's branch as i'm working on a keycloak adaptor for a project i'm on. I am able to get things to work, and would like to see this merged. If needed i can look at resolving the merge conflicts.

AaronFriel · 2019-04-03T01:12:14Z

@mann-ed do you want to take a look first?

mann-ed · 2019-04-03T04:43:12Z

@AaronFriel Yes, i'll take a look at it tomorrow. I'll get back to you by Friday April 5th as i have other obligations i need to attend to in the next few days.

…nwebtoken into decode-from-public-key-parts

mann-ed · 2019-04-06T05:20:46Z

@AaronFriel Ok, i have it updated, and i created a pull request to your branch. I don't know if that is what you wanted me to do.
AaronFriel#1

Let me know if you have time to review, or if there is something else i should do.

Keats · 2019-04-12T14:16:01Z

@mann-ed @AaronFriel I'll pull your changes directly next week unless there is an issue

Close button is too close...

Decode from public key parts

AaronFriel · 2019-05-19T22:21:01Z

@mann-ed @Keats Sorry, I haven't been doing the best at watching my GitHub notifications. I merged the PR from @mann-ed into my branch, which updated this pull request.

Keats · 2019-05-20T10:41:31Z

No worries @AaronFriel
Have you seen #89? It looks like a very sane approach to all the various key formats without duplicating all the functions

AaronFriel · 2019-05-30T19:01:46Z

@Keats would it be acceptable to add a key format for "KeyFormat::RSAModulusExponentPair"? Or what would you call it?

Keats · 2019-05-31T05:28:29Z

The name seems good to me. What do you think of this approach of using a KeyFormat enum?

AaronFriel · 2019-05-31T22:38:43Z

I'm not sure, to be honest, because it looks like it's assumed that the Key k must be a [u8], but that's not the case here. I'd have to create an (arbitrary) binary encoding of the modulus and exponent just to unwrap those later on, which seems silly.

I think the trait structure needs reworking, but I haven't dug into it enough to make a specific comment on how. verify taking a &[u8] is still not correct for my use case. I have an exponent and a modulus, which are already correctly encoded byte vectors. Concatenating them is not correct, and it seems very silly to serialize them to a byte array to pass to verify, then deserialize inside the method to recover the key pair.

It seems like instead maybe verify should be generic in a type K that is an instance of Key, and maybe Key has an associated type Data as well as a method that uses it. For PKCS8 and DER, Key::Data = &[u8], for my use case, Key::Data = (&[u8], &[u8])?

Keats · 2019-06-03T11:12:34Z

It seems like instead maybe verify should be generic in a type K that is an instance of Key, and maybe Key has an associated type Data as well as a method that uses it. For PKCS8 and DER, Key::Data = &[u8], for my use case, Key::Data = (&[u8], &[u8])?

Sounds like a good idea.

Keats · 2019-06-06T17:45:48Z

I'm thinking an enum would be better than a trait in the end, any reason to prefer a trait other than adding one being a breaking change?

cc @Jake-Shadle

Keats · 2019-06-16T16:08:59Z

@AaronFriel I pushed an enum version to #91
This way we can have a Key::ModulusExponent { n, e } enum member. I've made a temporary verify_ring_rsa in crypto.rs to extract the bytes from the enum which you can change/remove depending on what's easier.

Another thing I'm thinking is to maybe keep your sign_rsa/verify_rsa since that allows restricting the Key type at the type level rather than having an error at runtime.
Eg you can't verify a token HS512 with a modulus/exponent so it would be nice to error at compilation.

Keats · 2019-07-13T15:44:39Z

I've integrated that PR in 10105af

Can anyone try the next branch to see if that works for them?

Keats · 2019-08-12T21:26:22Z

Closing in favour of #91

AaronFriel force-pushed the decode-from-public-key-parts branch from bec027f to ca13b50 Compare January 3, 2019 23:34

Keats requested changes Jan 4, 2019

View reviewed changes

src/crypto.rs Outdated Show resolved Hide resolved

src/crypto.rs Outdated Show resolved Hide resolved

Implement decode_rsa, verify_rsa for using public key components inst…

8f02151

…ead of DER key. Signed-off-by: Aaron Friel <aaron@twochairs.com>

AaronFriel force-pushed the decode-from-public-key-parts branch from ca13b50 to 8f02151 Compare January 5, 2019 19:42

Keats mentioned this pull request Feb 4, 2019

v7 discussion #76

Closed

Keats changed the base branch from master to next March 22, 2019 08:27

ed-mann added 2 commits April 6, 2019 00:09

JWT fixes for pull request Keats#69

3b54ecf

Merge branch 'decode-from-public-key-parts' of github.com:mann-ed/jso…

5aaa1fe

…nwebtoken into decode-from-public-key-parts

Keats closed this Apr 12, 2019

Keats reopened this Apr 12, 2019

Keats mentioned this pull request May 15, 2019

Clarify Key formats #89

Merged

Merge pull request #1 from mann-ed/decode-from-public-key-parts

82128d6

Decode from public key parts

Keats mentioned this pull request Jun 22, 2019

Release new version #94

Closed

Keats closed this Aug 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement decode_rsa, verify_rsa for using public key components instead of DER key. #69

Implement decode_rsa, verify_rsa for using public key components instead of DER key. #69

AaronFriel commented Jan 3, 2019

Keats left a comment

AaronFriel commented Jan 5, 2019 •

edited

Loading

jamwaffles commented Jan 12, 2019 •

edited

Loading

AaronFriel commented Jan 30, 2019

Keats commented Feb 4, 2019

AaronFriel commented Feb 7, 2019

AaronFriel commented Feb 18, 2019

Keats commented Feb 19, 2019

Terkwood commented Feb 19, 2019 via email

AaronFriel commented Feb 26, 2019

Keats commented Feb 26, 2019

manifest commented Mar 5, 2019 •

edited

Loading

manifest commented Mar 5, 2019 •

edited

Loading

Keats commented Mar 8, 2019

manifest commented Mar 9, 2019 •

edited

Loading

Keats commented Mar 22, 2019

mann-ed commented Mar 30, 2019

AaronFriel commented Apr 3, 2019

mann-ed commented Apr 3, 2019 •

edited

Loading

mann-ed commented Apr 6, 2019

Keats commented Apr 12, 2019 •

edited

Loading

AaronFriel commented May 19, 2019 •

edited

Loading

Keats commented May 20, 2019

AaronFriel commented May 30, 2019

Keats commented May 31, 2019

AaronFriel commented May 31, 2019 •

edited

Loading

Keats commented Jun 3, 2019

Keats commented Jun 6, 2019

Keats commented Jun 16, 2019

Keats commented Jul 13, 2019

Keats commented Aug 12, 2019

Implement decode_rsa, verify_rsa for using public key components instead of DER key. #69

Implement decode_rsa, verify_rsa for using public key components instead of DER key. #69

Conversation

AaronFriel commented Jan 3, 2019

Keats left a comment

Choose a reason for hiding this comment

AaronFriel commented Jan 5, 2019 • edited Loading

jamwaffles commented Jan 12, 2019 • edited Loading

AaronFriel commented Jan 30, 2019

Keats commented Feb 4, 2019

AaronFriel commented Feb 7, 2019

AaronFriel commented Feb 18, 2019

Keats commented Feb 19, 2019

Terkwood commented Feb 19, 2019 via email

AaronFriel commented Feb 26, 2019

Keats commented Feb 26, 2019

manifest commented Mar 5, 2019 • edited Loading

manifest commented Mar 5, 2019 • edited Loading

Keats commented Mar 8, 2019

manifest commented Mar 9, 2019 • edited Loading

Keats commented Mar 22, 2019

mann-ed commented Mar 30, 2019

AaronFriel commented Apr 3, 2019

mann-ed commented Apr 3, 2019 • edited Loading

mann-ed commented Apr 6, 2019

Keats commented Apr 12, 2019 • edited Loading

AaronFriel commented May 19, 2019 • edited Loading

Keats commented May 20, 2019

AaronFriel commented May 30, 2019

Keats commented May 31, 2019

AaronFriel commented May 31, 2019 • edited Loading

Keats commented Jun 3, 2019

Keats commented Jun 6, 2019

Keats commented Jun 16, 2019

Keats commented Jul 13, 2019

Keats commented Aug 12, 2019

AaronFriel commented Jan 5, 2019 •

edited

Loading

jamwaffles commented Jan 12, 2019 •

edited

Loading

manifest commented Mar 5, 2019 •

edited

Loading

manifest commented Mar 5, 2019 •

edited

Loading

manifest commented Mar 9, 2019 •

edited

Loading

mann-ed commented Apr 3, 2019 •

edited

Loading

Keats commented Apr 12, 2019 •

edited

Loading

AaronFriel commented May 19, 2019 •

edited

Loading

AaronFriel commented May 31, 2019 •

edited

Loading