Skip to content

Commit 5dabd6f

Browse files
stanhustanhu
committed
Support verification of HS256-signed JWTs
When an OpenID provider such as Keycloak is configured to use the HS256 algorithm to sign JSON Web Tokens (JWTs), previously logins would fail with an obscure error: ``` JSON::JWS::UnexpectedAlgorithm (no implicit conversion of OpenSSL::PKey::RSA into String) ``` The HS256 algorithm relies on a shared secret between the client and the server, and this secret is not in the list of JSON Web Key Set (JWKS) that is typically retrieved via a public endpoint during OpenID Discovery. The error was happening because decoding a HS256-signed JWT with public key RSA key will fail. We should only attempt to decode with the configured `client_options.secret`. To do this, we need to decode the JWT to examine the header to determine how it was signed. For example: ```json { "typ": "JWT", "alg": "RS256", "kid": "RrHQomcBaw2FJZ9Q5skkNaPC6sHJFLUAf4uoeaItDPE" } ``` This indicates that the payload was signed with `RS256`, a public key algorithm. Once we know the algorithm used to decode, we can determine which key to use. For public key algorithms such as `RS256`, we use the JWKS. For signature-based algoriths such as `HS256`, we use the configured client secret. This merge request also cleans up some technical debt. Previously we didn't peek at the unverified JWT to determine the key ID (`kid`) that the payload was signed with. If we got a `KidNotFound` exception from the decoding, previously we couldn't tell whether this was happening because we didn't have a matching key in JWKS, or whether the JWT didn't have a `kid` to begin with. Now that we decode the header, we can tell these cases apart. If the JWT has no `kid` and we get `KidNotFound` from decoding, we know the server didn't supply the right set of keys. Otherwise, we can just use try key until we find one that works. Relates to https://gitlab.com/gitlab-org/ruby/gems/gitlab-omniauth-openid-connect/-/issues/1 This is part of the effort to upstream changes in the GitLab fork: https://gitlab.com/gitlab-org/ruby/gems/gitlab-omniauth-openid-connect/-/issues/5.
1 parent 63417c7 commit 5dabd6f

File tree

4 files changed

+202
-17
lines changed

4 files changed

+202
-17
lines changed

