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.

Sign up for GitHub

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

Jump to bottom

Add support for RS256, RS384, RS512 constants #163

Merged
merged 3 commits into from
Aug 11, 2023
Merged

Add support for RS256, RS384, RS512 constants #163

merged 3 commits into from
Aug 11, 2023

Conversation

hslatman
Copy link
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
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 [email protected]>
@hslatman
Add constants for RS384 and RS512
754dc9d 
Signed-off-by: hslatman <herman [email protected]>
@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
@@ -88,6 88,8 @@ func NewSigner(alg Algorithm, key crypto.Signer) (Signer, error) {
return &ed25519Signer{
key: key,
}, nil
case AlgorithmRS256, AlgorithmRS384, AlgorithmRS512:
Copy link
Contributor

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
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 hslatman force-pushed the rsassa-pkcs1-v1_5_constants branch from c8d6df5 to 0eac3fc Compare August 8, 2023 21:14
@codecov
Copy link

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
Contributor

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

@OR13, PTAL

OR13
OR13 approved these changes Aug 10, 2023
View reviewed changes
Copy link
Collaborator

@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
Contributor Author

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
4 checks passed
@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