From 8708afebe65c395056e91de9b19b2a1df41de629 Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Mon, 16 Dec 2024 09:06:36 +0100 Subject: [PATCH 1/2] Extract JWT parsing into separate class JWT parsing is rather involved, because we need to fetch proper certificates first. We will need to parse JWTs in a different context than authorization as well, so it makes sense to have the parsing centralized. This also allowed to add specs for this previously not (unit) tested piece of code. --- .../strategies/warden/jwt_oidc.rb | 69 ++------ .../openid_connect/provider_token_parser.rb | 95 +++++++++++ .../provider_token_parser_spec.rb | 147 ++++++++++++++++++ spec/requests/api/v3/authentication_spec.rb | 103 +++++++----- 4 files changed, 321 insertions(+), 93 deletions(-) create mode 100644 modules/openid_connect/app/services/openid_connect/provider_token_parser.rb create mode 100644 modules/openid_connect/spec/services/openid_connect/provider_token_parser_spec.rb diff --git a/lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb b/lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb index 94d3edb0d56e..c96ac9c087f8 100644 --- a/lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb +++ b/lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb @@ -5,12 +5,6 @@ module Warden class JwtOidc < ::Warden::Strategies::Base include FailWithHeader - SUPPORTED_ALG = %w[ - RS256 - RS384 - RS512 - ].freeze - # The strategy is supposed to only handle JWT. # These tokens are supposed to be issued by configured OIDC. def valid? @@ -19,60 +13,29 @@ def valid? ) return false if @access_token.blank? - @unverified_payload, @unverified_header = JWT.decode(@access_token, nil, false) - @unverified_header.present? && @unverified_payload.present? + unverified_payload, unverified_header = JWT.decode(@access_token, nil, false) + unverified_payload.present? && unverified_header.present? rescue JWT::DecodeError false end def authenticate! - issuer = @unverified_payload["iss"] - provider = OpenProject::OpenIDConnect.providers.find { |p| p.configuration[:issuer] == issuer } if issuer.present? - if provider.blank? - return fail_with_header!(error: "invalid_token", error_description: "The access token issuer is unknown") - end - - client_id = provider.configuration.fetch(:identifier) - alg = @unverified_header.fetch("alg") - if SUPPORTED_ALG.exclude?(alg) - return fail_with_header!(error: "invalid_token", error_description: "Token signature algorithm is not supported") - end - - kid = @unverified_header.fetch("kid") - jwks_uri = provider.configuration[:jwks_uri] - begin - key = JSON::JWK::Set::Fetcher.fetch(jwks_uri, kid:).to_key - rescue JSON::JWK::Set::KidNotFound - return fail_with_header!(error: "invalid_token", error_description: "The access token signature kid is unknown") - end - - begin - verified_payload, = JWT.decode( - @access_token, - key, - true, - { - algorithm: alg, - verify_iss: true, - verify_aud: true, - iss: issuer, - aud: client_id, - required_claims: ["sub", "iss", "aud"] - } - ) - rescue JWT::ExpiredSignature - return fail_with_header!(error: "invalid_token", error_description: "The access token expired") - rescue JWT::ImmatureSignature - # happens when nbf time is less than current - return fail_with_header!(error: "invalid_token", error_description: "The access token is used too early") - rescue JWT::InvalidIssuerError - return fail_with_header!(error: "invalid_token", error_description: "The access token issuer is wrong") - rescue JWT::InvalidAudError - return fail_with_header!(error: "invalid_token", error_description: "The access token audience claim is wrong") - end + verified_payload, provider = ::OpenIDConnect::ProviderTokenParser.new(required_claims: ["sub"]) + .parse(@access_token) - user = User.find_by(identity_url: "#{provider.name}:#{verified_payload['sub']}") + user = User.find_by(identity_url: "#{provider.slug}:#{verified_payload['sub']}") success!(user) if user + rescue JWT::ExpiredSignature + fail_with_header!(error: "invalid_token", error_description: "The access token expired") + rescue JWT::ImmatureSignature + # happens when nbf time is less than current + fail_with_header!(error: "invalid_token", error_description: "The access token is used too early") + rescue JWT::InvalidAudError + fail_with_header!(error: "invalid_token", error_description: "The access token audience claim is wrong") + rescue JSON::JWK::Set::KidNotFound + fail_with_header!(error: "invalid_token", error_description: "The access token signature kid is unknown") + rescue ::OpenIDConnect::ProviderTokenParser::Error => e + fail_with_header!(error: "invalid_token", error_description: e.message) end end end diff --git a/modules/openid_connect/app/services/openid_connect/provider_token_parser.rb b/modules/openid_connect/app/services/openid_connect/provider_token_parser.rb new file mode 100644 index 000000000000..e0e0e0a8ecce --- /dev/null +++ b/modules/openid_connect/app/services/openid_connect/provider_token_parser.rb @@ -0,0 +1,95 @@ +#-- copyright +# OpenProject is a project management system. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2017 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +# + + +module OpenIDConnect + class ProviderTokenParser + class Error < StandardError; end + + SUPPORTED_JWT_ALGORITHMS = %w[ + RS256 + RS384 + RS512 + ].freeze + + def initialize(verify_audience: true, required_claims: []) + @verify_audience = verify_audience + @required_claims = required_claims + end + + def parse(token) + issuer, alg, kid = parse_unverified_iss_alg_kid(token) + raise Error, "Token signature algorithm #{alg} is not supported" if SUPPORTED_JWT_ALGORITHMS.exclude?(alg) + + provider = fetch_provider(issuer) + raise Error, "The access token issuer is unknown" if provider.blank? + + jwks_uri = provider.jwks_uri + key = JSON::JWK::Set::Fetcher.fetch(jwks_uri, kid:).to_key + + verified_payload, = JWT.decode( + token, + key, + true, + { + algorithm: alg, + verify_aud: @verify_audience, + aud: provider.client_id, + required_claims: all_required_claims + } + ) + + [verified_payload, provider] + end + + private + + def parse_unverified_iss_alg_kid(token) + unverified_payload, unverified_header = JWT.decode(token, nil, false) + raise Error, "The token's Key Identifier (kid) is missing" unless unverified_header.key?("kid") + + [ + unverified_payload["iss"], + unverified_header.fetch("alg"), + unverified_header.fetch("kid") + ] + end + + def fetch_provider(issuer) + return nil if issuer.blank? + + OpenIDConnect::Provider.where(available: true).find { |p| p.issuer == issuer } + end + + def all_required_claims + claims = ["iss"] + @required_claims + claims << "aud" if @verify_audience + + claims.uniq + end + end +end diff --git a/modules/openid_connect/spec/services/openid_connect/provider_token_parser_spec.rb b/modules/openid_connect/spec/services/openid_connect/provider_token_parser_spec.rb new file mode 100644 index 000000000000..3084d0004d35 --- /dev/null +++ b/modules/openid_connect/spec/services/openid_connect/provider_token_parser_spec.rb @@ -0,0 +1,147 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ +require "spec_helper" + +RSpec.describe OpenIDConnect::ProviderTokenParser do + subject(:parse) { described_class.new.parse(token) } + + let(:private_key) { OpenSSL::PKey::RSA.generate(2048) } + let(:payload) { { "sub" => "M. Curie", "iss" => "International Space Station", "aud" => our_client_id } } + let(:token) { JWT.encode(payload, private_key, "RS256", { kid: "key-identifier" }) } + + let!(:provider) { create(:oidc_provider) } + let(:known_issuer) { "International Space Station" } + let(:our_client_id) { "openproject.org" } + + before do + allow(JSON::JWK::Set::Fetcher).to receive(:fetch).and_return( + instance_double(JSON::JWK, to_key: private_key.public_key) + ) + + provider.options["issuer"] = known_issuer + provider.options["client_id"] = our_client_id + provider.options["jwks_uri"] = "https://example.com/certs" + provider.save! + end + + it "parses the token" do + parsed, = parse + expect(parsed).to eq payload + end + + it "returns the provider configuration for the associated provider" do + _, p = parse + expect(p).to eq provider + end + + it "correctly queries for the token's public key" do + parse + + expect(JSON::JWK::Set::Fetcher).to have_received(:fetch).with("https://example.com/certs", kid: "key-identifier") + end + + context "when the provider signing the token is not known" do + let(:known_issuer) { "Lunar Gateway" } + + it "raises an error" do + expect { parse }.to raise_error(OpenIDConnect::ProviderTokenParser::Error, /issuer is unknown/) + end + end + + context "when the provider signing the token is not available" do + before do + provider.update!(available: false) + end + + it "raises an error" do + expect { parse }.to raise_error(OpenIDConnect::ProviderTokenParser::Error, /issuer is unknown/) + end + end + + context "when the token is not a valid JWT" do + let(:token) { Base64.encode64("banana").strip } + + it "raises an error" do + expect { parse }.to raise_error(JWT::DecodeError) + end + end + + context "when the token is signed using an unsupported signature" do + let(:token) { JWT.encode(payload, "secret", "HS256", { kid: "key-identifier" }) } + + it "raises an error" do + expect { parse }.to raise_error(OpenIDConnect::ProviderTokenParser::Error, /HS256 is not supported/) + end + end + + context "when we are not the token's audience" do + before do + payload["aud"] = "Alice" + end + + it "raises an error" do + expect { parse }.to raise_error(JWT::InvalidAudError) + end + + context "and the audience shall not be verified" do + subject(:parse) { described_class.new(verify_audience: false).parse(token) } + + it "parses the token" do + parsed, = parse + expect(parsed).to eq payload + end + end + end + + context "when the token does not indicate a Key Identifier" do + let(:token) { JWT.encode(payload, private_key, "RS256") } + + it "raises an error" do + expect { parse }.to raise_error(OpenIDConnect::ProviderTokenParser::Error, /Key Identifier .+ is missing/) + end + end + + context "when requiring a specific claim" do + subject(:parse) { described_class.new(required_claims: ["sub"]).parse(token) } + + it "parses the token" do + parsed, = parse + expect(parsed).to eq payload + end + + context "and when the required claim is missing" do + before do + payload.delete("sub") + end + + it "raises an error" do + expect { parse }.to raise_error(JWT::MissingRequiredClaim) + end + end + end +end diff --git a/spec/requests/api/v3/authentication_spec.rb b/spec/requests/api/v3/authentication_spec.rb index c95a1817b8b6..6309fd9d5b7c 100644 --- a/spec/requests/api/v3/authentication_spec.rb +++ b/spec/requests/api/v3/authentication_spec.rb @@ -367,14 +367,54 @@ def set_basic_auth_header(user, password) end describe("OIDC", :webmock) do - let(:rsa_signed_access_token_without_aud) do - "eyJhbGciOiJSUzI1NiIsInR5cCIgOiAiSldUIiwia2lkIiA6ICI5N0FteXZvUzhCRkZSZm01ODVHUGdBMTZHMUgyVjIyRWR4eHVBWVV1b0trIn0.eyJleHAiOjE3MjEyODM0MzAsImlhdCI6MTcyMTI4MzM3MCwianRpIjoiYzUyNmI0MzUtOTkxZi00NzRhLWFkMWItYzM3MTQ1NmQxZmQwIiwiaXNzIjoiaHR0cHM6Ly9rZXljbG9hay5sb2NhbC9yZWFsbXMvbWFzdGVyIiwiYXVkIjpbIm1hc3Rlci1yZWFsbSIsImFjY291bnQiXSwic3ViIjoiYjcwZTJmYmYtZWE2OC00MjBjLWE3YTUtMGEyODdjYjY4OWM2IiwidHlwIjoiQmVhcmVyIiwiYXpwIjoiaHR0cHM6Ly9vcGVucHJvamVjdC5sb2NhbCIsInNlc3Npb25fc3RhdGUiOiJlYjIzNTI0MC0wYjQ3LTQ4ZmEtOGIzZS1mM2IzMTBkMzUyZTMiLCJhY3IiOiIxIiwiYWxsb3dlZC1vcmlnaW5zIjpbImh0dHBzOi8vb3BlbnByb2plY3QubG9jYWwiXSwicmVhbG1fYWNjZXNzIjp7InJvbGVzIjpbImNyZWF0ZS1yZWFsbSIsImRlZmF1bHQtcm9sZXMtbWFzdGVyIiwib2ZmbGluZV9hY2Nlc3MiLCJhZG1pbiIsInVtYV9hdXRob3JpemF0aW9uIl19LCJyZXNvdXJjZV9hY2Nlc3MiOnsibWFzdGVyLXJlYWxtIjp7InJvbGVzIjpbInZpZXctcmVhbG0iLCJ2aWV3LWlkZW50aXR5LXByb3ZpZGVycyIsIm1hbmFnZS1pZGVudGl0eS1wcm92aWRlcnMiLCJpbXBlcnNvbmF0aW9uIiwiY3JlYXRlLWNsaWVudCIsIm1hbmFnZS11c2VycyIsInF1ZXJ5LXJlYWxtcyIsInZpZXctYXV0aG9yaXphdGlvbiIsInF1ZXJ5LWNsaWVudHMiLCJxdWVyeS11c2VycyIsIm1hbmFnZS1ldmVudHMiLCJtYW5hZ2UtcmVhbG0iLCJ2aWV3LWV2ZW50cyIsInZpZXctdXNlcnMiLCJ2aWV3LWNsaWVudHMiLCJtYW5hZ2UtYXV0aG9yaXphdGlvbiIsIm1hbmFnZS1jbGllbnRzIiwicXVlcnktZ3JvdXBzIl19LCJhY2NvdW50Ijp7InJvbGVzIjpbIm1hbmFnZS1hY2NvdW50IiwibWFuYWdlLWFjY291bnQtbGlua3MiLCJ2aWV3LXByb2ZpbGUiXX19LCJzY29wZSI6ImVtYWlsIHByb2ZpbGUiLCJzaWQiOiJlYjIzNTI0MC0wYjQ3LTQ4ZmEtOGIzZS1mM2IzMTBkMzUyZTMiLCJlbWFpbF92ZXJpZmllZCI6ZmFsc2UsInByZWZlcnJlZF91c2VybmFtZSI6ImFkbWluIn0.cLgbN9kygRwthUx0R0FazPfIUeEUVnw4HnDgN-Hsnm9oXVr6MqmfTRKEI-6n62dlnVKsdadF_tWf3jp26d6neLj1zlR-vojwaHm8A08S9m6IeMr9e0CGiYVHjrJtEeTgq6P9cJJfe7uuhSSvlG3ltFPDxaAe14Dz3BjhLO3iaCRkWfAZjKmnW-IMzzzHfGH-7of7qCAlF5ObEax38mf1Q0OmsPA4_5po-FFtw7H7FfDjsr6EXgtdwloDePkk2XIHs2XsIo0YugVHC9GqCWgBA8MBvCirFivqM53paZMnjhpQH-xgTpYGWlw3WNbG2Rny2GoEwIxdYOUO2amDQ_zkrQ" + let(:jwk) { JWT::JWK.new(OpenSSL::PKey::RSA.new(2048), kid: "my-kid", use: "sig", alg: "RS256") } + let(:payload) do + { + "exp" => token_exp.to_i, + "iat" => 1721283370, + "jti" => "c526b435-991f-474a-ad1b-c371456d1fd0", + "iss" => token_issuer, + "aud" => token_aud, + "sub" => token_sub, + "typ" => "Bearer", + "azp" => "https://openproject.local", + "session_state" => "eb235240-0b47-48fa-8b3e-f3b310d352e3", + "acr" => "1", + "allowed-origins" => ["https://openproject.local"], + "realm_access" => { "roles" => ["create-realm", "default-roles-master", "offline_access", "admin", "uma_authorization"] }, + "resource_access" => + { "master-realm" => + { "roles" => + ["view-realm", + "view-identity-providers", + "manage-identity-providers", + "impersonation", + "create-client", + "manage-users", + "query-realms", + "view-authorization", + "query-clients", + "query-users", + "manage-events", + "manage-realm", + "view-events", + "view-users", + "view-clients", + "manage-authorization", + "manage-clients", + "query-groups"] }, + "account" => { "roles" => ["manage-account", "manage-account-links", "view-profile"] } }, + "scope" => "email profile", + "sid" => "eb235240-0b47-48fa-8b3e-f3b310d352e3", + "email_verified" => false, + "preferred_username" => "admin" + } end - let(:rsa_signed_access_token_with_aud) do - "eyJhbGciOiJSUzI1NiIsInR5cCIgOiAiSldUIiwia2lkIiA6ICI5N0FteXZvUzhCRkZSZm01ODVHUGdBMTZHMUgyVjIyRWR4eHVBWVV1b0trIn0.eyJleHAiOjE3MjEyODQ3NjksImlhdCI6MTcyMTI4NDcwOSwianRpIjoiNjhiYzNmZTMtNDFhZi00MGUwLTg4NGEtNDgxNTM1MTU3NjIyIiwiaXNzIjoiaHR0cHM6Ly9rZXljbG9hay5sb2NhbC9yZWFsbXMvbWFzdGVyIiwiYXVkIjpbImh0dHBzOi8vb3BlbnByb2plY3QubG9jYWwiLCJtYXN0ZXItcmVhbG0iLCJhY2NvdW50Il0sInN1YiI6ImI3MGUyZmJmLWVhNjgtNDIwYy1hN2E1LTBhMjg3Y2I2ODljNiIsInR5cCI6IkJlYXJlciIsImF6cCI6Imh0dHBzOi8vb3BlbnByb2plY3QubG9jYWwiLCJzZXNzaW9uX3N0YXRlIjoiNWI5OWM3M2EtY2QwNS00N2MwLTgwZTctODRjYTNiYTI0MDQ1IiwiYWNyIjoiMSIsImFsbG93ZWQtb3JpZ2lucyI6WyJodHRwczovL29wZW5wcm9qZWN0LmxvY2FsIl0sInJlYWxtX2FjY2VzcyI6eyJyb2xlcyI6WyJjcmVhdGUtcmVhbG0iLCJkZWZhdWx0LXJvbGVzLW1hc3RlciIsIm9mZmxpbmVfYWNjZXNzIiwiYWRtaW4iLCJ1bWFfYXV0aG9yaXphdGlvbiJdfSwicmVzb3VyY2VfYWNjZXNzIjp7Im1hc3Rlci1yZWFsbSI6eyJyb2xlcyI6WyJ2aWV3LXJlYWxtIiwidmlldy1pZGVudGl0eS1wcm92aWRlcnMiLCJtYW5hZ2UtaWRlbnRpdHktcHJvdmlkZXJzIiwiaW1wZXJzb25hdGlvbiIsImNyZWF0ZS1jbGllbnQiLCJtYW5hZ2UtdXNlcnMiLCJxdWVyeS1yZWFsbXMiLCJ2aWV3LWF1dGhvcml6YXRpb24iLCJxdWVyeS1jbGllbnRzIiwicXVlcnktdXNlcnMiLCJtYW5hZ2UtZXZlbnRzIiwibWFuYWdlLXJlYWxtIiwidmlldy1ldmVudHMiLCJ2aWV3LXVzZXJzIiwidmlldy1jbGllbnRzIiwibWFuYWdlLWF1dGhvcml6YXRpb24iLCJtYW5hZ2UtY2xpZW50cyIsInF1ZXJ5LWdyb3VwcyJdfSwiYWNjb3VudCI6eyJyb2xlcyI6WyJtYW5hZ2UtYWNjb3VudCIsIm1hbmFnZS1hY2NvdW50LWxpbmtzIiwidmlldy1wcm9maWxlIl19fSwic2NvcGUiOiJlbWFpbCBwcm9maWxlIiwic2lkIjoiNWI5OWM3M2EtY2QwNS00N2MwLTgwZTctODRjYTNiYTI0MDQ1IiwiZW1haWxfdmVyaWZpZWQiOmZhbHNlLCJwcmVmZXJyZWRfdXNlcm5hbWUiOiJhZG1pbiJ9.WS2TWDHFU2Amglj6j4LYhsUY5oyw3J7PhllGf0MH3Kz_ETT7GZCR6MvtvY1EuOb11t_YKrQ6M8LBHhh5j9mrFNrg-vTXMaYmXwXCQxfKtHvTVbo4coEPpnW_8NEVBG8dvduLRVK_o7BbNhZH9FCe5sb_7EbA18E7evHNLWi9co4nLsSBQSeBoHRSJqD28Yr2Xj1u618bVz_grAlm0DiwhJhGzkv-JJtUGa1xQyIkNeogPWalnLpzspa2Q2i5LeLB02aoPDlQ_PkUF6Tn6IGY2for8HQQlYkjBvhxL_wMBDoNRKlFycqkCBSedsPx2m6NdmBK8ppLgaMfKe0uVGvaTg" - end - let(:token_exp) { Time.zone.at(JWT.decode(token, nil, false)[0]["exp"]) } - let(:token_sub) { JWT.decode(token, nil, false)[0]["sub"] } + let(:token) { JWT.encode(payload, jwk.signing_key, jwk[:alg], { kid: jwk[:kid] }) } + let(:token_exp) { 5.minutes.from_now } + let(:token_sub) { "b70e2fbf-ea68-420c-a7a5-0a287cb689c6" } + let(:token_aud) { ["https://openproject.local", "master-realm", "account"] } + let(:token_issuer) { "https://keycloak.local/realms/master" } let(:expected_message) { "You did not provide the correct credentials." } let(:keys_request_stub) { nil } @@ -387,9 +427,7 @@ def set_basic_auth_header(user, password) end context "when token is issued by provider not configured in OP" do - let(:token) do - "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyLCJpc3MiOiJpc3N1ZXIuY29tIn0.C9gEPaqdNSEZ4dZHz0z51VCylScIEqRLnwMCkNXuz6g" - end + let(:token_issuer) { "https://eve.example.com" } it do get resource @@ -402,15 +440,12 @@ def set_basic_auth_header(user, password) context "when token is issued by provider configured in OP" do context "when token signature algorithm is not supported" do - let(:token) do - "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5" \ - "MDIyLCJpc3MiOiJodHRwczovL2tleWNsb2FrLmxvY2FsL3JlYWxtcy9tYXN0ZXIifQ.Pwod8ZJqq3jWsbnrGw4ZU1-aLS2bSicb8PgiF78JHUc" - end + let(:token) { JWT.encode(payload, "secret", "HS256", { kid: "97AmyvoS8BFFRfm585GPgA16G1H2V22EdxxuAYUuoKk" }) } it do get resource expect(last_response).to have_http_status :unauthorized - error = "Token signature algorithm is not supported" + error = "Token signature algorithm HS256 is not supported" expect(last_response.header["WWW-Authenticate"]) .to eq(%{Bearer realm="OpenProject API", error="invalid_token", error_description="#{error}"}) expect(JSON.parse(last_response.body)).to eq(error_response_body) @@ -427,17 +462,16 @@ def set_basic_auth_header(user, password) "User-Agent" => "JSON::JWK::Set::Fetcher 2.9.0" } ) - .to_return(status: 200, body: '{"keys":[{"kid":"CANAG6lJUPKqKDoWxxXL5wAHf2U18BAzm_LJm7RPTGk","kty":"RSA","alg":"RSA-OAEP","use":"enc","n":"nqJexS6n-SxKSDUxXp_dsNwDW6cZ4Rtgqq9ut_lp1CNSph5wTnLG3aQQsTEvx5o3-SZ-pHjJ0gtEpg7clAz-w-YQyZoAXkFtQqmZJxsmdS4K0yILxO3WUNdJQlutjmq-Ri50Senn5IV7yEYWLo8St1qzUqWZhp0HKudyty24triC9UJTK03W3_Tr5c1X8vKL8duAjvLB7p_sYUOrnLq5pD5lqwxVSAiN8qS5zVNZMrhGV5aN1vN_vue_tw8c2SVOCLLTrUh3441rYaeo-UwQZF7ZTm30xflqAIfe8qMoB20wtWYAXR0D5iqkkdEH4XanCYVm5vdUFIPPvXZhRDWoNQ","e":"AQAB","x5c":["MIICmzCCAYMCBgGQupeGPzANBgkqhkiG9w0BAQsFADARMQ8wDQYDVQQDDAZtYXN0ZXIwHhcNMjQwNzE2MDgwODMwWhcNMzQwNzE2MDgxMDEwWjARMQ8wDQYDVQQDDAZtYXN0ZXIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCeol7FLqf5LEpINTFen92w3ANbpxnhG2Cqr263+WnUI1KmHnBOcsbdpBCxMS/Hmjf5Jn6keMnSC0SmDtyUDP7D5hDJmgBeQW1CqZknGyZ1LgrTIgvE7dZQ10lCW62Oar5GLnRJ6efkhXvIRhYujxK3WrNSpZmGnQcq53K3Lbi2uIL1QlMrTdbf9OvlzVfy8ovx24CO8sHun+xhQ6ucurmkPmWrDFVICI3ypLnNU1kyuEZXlo3W83++57+3DxzZJU4IstOtSHfjjWthp6j5TBBkXtlObfTF+WoAh97yoygHbTC1ZgBdHQPmKqSR0QfhdqcJhWbm91QUg8+9dmFENag1AgMBAAEwDQYJKoZIhvcNAQELBQADggEBAB/AGvP0gviPoJszj/oQgBsMpPGRHLpnTmrXnTaa7Xk2sgExAb4zUAwxGjtR347t697cpiKQYBkR2ndswnt93Sx/Ot+yn5BdYcNvZuEh5jb5bkH2V4h6/LrYljTymby+XPBEf+XLhBOjoI3SKtNJk4pEqVNwLuKKbObqJcE3G3VBVSdzRUcIrjZr7yAQeLnhczS3hJ0Ct6Y7S5Q6DK+/PU1+AvlW+7GfzpRMqVfLcqhNpRwdCVGlJYKaUJfIe1vav10D94xA0U1sKex3iA1S+1HlS2BCWx/0rXwgcquMpUZlOAKiT0K6SIFxBFFnM9eQbF97Dz7Bzw+jyqStGUcH9YA="],"x5t":"TuBfrOL00KXDrOWTv3jw7Uxx3hA","x5t#S256":"7su5lOXF5qcMuvp44ynsoyk3B0l9Sr_bOVlg768shpY"},{"kid":"97AmyvoS8BFFRfm585GPgA16G1H2V22EdxxuAYUuoKk","kty":"RSA","alg":"RS256","use":"sig","n":"jMB2r7BG4QJzLnA2_fgG1mxlh2RX_MSx0lc2lrPIVFGYBuAu8irwRLSexX5aQdD_AtnxLD4g9jiG6VEDwmWopEe0fr-QMl0IiES5tJuQMrjhajOkzr8xTYu6zl-knL0tu99iRbmKNYzEcv0TAgY_95n4gD5tPhYvY4gXuHrFKqYkJQPsSgoThlH7hAtfzsDt6yp3P2lQUESGg3pzc_J_NKnQkkggcNB06Hlz4DmcHxhWXK51P1V9cE7qh4PrhsJ-SOH5grcN9PtOZi6f2VlWdFdyisT-YehNklfVqBtdCLm7Ocghhl0HSgLuV-9dHCdwBLUpABsdsd0L3LRCUgRfjQ","e":"AQAB","x5c":["MIICmzCCAYMCBgGQupeFFTANBgkqhkiG9w0BAQsFADARMQ8wDQYDVQQDDAZtYXN0ZXIwHhcNMjQwNzE2MDgwODMwWhcNMzQwNzE2MDgxMDEwWjARMQ8wDQYDVQQDDAZtYXN0ZXIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCMwHavsEbhAnMucDb9+AbWbGWHZFf8xLHSVzaWs8hUUZgG4C7yKvBEtJ7FflpB0P8C2fEsPiD2OIbpUQPCZaikR7R+v5AyXQiIRLm0m5AyuOFqM6TOvzFNi7rOX6ScvS2732JFuYo1jMRy/RMCBj/3mfiAPm0+Fi9jiBe4esUqpiQlA+xKChOGUfuEC1/OwO3rKnc/aVBQRIaDenNz8n80qdCSSCBw0HToeXPgOZwfGFZcrnU/VX1wTuqHg+uGwn5I4fmCtw30+05mLp/ZWVZ0V3KKxP5h6E2SV9WoG10Iubs5yCGGXQdKAu5X710cJ3AEtSkAGx2x3QvctEJSBF+NAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAIoBCsOO0bXiVspoXkqdOts4+3sULbbp5aEwQscmLX017Zvv5jxdkZxUYk8L08lNB+WlC1ES4VlmtE06D0cWYErGpArJzVBKgYSA3CkA9veBEugHviMqfwg3suNc8S+GtaRBvpbVZtXydjjqA8GZ4eKhPoJLHHCX6X2Ad33Cdt0/ftucjTqAKVzzzgWZejy+ZKP6ybAqYJ+EZoPUXlyWT3uwcpGEJ3nzOYYGTfxOSmAwnH2v5Z/JWr9ex5o/+QBuBhFcg0z8NcHa3Z0E6ZC9GGxV7XztBqYicO+nONHTLCctoJmyXvLM4j8qIG2UQgPIiwIL0Jkz6xQAYyXvsb+LhM8="],"x5t":"BFrni6MoX-CJwtMT4vzij1HBSTI","x5t#S256":"-Ge3y4JRezxhGTDfbkNoz7prkokzYtbKQ9ardPtfcz4"}]}', headers: {}) + .to_return(status: 200, body: JWT::JWK::Set.new(jwk).export.to_json, headers: {}) end context "when access token has not expired yet" do context "when aud does not contain client_id" do - let(:token) { rsa_signed_access_token_without_aud } + let(:token_aud) { ["master-realm", "account"] } it do - Timecop.freeze(token_exp - 20.seconds) do - get resource - end + get resource + expect(last_response).to have_http_status :unauthorized error = "The access token audience claim is wrong" expect(last_response.header["WWW-Authenticate"]) @@ -447,24 +481,19 @@ def set_basic_auth_header(user, password) end context "when aud contains client_id" do - let(:token) { rsa_signed_access_token_with_aud } - it do - Timecop.freeze(token_exp - 20.seconds) do - get resource - end + get resource + expect(last_response).to have_http_status :ok end end end context "when access token has expired already" do - let(:token) { rsa_signed_access_token_without_aud } + let(:token_exp) { 5.minutes.ago } it do - Timecop.freeze(token_exp + 20.seconds) do - get resource - end + get resource expect(last_response).to have_http_status :unauthorized expect(last_response.header["WWW-Authenticate"]) @@ -473,14 +502,10 @@ def set_basic_auth_header(user, password) end it "caches keys request to keycloak" do - Timecop.freeze(token_exp + 20.seconds) do - get resource - end + get resource expect(last_response).to have_http_status :unauthorized - Timecop.freeze(token_exp + 20.seconds) do - get resource - end + get resource expect(last_response).to have_http_status :unauthorized expect(keys_request_stub).to have_been_made.once @@ -490,6 +515,7 @@ def set_basic_auth_header(user, password) context "when kid is absent in keycloak keys response" do let(:keys_request_stub) do + wrong_key = JWT::JWK.new(OpenSSL::PKey::RSA.new(2048), kid: "your-kid", use: "sig", alg: "RS256") stub_request(:get, "https://keycloak.local/realms/master/protocol/openid-connect/certs") .with( headers: { @@ -498,14 +524,11 @@ def set_basic_auth_header(user, password) "User-Agent" => "JSON::JWK::Set::Fetcher 2.9.0" } ) - .to_return(status: 200, body: '{"keys":[{"kid":"CANAG6lJUPKqKDoWxxXL5wAHf2U18BAzm_LJm7RPTGk","kty":"RSA","alg":"RSA-OAEP","use":"enc","n":"nqJexS6n-SxKSDUxXp_dsNwDW6cZ4Rtgqq9ut_lp1CNSph5wTnLG3aQQsTEvx5o3-SZ-pHjJ0gtEpg7clAz-w-YQyZoAXkFtQqmZJxsmdS4K0yILxO3WUNdJQlutjmq-Ri50Senn5IV7yEYWLo8St1qzUqWZhp0HKudyty24triC9UJTK03W3_Tr5c1X8vKL8duAjvLB7p_sYUOrnLq5pD5lqwxVSAiN8qS5zVNZMrhGV5aN1vN_vue_tw8c2SVOCLLTrUh3441rYaeo-UwQZF7ZTm30xflqAIfe8qMoB20wtWYAXR0D5iqkkdEH4XanCYVm5vdUFIPPvXZhRDWoNQ","e":"AQAB","x5c":["MIICmzCCAYMCBgGQupeGPzANBgkqhkiG9w0BAQsFADARMQ8wDQYDVQQDDAZtYXN0ZXIwHhcNMjQwNzE2MDgwODMwWhcNMzQwNzE2MDgxMDEwWjARMQ8wDQYDVQQDDAZtYXN0ZXIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCeol7FLqf5LEpINTFen92w3ANbpxnhG2Cqr263+WnUI1KmHnBOcsbdpBCxMS/Hmjf5Jn6keMnSC0SmDtyUDP7D5hDJmgBeQW1CqZknGyZ1LgrTIgvE7dZQ10lCW62Oar5GLnRJ6efkhXvIRhYujxK3WrNSpZmGnQcq53K3Lbi2uIL1QlMrTdbf9OvlzVfy8ovx24CO8sHun+xhQ6ucurmkPmWrDFVICI3ypLnNU1kyuEZXlo3W83++57+3DxzZJU4IstOtSHfjjWthp6j5TBBkXtlObfTF+WoAh97yoygHbTC1ZgBdHQPmKqSR0QfhdqcJhWbm91QUg8+9dmFENag1AgMBAAEwDQYJKoZIhvcNAQELBQADggEBAB/AGvP0gviPoJszj/oQgBsMpPGRHLpnTmrXnTaa7Xk2sgExAb4zUAwxGjtR347t697cpiKQYBkR2ndswnt93Sx/Ot+yn5BdYcNvZuEh5jb5bkH2V4h6/LrYljTymby+XPBEf+XLhBOjoI3SKtNJk4pEqVNwLuKKbObqJcE3G3VBVSdzRUcIrjZr7yAQeLnhczS3hJ0Ct6Y7S5Q6DK+/PU1+AvlW+7GfzpRMqVfLcqhNpRwdCVGlJYKaUJfIe1vav10D94xA0U1sKex3iA1S+1HlS2BCWx/0rXwgcquMpUZlOAKiT0K6SIFxBFFnM9eQbF97Dz7Bzw+jyqStGUcH9YA="],"x5t":"TuBfrOL00KXDrOWTv3jw7Uxx3hA","x5t#S256":"7su5lOXF5qcMuvp44ynsoyk3B0l9Sr_bOVlg768shpY"},{"kid":"9755555S8BFFRfm585GPgA16G1H2V22EdxxuAYUuoKk","kty":"RSA","alg":"RS256","use":"sig","n":"jMB2r7BG4QJzLnA2_fgG1mxlh2RX_MSx0lc2lrPIVFGYBuAu8irwRLSexX5aQdD_AtnxLD4g9jiG6VEDwmWopEe0fr-QMl0IiES5tJuQMrjhajOkzr8xTYu6zl-knL0tu99iRbmKNYzEcv0TAgY_95n4gD5tPhYvY4gXuHrFKqYkJQPsSgoThlH7hAtfzsDt6yp3P2lQUESGg3pzc_J_NKnQkkggcNB06Hlz4DmcHxhWXK51P1V9cE7qh4PrhsJ-SOH5grcN9PtOZi6f2VlWdFdyisT-YehNklfVqBtdCLm7Ocghhl0HSgLuV-9dHCdwBLUpABsdsd0L3LRCUgRfjQ","e":"AQAB","x5c":["MIICmzCCAYMCBgGQupeFFTANBgkqhkiG9w0BAQsFADARMQ8wDQYDVQQDDAZtYXN0ZXIwHhcNMjQwNzE2MDgwODMwWhcNMzQwNzE2MDgxMDEwWjARMQ8wDQYDVQQDDAZtYXN0ZXIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCMwHavsEbhAnMucDb9+AbWbGWHZFf8xLHSVzaWs8hUUZgG4C7yKvBEtJ7FflpB0P8C2fEsPiD2OIbpUQPCZaikR7R+v5AyXQiIRLm0m5AyuOFqM6TOvzFNi7rOX6ScvS2732JFuYo1jMRy/RMCBj/3mfiAPm0+Fi9jiBe4esUqpiQlA+xKChOGUfuEC1/OwO3rKnc/aVBQRIaDenNz8n80qdCSSCBw0HToeXPgOZwfGFZcrnU/VX1wTuqHg+uGwn5I4fmCtw30+05mLp/ZWVZ0V3KKxP5h6E2SV9WoG10Iubs5yCGGXQdKAu5X710cJ3AEtSkAGx2x3QvctEJSBF+NAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAIoBCsOO0bXiVspoXkqdOts4+3sULbbp5aEwQscmLX017Zvv5jxdkZxUYk8L08lNB+WlC1ES4VlmtE06D0cWYErGpArJzVBKgYSA3CkA9veBEugHviMqfwg3suNc8S+GtaRBvpbVZtXydjjqA8GZ4eKhPoJLHHCX6X2Ad33Cdt0/ftucjTqAKVzzzgWZejy+ZKP6ybAqYJ+EZoPUXlyWT3uwcpGEJ3nzOYYGTfxOSmAwnH2v5Z/JWr9ex5o/+QBuBhFcg0z8NcHa3Z0E6ZC9GGxV7XztBqYicO+nONHTLCctoJmyXvLM4j8qIG2UQgPIiwIL0Jkz6xQAYyXvsb+LhM8="],"x5t":"BFrni6MoX-CJwtMT4vzij1HBSTI","x5t#S256":"-Ge3y4JRezxhGTDfbkNoz7prkokzYtbKQ9ardPtfcz4"}]}', headers: {}) + .to_return(status: 200, body: JWT::JWK::Set.new(wrong_key).export.to_json, headers: {}) end - let(:token) { rsa_signed_access_token_with_aud } it do - Timecop.freeze(token_exp - 20.seconds) do - get resource - end + get resource expect(last_response).to have_http_status :unauthorized expect(JSON.parse(last_response.body)).to eq(error_response_body) error = "The access token signature kid is unknown" From 555e27a47532624b1673dbd7ed898062a6514c2d Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Mon, 16 Dec 2024 10:12:57 +0100 Subject: [PATCH 2/2] Discover audiences from access token We want to know for which purposes tokens can be used. Assuming that we receive JWTs as access tokens, it's possible to read their audience and thus check where these tokens are usable. Importantly, it's still possible that an access token is not a JWT, so we have to allow that as well. The code could be extended in the future to send such tokens to the introspection endpoint of the IDP, hoping to receive an audience list as a result of that. --- .../openid_connect/associate_user_token.rb | 17 +++- .../associate_user_token_spec.rb | 91 +++++++++++++++++-- 2 files changed, 100 insertions(+), 8 deletions(-) diff --git a/modules/openid_connect/app/services/openid_connect/associate_user_token.rb b/modules/openid_connect/app/services/openid_connect/associate_user_token.rb index 74b3369a9472..d185f82228e9 100644 --- a/modules/openid_connect/app/services/openid_connect/associate_user_token.rb +++ b/modules/openid_connect/app/services/openid_connect/associate_user_token.rb @@ -45,9 +45,22 @@ def call(access_token:, refresh_token: nil, known_audiences: [], clear_previous: @user.oidc_user_tokens.destroy_all if clear_previous - token = @user.oidc_user_tokens.build(access_token:, refresh_token:, audiences: Array(known_audiences)) - # We should discover further audiences from the token in the future + token = @user.oidc_user_tokens.build(access_token:, refresh_token:) + token.audiences = merge_audiences(known_audiences, discover_audiences(access_token)) token.save! if token.audiences.any? end + + private + + def discover_audiences(access_token) + decoded, = ProviderTokenParser.new(verify_audience: false, required_claims: ["aud"]).parse(access_token) + Array(decoded["aud"]) + rescue StandardError + [] + end + + def merge_audiences(*args) + args.flatten.uniq + end end end diff --git a/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb b/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb index 98743ef0d014..4f688f047ad5 100644 --- a/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb +++ b/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb @@ -37,8 +37,12 @@ let(:access_token) { "access-token-foo" } let(:refresh_token) { "refresh-token-bar" } + let(:parser) { instance_double(OpenIDConnect::ProviderTokenParser, parse: [parsed_jwt, nil]) } + let(:parsed_jwt) { { "aud" => ["aud1", "aud2"] } } + before do allow(Rails.logger).to receive(:error) + allow(OpenIDConnect::ProviderTokenParser).to receive(:new).and_return(parser) end it "creates a correct user token", :aggregate_failures do @@ -48,7 +52,7 @@ expect(token.user_id).to eq user.id expect(token.access_token).to eq access_token expect(token.refresh_token).to eq refresh_token - expect(token.audiences).to eq ["io"] + expect(token.audiences).to contain_exactly("io", "aud1", "aud2") end it "logs no error" do @@ -56,6 +60,52 @@ expect(Rails.logger).not_to have_received(:error) end + it "correctly tries parsing the access token" do + subject + + expect(OpenIDConnect::ProviderTokenParser).to have_received(:new) + .with(verify_audience: false, required_claims: ["aud"]) + expect(parser).to have_received(:parse).with(access_token) + end + + context "when the JWT encodes aud as a string" do + let(:parsed_jwt) { { "aud" => "aud1" } } + + it "creates a correct user token", :aggregate_failures do + expect { subject }.to change(OpenIDConnect::UserToken, :count).by(1) + + token = OpenIDConnect::UserToken.last + expect(token.access_token).to eq access_token + expect(token.refresh_token).to eq refresh_token + expect(token.audiences).to contain_exactly("io", "aud1") + end + + it "logs no error" do + subject + expect(Rails.logger).not_to have_received(:error) + end + end + + context "when the access token is not a valid JWT" do + before do + allow(parser).to receive(:parse).and_raise("Oops, not a JWT!") + end + + it "creates a correct user token", :aggregate_failures do + expect { subject }.to change(OpenIDConnect::UserToken, :count).by(1) + + token = OpenIDConnect::UserToken.last + expect(token.access_token).to eq access_token + expect(token.refresh_token).to eq refresh_token + expect(token.audiences).to contain_exactly("io") + end + + it "logs no error" do + subject + expect(Rails.logger).not_to have_received(:error) + end + end + context "when there is no refresh token" do let(:refresh_token) { nil } @@ -66,7 +116,7 @@ expect(token.user_id).to eq user.id expect(token.access_token).to eq access_token expect(token.refresh_token).to be_nil - expect(token.audiences).to eq ["io"] + expect(token.audiences).to contain_exactly("io", "aud1", "aud2") end it "logs no error" do @@ -104,14 +154,32 @@ context "when there is no audience" do let(:args) { { access_token:, refresh_token:, known_audiences: [] } } - it "does not create a user token" do - expect { subject }.not_to change(OpenIDConnect::UserToken, :count) + it "creates a correct user token", :aggregate_failures do + expect { subject }.to change(OpenIDConnect::UserToken, :count).by(1) + + token = OpenIDConnect::UserToken.last + expect(token.access_token).to eq access_token + expect(token.refresh_token).to eq refresh_token + expect(token.audiences).to contain_exactly("aud1", "aud2") end it "logs no error" do subject expect(Rails.logger).not_to have_received(:error) end + + context "and the token has no audience defined" do + let(:parsed_jwt) { { "sub" => "ject" } } + + it "does not create a user token" do + expect { subject }.not_to change(OpenIDConnect::UserToken, :count) + end + + it "logs no error" do + subject + expect(Rails.logger).not_to have_received(:error) + end + end end context "when another user token existed before" do @@ -129,7 +197,7 @@ expect(token.user_id).to eq user.id expect(token.access_token).to eq access_token expect(token.refresh_token).to eq refresh_token - expect(token.audiences).to eq ["io"] + expect(token.audiences).to contain_exactly("io", "aud1", "aud2") end context "and when previous tokens shall be cleared" do @@ -147,7 +215,7 @@ expect(token.user_id).to eq user.id expect(token.access_token).to eq access_token expect(token.refresh_token).to eq refresh_token - expect(token.audiences).to eq ["io"] + expect(token.audiences).to contain_exactly("io", "aud1", "aud2") end it "logs no error" do @@ -156,4 +224,15 @@ end end end + + context "when audiences from token and arguments overlap" do + let(:parsed_jwt) { { "aud" => ["io", "aud2"] } } + + it "normalizes the audience array" do + subject + + token = OpenIDConnect::UserToken.last + expect(token.audiences).to contain_exactly("io", "aud2") + end + end end