-
Notifications
You must be signed in to change notification settings - Fork 276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add PEM decoding support #106
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sentence seems like it's missing stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put all of those at the top above the struct.
} | ||
|
||
#[derive(Debug)] | ||
#[derive(PartialEq)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can remove that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yes definitely, that was a debugging helper.
} | ||
simple_asn1::ASN1Block::ObjectIdentifier(_, oid) => { | ||
if oid == ec_public_key_oid { | ||
return Option::Some(Classification::EC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for Option::
} | ||
} | ||
} | ||
return Option::default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can replace that line by None
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_key
on the result type for use withsign
andverify
.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
openssl
It 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.