-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add support for RS256, RS384, RS512 constants #163
Conversation
7370f99
to
5e6476d
Compare
The `AlgorithmRS256` was added as a known COSE signing algorithm. Attempts to create a new Signer through [NewSigner] will result in an error, because the `rsaSigner` does not implement PKCS#1 v1.5 signing nor verification. If PKCS#1 v1.5 support is required, one has to provide an external implementation of a [cose.Signer]. Signed-off-by: hslatman <herman+github@smallstep.com>
Signed-off-by: hslatman <herman+github@smallstep.com>
5e6476d
to
754dc9d
Compare
signer.go
Outdated
@@ -88,6 +88,8 @@ func NewSigner(alg Algorithm, key crypto.Signer) (Signer, error) { | |||
return &ed25519Signer{ | |||
key: key, | |||
}, nil | |||
case AlgorithmRS256, AlgorithmRS384, AlgorithmRS512: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the extended error message, we could use it for all unknown and unimplemented algorithms. This way we don't special-case unimplemented algorithms (which I would like to avoid).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: hslatman <herman+github@smallstep.com>
c8d6df5
to
0eac3fc
Compare
Codecov Report
@@ Coverage Diff @@
## main #163 +/- ##
==========================================
+ Coverage 93.80% 93.82% +0.02%
==========================================
Files 11 11
Lines 1695 1701 +6
==========================================
+ Hits 1590 1596 +6
Misses 72 72
Partials 33 33
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM
@OR13, PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message could be better.
Algorithm not supported, external signer required (link to documentation).
As is, the user is likely to assume it's not possible to use the library with the algorithm... Unless they read the comments in the code.
I think you mean the general error message; not just for the |
This PR replaces #140. It closes #141.
It only provides the constants for the algorithms. A
cose.Signer
orcose.Verifier
needs to be implemented and provided from outside the package to use these algorithms. UsingNewSigner
andNewVerifier
will return an error when called with one of these algorithms, because PKCS#1 v1.5 signing and verification aren't implemented ingo-cose
itself.Example usage with external
mycose
package: