lestrrat-go / jwx

Implementation of various JWx (Javascript Object Signing and Encryption/JOSE) technologies
MIT License
1.95k stars 164 forks source link

Add support for RSA-OAEP-384 and RSA-OAEP-512 #1142

Closed Hannes-Kunnen closed 4 months ago

Hannes-Kunnen commented 4 months ago

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.

codecov[bot] commented 4 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lestrrat commented 4 months ago

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