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

Make built-in types implement the new DigestSigner and DigestVerify interface #144

Merged
merged 7 commits into from
Jul 24, 2023

Conversation

qmuntal
Copy link
Contributor

@qmuntal qmuntal commented May 12, 2023

Fixes #120

@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #144 (5548b0a) into main (2f778da) will increase coverage by 2.79%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #144      +/-   ##
==========================================
+ Coverage   91.05%   93.84%   +2.79%     
==========================================
  Files          12       12              
  Lines        1542     1900     +358     
==========================================
+ Hits         1404     1783     +379     
+ Misses        101       82      -19     
+ Partials       37       35       -2     
Impacted Files Coverage Δ
signer.go 100.00% <ø> (ø)
verifier.go 100.00% <ø> (ø)
ecdsa.go 96.25% <100.00%> (+0.35%) ⬆️
rsa.go 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

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

@@ -23,6 +23,16 @@ type Signer interface {
Sign(rand io.Reader, content []byte) ([]byte, error)
}

// DigestSigner is an interface for private keys to sign digested COSE signatures.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is interface also an integration point with remote KMS?

any time you have hardware isolated keys, you need a signer interface to request signatures from them.

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

looks good to me, but better to get a review from a better go dev

@yogeshbdeshpande
Copy link
Contributor

@qmuntal as mentioned in the meeting, please add the README explaining the new API

@qmuntal
Copy link
Contributor Author

qmuntal commented Jul 10, 2023

@qmuntal as mentioned in the meeting, please add the README explaining the new API

Done.

@SteveLasker SteveLasker requested a review from setrofim July 14, 2023 15:24
@SteveLasker
Copy link
Contributor

@yogeshbdeshpande, @shizhMSFT, @ivarprudnikov, @setrofim can you PTAL

rsa.go Outdated Show resolved Hide resolved
@ivarprudnikov
Copy link

Unit tests would be necessary to prove it works.

  1. Create a keypair (both for RSA and ECDSA), then for each
  2. Create a signer, cast it to DigestSigner and sign some "foobar" digest
  3. Create a verifier, cast it to DigestVerifier and verify the signature along with the digest

Otherwise it looks OK

@OR13
Copy link
Contributor

OR13 commented Jul 14, 2023

You might want your test strings to include some nasty unicode, unless you use some standard test strings from some the RFCs

Copy link
Contributor

@setrofim setrofim left a comment

Choose a reason for hiding this comment

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

LGTM

verifier.go Outdated Show resolved Hide resolved
signer.go Outdated Show resolved Hide resolved
@qmuntal
Copy link
Contributor Author

qmuntal commented Jul 18, 2023

Unit tests would be necessary to prove it works.

  1. Create a keypair (both for RSA and ECDSA), then for each
  2. Create a signer, cast it to DigestSigner and sign some "foobar" digest
  3. Create a verifier, cast it to DigestVerifier and verify the signature along with the digest

Otherwise it looks OK

You might want your test strings to include some nasty unicode, unless you use some standard test strings from some the RFCs

@ivarprudnikov @OR13 done. I initially didn't add any test case because the built-in signers/verifiers are internally implemented in terms of DigestSigner and DigestVerifier, therefore the existing test coverage already applies to the new interfaces. I've added some basic tests, as suggested, to be more explicit.

@@ -23,6 +23,17 @@ type Signer interface {
Sign(rand io.Reader, content []byte) ([]byte, error)
}

// DigestSigner is an interface for private keys to sign digested COSE signatures.
type DigestSigner interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Does this interface support signing digest from any Digest Algorithm ? Meaning should go-cose not restrict the digest signing to ONLY a known set of digest algorithms? This interface relies on caller to just pass digest as a substitute to content[] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question: Does this interface support signing digest from any Digest Algorithm ? Meaning should go-cose not restrict the digest signing to ONLY a known set of digest algorithms

How could we restrict that? Everyone is free to implement the interface as needed, Go doesn't allow intercepting interface implementations to do custom validations. Also, in this PR we don't use this interface anywhere, users would have to construct the cose.Sign1Message themselves, so it's an experts-only feature.

This interface relies on caller to just pass digest as a substitute to content[] ?

Yes

README.md Outdated Show resolved Hide resolved
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!
Thanks, I suggested some minor nits, but in general look ok!

Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: qmuntal <[email protected]>
@qmuntal qmuntal merged commit a5dc571 into main Jul 24, 2023
@qmuntal qmuntal deleted the digest branch July 24, 2023 10:49
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.

Cannot sign/verify digests anymore (think Merkle tree, ledger, detached payload)
7 participants