diff --git a/app/controllers/graphql_controller.rb b/app/controllers/graphql_controller.rb index 8781fce439f47c707f4e32deb7154b95334b0c1a..2c9166fbedc116367fa46b247315a13adc9f1dab 100644 --- a/app/controllers/graphql_controller.rb +++ b/app/controllers/graphql_controller.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class GraphqlController < ApplicationController + include Gitlab::Auth::AuthFinders extend ::Gitlab::Utils::Override # Unauthenticated users have access to the API for public data @@ -29,6 +30,7 @@ class GraphqlController < ApplicationController prepend_before_action { authenticate_sessionless_user!(:graphql_api) } before_action :authorize_access_api! + before_action(only: [:execute]) { check_dpop! } before_action :set_user_last_activity before_action :track_vs_code_usage before_action :track_jetbrains_usage @@ -78,6 +80,12 @@ def execute end end + rescue_from Gitlab::Auth::DpopValidationError do |exception| + log_exception(exception) + + render_error(exception.message, status: :unauthorized) + end + # ApplicationController has similar rescues but we declare these again here because the # `rescue_from StandardError` above would prevent these from bubbling up to ApplicationController. # These also return errors in a JSON format similar to GraphQL errors. @@ -120,6 +128,18 @@ def feature_category private + def check_dpop! + return unless current_user && Feature.enabled?(:dpop_authentication, current_user) + + token = extract_personal_access_token + return unless PersonalAccessToken.find_by_token(token.to_s) # The token is not PAT, exit early + + # For authenticated requests we check if the user has DPoP enabled + ::Auth::DpopAuthenticationService.new(current_user: current_user, + personal_access_token_plaintext: token, + request: current_request).execute + end + def permitted_params @permitted_params ||= multiplex? ? permitted_multiplex_params : permitted_standalone_query_params end diff --git a/app/services/auth/dpop_authentication_service.rb b/app/services/auth/dpop_authentication_service.rb index cd743a620e1c488ee4336e017e9187f56ecb078f..53fd294b11d49f04b373917fb8c54276bb70ea44 100644 --- a/app/services/auth/dpop_authentication_service.rb +++ b/app/services/auth/dpop_authentication_service.rb @@ -9,7 +9,7 @@ def initialize(current_user:, personal_access_token_plaintext:, request:) end def execute - raise Gitlab::Auth::DpopValidationError, 'DPoP is not enabled for the user' unless current_user.dpop_enabled + return ServiceResponse.success unless current_user.dpop_enabled dpop_token = Gitlab::Auth::DpopToken.new(data: extract_dpop_from_request!(request)) diff --git a/config/feature_flags/beta/dpop_authentication.yml b/config/feature_flags/beta/dpop_authentication.yml new file mode 100644 index 0000000000000000000000000000000000000000..29ae1ba0a69e4792af78c07683fb6e7f4c5e564f --- /dev/null +++ b/config/feature_flags/beta/dpop_authentication.yml @@ -0,0 +1,9 @@ +--- +name: dpop_authentication +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/425130 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/169013 +rollout_issue_url: https://gitlab.com/gitlab-com/gl-infra/production/-/issues/17713 +milestone: '17.9' +group: group::authentication +type: beta +default_enabled: false diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index 279d653efb9ba66b930e9076a7356a2ff21bba46..165e035b3629217b762f5c8a10873e55c9a154cc 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -63,6 +63,8 @@ def find_current_user! forbidden!(api_access_denied_message(user)) end + check_dpop!(user) + user end @@ -114,6 +116,18 @@ def api_access_denied_message(user) Gitlab::Auth::UserAccessDeniedReason.new(user).rejection_message end + def check_dpop!(user) + return unless Feature.enabled?(:dpop_authentication, user) + return unless api_request? && user.is_a?(User) + + token = extract_personal_access_token + return unless PersonalAccessToken.find_by_token(token.to_s) # The token is not PAT, exit early + + ::Auth::DpopAuthenticationService.new(current_user: user, + personal_access_token_plaintext: token, + request: current_request).execute + end + def user_allowed_or_deploy_token?(user) Gitlab::UserAccess.new(user).allowed? || user.is_a?(DeployToken) end @@ -147,7 +161,8 @@ def install_error_responders(base) Gitlab::Auth::ExpiredError, Gitlab::Auth::RevokedError, Gitlab::Auth::ImpersonationDisabled, - Gitlab::Auth::InsufficientScopeError] + Gitlab::Auth::InsufficientScopeError, + Gitlab::Auth::DpopValidationError] base.__send__(:rescue_from, *error_classes, oauth2_bearer_token_error_handler) # rubocop:disable GitlabSecurity/PublicSend end @@ -186,6 +201,11 @@ def oauth2_bearer_token_error_handler :insufficient_scope, Rack::OAuth2::Server::Resource::ErrorMethods::DEFAULT_DESCRIPTION[:insufficient_scope], { scope: e.scopes }) + + when Gitlab::Auth::DpopValidationError + Rack::OAuth2::Server::Resource::Bearer::Unauthorized.new( + :dpop_error, + e) end status, headers, body = response.finish diff --git a/lib/gitlab/auth/auth_finders.rb b/lib/gitlab/auth/auth_finders.rb index beb13c7e981114d7465059337d673caf7791cbfd..8d23acb73e27d54164a94e693596f602bf6487db 100644 --- a/lib/gitlab/auth/auth_finders.rb +++ b/lib/gitlab/auth/auth_finders.rb @@ -224,6 +224,12 @@ def authentication_token_present? private + def extract_personal_access_token + current_request.params[PRIVATE_TOKEN_PARAM].presence || + current_request.env[PRIVATE_TOKEN_HEADER].presence || + parsed_oauth_token + end + def save_current_token_in_env ::Current.token_info = { token_id: access_token.id, @@ -284,10 +290,7 @@ def access_token end def find_personal_access_token - token = - current_request.params[PRIVATE_TOKEN_PARAM].presence || - current_request.env[PRIVATE_TOKEN_HEADER].presence || - parsed_oauth_token + token = extract_personal_access_token return unless token # Expiration, revocation and scopes are verified in `validate_access_token!` diff --git a/lib/gitlab/auth/dpop_token_user.rb b/lib/gitlab/auth/dpop_token_user.rb index 784c7ae92a999cb463c3932c12a19f57756c9900..6bdcf1b90a740e4ca7a90a4dd5460c1b865b8c9f 100644 --- a/lib/gitlab/auth/dpop_token_user.rb +++ b/lib/gitlab/auth/dpop_token_user.rb @@ -41,14 +41,30 @@ def pat_belongs_to_user! # Check that the DPoP is signed with a SSH key belonging to the user def valid_token_for_user! user_public_key = signing_key_for_user! - algorithm = algorithm_for_dpop_validation(user_public_key) openssh_public_key = convert_public_key_to_openssh_key!(user_public_key) + payload, header = decode_json_token!(user_public_key, openssh_public_key) + raise Gitlab::Auth::DpopValidationError, 'Unable to decode JWT' if payload.nil? || header.nil? + + jwk = header['jwk'] + + begin + unless openssh_public_key.to_s == OpenSSL::PKey.read(JWT::JWK::RSA.import(jwk).public_key.to_pem).to_s + raise 'Failed to parse JWK: invalid JWK' + end + rescue StandardError => e + raise Gitlab::Auth::DpopValidationError, e + end + end + + def decode_json_token!(user_public_key, openssh_public_key) # Decode the JSON token again, this time with the key, # the expected algorithm, verifying all the timestamps, etc # Overwrites the attrs, in case .decode returns a different result # when verify is true. - payload, header = JWT.decode( + algorithm = algorithm_for_dpop_validation(user_public_key) + + JWT.decode( token.data, openssh_public_key, true, @@ -58,18 +74,8 @@ def valid_token_for_user! verify_iat: true } ) - - raise Gitlab::Auth::DpopValidationError, 'Unable to decode JWT' if payload.nil? || header.nil? - - jwk = header['jwk'] - - begin - unless openssh_public_key.to_s == OpenSSL::PKey.read(JWT::JWK::RSA.import(jwk).public_key.to_pem).to_s - raise 'Failed to parse JWK: invalid JWK' - end - rescue StandardError => e - raise Gitlab::Auth::DpopValidationError, e - end + rescue JWT::DecodeError => e + raise Gitlab::Auth::DpopValidationError, "Malformed JWT, unable to decode. #{e.message}" end def signing_key_for_user! diff --git a/spec/controllers/graphql_controller_spec.rb b/spec/controllers/graphql_controller_spec.rb index 22edbbd5e211d354692372f46c07fd1c89f781f2..dde2f4d2639c98b2bc630772c131a7d766943a7d 100644 --- a/spec/controllers/graphql_controller_spec.rb +++ b/spec/controllers/graphql_controller_spec.rb @@ -4,6 +4,7 @@ RSpec.describe GraphqlController, feature_category: :integrations do include GraphqlHelpers + include Auth::DpopTokenHelper # two days is enough to make timezones irrelevant let_it_be(:last_activity_on) { 2.days.ago.to_date } @@ -530,6 +531,77 @@ end end + describe 'DPoP authentication' do + context 'when :dpop_authentication FF is disabled' do + let(:user) { create(:user, last_activity_on: last_activity_on) } + let(:personal_access_token) { create(:personal_access_token, user: user, scopes: [:api]) } + + it 'does not check for DPoP token' do + stub_feature_flags(dpop_authentication: false) + + post :execute, params: { access_token: personal_access_token.token } + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'when :dpop_authentication FF is enabled' do + before do + stub_feature_flags(dpop_authentication: true) + end + + context 'when DPoP is disabled for the user' do + let(:user) { create(:user, last_activity_on: last_activity_on) } + let(:personal_access_token) { create(:personal_access_token, user: user, scopes: [:api]) } + + it 'does not check for DPoP token' do + post :execute, params: { access_token: personal_access_token.token } + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'when DPoP is enabled for the user' do + let_it_be(:user) { create(:user, last_activity_on: last_activity_on, dpop_enabled: true) } + let_it_be(:personal_access_token) { create(:personal_access_token, user: user, scopes: [:api]) } + let_it_be(:oauth_token) { create(:oauth_access_token, user: user, scopes: [:api]) } + let_it_be(:dpop_proof) { generate_dpop_proof_for(user) } + + context 'when API is called with an OAuth token' do + it 'does not invoke DPoP' do + request.headers["Authorization"] = "Bearer #{oauth_token.plaintext_token}" + post :execute + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'with a missing DPoP token' do + it 'returns 401' do + post :execute, params: { access_token: personal_access_token.token } + expect(response).to have_gitlab_http_status(:unauthorized) + expect(json_response["errors"][0]["message"]).to eq("DPoP validation error: DPoP header is missing") + end + end + + context 'with a valid DPoP token' do + it 'returns 200' do + request.headers["dpop"] = dpop_proof.proof + post :execute, params: { access_token: personal_access_token.token } + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'with a malformed DPoP token' do + it 'returns 401' do + request.headers["dpop"] = "invalid" + post :execute, params: { access_token: personal_access_token.token } # -- We need the entire error message + expect(json_response["errors"][0]["message"]) + .to eq("DPoP validation error: Malformed JWT, unable to decode. Not enough or too many segments") + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + end + end + end + context 'when user is not logged in' do it 'returns 200' do post :execute diff --git a/spec/requests/api/api_spec.rb b/spec/requests/api/api_spec.rb index e0a4557c880e81d62665e13baf219bd3a0887a96..54ee2df8230d45ccd4e85f8e745532dcbf4b27a7 100644 --- a/spec/requests/api/api_spec.rb +++ b/spec/requests/api/api_spec.rb @@ -4,6 +4,7 @@ RSpec.describe API::API, feature_category: :system_access do include GroupAPIHelpers + include Auth::DpopTokenHelper describe 'Record user last activity in after hook' do # It does not matter which endpoint is used because last_activity_on should @@ -30,6 +31,73 @@ end end + describe 'DPoP authentication' do + context 'when :dpop_authentication FF is disabled' do + let(:user) { create(:user) } + + it 'does not check for DPoP token' do + stub_feature_flags(dpop_authentication: false) + + get api('/groups') + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'when :dpop_authentication FF is enabled' do + before do + stub_feature_flags(dpop_authentication: true) + end + + context 'when DPoP is disabled for the user' do + let(:user) { create(:user) } + + it 'does not check for DPoP token' do + get api('/groups') + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'when DPoP is enabled for the user' do + let_it_be(:user) { create(:user, dpop_enabled: true) } + let_it_be(:personal_access_token) { create(:personal_access_token, user: user, scopes: [:api]) } + let_it_be(:oauth_token) { create(:oauth_access_token, user: user, scopes: [:api]) } + let_it_be(:dpop_proof) { generate_dpop_proof_for(user) } + + context 'when API is called with an OAuth token' do + it 'does not invoke DPoP' do + get api('/groups', oauth_access_token: oauth_token) + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'with a missing DPoP token' do + it 'returns 401' do + get api('/groups', personal_access_token: personal_access_token) + expect(json_response["error_description"]).to eq("DPoP validation error: DPoP header is missing") + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + + context 'with a valid DPoP token' do + it 'returns 200' do + get(api('/groups', personal_access_token: personal_access_token), headers: { "dpop" => dpop_proof.proof }) + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'with a malformed DPoP token' do + it 'returns 401' do + get(api('/groups', personal_access_token: personal_access_token), headers: { "dpop" => 'invalid' }) + # rubocop:disable Layout/LineLength -- We need the entire error message + expect(json_response["error_description"]).to eq("DPoP validation error: Malformed JWT, unable to decode. Not enough or too many segments") + # rubocop:enable Layout/LineLength + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + end + end + end + describe 'User with only read_api scope personal access token' do # It does not matter which endpoint is used because this should behave # in the same way for every request. `/groups` is used as an example diff --git a/spec/services/auth/dpop_authentication_service_spec.rb b/spec/services/auth/dpop_authentication_service_spec.rb index a289bad26e3f43e8ed359012f3ddd8c373428e47..1eec312a96317abf5f85b14575ca0db8ce457556 100644 --- a/spec/services/auth/dpop_authentication_service_spec.rb +++ b/spec/services/auth/dpop_authentication_service_spec.rb @@ -27,10 +27,8 @@ context 'when DPoP is not enabled for the user' do let(:dpop_enabled) { false } - it 'raises a DpopValidationError' do - expect do - service.execute - end.to raise_error(Gitlab::Auth::DpopValidationError, /DPoP is not enabled for the user/) + it 'succeeds' do + expect(service.execute).to be_success end end