Looks up for `kid` and `alg` also in unprotected header #25

lmammino · 2021-11-26T09:21:04Z

Fixes #1

Parses kid and alg from unprotected header.
Uses SHA256 hash to calculate kid from certificates
Removes header_unprotected (not used anymore) and makes header_protected_raw and payload_raw private in Cwt (it felt like a leaking abstraction)
Makes sure all the tests originally skipped because of Support kid in unprotected header #1 now run correctly

~~Note: It is better to merge #23 first and then rebase this branch, so we can easily test against all the previously failing tests.~~ Done

CC: @rez23 who was already looking at this!

codecov-commenter · 2021-11-26T09:25:07Z

Codecov Report

Merging #25 (672e19f) into main (ff6ae22) will increase coverage by 0.02%.
The diff coverage is 82.19%.

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   82.34%   82.36%   +0.02%     
==========================================
  Files          10       10              
  Lines        1048     1072      +24     
==========================================
+ Hits          863      883      +20     
- Misses        185      189       +4

Impacted Files	Coverage Δ
src/trustlist.rs	`58.87% <75.00%> (+0.67%)`	⬆️
src/cwt.rs	`79.41% <80.70%> (-1.10%)`	⬇️
src/parse.rs	`73.91% <87.50%> (+0.72%)`	⬆️
src/dgc_cert.rs	`97.08% <100.00%> (+0.11%)`	⬆️

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 ff6ae22...672e19f. Read the comment docs.

lu-zero

You may squash the two commits in one while at it.

src/cwt.rs

lmammino · 2021-11-26T19:55:32Z

src/dgc_cert.rs

+fn t_empty_if_null<'de, D>(deserializer: D) -> Result<Vec<Test>, D::Error>
+where
+    D: Deserializer<'de>,
+{
+    let opt = Option::deserialize(deserializer)?;
+    Ok(opt.unwrap_or_default())
+}
+
+fn v_empty_if_null<'de, D>(deserializer: D) -> Result<Vec<Vaccination>, D::Error>
+where
+    D: Deserializer<'de>,
+{
+    let opt = Option::deserialize(deserializer)?;
+    Ok(opt.unwrap_or_default())
+}
+
+fn r_empty_if_null<'de, D>(deserializer: D) -> Result<Vec<Recovery>, D::Error>
+where
+    D: Deserializer<'de>,
+{
+    let opt = Option::deserialize(deserializer)?;
+    Ok(opt.unwrap_or_default())
+}
+


Note that I had to do this because there were 2 tests (originally skipped because of the "kid in unprotected header" problem) that have the following content encoded:

