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

refactor!: accept message content instead of digest for sign and verify #101

Merged
merged 19 commits into from
Aug 24, 2022

Conversation

shizhMSFT
Copy link
Contributor

@shizhMSFT shizhMSFT commented Aug 19, 2022

Resolves #100

Changes:

  • Sign() in the Signer interface accepts message content instead of digest.
  • Verify() in the Verifier interface accepts message content instead of digest.
  • External algorithm registration is removed since signing and verification no longer require digest computation.
  • Improved doc comments.
  • Fixes tests.

Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
@codecov
Copy link

codecov bot commented Aug 19, 2022

Codecov Report

Merging #101 (aa2f81c) into main (66b4a5a) will increase coverage by 2.61%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #101      +/-   ##
==========================================
+ Coverage   89.48%   92.10%   +2.61%     
==========================================
  Files          10       10              
  Lines        1018      975      -43     
==========================================
- Hits          911      898      -13     
+ Misses         72       51      -21     
+ Partials       35       26       -9     
Impacted Files Coverage Δ
signer.go 100.00% <ø> (ø)
verifier.go 100.00% <ø> (ø)
algorithm.go 100.00% <100.00%> (+3.44%) ⬆️
ecdsa.go 95.89% <100.00%> (+19.32%) ⬆️
ed25519.go 100.00% <100.00%> (ø)
rsa.go 100.00% <100.00%> (+25.00%) ⬆️
sign.go 88.93% <100.00%> (+1.03%) ⬆️
sign1.go 86.86% <100.00%> (+1.75%) ⬆️
headers.go 93.05% <0.00%> (+0.90%) ⬆️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
algorithm.go Show resolved Hide resolved
@qmuntal
Copy link
Contributor

qmuntal commented Aug 19, 2022

If I'm not mistaken, one could previously register a custom algorithm and reuse the built-in signers. This PR removes this capability. Is this intentional?

Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
@shizhMSFT
Copy link
Contributor Author

shizhMSFT commented Aug 19, 2022

If I'm not mistaken, one could previously register a custom algorithm and reuse the built-in signers. This PR removes this capability. Is this intentional?

Previously, we needed to register the external algorithm to access hash algorithms. Now, we don't need them since the digests are computed by the Signer and the Verifier.

External algorithm implementors can implement the Signer and Verifier interfaces and can be used by go-cose directly without any registration. Here's an example implementation.

@qmuntal
Copy link
Contributor

qmuntal commented Aug 19, 2022

If I'm not mistaken, one could previously register a custom algorithm and reuse the built-in signers. This PR removes this capability. Is this intentional?

Previously, we needed to register the external algorithm to access hash algorithms. Now, we don't need them since the digests are computed by the Signer and the Verifier.

External algorithm implementors can implement the Signer and Verifier interfaces and can be used by go-cose directly without any registration. Here's an example implementation.

You are right, I though a caller could instantiate a built-in signer with a custom algorithm, but it is not possible, so we are fine 😄 .

Copy link
Contributor

@qmuntal qmuntal left a comment

Choose a reason for hiding this comment

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

I was not fond of RegisterAlgorithm, so LGTM++!

Copy link
Contributor

@yogeshbdeshpande yogeshbdeshpande left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@SteveLasker SteveLasker left a comment

Choose a reason for hiding this comment

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

LGTM

@SteveLasker SteveLasker merged commit 9d2fab6 into veraison:main Aug 24, 2022
@shizhMSFT shizhMSFT deleted the content branch August 25, 2022 01:57
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 ability to swap crypto implementation
5 participants