veraison / go-cose

go library for CBOR Object Signing and Encryption (COSE)
Mozilla Public License 2.0
50 stars 26 forks source link

Add support for `RS256`, `COSEAlgorithmIdentifier -257` #140

Closed hslatman closed 1 year ago

hslatman commented 1 year ago

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 https://github.com/veraison/go-cose/pull/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 the RS256 in all required places seemed to be the way to go, or will RegisterAlgorithm be reconsidered? I noticed signing and verifying can be done backed by a user-provided Signer and Verifier. Should I remove the signing and verification code, and just keep the AlgorithmRS256 around?

codecov[bot] commented 1 year ago

Codecov Report

Merging #140 (a745f5a) into main (bfa9f54) will increase coverage by 0.09%. The diff coverage is 100.00%.

@@            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              
Impacted Files Coverage Δ
algorithm.go 100.00% <100.00%> (ø)
rsa.go 100.00% <100.00%> (ø)
signer.go 100.00% <100.00%> (ø)
verifier.go 100.00% <100.00%> (ø)

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

thomas-fossati commented 1 year ago

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!

hslatman commented 1 year ago

Thank you, @thomas-fossati,

I've opened an issue: https://github.com/veraison/go-cose/issues/141.

For my use case, having just the AlgorithmRS256 as a constant is probably fine. There's still a way to use RSASSA PKCS#1 v1.5 with the external Signer and Verifier approach, so that could be used if one needs it. The actual implementation could also be in a separate package, providing broader/legacy/interoperability cipher support, thus keeping the core go-cose package small and clean.

thomas-fossati commented 1 year ago

Thank you, @thomas-fossati,

I've opened an issue: #141.

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

yogeshbdeshpande commented 1 year ago

@hslatman Request please create a new PR fr adding the RS256 constant

SteveLasker commented 1 year ago

Thanks, @hslatman for the PR and discussion #141 After discussion with the maintainers, we'd like to keep the constant and remove the additional code. Much appreciated for your engagement.

roywill commented 1 year ago

We are not going to implement this algorithms but allowed the constant to be added in another pull request