Conversation
Keats
left a comment
There was a problem hiding this comment.
That looks good!
I will probably change the API to make it easy for users but all the effort it very appreciated.
How did you generate the PEM keys? I will add all the defaut ones from jwt.io later on to ensure everything is fine.
README.md
Outdated
| you can run the following commands to obtain the DER keys from PKCS#1 (ie with `BEGIN RSA PUBLIC KEY`) .pem. | ||
| `jsonwebtoken` can read DER and PEM encoded keys. | ||
|
|
||
| If you want to use DER encoded keys review the following. |
There was a problem hiding this comment.
this sentence seems like it's missing stuff?
There was a problem hiding this comment.
I've updated the readme a little.
All of the key data is derived from what was already in the repo. I renamed the The PKCS#1 keys are the base64 encoded The PKCS#8 RSA public key was created from the private key with an openssl command. The PKCS#8 EC private key was created from the And the public key was then derived from that |
Sure, I'm not set on how the API came out, though I struggled with lifetimes. There's the challenges I experienced that resulted in this API.
This is the first real rust and opensource contribution I've made so I appreciate the critique you give. |
|
Your PR seems to have landed Keats, and is now on crate I'll update the PR by bumping the pem version |
|
Thanks!
I'll give a better review then but that will have to wait the weekend/next week. |
Keats
left a comment
There was a problem hiding this comment.
Not much to say, it's pretty good!
Don't worry about fixing them, the API might change significantly
| enum Classification { | ||
| EC, | ||
| RSA, | ||
| } |
There was a problem hiding this comment.
I would put all of those at the top above the struct.
| } | ||
|
|
||
| #[derive(Debug)] | ||
| #[derive(PartialEq)] |
There was a problem hiding this comment.
You can (and should) put those derive in a single command: #[derive(Debug, PartialEq)]
src/pem_decoder.rs
Outdated
| } | ||
| } | ||
| _ => { | ||
| println!("Ignoring {:?}", asn1_entry); |
There was a problem hiding this comment.
oh yes definitely, that was a debugging helper.
| } | ||
| simple_asn1::ASN1Block::ObjectIdentifier(_, oid) => { | ||
| if oid == ec_public_key_oid { | ||
| return Option::Some(Classification::EC); |
| } | ||
| } | ||
| } | ||
| return Option::default(); |
|
Note: I'll merge it in the pem-handling branch first to experiment with the API and to make it easy for you to follow all the changes/give feedback. |
|
I look forward to it. |
* Add PEM support with pem and simple_asn1. Documentation TODO * Make pkcs1 and pkcs8 versions of the RSA key, confirm they pass tests. * Add documentation, simplify * Update readme * Bump pem version * Remove extra print
Add PEM decoding support (#106)
This adds a new top level function
decode_pem, and a subsequent functionas_keyon the result type for use withsignandverify.This contributes to #76 and #77
However it does not include X.509 certificate support.
I have an idea on how to do that but this should handle many cases that users want as PEM files are the common output from
opensslIt supports both PKCS#1 and PKCS#8 RSA keys, it supports PKCS#8 EC keys, however it doesn't assert that the curve ID but within JWT contexts the curve is predefined by the spec.