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

Resolve CWT tagged and COSE-untagged messages #22

Merged
merged 1 commit into from
Nov 27, 2021

Conversation

dodomorandi
Copy link
Contributor

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

  • Do untagged messages need additional checks?

@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2021

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%) ⬆️

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
Copy link
Member

lmammino commented Nov 25, 2021

Found this one eu-digital-green-certificates/dgc-testdata#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
Copy link
Member

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
Copy link
Contributor Author

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 lmammino mentioned this pull request Nov 25, 2021
@lmammino lmammino self-requested a review November 25, 2021 20:09
@lmammino
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Member

@lmammino lmammino left a comment

Choose a reason for hiding this comment

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

I would be happy to get this one merged :)

This makes a few more tests pass.

Co-authored-by: Tommaso Allevi <tomallevi@gmail.com>
@dodomorandi dodomorandi merged commit ff6ae22 into main Nov 27, 2021
@lu-zero lu-zero deleted the improved-tag-check branch November 27, 2021 08:02
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.

5 participants