Skip to content

Add support for RS256, RS384, RS512 constants#163

Merged
roywill merged 3 commits intoveraison:mainfrom
hslatman:rsassa-pkcs1-v1_5_constants
Aug 11, 2023
Merged

Add support for RS256, RS384, RS512 constants#163
roywill merged 3 commits intoveraison:mainfrom
hslatman:rsassa-pkcs1-v1_5_constants

Conversation

@hslatman
Copy link
Copy Markdown
Contributor

@hslatman hslatman commented Aug 8, 2023 •
edited
Loading

This PR replaces #140. It closes #141.

It only provides the constants for the algorithms. A cose.Signer or cose.Verifier needs to be implemented and provided from outside the package to use these algorithms. Using NewSigner and NewVerifier will return an error when called with one of these algorithms, because PKCS#1 v1.5 signing and verification aren't implemented in go-cose itself.

Example usage with external mycose package:

import (
        "crypto/rand"

	"github.com/veraison/go-cose"
	"go.step.sm/crypto/keyutil"
	
	"mycose"
)

func main(){
	key, _ := keyutil.GenerateSigner("RSA", "", 2048)
	cs := mycose.NewRS256Signer(key)  // adheres to cose.Signer

        data := []byte("1234")
	sig, _ := cs.Sign(rand.Reader, data)
	
	headers := cose.Headers{
		Protected: cose.ProtectedHeader{
			cose.HeaderLabelAlgorithm: cose.AlgorithmRS256,
		},
	}
	sig, _ = cose.Sign1(rand.Reader, cs, headers, data, nil)
	
}

@hslatman hslatman mentioned this pull request Aug 8, 2023
Closed
@hslatman hslatman force-pushed the rsassa-pkcs1-v1_5_constants branch from 7370f99 to 5e6476d Compare August 8, 2023 08:19
hslatman added 2 commits August 8, 2023 10:31
@hslatman
Add RS256 algorithm constant
c9e2ec5 
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>
@hslatman
Add constants for RS384 and RS512
754dc9d 
Signed-off-by: hslatman <herman+github@smallstep.com>
@hslatman hslatman force-pushed the rsassa-pkcs1-v1_5_constants branch from 5e6476d to 754dc9d Compare August 8, 2023 08:32
@hslatman hslatman changed the title Add support for RS256, RS384, RS512 Add support for RS256, RS384, RS512 constants Aug 8, 2023
qmuntal
qmuntal reviewed Aug 8, 2023
View reviewed changes
signer.go Outdated
return &ed25519Signer{
key: key,
}, nil
case AlgorithmRS256, AlgorithmRS384, AlgorithmRS512:
Copy link
Copy Markdown
Member

@qmuntal qmuntal Aug 8, 2023

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).

Copy link
Copy Markdown
Contributor Author

@hslatman hslatman Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@qmuntal changed that in 0eac3fc. Now all unsupported algorithms will return either the textual name or the numeric identifier, similar to "unknown algorithm value -1", suffixed with the ErrAlgorithmNotSupported.

@hslatman
Make all unsupported algorithms return a more informative error
0eac3fc 
Signed-off-by: hslatman <herman+github@smallstep.com>
@hslatman hslatman force-pushed the rsassa-pkcs1-v1_5_constants branch from c8d6df5 to 0eac3fc Compare August 8, 2023 21:14
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 8, 2023

Codecov Report

Merging #163 (0eac3fc) into main (160b7e0) will increase coverage by 0.02%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

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

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

@hslatman hslatman requested a review from qmuntal August 8, 2023 21:19
qmuntal
qmuntal approved these changes Aug 8, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@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.

Thanks! LGTM

@SteveLasker
Copy link
Copy Markdown
Contributor

SteveLasker commented Aug 10, 2023

@OR13, PTAL

OR13
OR13 approved these changes Aug 10, 2023
View reviewed changes
Copy link
Copy Markdown
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.

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.

@hslatman
Copy link
Copy Markdown
Contributor Author

hslatman commented Aug 10, 2023

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 RSx constants? I agree that the error could be improved. That might need more valid COSE algorithm constants to be added, so that the error message can distinguish between valid algorithm identifiers (that require an external signer) vs. invalid algorithm identifiers (that won't/shouldn't work with external signers)

@roywill roywill merged commit 4451940 into veraison:main Aug 11, 2023
@hslatman hslatman mentioned this pull request Aug 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@qmuntal qmuntal qmuntal approved these changes

@OR13 OR13 OR13 approved these changes

@roywill roywill roywill approved these changes

@thomas-fossati thomas-fossati Awaiting requested review from thomas-fossati thomas-fossati is a code owner

@yogeshbdeshpande yogeshbdeshpande Awaiting requested review from yogeshbdeshpande yogeshbdeshpande is a code owner

@shizhMSFT shizhMSFT Awaiting requested review from shizhMSFT shizhMSFT is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Consider adding support for RS256 algorithm identifier

5 participants

@hslatman @SteveLasker @qmuntal @OR13 @roywill