{ "r": null, "t": null, "v": [ // ... ], }

Instead of having only one value set between r, t, or v they set two of them to null and the other to an appropriate array.

Solution based on serde-rs/serde#1098

I have generalised this to one generic empty_if_null function...

FYI: https://docs.rs/serde_with/1.11.0/serde_with/
This can be pretty useful...

Nice one. Is there any utility there that would do what we are doing here? I couldn't find one at first glance...

In this case I am not sure. However, this a lifesaver to avoid skip_serialization_if(Option::is_none) everywhere. For some of my projects I discovered this a bit too late... 😓

lmammino · 2021-11-26T19:56:46Z

@lu-zero, this is now rebases with main. I can do a "Squash and merge" once this is ready to go rather than squashing everything upfront. Would that work for you?

lmammino · 2021-11-26T19:59:07Z

Note that with this PR we solve most of the broken test. We go from:

test result: ok. 528 passed; 0 failed; 52 ignored; 0 measured; 0 filtered out; finished in 0.17s

To:

test result: ok. 558 passed; 0 failed; 22 ignored; 0 measured; 0 filtered out; finished in 0.17s

So 30 new tests are now passing! 🎉

dodomorandi

I am going to propose a commit (it would be a little bit a mess using diff changes), feel free to reject it if you find it out of scope. I noticed that some parts have been refactored but they could be improved a little bit.

dodomorandi · 2021-11-27T08:46:05Z

This is the change I was talking about. Let me know what do you think about it.

guerinoni

LGTM :)

lmammino · 2021-11-27T09:40:16Z

I love the suggested changes @dodomorandi! Thanks.

I actually contributed to ciborium adding the existing as_x functions to Value. It would be nice to submit a PR with these new features. You should do it 😉

I'll try to fix the conflict, rebase and get this merged today 🤞

dodomorandi · 2021-11-27T09:42:49Z

I'll try to fix the conflict, rebase and get this merged today 🤞

No worries, I'll do it because I caused the conflict 😁. I was just waiting for your feedback before performing a forced push.

It would be nice to submit a PR with these new features. You should do it 😉

I totally agree!!

…late kid from certificates

Unfortunately `ciborium` misses some ergonomic features in order to keep things easy. I added an _extension trait_ to overcome this problem and I used it to keep code a little bit cleaner.

lmammino requested review from dodomorandi, lu-zero, nappa85 and Rimpampa November 26, 2021 09:21

lmammino self-assigned this Nov 26, 2021

lmammino requested a review from rez23 November 26, 2021 11:04

lu-zero requested changes Nov 26, 2021

View reviewed changes

src/cwt.rs Outdated Show resolved Hide resolved

lmammino force-pushed the support-kid-in-unprotected-header branch from b4531ae to e268f09 Compare November 26, 2021 18:01

lmammino requested a review from lu-zero November 26, 2021 19:51

lmammino commented Nov 26, 2021

View reviewed changes

lmammino requested review from guerinoni, la10736 and wolf4ood November 26, 2021 22:07

dodomorandi reviewed Nov 27, 2021

View reviewed changes

guerinoni approved these changes Nov 27, 2021

View reviewed changes

lmammino and others added 6 commits November 27, 2021 10:43

Parses kid and alg from unprotected header. Uses SHA256 hash to calcu…

502deba

…late kid from certificates

console.log like there is no tomorrow

9bca10d

Removed unwrap and header_unprotected from Cwt (not used)

e3d55b4

Better parsing of headers

4a803ab

Generalised x_empty_if_null into one generic function

88ee8ab

Directly use owned data instead of cloning

672e19f

Unfortunately `ciborium` misses some ergonomic features in order to keep things easy. I added an _extension trait_ to overcome this problem and I used it to keep code a little bit cleaner.

dodomorandi force-pushed the support-kid-in-unprotected-header branch from b4e22f3 to 672e19f Compare November 27, 2021 10:49

dodomorandi merged commit 5e4db0f into main Nov 27, 2021

dodomorandi deleted the support-kid-in-unprotected-header branch December 9, 2021 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Looks up for `kid` and `alg` also in unprotected header #25

Looks up for `kid` and `alg` also in unprotected header #25

lmammino commented Nov 26, 2021 •

edited

Loading

codecov-commenter commented Nov 26, 2021 •

edited

Loading

lu-zero left a comment

lmammino Nov 26, 2021

lmammino Nov 26, 2021

dodomorandi Nov 27, 2021

lmammino Nov 27, 2021

dodomorandi Nov 27, 2021 •

edited

Loading

lmammino commented Nov 26, 2021

lmammino commented Nov 26, 2021

dodomorandi left a comment

dodomorandi commented Nov 27, 2021

guerinoni left a comment

lmammino commented Nov 27, 2021

dodomorandi commented Nov 27, 2021

Looks up for kid and alg also in unprotected header #25

Looks up for kid and alg also in unprotected header #25

Conversation

lmammino commented Nov 26, 2021 • edited Loading

codecov-commenter commented Nov 26, 2021 • edited Loading

Codecov Report

lu-zero left a comment

Choose a reason for hiding this comment

lmammino Nov 26, 2021

Choose a reason for hiding this comment

lmammino Nov 26, 2021

Choose a reason for hiding this comment

dodomorandi Nov 27, 2021

Choose a reason for hiding this comment

lmammino Nov 27, 2021

Choose a reason for hiding this comment

dodomorandi Nov 27, 2021 • edited Loading

Choose a reason for hiding this comment

lmammino commented Nov 26, 2021

lmammino commented Nov 26, 2021

dodomorandi left a comment

Choose a reason for hiding this comment

dodomorandi commented Nov 27, 2021

guerinoni left a comment

Choose a reason for hiding this comment

lmammino commented Nov 27, 2021

dodomorandi commented Nov 27, 2021

Looks up for `kid` and `alg` also in unprotected header #25

Looks up for `kid` and `alg` also in unprotected header #25

lmammino commented Nov 26, 2021 •

edited

Loading

codecov-commenter commented Nov 26, 2021 •

edited

Loading

dodomorandi Nov 27, 2021 •

edited

Loading