-
Notifications
You must be signed in to change notification settings - Fork 11
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 RSA support #33
Add RSA support #33
Conversation
Codecov Report
@@ Coverage Diff @@
## main #33 +/- ##
==========================================
+ Coverage 85.42% 86.22% +0.79%
==========================================
Files 10 10
Lines 1077 1016 -61
==========================================
- Hits 920 876 -44
+ Misses 157 140 -17
Continue to review full report at Codecov.
|
tests/lib.rs
Outdated
@@ -116,11 +110,11 @@ use std::path::PathBuf; | |||
#[case::es_2dcode_raw_2101_json("ES/2DCode/raw/2101.json")] | |||
#[case::es_2dcode_raw_2102_json("ES/2DCode/raw/2102.json")] | |||
#[case::es_2dcode_raw_2103_json("ES/2DCode/raw/2103.json")] | |||
#[ignore = "RSA signature. See #2"] | |||
#[ignore = "Key is P-384"] |
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.
Should we open a dedicated issue for those?
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 agree: even if it's a problem related to the test files, at least we have a public issue (which could be also referenced from the official repos...)
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 am not really sure i can suggest anything on the following point:
Other than that I really like where this PR is going! Thanks for the awesome work @Rimpampa! 🙌🏽 |
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.
Just a few minor comments, good work @Rimpampa!
tests/lib.rs
Outdated
@@ -116,11 +110,11 @@ use std::path::PathBuf; | |||
#[case::es_2dcode_raw_2101_json("ES/2DCode/raw/2101.json")] | |||
#[case::es_2dcode_raw_2102_json("ES/2DCode/raw/2102.json")] | |||
#[case::es_2dcode_raw_2103_json("ES/2DCode/raw/2103.json")] | |||
#[ignore = "RSA signature. See #2"] | |||
#[ignore = "Key is P-384"] |
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 agree: even if it's a problem related to the test files, at least we have a public issue (which could be also referenced from the official repos...)
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.
Wonderful! Nice work indeed!
Based on the work made by @dodomorandi and @lu-zero and the discussion we had on Telegram, I came up with an implementation for the RSA support that, in my opinion, is clean and effective. All test that use the PS256 alg now don't fail.
There were some test that were excluded because they used RSA but in reality the problem is that they use a NIST P-384 public key.
There is still some work to do on the verification of the certificates as most of the certificates used in the test schemas are part of a bigger certificate chain that (at least for me) is nowhere to be found. Currently is disabled.
I had to change the public key data used in parse::tests::it_validates because before it included some DER encoding "metadata" (the ASN1 tag and the length of the key) that the current implementation doesn't1 need.
Now public keys are stored without checking the validity and that's because it cannot be done at that stage:
the key must be validated based on the alg field of the DGC, and we can't know that when building the TrustList.
The key validation is now done automatically by the ring functions at the moment of signature verification.
Closes #2, #14