Raise EncodeError for every signing failure

Signing with an ECDSA key on a mismatched or unsupported curve raised
IncorrectAlgorithm or UnsupportedEcdsaCurve, and an invalid HMAC key
raised through the verify helper. Under the new hierarchy those are
SignatureError < TokenError, so `JWT.encode` could fail with an error
that is not an EncodeError, unlike its other signing failures and the
Token#sign! contract.

The ECDSA sign path now resolves the curve through a signing-side
helper that raises EncodeError, and the HMAC key validation yields its
message so sign and verify each raise their own error class. Verify
paths are unchanged.

Specs: rename the ECDSA sign examples that claimed VerificationError
while asserting EncodeError, cover the curve mismatch on sign, and
split the curve_name spec into an encode and a decode case. The JWK
"ES384 key pointed to as ES512 key" spec was loading the P-384 fixture
for both keys and only passed because encoding failed first; it now
uses the P-521 fixture and exercises verification.

Author: anakinj

CHANGELOG.md

@@ -6,7 +6,7 @@
 
 **Features:**
 
-- Revamp error hierarchy: introduce `JWT::Error`, `JWT::TokenError`, `JWT::MalformedTokenError`, `JWT::SignatureError`, and `JWT::ClaimValidationError` grouping classes. `JWT::DecodeError` is now a deprecated alias for `JWT::Error`; because of this, `rescue JWT::DecodeError` now also catches `JWT::EncodeError` [#722](https://github.com/jwt/ruby-jwt/pull/722) ([@anakinj](https://github.com/anakinj))
+- Revamp error hierarchy: introduce `JWT::Error`, `JWT::TokenError`, `JWT::MalformedTokenError`, `JWT::SignatureError`, and `JWT::ClaimValidationError` grouping classes. `JWT::DecodeError` is now a deprecated alias for `JWT::Error`; because of this, `rescue JWT::DecodeError` now also catches `JWT::EncodeError`. Signing failures now consistently raise `JWT::EncodeError`: an ECDSA signing key with a mismatched or unsupported curve and an invalid HMAC signing key previously surfaced as `JWT::IncorrectAlgorithm`, `JWT::UnsupportedEcdsaCurve` or `JWT::DecodeError` from `JWT.encode` [#722](https://github.com/jwt/ruby-jwt/pull/722) ([@anakinj](https://github.com/anakinj))
 - Your contribution here
 
 **Fixes and enhancements:**

lib/jwt/jwa/ecdsa.rb

@@ -15,10 +15,8 @@ def sign(data:, signing_key:)
         raise_sign_error!("The given key is a #{signing_key.class}. It has to be an OpenSSL::PKey::EC instance") unless signing_key.is_a?(::OpenSSL::PKey::EC)
         raise_sign_error!('The given key is not a private key') unless signing_key.private?
 
-        curve_definition = curve_by_name(signing_key.group.curve_name)
-        key_algorithm = curve_definition[:algorithm]
-
-        raise IncorrectAlgorithm, "payload algorithm is #{alg} but #{key_algorithm} signing key was provided" if alg != key_algorithm
+        key_algorithm = signing_key_algorithm(signing_key)
+        raise_sign_error!("payload algorithm is #{alg} but #{key_algorithm} signing key was provided") if alg != key_algorithm
 
         asn1_to_raw(signing_key.dsa_sign_asn1(OpenSSL::Digest.new(digest).digest(data)), signing_key)
       end
@@ -95,6 +93,13 @@ def curve_by_name(name)
         self.class.curve_by_name(name)
       end
 
+      # Signing-side counterpart of {.curve_by_name}. An unsupported curve on the
+      # signing key is an encoding problem, so it raises a JWT::EncodeError.
+      def signing_key_algorithm(signing_key)
+        curve_name = signing_key.group.curve_name
+        NAMED_CURVES.fetch(curve_name) { raise_sign_error!("The ECDSA curve '#{curve_name}' is not supported") }[:algorithm]
+      end
+
       def raw_to_asn1(signature, private_key)
         byte_size = (private_key.group.degree + 7) / 8
         sig_bytes = signature[0..(byte_size - 1)]

lib/jwt/jwa/hmac.rb

@@ -21,15 +21,13 @@ def initialize(alg, digest)
       end
 
       def sign(data:, signing_key:)
-        ensure_valid_key!(signing_key)
-        validate_key_length!(signing_key)
+        validate_key!(signing_key) { |message| raise_sign_error!(message) }
 
         OpenSSL::HMAC.digest(digest.new, signing_key, data)
       end
 
       def verify(data:, signature:, verification_key:)
-        ensure_valid_key!(verification_key)
-        validate_key_length!(verification_key)
+        validate_key!(verification_key) { |message| raise_verify_error!(message) }
 
         SecurityUtils.secure_compare(signature, OpenSSL::HMAC.digest(digest.new, verification_key, data))
       end
@@ -42,18 +40,16 @@ def verify(data:, signature:, verification_key:)
 
       attr_reader :digest
 
-      def ensure_valid_key!(key)
-        raise_verify_error!('HMAC key expected to be a String') unless key.is_a?(String)
-        raise_verify_error!('HMAC key cannot be empty') if key.empty?
-      end
+      # Yields a message for the first problem found with the key. The caller
+      # raises it, so signing and verification failures keep their own error class.
+      def validate_key!(key)
+        yield 'HMAC key expected to be a String' unless key.is_a?(String)
+        yield 'HMAC key cannot be empty' if key.empty?
 
-      def validate_key_length!(key)
         return unless JWT.configuration.decode.enforce_hmac_key_length
 
         min_length = MIN_KEY_LENGTHS[alg]
-        return if key.bytesize >= min_length
-
-        raise_verify_error!("HMAC key must be at least #{min_length} bytes for #{alg} algorithm")
+        yield "HMAC key must be at least #{min_length} bytes for #{alg} algorithm" if key.bytesize < min_length
       end
 
       # Copy of https://github.com/rails/rails/blob/v7.0.3.1/activesupport/lib/active_support/security_utils.rb

spec/jwt/jwa/ecdsa_spec.rb

@@ -82,7 +82,7 @@
     end
 
     context 'when the signing key is a public key' do
-      it 'raises a JWT::VerificationError' do
+      it 'raises a JWT::EncodeError' do
         public_key = test_pkey('ec256-public.pem')
         expect do
           instance.sign(data: data, signing_key: public_key)
@@ -91,19 +91,28 @@
     end
 
     context 'when the signing key is not an OpenSSL::PKey::EC instance' do
-      it 'raises a JWT::VerificationError' do
+      it 'raises a JWT::EncodeError' do
         expect do
           instance.sign(data: data, signing_key: 'not_a_key')
         end.to raise_error(JWT::EncodeError, 'The given key is a String. It has to be an OpenSSL::PKey::EC instance')
       end
     end
 
-    context 'when the signing key is invalid' do
-      it 'raises a JWT::UnsupportedEcdsaCurve' do
+    context 'when the signing key uses an unsupported curve' do
+      it 'raises a JWT::EncodeError' do
         invalid_key = OpenSSL::PKey::EC.generate('sect571r1')
         expect do
           instance.sign(data: data, signing_key: invalid_key)
-        end.to raise_error(JWT::UnsupportedEcdsaCurve, "The ECDSA curve 'sect571r1' is not supported")
+        end.to raise_error(JWT::EncodeError, "The ECDSA curve 'sect571r1' is not supported")
+      end
+    end
+
+    context 'when the signing key is for another curve' do
+      it 'raises a JWT::EncodeError' do
+        other_curve_key = OpenSSL::PKey::EC.generate('secp384r1')
+        expect do
+          instance.sign(data: data, signing_key: other_curve_key)
+        end.to raise_error(JWT::EncodeError, 'payload algorithm is ES256 but ES384 signing key was provided')
       end
     end
   end

spec/jwt/jwa/hmac_spec.rb

@@ -17,26 +17,26 @@
     context 'when nil hmac_secret is passed' do
       let(:hmac_secret) { nil }
 
-      it 'raises JWT::VerificationError' do
-        expect { subject }.to raise_error(JWT::VerificationError, 'HMAC key expected to be a String')
+      it 'raises JWT::EncodeError' do
+        expect { subject }.to raise_error(JWT::EncodeError, 'HMAC key expected to be a String')
       end
 
       it 'does not call OpenSSL::HMAC.digest' do
         expect(OpenSSL::HMAC).not_to receive(:digest)
-        expect { subject }.to raise_error(JWT::VerificationError)
+        expect { subject }.to raise_error(JWT::EncodeError)
       end
     end
 
     context 'when blank hmac_secret is passed' do
       let(:hmac_secret) { '' }
 
-      it 'raises JWT::VerificationError' do
-        expect { subject }.to raise_error(JWT::VerificationError, 'HMAC key cannot be empty')
+      it 'raises JWT::EncodeError' do
+        expect { subject }.to raise_error(JWT::EncodeError, 'HMAC key cannot be empty')
       end
 
       it 'does not call OpenSSL::HMAC.digest' do
         expect(OpenSSL::HMAC).not_to receive(:digest)
-        expect { subject }.to raise_error(JWT::VerificationError)
+        expect { subject }.to raise_error(JWT::EncodeError)
       end
     end
 
@@ -85,7 +85,7 @@
         let(:hmac_secret) { 'short' }
 
         it 'raises error' do
-          expect { subject }.to raise_error(JWT::VerificationError, 'HMAC key must be at least 32 bytes for HS256 algorithm')
+          expect { subject }.to raise_error(JWT::EncodeError, 'HMAC key must be at least 32 bytes for HS256 algorithm')
         end
       end
 

spec/jwt/jwk/decode_with_jwk_spec.rb

@@ -125,7 +125,7 @@
       let(:hmac_jwk)           { JWT::JWK.new('secret') }
       let(:rsa_jwk)            { JWT::JWK.new(test_pkey('rsa-2048-private.pem')) }
       let(:ec_jwk_secp384r1)   { JWT::JWK.new(test_pkey('ec384-private.pem')) }
-      let(:ec_jwk_secp521r1)   { JWT::JWK.new(test_pkey('ec384-private.pem')) }
+      let(:ec_jwk_secp521r1)   { JWT::JWK.new(test_pkey('ec512-private.pem')) }
       let(:jwks)               { { keys: [hmac_jwk.export(include_private: true), rsa_jwk.export, ec_jwk_secp384r1.export, ec_jwk_secp521r1.export] } }
 
       context 'when RSA key is pointed to as HMAC secret' do
@@ -175,11 +175,11 @@
       end
 
       context 'when ES384 key is pointed to as ES512 key' do
-        let(:signed_token) { described_class.encode({ 'foo' => 'bar' }, ec_jwk_secp384r1.signing_key, 'ES512', { kid: ec_jwk_secp521r1.kid }) }
+        let(:signed_token) { described_class.encode({ 'foo' => 'bar' }, ec_jwk_secp384r1.signing_key, 'ES384', { kid: ec_jwk_secp521r1.kid }) }
 
         it 'fails in some way' do
-          expect { described_class.decode(signed_token, nil, true, algorithms: ['ES512'], jwks: jwks) }.to(
-            raise_error(JWT::IncorrectAlgorithm, 'payload algorithm is ES512 but ES384 signing key was provided')
+          expect { described_class.decode(signed_token, nil, true, algorithms: ['ES384'], jwks: jwks) }.to(
+            raise_error(JWT::IncorrectAlgorithm, 'payload algorithm is ES384 but ES512 verification key was provided')
           )
         end
       end

spec/jwt/jwt_spec.rb

@@ -260,18 +260,21 @@
       expect { JWT.decode(token, nil, true) }.to raise_error(JWT::SignatureError, 'No verification key available')
     end
 
-    it 'ECDSA curve_name should raise JWT::IncorrectAlgorithm' do
+    it 'ECDSA curve_name mismatch should raise JWT::EncodeError when encoding' do
       key = OpenSSL::PKey::EC.generate('secp256k1')
 
       expect do
         JWT.encode payload, key, 'ES256'
-      end.to raise_error JWT::IncorrectAlgorithm
+      end.to raise_error JWT::EncodeError, 'payload algorithm is ES256 but ES256K signing key was provided'
+    end
 
+    it 'ECDSA curve_name mismatch should raise JWT::IncorrectAlgorithm when decoding' do
+      key = OpenSSL::PKey::EC.generate('secp256k1')
       token = JWT.encode payload, data['ES256_private'], 'ES256'
 
       expect do
-        JWT.decode token, key
-      end.to raise_error JWT::IncorrectAlgorithm
+        JWT.decode token, key, true, algorithm: 'ES256'
+      end.to raise_error JWT::IncorrectAlgorithm, 'payload algorithm is ES256 but ES256K verification key was provided'
     end
   end