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

Support multiple recipient COSE operations #10

Merged
merged 12 commits into from
Jan 23, 2023

Conversation

falko17
Copy link
Member

@falko17 falko17 commented Sep 20, 2022

This rewrites the existing ciphers to work without requiring a self reference (instead taking in a key), and adds support for CoseSign, CoseEncrypt, and CoseMac rather than just the single-recipient variants. For this purpose, token methods with the suffix _multiple have also been implemented. Tests and documentation have been written for all new traits, macros, functions, and methods.

@falko17 falko17 added the enhancement New feature or request label Sep 20, 2022
@falko17 falko17 added this to the Version 0.4 milestone Sep 20, 2022
@falko17 falko17 self-assigned this Sep 20, 2022
@falko17 falko17 linked an issue Sep 20, 2022 that may be closed by this pull request
@coveralls
Copy link

coveralls commented Oct 24, 2022

Pull Request Test Coverage Report for Build 3981217710

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 276 of 309 (89.32%) changed or added relevant lines in 3 files are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.1%) to 90.455%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/token/mod.rs 148 151 98.01%
src/token/tests.rs 122 128 95.31%
src/error/mod.rs 6 30 20.0%
Files with Coverage Reduction New Missed Lines %
src/token/mod.rs 1 97.58%
src/token/tests.rs 3 94.16%
Totals Coverage Status
Change from base Build 3066317205: 0.1%
Covered Lines: 1630
Relevant Lines: 1802

💛 - Coveralls

@falko17 falko17 force-pushed the 9-support-other-cose-ciphers branch 3 times, most recently from 8f0a432 to ae08517 Compare October 24, 2022 13:33
@falko17 falko17 marked this pull request as ready for review October 24, 2022 13:33
@falko17 falko17 requested a review from pulsastrix October 24, 2022 13:33
@falko17 falko17 force-pushed the 9-support-other-cose-ciphers branch from 3e2e8cb to 69ef114 Compare October 25, 2022 20:21
@falko17 falko17 force-pushed the 9-support-other-cose-ciphers branch 2 times, most recently from 79681e0 to 75f1696 Compare October 26, 2022 11:39
Copy link
Member

@pulsastrix pulsastrix left a comment

Choose a reason for hiding this comment

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

Some initial comments for the parts that I got to read up until now (will do the rest as well as a deeper pass over the entire PR tomorrow).

I assume that there won't be any large comments for the remainder either, as we have already discussed the general idea behind the cipher implementations in person.

src/error/mod.rs Show resolved Hide resolved
src/token/mod.rs Outdated Show resolved Hide resolved
src/token/mod.rs Outdated Show resolved Hide resolved
src/token/mod.rs Outdated Show resolved Hide resolved
src/token/mod.rs Show resolved Hide resolved
@falko17 falko17 force-pushed the 9-support-other-cose-ciphers branch from 974917a to c5f91fa Compare November 6, 2022 12:57
Copy link
Member

@pulsastrix pulsastrix left a comment

Choose a reason for hiding this comment

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

Here's the second part of the review. As already predicted, I did not find any glaring issues that would make larger changes necessary.
If there are any remaining larger issues, I'd assume that we'd find them when we add actual implementations.

One suggestion regarding the release process for 0.4.0: In order to ensure that we don't ship a release that isn't actually functional, I'd hold off on releasing until we have at least one implementation that isn't FakeCrypto and actually links to an actual crypto library.
(I'll try to make time to write a proof-of-concept for either OpenSSL or RustCrypto this week)

src/token/mod.rs Show resolved Hide resolved
src/token/mod.rs Show resolved Hide resolved
src/token/mod.rs Show resolved Hide resolved
src/token/tests.rs Show resolved Hide resolved
src/token/tests.rs Show resolved Hide resolved
@falko17 falko17 force-pushed the 9-support-other-cose-ciphers branch from eb78ca9 to b2a64d1 Compare January 22, 2023 19:33
@falko17 falko17 requested a review from pulsastrix January 22, 2023 19:33
Copy link
Member

@JKRhb JKRhb left a comment

Choose a reason for hiding this comment

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

(See a few very minor comments/suggestions below.)

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/common/test_helper.rs Outdated Show resolved Hide resolved
@falko17 falko17 force-pushed the 9-support-other-cose-ciphers branch 4 times, most recently from 177a593 to bdcb6ad Compare January 23, 2023 11:38
@falko17
Copy link
Member Author

falko17 commented Jan 23, 2023

@JKRhb Thank you for the review! All points should now have been resolved.

pulsastrix
pulsastrix previously approved these changes Jan 23, 2023
Copy link
Member

@pulsastrix pulsastrix left a comment

Choose a reason for hiding this comment

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

Just one minor remark. Aside from that, there's not really much left to comment on ^^

LGTM.

src/common/test_helper.rs Outdated Show resolved Hide resolved
@falko17 falko17 merged commit 879c55a into main Jan 23, 2023
@falko17 falko17 deleted the 9-support-other-cose-ciphers branch January 23, 2023 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support COSE_Encrypt, COSE_Sign, and COSE_Mac
4 participants