‎README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ config.omniauth :openid_connect, {
6868
| pkce_verifier | Specify a custom PKCE verifier code. | no | A random 128-char string | Proc.new { SecureRandom.hex(64) } |
6969
| pkce_options | Specify a custom implementation of the PKCE code challenge/method. | no | SHA256(code_challenge) in hex | Proc to customise the code challenge generation |
7070
| client_options | A hash of client options detailed in its own section | yes | | |
71+
| jwt_secret_base64 | For HMAC with SHA2 (e.g. HS256) signing algorithms, specify the base64-encoded secret used to sign the JWT token. Defaults to the OAuth2 client secret if not specified. | no | client_options.secret | "bXlzZWNyZXQ=\n"
7172

7273
### Client Config Options
7374

‎lib/omniauth/strategies/openid_connect.rb

Lines changed: 77 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
# frozen_string_literal: true
22

3+
require 'base64'
34
require 'timeout'
45
require 'net/http'
56
require 'open-uri'
@@ -35,7 +36,8 @@ class OpenIDConnect # rubocop:disable Metrics/ClassLength
3536

3637
option :issuer
3738
option :discovery, false
38-
option :client_signing_alg
39+
option :client_signing_alg # Deprecated since we detect what is used to sign the JWT
40+
option :jwt_secret_base64
3941
option :client_jwk_signing_key
4042
option :client_x509_signing_key
4143
option :scope, [:openid]
@@ -198,13 +200,19 @@ def authorize_uri
198200
def public_key
199201
@public_key ||= if options.discovery
200202
config.jwks
201-
elsif key_or_secret
202-
key_or_secret
203+
elsif configured_public_key
204+
configured_public_key
203205
elsif client_options.jwks_uri
204206
fetch_key
205207
end
206208
end
207209

210+
# Some OpenID providers use the OAuth2 client secret as the shared secret, but
211+
# Keycloak uses a separate key that's stored inside the database.
212+
def secret
213+
base64_decoded_jwt_secret || client_options.secret
214+
end
215+
208216
def pkce_authorize_params(verifier)
209217
# NOTE: see https://tools.ietf.org/html/rfc7636#appendix-A
210218
{
@@ -219,6 +227,12 @@ def fetch_key
219227
@fetch_key ||= parse_jwk_key(::OpenIDConnect.http_client.get_content(client_options.jwks_uri))
220228
end
221229

230+
def base64_decoded_jwt_secret
231+
return unless options.jwt_secret_base64
232+
233+
Base64.decode64(options.jwt_secret_base64)
234+
end
235+
222236
def issuer
223237
resource = "#{ client_options.scheme }://#{ client_options.host }"
224238
resource = "#{ resource }:#{ client_options.port }" if client_options.port
@@ -263,8 +277,61 @@ def access_token
263277
@access_token
264278
end
265279

280+
# Unlike ::OpenIDConnect::ResponseObject::IdToken.decode, this
281+
# method splits the decoding and verification of JWT into two
282+
# steps. First, we decode the JWT without verifying it to
283+
# determine the algorithm used to sign. Then, we verify it using
284+
# the appropriate public key (e.g. if algorithm is RS256) or
285+
# shared secret (e.g. if algorithm is HS256). This works around a
286+
# limitation in the openid_connect gem:
287+
# https://github.com/nov/openid_connect/issues/61
266288
def decode_id_token(id_token)
267-
::OpenIDConnect::ResponseObject::IdToken.decode(id_token, public_key)
289+
decoded = JSON::JWT.decode(id_token, :skip_verification)
290+
algorithm = decoded.algorithm.to_sym
291+
292+
keyset =
293+
case algorithm
294+
when :HS256, :HS384, :HS512
295+
secret
296+
else
297+
public_key
298+
end
299+
300+
decoded.verify!(keyset)
301+
::OpenIDConnect::ResponseObject::IdToken.new(decoded)
302+
rescue JSON::JWK::Set::KidNotFound
303+
# If the JWT has a key ID (kid), then we know that the set of
304+
# keys supplied doesn't contain the one we want, and we're
305+
# done. However, if there is no kid, then we try each key
306+
# individually to see if one works:
307+
# https://github.com/nov/json-jwt/pull/92#issuecomment-824654949
308+
raise if decoded&.header&.key?('kid')
309+
310+
decoded = decode_with_each_key!(id_token, keyset)
311+
312+
raise unless decoded
313+
314+
decoded
315+
end
316+
317+
def decode!(id_token, key)
318+
::OpenIDConnect::ResponseObject::IdToken.decode(id_token, key)
319+
end
320+
321+
def decode_with_each_key!(id_token, keyset)
322+
return unless keyset.is_a?(JSON::JWK::Set)
323+
324+
keyset.each do |key|
325+
begin
326+
decoded = decode!(id_token, key)
327+
rescue JSON::JWS::VerificationFailed, JSON::JWS::UnexpectedAlgorithm, JSON::JWS::UnknownAlgorithm
328+
next
329+
end
330+
331+
return decoded if decoded
332+
end
333+
334+
nil
268335
end
269336

270337
def client_options
@@ -306,18 +373,12 @@ def session
306373
super
307374
end
308375

309-
def key_or_secret
310-
@key_or_secret ||=
311-
case options.client_signing_alg&.to_sym
312-
when :HS256, :HS384, :HS512
313-
client_options.secret
314-
when :RS256, :RS384, :RS512
315-
if options.client_jwk_signing_key
316-
parse_jwk_key(options.client_jwk_signing_key)
317-
elsif options.client_x509_signing_key
318-
parse_x509_key(options.client_x509_signing_key)
319-
end
320-
end
376+
def configured_public_key
377+
@configured_public_key ||= if options.client_jwk_signing_key
378+
parse_jwk_key(options.client_jwk_signing_key)
379+
elsif options.client_x509_signing_key
380+
parse_x509_key(options.client_x509_signing_key)
381+
end
321382
end
322383

323384
def parse_x509_key(key)

‎test/lib/omniauth/strategies/openid_connect_test.rb

Lines changed: 116 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,121 @@ def test_callback_phase_with_id_token
237237
strategy.callback_phase
238238
end
239239

240+
def test_callback_phase_with_id_token_no_kid
241+
other_rsa_private = OpenSSL::PKey::RSA.generate(2048)
242+
243+
key = JSON::JWK.new(private_key)
244+
other_key = JSON::JWK.new(other_rsa_private)
245+
state = SecureRandom.hex(16)
246+
request.stubs(:params).returns('id_token' => jwt.to_s, 'state' => state)
247+
request.stubs(:path_info).returns('')
248+
249+
strategy.options.issuer = issuer
250+
strategy.options.client_signing_alg = :RS256
251+
strategy.options.client_jwk_signing_key = { 'keys' => [other_key, key] }.to_json
252+
strategy.options.response_type = 'id_token'
253+
254+
strategy.unstub(:user_info)
255+
strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce })
256+
strategy.callback_phase
257+
end
258+
259+
def test_callback_phase_with_id_token_with_kid
260+
other_rsa_private = OpenSSL::PKey::RSA.generate(2048)
261+
262+
key = JSON::JWK.new(private_key)
263+
other_key = JSON::JWK.new(other_rsa_private)
264+
state = SecureRandom.hex(16)
265+
jwt_with_kid = JSON::JWT.new(payload).sign(key, :RS256)
266+
request.stubs(:params).returns('id_token' => jwt_with_kid.to_s, 'state' => state)
267+
request.stubs(:path_info).returns('')
268+
269+
strategy.options.issuer = issuer
270+
strategy.options.client_signing_alg = :RS256
271+
strategy.options.client_jwk_signing_key = { 'keys' => [other_key, key] }.to_json
272+
strategy.options.response_type = 'id_token'
273+
274+
strategy.unstub(:user_info)
275+
strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce })
276+
strategy.callback_phase
277+
end
278+
279+
def test_callback_phase_with_id_token_with_kid_and_no_matching_kid
280+
other_rsa_private = OpenSSL::PKey::RSA.generate(2048)
281+
282+
key = JSON::JWK.new(private_key)
283+
other_key = JSON::JWK.new(other_rsa_private)
284+
state = SecureRandom.hex(16)
285+
jwt_with_kid = JSON::JWT.new(payload).sign(key, :RS256)
286+
request.stubs(:params).returns('id_token' => jwt_with_kid.to_s, 'state' => state)
287+
request.stubs(:path_info).returns('')
288+
289+
strategy.options.issuer = issuer
290+
strategy.options.client_signing_alg = :RS256
291+
# We use private_key here instead of the wrapped key, which contains a kid
292+
strategy.options.client_jwk_signing_key = { 'keys' => [other_key, private_key] }.to_json
293+
strategy.options.response_type = 'id_token'
294+
295+
strategy.unstub(:user_info)
296+
strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce })
297+
298+
assert_raises JSON::JWK::Set::KidNotFound do
299+
strategy.callback_phase
300+
end
301+
end
302+
303+
def test_callback_phase_with_id_token_with_hs256
304+
state = SecureRandom.hex(16)
305+
request.stubs(:params).returns('id_token' => jwt_with_hs256.to_s, 'state' => state)
306+
request.stubs(:path_info).returns('')
307+
308+
strategy.options.issuer = issuer
309+
strategy.options.client_options.secret = hmac_secret
310+
strategy.options.client_signing_alg = :HS256
311+
strategy.options.response_type = 'id_token'
312+
313+
strategy.unstub(:user_info)
314+
strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce })
315+
strategy.callback_phase
316+
end
317+
318+
def test_callback_phase_with_hs256_base64_jwt_secret
319+
state = SecureRandom.hex(16)
320+
request.stubs(:params).returns('id_token' => jwt_with_hs256.to_s, 'state' => state)
321+
request.stubs(:path_info).returns('')
322+
323+
strategy.options.issuer = issuer
324+
strategy.options.jwt_secret_base64 = Base64.encode64(hmac_secret)
325+
strategy.options.response_type = 'id_token'
326+
327+
strategy.unstub(:user_info)
328+
strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce })
329+
strategy.callback_phase
330+
end
331+
332+
def test_callback_phase_with_id_token_no_matching_key
333+
rsa_private = OpenSSL::PKey::RSA.generate(2048)
334+
other_rsa_private = OpenSSL::PKey::RSA.generate(2048)
335+
336+
other_key = JSON::JWK.new(other_rsa_private)
337+
token = JSON::JWT.new(payload).sign(rsa_private, :RS256).to_s
338+
state = SecureRandom.hex(16)
339+
request.stubs(:params).returns('id_token' => token, 'state' => state)
340+
request.stubs(:path_info).returns('')
341+
342+
strategy.options.issuer = issuer
343+
strategy.options.client_signing_alg = :RS256
344+
strategy.options.client_jwk_signing_key = { 'keys' => [other_key] }.to_json
345+
strategy.options.response_type = 'id_token'
346+
347+
strategy.unstub(:user_info)
348+
strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce })
349+
350+
assert_raises JSON::JWK::Set::KidNotFound do
351+
strategy.callback_phase
352+
end
353+
end
354+
240355
def test_callback_phase_with_discovery # rubocop:disable Metrics/AbcSize
241356
state = SecureRandom.hex(16)
242357

@@ -609,7 +724,7 @@ def test_public_key_with_x509
609724
def test_public_key_with_hmac
610725
strategy.options.client_options.secret = 'secret'
611726
strategy.options.client_signing_alg = :HS256
612-
assert_equal strategy.options.client_options.secret, strategy.public_key
727+
assert_equal strategy.options.client_options.secret, strategy.secret
613728
end
614729

615730
def test_id_token_auth_hash

‎test/strategy_test_case.rb

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,14 @@ def jwt
3737
@jwt ||= JSON::JWT.new(payload).sign(private_key, :RS256)
3838
end
3939

40+
def hmac_secret
41+
@hmac_secret ||= SecureRandom.hex(16)
42+
end
43+
44+
def jwt_with_hs256
45+
@jwt_with_hs256 ||= JSON::JWT.new(payload).sign(hmac_secret, :HS256)
46+
end
47+
4048
def jwks
4149
@jwks ||= begin
4250
key = JSON::JWK.new(private_key)

0 commit comments

Comments
 (0)