Skip to content
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

Merged
merged 5 commits into from
Dec 9, 2021
Merged

Add RSA support #33

merged 5 commits into from
Dec 9, 2021

Conversation

Rimpampa
Copy link
Collaborator

@Rimpampa Rimpampa commented Dec 5, 2021

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

@codecov-commenter
Copy link

Codecov Report

Merging #33 (1ba7506) into main (1c767d0) will increase coverage by 0.79%.
The diff coverage is 74.19%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/parse.rs 74.80% <64.70%> (+3.15%) ⬆️
src/trustlist.rs 58.33% <81.81%> (-0.54%) ⬇️
src/cwt.rs 80.00% <100.00%> (+0.58%) ⬆️
src/lib.rs 95.74% <0.00%> (+8.82%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c767d0...1ba7506. Read the comment docs.

src/cwt.rs Outdated Show resolved Hide resolved
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"]
Copy link
Member

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?

Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a pretty generic issue (#34). We could use it to track down all the strange issues with tests. @Rimpampa do you want to add See #34. in the ignored tests?

tests/lib.rs Outdated Show resolved Hide resolved
@lmammino
Copy link
Member

lmammino commented Dec 5, 2021

I am not really sure i can suggest anything on the following point:

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.

Other than that I really like where this PR is going!

Thanks for the awesome work @Rimpampa! 🙌🏽

Copy link
Contributor

@dodomorandi dodomorandi left a 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!

src/trustlist.rs Outdated Show resolved Hide resolved
src/trustlist.rs Show resolved Hide resolved
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"]
Copy link
Contributor

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

tests/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@dodomorandi dodomorandi left a 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!

@dodomorandi dodomorandi merged commit 26d85d1 into rust-italia:main Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support to verify RSA signatures
4 participants