From efe302f09c5df426b307f16b25aceeebe7ba8940 Mon Sep 17 00:00:00 2001 From: Julik Tarkhanov Date: Sun, 28 Jan 2024 13:56:58 +0100 Subject: [PATCH 1/8] Add a failing test --- spec/jwt/jwk/ec_spec.rb | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/spec/jwt/jwk/ec_spec.rb b/spec/jwt/jwk/ec_spec.rb index f2d947de..24b941c3 100644 --- a/spec/jwt/jwk/ec_spec.rb +++ b/spec/jwt/jwk/ec_spec.rb @@ -138,6 +138,28 @@ end end end + + context 'with missing 0-byte at the start of EC coordinates' do + let(:example_keysets) do + [ + "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"0Nv5IKAlkvXuAKmOmFgmrwXKR7qGePOzu_7RXg5msw\",\"y\":\"FqnPSNutcjfvXNlufwb7nLJuUEnBkbMdZ3P79nY9c3k\"}", + "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"xGjPg-7meZamM_yfkGeBUB2eJ5c82Y8vQdXwi5cVGw\",\"y\":\"9FwKAuJacVyEy71yoVn1u1ETsQoiwF7QfkfXURGxg14\"}", + "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"yTvy0bwt5s29mIg1DMq-IjZH4pDgZIN9keEEaSuWZhk\",\"y\":\"a0nrmd8qz8jpZDgpY82Rgv3vZ5xiJuiAoMIuRlGnaw\"}", + "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"yJen7AW4lLUTMH4luDj0wlMNSGCuOBB5R-ZoxlAU_g\",\"y\":\"aMbA-M6ORHePSatiPVz_Pzu7z2XRnKMzK-HIscpfud8\"}", + "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"p_D00Z1ydC7mBIpSKPUUrzVzY9Fr5NMhhGfnf4P9guw\",\"y\":\"lCqM3B_s04uhm7_91oycBvoWzuQWJCbMoZc46uqHXA\"}", + "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"hKS-vxV1bvfZ2xOuHv6Qt3lmHIiArTnhWac31kXw3w\",\"y\":\"f_UWjrTpmq_oTdfss7YJ-9dEiYw_JC90kwAE-y0Yu-w\"}", + "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"3W22hN16OJN1XPpUQuCxtwoBRlf-wGyBNIihQiTmSdI\",\"y\":\"eUaveaPQ4CpyfY7sfCqEF1DCOoxHdMpPHW15BmUF0w\"}", + "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"oq_00cGL3SxUZTA-JvcXALhfQya7elFuC7jcJScN7Bs\",\"y\":\"1nNPIinv_gQiwStfx7vqs7Vt_MSyzoQDy9sCnZlFfg\"}", + "{\"crv\":\"P-521\",\"kty\":\"EC\",\"x\":\"AMNQr/q+YGv4GfkEjrXH2N0+hnGes4cCqahJlV39m3aJpqSK+uiAvkRE5SDm2bZBc3YHGzhDzfMTUpnvXwjugUQP\",\"y\":\"fIwouWsnp44Fjh2gBmO8ZafnpXZwLOCoaT5itu/Q4Z6j3duRfqmDsqyxZueDA3Gaac2LkbWGplT7mg4j7vCuGsw=\"}" + ] + end + + it 'prepends a 0-byte so that the keys parse correctly' do + example_keysets.each do |keyset_json| + keypair = described_class.import(JSON.parse(keyset_json)) + end + end + end end end end From 6d473ab2ef6a0c1c3b00faa76d6c638e146c5ef1 Mon Sep 17 00:00:00 2001 From: Julik Tarkhanov Date: Sun, 28 Jan 2024 14:01:21 +0100 Subject: [PATCH 2/8] Fix the bug --- lib/jwt/jwk/ec.rb | 10 +++++++--- spec/jwt/jwk/ec_spec.rb | 5 +++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/lib/jwt/jwk/ec.rb b/lib/jwt/jwk/ec.rb index cb545f89..11af329d 100644 --- a/lib/jwt/jwk/ec.rb +++ b/lib/jwt/jwk/ec.rb @@ -143,7 +143,6 @@ def parse_ec_key(key) if ::JWT.openssl_3? def create_ec_key(jwk_crv, jwk_x, jwk_y, jwk_d) # rubocop:disable Metrics/MethodLength curve = EC.to_openssl_curve(jwk_crv) - x_octets = decode_octets(jwk_x) y_octets = decode_octets(jwk_y) @@ -205,8 +204,13 @@ def create_ec_key(jwk_crv, jwk_x, jwk_y, jwk_d) end end - def decode_octets(jwk_data) - ::JWT::Base64.url_decode(jwk_data) + def decode_octets(base64_encoded_coordinate) + bytes = ::JWT::Base64.url_decode(base64_encoded_coordinate) + if bytes.bytesize.odd? + "\0".b + bytes + else + bytes + end end def decode_open_ssl_bn(jwk_data) diff --git a/spec/jwt/jwk/ec_spec.rb b/spec/jwt/jwk/ec_spec.rb index 24b941c3..3ac40a5d 100644 --- a/spec/jwt/jwk/ec_spec.rb +++ b/spec/jwt/jwk/ec_spec.rb @@ -154,9 +154,10 @@ ] end - it 'prepends a 0-byte so that the keys parse correctly' do + it 'prepends a 0-byte to either X or Y coordinate so that the keys decode correctly' do example_keysets.each do |keyset_json| - keypair = described_class.import(JSON.parse(keyset_json)) + jwk = described_class.import(JSON.parse(keyset_json)) + expect(jwk).to be_kind_of(JWT::JWK::EC) end end end From 57d438caa60339613a7256cf979cb05763cac6b8 Mon Sep 17 00:00:00 2001 From: Julik Tarkhanov Date: Sun, 28 Jan 2024 14:06:19 +0100 Subject: [PATCH 3/8] Add a good explainer --- lib/jwt/jwk/ec.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/jwt/jwk/ec.rb b/lib/jwt/jwk/ec.rb index 11af329d..ed2d63f6 100644 --- a/lib/jwt/jwk/ec.rb +++ b/lib/jwt/jwk/ec.rb @@ -206,6 +206,20 @@ def create_ec_key(jwk_crv, jwk_x, jwk_y, jwk_d) def decode_octets(base64_encoded_coordinate) bytes = ::JWT::Base64.url_decode(base64_encoded_coordinate) + # Some base64 encoders on some platform omit a single 0-byte at + # the start of either Y or X coordinate of the elliptic curve point. + # This leads to an encoding error when data is passed to OpenSSL BN. + # It is know to have happend to exported JWKs on a Java application and + # on a Flutter/Dart application (both iOS and Android). All that is + # needed to fix the problem is adding a leading 0-byte. We know the + # required byte is 0 because with any other byte the point is no longer + # on the curve - and OpenSSL will actually communicate this via another + # exception. The indication of a stripped byte will be the fact that the + # coordinates - once decoded into bytes - should always be an even + # bytesize. For example, with a P-521 curve, both x and y must be 66 bytes. + # With a P-256 curve, both x and y must be 32 and so on. The simplest way + # to check for this truncation is thus to check whether the number of bytes + # is odd, and restore the leading 0-byte if it is. if bytes.bytesize.odd? "\0".b + bytes else From 56d940d3bcfe8d357b058e57065de336440cb3b9 Mon Sep 17 00:00:00 2001 From: Julik Tarkhanov Date: Sun, 28 Jan 2024 14:08:17 +0100 Subject: [PATCH 4/8] Apply Rubocop --- spec/jwt/jwk/ec_spec.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/spec/jwt/jwk/ec_spec.rb b/spec/jwt/jwk/ec_spec.rb index 3ac40a5d..cc95f627 100644 --- a/spec/jwt/jwk/ec_spec.rb +++ b/spec/jwt/jwk/ec_spec.rb @@ -142,15 +142,15 @@ context 'with missing 0-byte at the start of EC coordinates' do let(:example_keysets) do [ - "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"0Nv5IKAlkvXuAKmOmFgmrwXKR7qGePOzu_7RXg5msw\",\"y\":\"FqnPSNutcjfvXNlufwb7nLJuUEnBkbMdZ3P79nY9c3k\"}", - "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"xGjPg-7meZamM_yfkGeBUB2eJ5c82Y8vQdXwi5cVGw\",\"y\":\"9FwKAuJacVyEy71yoVn1u1ETsQoiwF7QfkfXURGxg14\"}", - "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"yTvy0bwt5s29mIg1DMq-IjZH4pDgZIN9keEEaSuWZhk\",\"y\":\"a0nrmd8qz8jpZDgpY82Rgv3vZ5xiJuiAoMIuRlGnaw\"}", - "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"yJen7AW4lLUTMH4luDj0wlMNSGCuOBB5R-ZoxlAU_g\",\"y\":\"aMbA-M6ORHePSatiPVz_Pzu7z2XRnKMzK-HIscpfud8\"}", - "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"p_D00Z1ydC7mBIpSKPUUrzVzY9Fr5NMhhGfnf4P9guw\",\"y\":\"lCqM3B_s04uhm7_91oycBvoWzuQWJCbMoZc46uqHXA\"}", - "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"hKS-vxV1bvfZ2xOuHv6Qt3lmHIiArTnhWac31kXw3w\",\"y\":\"f_UWjrTpmq_oTdfss7YJ-9dEiYw_JC90kwAE-y0Yu-w\"}", - "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"3W22hN16OJN1XPpUQuCxtwoBRlf-wGyBNIihQiTmSdI\",\"y\":\"eUaveaPQ4CpyfY7sfCqEF1DCOoxHdMpPHW15BmUF0w\"}", - "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"oq_00cGL3SxUZTA-JvcXALhfQya7elFuC7jcJScN7Bs\",\"y\":\"1nNPIinv_gQiwStfx7vqs7Vt_MSyzoQDy9sCnZlFfg\"}", - "{\"crv\":\"P-521\",\"kty\":\"EC\",\"x\":\"AMNQr/q+YGv4GfkEjrXH2N0+hnGes4cCqahJlV39m3aJpqSK+uiAvkRE5SDm2bZBc3YHGzhDzfMTUpnvXwjugUQP\",\"y\":\"fIwouWsnp44Fjh2gBmO8ZafnpXZwLOCoaT5itu/Q4Z6j3duRfqmDsqyxZueDA3Gaac2LkbWGplT7mg4j7vCuGsw=\"}" + '{"kty":"EC","crv":"P-256","x":"0Nv5IKAlkvXuAKmOmFgmrwXKR7qGePOzu_7RXg5msw","y":"FqnPSNutcjfvXNlufwb7nLJuUEnBkbMdZ3P79nY9c3k"}', + '{"kty":"EC","crv":"P-256","x":"xGjPg-7meZamM_yfkGeBUB2eJ5c82Y8vQdXwi5cVGw","y":"9FwKAuJacVyEy71yoVn1u1ETsQoiwF7QfkfXURGxg14"}', + '{"kty":"EC","crv":"P-256","x":"yTvy0bwt5s29mIg1DMq-IjZH4pDgZIN9keEEaSuWZhk","y":"a0nrmd8qz8jpZDgpY82Rgv3vZ5xiJuiAoMIuRlGnaw"}', + '{"kty":"EC","crv":"P-256","x":"yJen7AW4lLUTMH4luDj0wlMNSGCuOBB5R-ZoxlAU_g","y":"aMbA-M6ORHePSatiPVz_Pzu7z2XRnKMzK-HIscpfud8"}', + '{"kty":"EC","crv":"P-256","x":"p_D00Z1ydC7mBIpSKPUUrzVzY9Fr5NMhhGfnf4P9guw","y":"lCqM3B_s04uhm7_91oycBvoWzuQWJCbMoZc46uqHXA"}', + '{"kty":"EC","crv":"P-256","x":"hKS-vxV1bvfZ2xOuHv6Qt3lmHIiArTnhWac31kXw3w","y":"f_UWjrTpmq_oTdfss7YJ-9dEiYw_JC90kwAE-y0Yu-w"}', + '{"kty":"EC","crv":"P-256","x":"3W22hN16OJN1XPpUQuCxtwoBRlf-wGyBNIihQiTmSdI","y":"eUaveaPQ4CpyfY7sfCqEF1DCOoxHdMpPHW15BmUF0w"}', + '{"kty":"EC","crv":"P-256","x":"oq_00cGL3SxUZTA-JvcXALhfQya7elFuC7jcJScN7Bs","y":"1nNPIinv_gQiwStfx7vqs7Vt_MSyzoQDy9sCnZlFfg"}', + '{"crv":"P-521","kty":"EC","x":"AMNQr/q+YGv4GfkEjrXH2N0+hnGes4cCqahJlV39m3aJpqSK+uiAvkRE5SDm2bZBc3YHGzhDzfMTUpnvXwjugUQP","y":"fIwouWsnp44Fjh2gBmO8ZafnpXZwLOCoaT5itu/Q4Z6j3duRfqmDsqyxZueDA3Gaac2LkbWGplT7mg4j7vCuGsw="}' ] end From 16835afc184e5db7c87e40a46144f3ea4770883c Mon Sep 17 00:00:00 2001 From: Julik Tarkhanov Date: Sun, 28 Jan 2024 14:08:54 +0100 Subject: [PATCH 5/8] Remove unused method it was apparently copied over by mistake - no tests reach it --- lib/jwt/jwk/ec.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/jwt/jwk/ec.rb b/lib/jwt/jwk/ec.rb index ed2d63f6..99f924d4 100644 --- a/lib/jwt/jwk/ec.rb +++ b/lib/jwt/jwk/ec.rb @@ -227,10 +227,6 @@ def decode_octets(base64_encoded_coordinate) end end - def decode_open_ssl_bn(jwk_data) - OpenSSL::BN.new(::JWT::Base64.url_decode(jwk_data), BINARY) - end - class << self def import(jwk_data) new(jwk_data) From b7ed51f2022c5ba8494b1fd5558825192d1b9bc3 Mon Sep 17 00:00:00 2001 From: Julik Tarkhanov Date: Sun, 28 Jan 2024 23:23:39 +0100 Subject: [PATCH 6/8] Update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index eefc178e..05a0d8ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +**Fixes and enhancements:** + +- Repair EC x/y coordinates when importing JWK [#585](https://github.com/jwt/ruby-jwt/pull/585) - [@julik](https://github.com/julik). + ## [v2.7.2](https://github.com/jwt/ruby-jwt/tree/v2.7.2) (NEXT) [Full Changelog](https://github.com/jwt/ruby-jwt/compare/v2.7.1...v2.7.2) From 1f86372a593fc25d63de88775ece6d6bbedb27f6 Mon Sep 17 00:00:00 2001 From: Julik Tarkhanov Date: Mon, 29 Jan 2024 11:00:25 +0100 Subject: [PATCH 7/8] Relocate into constant --- lib/jwt/jwk/ec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/jwt/jwk/ec.rb b/lib/jwt/jwk/ec.rb index 99f924d4..32dee96f 100644 --- a/lib/jwt/jwk/ec.rb +++ b/lib/jwt/jwk/ec.rb @@ -11,6 +11,7 @@ class EC < KeyBase # rubocop:disable Metrics/ClassLength EC_PUBLIC_KEY_ELEMENTS = %i[kty crv x y].freeze EC_PRIVATE_KEY_ELEMENTS = %i[d].freeze EC_KEY_ELEMENTS = (EC_PRIVATE_KEY_ELEMENTS + EC_PUBLIC_KEY_ELEMENTS).freeze + ZERO_BYTE = "\0".b.freeze def initialize(key, params = nil, options = {}) params ||= {} @@ -221,7 +222,7 @@ def decode_octets(base64_encoded_coordinate) # to check for this truncation is thus to check whether the number of bytes # is odd, and restore the leading 0-byte if it is. if bytes.bytesize.odd? - "\0".b + bytes + ZERO_BYTE + bytes else bytes end From 73cb83f62b4b6515a257214f744f6ea977019434 Mon Sep 17 00:00:00 2001 From: Julik Tarkhanov Date: Mon, 29 Jan 2024 11:01:17 +0100 Subject: [PATCH 8/8] Relocate changelog entry --- CHANGELOG.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05a0d8ac..52888015 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,5 @@ # Changelog -**Fixes and enhancements:** - -- Repair EC x/y coordinates when importing JWK [#585](https://github.com/jwt/ruby-jwt/pull/585) - [@julik](https://github.com/julik). - ## [v2.7.2](https://github.com/jwt/ruby-jwt/tree/v2.7.2) (NEXT) [Full Changelog](https://github.com/jwt/ruby-jwt/compare/v2.7.1...v2.7.2) @@ -20,6 +16,7 @@ - Fix key base equality and spaceship operators [#569](https://github.com/jwt/ruby-jwt/pull/569) - [@magneland](https://github.com/magneland). - Remove explicit base64 require from x5c_key_finder [#580](https://github.com/jwt/ruby-jwt/pull/580) - [@anakinj](https://github.com/anakinj). - Performance improvements and cleanup of tests [#581](https://github.com/jwt/ruby-jwt/pull/581) - [@anakinj](https://github.com/anakinj). +- Repair EC x/y coordinates when importing JWK [#585](https://github.com/jwt/ruby-jwt/pull/585) - [@julik](https://github.com/julik). - Your contribution here ## [v2.7.1](https://github.com/jwt/ruby-jwt/tree/v2.8.0) (2023-06-09)