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.

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 RSA-OAEP-384 and RSA-OAEP-512 #1142

Conversation

Hannes-Kunnen
Copy link
Contributor

This pull request would add support for the RSA-OAEP-384 and RSA-OAEP-512 key encryption algorithms. I think this is just a minor change as the base algorithms RSA-OAEP and RSA-OAEP-256 are already implemented. The only difference these new algorithms have is that they use a different hashing algorithm SHA384 and SHA512 respectively instead of SHA1 or SHA256.

The hashing algorithms SHA384and SHA512 are already used in different algorithms, so these are not new. The only thing I wasn't sure about it how the following case should be handled:
permalink

func (e RSAOAEPEncrypt) EncryptKey(cek []byte) (keygen.ByteSource, error) {
	var hash hash.Hash
	switch e.alg {
	case jwa.RSA_OAEP:
		hash = sha1.New()
	case jwa.RSA_OAEP_256:
		hash = sha256.New()
	case jwa.RSA_OAEP_384:
		hash = sha512.New384()
	case jwa.RSA_OAEP_512:
		hash = sha512.New()
	default:
		return nil, fmt.Errorf(`failed to generate key encrypter for RSA-OAEP: RSA_OAEP/RSA_OAEP_256/RSA_OAEP_384/RSA_OAEP_512 required`)
	}
	encrypted, err := rsa.EncryptOAEP(hash, rand.Reader, e.pubkey, cek, []byte{})
	if err != nil {
		return nil, fmt.Errorf(`failed to OAEP encrypt: %w`, err)
	}
	return keygen.ByteKey(encrypted), nil
}

In the original code the 'hash'.New() method was used, so I continued this usage, however when searching where the SHA384 and SHA512 algorithms were used already I found an other method which would result in this diff:

func (e RSAOAEPEncrypt) EncryptKey(cek []byte) (keygen.ByteSource, error) {
-	var hash hash.Hash
+	var hash crypto.Hash
	switch e.alg {
	case jwa.RSA_OAEP:
-		hash = sha1.New()
+		hash = crypto.SHA1
	case jwa.RSA_OAEP_256:
-		hash = sha256.New()
+		hash = crypto.SHA256
	case jwa.RSA_OAEP_384:
-		hash = sha512.New384()
+		hash = crypto.SHA384
	case jwa.RSA_OAEP_512:
-		hash = sha512.New()
+		hash = crypto.SHA512
	default:
		return nil, fmt.Errorf(`failed to generate key encrypter for RSA-OAEP: RSA_OAEP/RSA_OAEP_256/RSA_OAEP_384/RSA_OAEP_512 required`)
	}
-	encrypted, err := rsa.EncryptOAEP(hash, rand.Reader, e.pubkey, cek, []byte{})
+	encrypted, err := rsa.EncryptOAEP(hash.New(), rand.Reader, e.pubkey, cek, []byte{})
	if err != nil {
		return nil, fmt.Errorf(`failed to OAEP encrypt: %w`, err)
	}
	return keygen.ByteKey(encrypted), nil
}

I am not sure which one would be preferred (I assumed the method that was already used).

If I missed something or there is a reason these algorithms weren't implemented I would love to hear it.

Copy link

codecov bot commented Jun 19, 2024

Codecov Report

Attention: Patch coverage is 37.50000% with 10 lines in your changes missing coverage. Please review.

Project coverage is 75.03%. Comparing base (0a15b4d) to head (458bea5).
Report is 84 commits behind head on develop/v2.

Files Patch % Lines
jwe/internal/keyenc/keyenc.go 16.66% 10 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           develop/v2    #1142      +/-   ##
==============================================
+ Coverage       72.00%   75.03%   +3.03%     
==============================================
  Files              93       95       +2     
  Lines           13795    11965    -1830     
==============================================
- Hits             9933     8978     -955     
+ Misses           3044     2168     -876     
- Partials          818      819       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lestrrat
Copy link
Collaborator

LGTM! I'll merge, but please note that I won't be making a release anytime soon. I usually like to wait and see if anything breaks before making releases.

Thanks!

P.S.

I am not sure which one would be preferred (I assumed the method that was already used).

It doesn't really matter. They end up calling the same thing. The rest is stylistic, but for this particular part I didn't have a preference :)

@lestrrat lestrrat merged commit 84cf8ec into lestrrat-go:develop/v2 Jun 19, 2024
23 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants