rust-italia / dgc

A parser and validator for the EU Digital Green Certificate (dgc) a.k.a. greenpass
https://github.com/rust-italia/dgc
MIT License
26 stars 11 forks source link

Resolve CWT tagged and COSE-untagged messages #22

Closed dodomorandi closed 2 years ago

dodomorandi commented 2 years ago

I think we should discuss if this is a valid change or not. If we assume the given files are compliant, then it is ok. Otherwise this PR shall not be merged.

Open questions

codecov-commenter commented 2 years ago

Codecov Report

Merging #22 (f6ea37f) into main (c6ca6f6) will increase coverage by 0.15%. The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
+ Coverage   82.24%   82.39%   +0.15%     
==========================================
  Files          10       10              
  Lines        1042     1051       +9     
==========================================
+ Hits          857      866       +9     
  Misses        185      185              
Impacted Files Coverage Δ
src/cwt.rs 80.99% <92.85%> (+1.52%) :arrow_up:

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 c6ca6f6...f6ea37f. Read the comment docs.

lmammino commented 2 years ago

Found this one https://github.com/eu-digital-green-certificates/dgc-testdata/issues/92

Section 2 of RFC8152 states that the COSE tag can be left out if the context in which the COSE object appears tells which type of object that is expected. SE (DK uses this impl.) did not include the tag, but we changed that because some of the app-vendors get confused.

lmammino commented 2 years ago

I am in favour of merging this (since we found some test data already that requires these changes).

If we want to take a more conservative approach, we could wait for #7 to be completed just to have a better feeling for what's the percentage of the test data that comes without a CWT tag...

dodomorandi commented 2 years ago

This can be a good idea indeed. Moreover, we could also wait for #2 to be closed, in order to have even better overview of the unagged cases.

Just a note: this change allows for messages tagged with CBOR Web token and then with the normal COSE tag. Did you find any discussion about this?

lmammino commented 2 years ago

Based on what I have seen in #23 I would get this one shipped.

On this point:

Just a note: this change allows for messages tagged with CBOR Web token and then with the normal COSE tag. Did you find any discussion about this?

I haven't seen the normal COSE tag in the test data. I am tempted to suggest to remove the support for it and keep the code simple... What do you think @dodomorandi?

dodomorandi commented 2 years ago

My fault, I've been lazy: for normal tag I meant the usual COSE_SIGN1_CBOR_TAG. I found the CBOR Web Token tag (CBOR_WEB_TOKEN_TAG) only in common/2DCode/raw/CO28.json test (for now).

To be honest, I am extremely doubtful about the validity of that test, mostly because the official validator goes crazy as well with that. If you think that it is better for now to avoid that strange edge case, I can remove the code to handle it and keep things simpler.

lmammino commented 2 years ago

I am a bit conflicted as well, but if we can support an additional test case maybe it's best to ship the feature as it is. I think the additional level of complexity is acceptable...