Add support for RS256, COSEAlgorithmIdentifier -257
#140
Conversation
hslatman
force-pushed
the
rsassa-pkcs1-v1_5
branch
3 times, most recently
from
5ca7980
to
8bab3b9
Compare
Signed-off-by: Herman Slatman <hermanslatman@hotmail.com>
hslatman
force-pushed
the
rsassa-pkcs1-v1_5
branch
from
8bab3b9
to
a745f5a
Compare
Codecov Report
@@ Coverage Diff @@
## main #140 +/- ##
==========================================
+ Coverage 92.77% 92.87% +0.09%
==========================================
Files 10 10
Lines 1024 1038 +14
==========================================
+ Hits 950 964 +14
Misses 49 49
Partials 25 25
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Hi @hslatman, thanks very much for initiating the contribution, much appreciated! As you probably know already RS256 is not amongst the recommended algorithms for COSE -- and for pretty good reasons :-) So, it'd be good if you could file an associated issue where you describe the use case and where we can have a conversation on requirements, caveats, etc. cheers! |
|
Thank you, @thomas-fossati, I've opened an issue: #141. For my use case, having just the |
Fantastic, thanks! I'm on holiday until the 11th, so I can't join the discussion until then. @SteveLasker @qmuntal @shizhMSFT @roywill @yogeshbdeshpande could you please chime in so we don't let Herman wait too long? TIA |
|
@hslatman Request please create a new PR fr adding the RS256 constant |
| @@ -22,6 +22,10 @@ const ( | |||
| // Requires an available crypto.SHA512. | |||
| AlgorithmPS512 Algorithm = -39 | |||
|
|
|||
| // RSASSA-PKCS1-v1_5 using SHA-256 by RFC 8812. | |||
| // Requires an available crypto.SHA256. | |||
| AlgorithmRS256 Algorithm = -257 | |||
There was a problem hiding this comment.
consider a link / comment to webauthn: https://www.w3.org/TR/webauthn-2/#sctn-sample-registration
| return ErrVerification | ||
| switch rv.alg { | ||
| case AlgorithmRS256: | ||
| if err := rsa.VerifyPKCS1v15(rv.key, hash, digest, signature); err != nil { |
There was a problem hiding this comment.
maybe you want to remove this part
| } | ||
|
|
||
| // set up signer for RS256 | ||
| algRS256 := AlgorithmRS256 |
There was a problem hiding this comment.
maybe you want to remove this part
|
We are not going to implement this algorithms but allowed the constant to be added in another pull request |
This PR adds support for RSASSA PKCS#1 v1.5 signing and verification. I started with just
RS256, but I can add the others too if this PR will be considered to be merged at some point.Let me know if you want the tests to be more like a table; currently they're more like multiple copies.
I noticed that registering an algorithm was removed with #101, but I could use the
RS256(and potentially others) for some things, primarily for checking/validating values to be valid COSE values. Adding in theRS256in all required places seemed to be the way to go,or willI noticed signing and verifying can be done backed by a user-providedRegisterAlgorithmbe reconsidered?SignerandVerifier. Should I remove the signing and verification code, and just keep theAlgorithmRS256around?