From 9d229393ee0b84d9bf653664d991be07a21a2fd1 Mon Sep 17 00:00:00 2001 From: Hordur Freyr Yngvason Date: Wed, 16 Nov 2022 21:51:54 -0500 Subject: [PATCH 01/18] Add KAS user authorization behind a feature flag --- app/controllers/application_controller.rb | 13 ++ app/policies/clusters/instance_policy.rb | 1 + app/policies/group_policy.rb | 1 + app/policies/project_policy.rb | 1 + .../agents/authorize_proxy_user_service.rb | 95 ++++++++++++++ .../development/kas_user_access.yml | 8 ++ .../development/kas_user_access_project.yml | 8 ++ .../agents/authorize_proxy_user_service.rb | 46 +++++++ .../authorize_proxy_user_service_spec.rb | 51 ++++++++ lib/api/internal/kubernetes.rb | 47 ++++++- lib/gitlab/kas/user_access.rb | 73 +++++++++++ .../application_controller_spec.rb | 40 ++++++ spec/lib/gitlab/kas/user_access_spec.rb | 96 ++++++++++++++ spec/requests/api/internal/kubernetes_spec.rb | 117 ++++++++++++++++++ .../authorize_proxy_user_service_spec.rb | 68 ++++++++++ 15 files changed, 664 insertions(+), 1 deletion(-) create mode 100644 app/services/clusters/agents/authorize_proxy_user_service.rb create mode 100644 config/feature_flags/development/kas_user_access.yml create mode 100644 config/feature_flags/development/kas_user_access_project.yml create mode 100644 ee/app/services/ee/clusters/agents/authorize_proxy_user_service.rb create mode 100644 ee/spec/services/ee/clusters/agents/authorize_proxy_user_service_spec.rb create mode 100644 lib/gitlab/kas/user_access.rb create mode 100644 spec/lib/gitlab/kas/user_access_spec.rb create mode 100644 spec/services/clusters/agents/authorize_proxy_user_service_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index ff888cf9d72e81..fc398e05c175ac 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -55,6 +55,8 @@ class ApplicationController < ActionController::Base after_action :set_page_title_header, if: :json_request? after_action :ensure_authenticated_session_time, if: -> { current_user } + after_action :set_kas_cookie, if: -> { current_user } + protect_from_forgery with: :exception, prepend: true helper_method :can? @@ -588,6 +590,17 @@ def required_signup_info redirect_to users_sign_up_welcome_path end + + def set_kas_cookie + return unless ::Gitlab::Kas::UserAccess.enabled? + + public_session_id = Gitlab::Session.current&.id&.public_id + return unless public_session_id + + cookie_data = ::Gitlab::Kas::UserAccess.cookie_data(public_session_id) + + cookies[::Gitlab::Kas::UserAccess.cookie_key] = cookie_data + end end ApplicationController.prepend_mod diff --git a/app/policies/clusters/instance_policy.rb b/app/policies/clusters/instance_policy.rb index 3c5ca4bf4e1982..2781e943bae0b8 100644 --- a/app/policies/clusters/instance_policy.rb +++ b/app/policies/clusters/instance_policy.rb @@ -9,6 +9,7 @@ class InstancePolicy < BasePolicy enable :update_cluster enable :admin_cluster enable :read_prometheus + enable :use_k8s_proxies end end end diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index ba6e760fd2be09..9f06fe4db538ae 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -169,6 +169,7 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy enable :admin_crm_contact enable :read_cluster enable :read_group_all_available_runners + enable :use_k8s_proxies end rule { reporter }.policy do diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 2bdd8b23c62830..091ee08e9617bd 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -455,6 +455,7 @@ class ProjectPolicy < BasePolicy enable :create_deployment enable :update_deployment enable :read_cluster + enable :use_k8s_proxies enable :create_release enable :update_release enable :destroy_release diff --git a/app/services/clusters/agents/authorize_proxy_user_service.rb b/app/services/clusters/agents/authorize_proxy_user_service.rb new file mode 100644 index 00000000000000..5e38ed8d068e3f --- /dev/null +++ b/app/services/clusters/agents/authorize_proxy_user_service.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +module Clusters + module Agents + class AuthorizeProxyUserService < ::BaseService + include ::Gitlab::Utils::StrongMemoize + + def initialize(current_user, agent) + @current_user = current_user + @agent = agent + end + + def execute + return forbidden unless user_access_config.present? + + access_as = user_access_config[:access_as] + return forbidden unless access_as.present? + return forbidden if access_as.size != 1 + + if authorizations = handle_access(access_as, user_access_config) + return success(payload: authorizations) + end + + forbidden + end + + private + + attr_reader :current_user, :agent + + # Override in EE + def handle_access(access_as, user_access) + access_as_agent(user_access) if access_as.key?(:agent) + end + + def response_base + { + agent: { + id: agent.id, + config_project: { id: agent.project.id } + }, + user: { + id: current_user.id, + username: current_user.username + } + } + end + + def access_as_agent(user_access) + projects = authorized_projects(user_access) + groups = authorized_groups(user_access) + return unless projects.size + groups.size > 0 + + response_base.merge(authorizations: { agent: {} }) + end + + def authorized_projects(user_access) + user_access.fetch(:projects, []) + .first(::Clusters::Agents::RefreshAuthorizationService::AUTHORIZED_ENTITY_LIMIT) + .map { |project| ::Project.find_by_full_path(project[:id]) } + .select { |project| current_user.can?(:use_k8s_proxies, project) } + end + + def authorized_groups(user_access) + user_access.fetch(:groups, []) + .first(::Clusters::Agents::RefreshAuthorizationService::AUTHORIZED_ENTITY_LIMIT) + .map { |group| ::Group.find_by_full_path(group[:id]) } + .select { |group| current_user.can?(:use_k8s_proxies, group) } + end + + def user_access_config + # TODO: Read the configuration from the database once it has been + # indexed. See https://gitlab.com/gitlab-org/gitlab/-/issues/389430 + branch = agent.project.default_branch_or_main + path = ".gitlab/agents/#{agent.name}/config.yaml" + config_yaml = agent.project.repository + &.blob_at_branch(branch, path) + &.data + return unless config_yaml.present? + + config = YAML.safe_load(config_yaml, aliases: true, symbolize_names: true) + config[:user_access] + end + strong_memoize_attr :user_access_config + + delegate :success, to: ServiceResponse, private: true + + def forbidden + ServiceResponse.error(reason: :forbidden, message: '403 Forbidden') + end + end + end +end + +Clusters::Agents::AuthorizeProxyUserService.prepend_mod diff --git a/config/feature_flags/development/kas_user_access.yml b/config/feature_flags/development/kas_user_access.yml new file mode 100644 index 00000000000000..e1b541baaa3267 --- /dev/null +++ b/config/feature_flags/development/kas_user_access.yml @@ -0,0 +1,8 @@ +--- +name: kas_user_access +introduced_by_url: "https://gitlab.com/gitlab-org/gitlab/-/merge_requests/104504" +rollout_issue_url: +milestone: '15.8' +type: development +group: group::configure +default_enabled: false diff --git a/config/feature_flags/development/kas_user_access_project.yml b/config/feature_flags/development/kas_user_access_project.yml new file mode 100644 index 00000000000000..b23949ca75b6da --- /dev/null +++ b/config/feature_flags/development/kas_user_access_project.yml @@ -0,0 +1,8 @@ +--- +name: kas_user_access_project +introduced_by_url: "https://gitlab.com/gitlab-org/gitlab/-/merge_requests/104504" +rollout_issue_url: +milestone: '15.8' +type: development +group: group::configure +default_enabled: false diff --git a/ee/app/services/ee/clusters/agents/authorize_proxy_user_service.rb b/ee/app/services/ee/clusters/agents/authorize_proxy_user_service.rb new file mode 100644 index 00000000000000..ce976903ac5b41 --- /dev/null +++ b/ee/app/services/ee/clusters/agents/authorize_proxy_user_service.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module EE + module Clusters + module Agents + module AuthorizeProxyUserService + extend ::Gitlab::Utils::Override + + override :handle_access + def handle_access(access_as, user_access) + super || (access_as.key?(:user) && access_as_user(user_access)) + end + + def access_as_user(user_access) + projects = authorized_projects(user_access) + groups = authorized_groups(user_access) + return unless projects.size + groups.size > 0 + + response_base.merge( + authorizations: { + user: { + # FIXME: N+1 queries on projects and groups + projects: projects.map { |p| { id: p.id, roles: project_roles(p) } }, + groups: groups.map { |g| { id: g.id, roles: group_roles(g) } } + } + } + ) + end + + def project_roles(project) + user_access_level = current_user.max_member_access_for_project(project.id) + ::Gitlab::Access.sym_options_with_owner + .select { |_role, role_access_level| role_access_level <= user_access_level } + .map(&:first) + end + + def group_roles(group) + user_access_level = current_user.max_member_access_for_group(group.id) + ::Gitlab::Access.sym_options_with_owner + .select { |_role, role_access_level| role_access_level <= user_access_level } + .map(&:first) + end + end + end + end +end diff --git a/ee/spec/services/ee/clusters/agents/authorize_proxy_user_service_spec.rb b/ee/spec/services/ee/clusters/agents/authorize_proxy_user_service_spec.rb new file mode 100644 index 00000000000000..6e5382a5971e94 --- /dev/null +++ b/ee/spec/services/ee/clusters/agents/authorize_proxy_user_service_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Clusters::Agents::AuthorizeProxyUserService, feature_category: :kubernetes_management do + subject(:service_response) { service.execute } + + let(:service) { described_class.new(user, agent) } + let(:user) { create(:user) } + + let_it_be(:project) { create(:project) } + let_it_be(:group) { create(:group) } + let_it_be(:user_access_config) do + { + 'user_access' => { + 'access_as' => { 'user' => {} }, + 'projects' => [{ 'id' => project.full_path }], + 'groups' => [{ 'id' => group.full_path }] + } + } + end + + let_it_be(:configuration_project) do + create( + :project, :custom_repo, + files: { + ".gitlab/agents/the-agent/config.yaml" => user_access_config.to_yaml + } + ) + end + + let_it_be(:agent) { create(:cluster_agent, name: 'the-agent', project: configuration_project) } + + it 'returns forbidden when user has no access to any project', :aggregate_failures do + expect(service_response).to be_error + expect(service_response.reason).to eq :forbidden + end + + it "returns the user's authorizations when they have access", :aggregate_failures do + project.add_member(user, :developer) + group.add_member(user, :maintainer) + + expect(service_response).to be_success + expect(service_response.payload[:authorizations]).to eq({ + user: { + projects: [{ id: project.id, roles: %i[guest reporter developer] }], + groups: [{ id: group.id, roles: %i[guest reporter developer maintainer] }] + } + }) + end +end diff --git a/lib/api/internal/kubernetes.rb b/lib/api/internal/kubernetes.rb index 5f12275b7a06de..6e227d85b32f29 100644 --- a/lib/api/internal/kubernetes.rb +++ b/lib/api/internal/kubernetes.rb @@ -47,7 +47,7 @@ def gitaly_repository(project) end def check_feature_enabled - not_found! unless Feature.enabled?(:kubernetes_agent_internal_api, type: :ops) + not_found!('Internal API not found') unless Feature.enabled?(:kubernetes_agent_internal_api, type: :ops) end def check_agent_token @@ -135,6 +135,51 @@ def increment_count_events end end + namespace 'kubernetes/authorize_proxy_user' do + desc 'Authorize a proxy user request' + params do + requires :agent_id, type: Integer, desc: 'ID of the agent accessed' + requires :access_type, type: String, values: ['session_cookie'], desc: 'The type of the access key being verified.' + requires :access_key, type: String, desc: 'The authentication secret for the given access type.' + given access_type: ->(val) { val == 'session_cookie' } do + requires :csrf_token, type: String, allow_blank: false, desc: 'CSRF token that must be checked when access_type is "session_cookie", to ensure the request originates from a GitLab browsing session.' + end + end + post '/', feature_category: :kubernetes_management do + # Load session + public_session_id_string = + begin + Gitlab::Kas::UserAccess.decrypt_public_session_id(params[:access_key]) + rescue StandardError + bad_request!('Invalid access_key') + end + + session_id = Rack::Session::SessionId.new(public_session_id_string) + session = ActiveSession.sessions_from_ids([session_id.private_id]).first&.symbolize_keys + not_found!('ActiveSession') unless session + + # CSRF check + unless ::Gitlab::Kas::UserAccess.valid_authenticity_token?(session, params[:csrf_token]) + unauthorized!('CSRF token does not match') + end + + # Load user + user_id = session.dig(:'warden.user.user.key', 0, 0) + render_api_error!('500 Internal server error - Unexpected session data', 500) unless user_id + + user = User.find(user_id) + + # Load agent + agent = ::Clusters::Agent.find(params[:agent_id]) + unauthorized!('Feature disabled for agent') unless ::Gitlab::Kas::UserAccess.enabled_for?(agent) + + service_response = ::Clusters::Agents::AuthorizeProxyUserService.new(user, agent).execute + render_api_error!(service_response[:message], service_response[:reason]) unless service_response.success? + + service_response.payload + end + end + namespace 'kubernetes/usage_metrics' do desc 'POST usage metrics' do detail 'Updates usage metrics for agent' diff --git a/lib/gitlab/kas/user_access.rb b/lib/gitlab/kas/user_access.rb new file mode 100644 index 00000000000000..fd235d688976c2 --- /dev/null +++ b/lib/gitlab/kas/user_access.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +module Gitlab + module Kas + class UserAccess + class << self + def enabled? + ::Gitlab::Kas.enabled? && ::Feature.enabled?(:kas_user_access) + end + + def enabled_for?(agent) + enabled? && ::Feature.enabled?(:kas_user_access_project, agent.project) + end + + def encrypt_public_session_id(data) + encryptor.encrypt_and_sign(data.to_json, purpose: public_session_id_purpose) + end + + def decrypt_public_session_id(data) + decrypted = encryptor.decrypt_and_verify(data, purpose: public_session_id_purpose) + ::Gitlab::Json.parse(decrypted) + end + + def valid_authenticity_token?(session, masked_authenticity_token) + # rubocop:disable GitlabSecurity/PublicSend + ActionController::Base.new.send(:valid_authenticity_token?, session, masked_authenticity_token) + # rubocop:enable GitlabSecurity/PublicSend + end + + def cookie_key + '_gitlab_kas' + end + + def cookie_data(public_session_id) + uri = URI(::Gitlab::Kas.tunnel_url) + + cookie = { + value: encrypt_public_session_id(public_session_id), + expires: 1.day, + httponly: true, + path: uri.path.presence || '/', + secure: true # the tunnel only works over HTTPS + } + # Only set domain attribute if KAS is on a subdomain. + # When on the same domain, we can omit the attribute. + gitlab_host = Gitlab.config.gitlab.host + cookie[:domain] = gitlab_host if uri.host.end_with?(".#{gitlab_host}") + + cookie + end + + private + + def encryptor + action_dispatch_config = Gitlab::Application.config.action_dispatch + serializer = ActiveSupport::MessageEncryptor::NullSerializer + key_generator = ::Gitlab::Application.key_generator + + cipher = action_dispatch_config.encrypted_cookie_cipher + salt = action_dispatch_config.authenticated_encrypted_cookie_salt + key_len = ActiveSupport::MessageEncryptor.key_len + secret = key_generator.generate_key(salt, key_len) + + ActiveSupport::MessageEncryptor.new(secret, cipher: cipher, serializer: serializer) + end + + def public_session_id_purpose + "kas.user_public_session_id" + end + end + end + end +end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 35e374d3b7fe89..b3fd02cc22e68d 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -1117,4 +1117,44 @@ def redirect_to_example end end end + + describe '#set_kas_cookie', feature_category: :kubernetes_management do + controller do + def index + render json: {}, status: :ok + end + end + + before do + allow(::Gitlab::Kas).to receive(:enabled?).and_return(true) + end + + subject(:kas_cookie) do + get :index + + request.env['action_dispatch.cookies'][Gitlab::Kas::UserAccess.cookie_key] + end + + context 'when user is signed out' do + it { is_expected.to be_blank } + end + + context 'when user is signed in' do + let_it_be(:user) { create(:user) } + + before do + sign_in(user) + end + + it { is_expected.to be_present } + + context 'when feature flag is disabled' do + before do + stub_feature_flags(kas_user_access: false) + end + + it { is_expected.to be_blank } + end + end + end end diff --git a/spec/lib/gitlab/kas/user_access_spec.rb b/spec/lib/gitlab/kas/user_access_spec.rb new file mode 100644 index 00000000000000..106cf268842826 --- /dev/null +++ b/spec/lib/gitlab/kas/user_access_spec.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Kas::UserAccess, feature_category: :kubernetes_management do + describe '.enabled?' do + subject { described_class.enabled? } + + before do + allow(::Gitlab::Kas).to receive(:enabled?).and_return true + end + + it { is_expected.to be true } + + context 'when flag kas_user_access is disabled' do + before do + stub_feature_flags(kas_user_access: false) + end + + it { is_expected.to be false } + end + end + + describe '.enabled_for?' do + subject { described_class.enabled_for?(agent) } + + let(:agent) { build(:cluster_agent) } + + before do + allow(::Gitlab::Kas).to receive(:enabled?).and_return true + end + + it { is_expected.to be true } + + context 'when flag kas_user_access is disabled' do + before do + stub_feature_flags(kas_user_access: false) + end + + it { is_expected.to be false } + end + + context 'when flag kas_user_access_project is disabled' do + before do + stub_feature_flags(kas_user_access_project: false) + end + + it { is_expected.to be false } + end + end + + describe '.{encrypt,decrypt}_public_session_id' do + let(:data) { 'the data' } + let(:encrypted) { described_class.encrypt_public_session_id(data) } + let(:decrypted) { described_class.decrypt_public_session_id(encrypted) } + + it { expect(encrypted).not_to include data } + it { expect(decrypted).to eq data } + end + + describe '.cookie_data' do + subject(:cookie_data) { described_class.cookie_data(public_session_id) } + + let(:public_session_id) { 'the-public-session-id' } + let(:external_k8s_proxy_url) { 'https://example.com:1234' } + + before do + stub_config( + gitlab: { host: 'example.com' }, + gitlab_kas: { external_k8s_proxy_url: external_k8s_proxy_url } + ) + end + + it 'is encrypted, secure, httponly', :aggregate_failures do + expect(cookie_data[:value]).not_to include public_session_id + expect(cookie_data).to include(httponly: true, secure: true, path: '/') + expect(cookie_data).not_to have_key(:domain) + end + + context 'when on non-root path' do + let(:external_k8s_proxy_url) { 'https://example.com/k8s-proxy' } + + it 'sets :path' do + expect(cookie_data).to include(httponly: true, secure: true, path: '/k8s-proxy') + end + end + + context 'when on subdomain' do + let(:external_k8s_proxy_url) { 'https://k8s-proxy.example.com' } + + it 'sets :domain' do + expect(cookie_data[:domain]).to eq "example.com" + end + end + end +end diff --git a/spec/requests/api/internal/kubernetes_spec.rb b/spec/requests/api/internal/kubernetes_spec.rb index be76e55269ab70..ce14720d50365d 100644 --- a/spec/requests/api/internal/kubernetes_spec.rb +++ b/spec/requests/api/internal/kubernetes_spec.rb @@ -306,4 +306,121 @@ def send_request(headers: {}, params: {}) end end end + + describe 'POST /internal/kubernetes/authorize_proxy_user', :clean_gitlab_redis_sessions do + include SessionHelpers + + def send_request(headers: {}, params: {}) + post api('/internal/kubernetes/authorize_proxy_user'), params: params, headers: headers.reverse_merge(jwt_auth_headers) + end + + def stub_user_session(user, csrf_token) + stub_session( + { + 'warden.user.user.key' => [[user.id], user.authenticatable_salt], + '_csrf_token' => csrf_token + } + ) + end + + def mask_token(encoded_token) + controller = ActionController::Base.new + raw_token = controller.send(:decode_csrf_token, encoded_token) + controller.send(:mask_token, raw_token) + end + + def new_token + ActionController::Base.new.send(:generate_csrf_token) + end + + let_it_be(:project) { create(:project) } + let_it_be(:group) { create(:group) } + let_it_be(:user_access_config) do + { + 'user_access' => { + 'access_as' => { 'agent' => {} }, + 'projects' => [{ 'id' => project.full_path }], + 'groups' => [{ 'id' => group.full_path }] + } + } + end + + let_it_be(:configuration_project) do + create( + :project, :custom_repo, + files: { + ".gitlab/agents/the-agent/config.yaml" => user_access_config.to_yaml + } + ) + end + + let_it_be(:agent) { create(:cluster_agent, name: 'the-agent', project: configuration_project) } + + let(:user) { create(:user) } + + before do + allow(::Gitlab::Kas).to receive(:enabled?).and_return true + end + + it 'returns 400 when cookie is invalid' do + send_request(params: { agent_id: agent.id, access_type: 'session_cookie', access_key: '123', csrf_token: mask_token(new_token) }) + + expect(response).to have_gitlab_http_status(:bad_request) + end + + it 'returns 404 when session is not found' do + access_key = Gitlab::Kas::UserAccess.encrypt_public_session_id('abc') + send_request(params: { agent_id: agent.id, access_type: 'session_cookie', access_key: access_key, csrf_token: mask_token(new_token) }) + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'returns 401 when CSRF token does not match' do + public_id = stub_user_session(user, new_token) + access_key = Gitlab::Kas::UserAccess.encrypt_public_session_id(public_id) + send_request(params: { agent_id: agent.id, access_type: 'session_cookie', access_key: access_key, csrf_token: mask_token(new_token) }) + + expect(response).to have_gitlab_http_status(:unauthorized) + end + + it 'returns 404 for non-existent agent' do + token = new_token + public_id = stub_user_session(user, token) + access_key = Gitlab::Kas::UserAccess.encrypt_public_session_id(public_id) + send_request(params: { agent_id: non_existing_record_id, access_type: 'session_cookie', access_key: access_key, csrf_token: mask_token(token) }) + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'returns 403 when user has no access' do + token = new_token + public_id = stub_user_session(user, token) + access_key = Gitlab::Kas::UserAccess.encrypt_public_session_id(public_id) + send_request(params: { agent_id: agent.id, access_type: 'session_cookie', access_key: access_key, csrf_token: mask_token(token) }) + + expect(response).to have_gitlab_http_status(:forbidden) + end + + it 'returns 200 when user has access' do + project.add_member(user, :developer) + token = new_token + public_id = stub_user_session(user, token) + access_key = Gitlab::Kas::UserAccess.encrypt_public_session_id(public_id) + send_request(params: { agent_id: agent.id, access_type: 'session_cookie', access_key: access_key, csrf_token: mask_token(token) }) + + expect(response).to have_gitlab_http_status(:success) + end + + it 'returns 401 when global flag is disabled' do + stub_feature_flags(kas_user_access: false) + + project.add_member(user, :developer) + token = new_token + public_id = stub_user_session(user, token) + access_key = Gitlab::Kas::UserAccess.encrypt_public_session_id(public_id) + send_request(params: { agent_id: agent.id, access_type: 'session_cookie', access_key: access_key, csrf_token: mask_token(token) }) + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end end diff --git a/spec/services/clusters/agents/authorize_proxy_user_service_spec.rb b/spec/services/clusters/agents/authorize_proxy_user_service_spec.rb new file mode 100644 index 00000000000000..c099d87f6eb629 --- /dev/null +++ b/spec/services/clusters/agents/authorize_proxy_user_service_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Clusters::Agents::AuthorizeProxyUserService, feature_category: :kubernetes_management do + subject(:service_response) { service.execute } + + let(:service) { described_class.new(user, agent) } + let(:user) { create(:user) } + + let_it_be(:project) { create(:project) } + let_it_be(:group) { create(:group) } + let_it_be(:user_access_config) do + { + 'user_access' => { + 'access_as' => { 'agent' => {} }, + 'projects' => [{ 'id' => project.full_path }], + 'groups' => [{ 'id' => group.full_path }] + } + } + end + + let_it_be(:configuration_project) do + create( + :project, :custom_repo, + files: { + ".gitlab/agents/the-agent/config.yaml" => user_access_config.to_yaml + } + ) + end + + let_it_be(:agent) { create(:cluster_agent, name: 'the-agent', project: configuration_project) } + + it 'returns forbidden when user has no access to any project', :aggregate_failures do + expect(service_response).to be_error + expect(service_response.reason).to eq :forbidden + end + + context 'when user is member of an authorized group' do + it 'authorizes developers', :aggregate_failures do + group.add_member(user, :developer) + expect(service_response).to be_success + expect(service_response.payload[:user]).to include(id: user.id, username: user.username) + expect(service_response.payload[:agent]).to include(id: agent.id, config_project: { id: agent.project.id }) + end + + it 'does not authorize reporters', :aggregate_failures do + group.add_member(user, :reporter) + expect(service_response).to be_error + expect(service_response.reason).to eq :forbidden + end + end + + context 'when user is member of an authorized project' do + it 'authorizes developers', :aggregate_failures do + project.add_member(user, :developer) + expect(service_response).to be_success + expect(service_response.payload[:user]).to include(id: user.id, username: user.username) + expect(service_response.payload[:agent]).to include(id: agent.id, config_project: { id: agent.project.id }) + end + + it 'does not authorize reporters', :aggregate_failures do + project.add_member(user, :reporter) + expect(service_response).to be_error + expect(service_response.reason).to eq :forbidden + end + end +end -- GitLab From 7dd9e5aa13c272fb026789e89331f3c33cb1e094 Mon Sep 17 00:00:00 2001 From: Hordur Freyr Yngvason Date: Thu, 9 Feb 2023 13:12:55 +0000 Subject: [PATCH 02/18] Add rollout issues for feature flags --- config/feature_flags/development/kas_user_access.yml | 4 ++-- config/feature_flags/development/kas_user_access_project.yml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/config/feature_flags/development/kas_user_access.yml b/config/feature_flags/development/kas_user_access.yml index e1b541baaa3267..ac5954dcfd6a9f 100644 --- a/config/feature_flags/development/kas_user_access.yml +++ b/config/feature_flags/development/kas_user_access.yml @@ -1,8 +1,8 @@ --- name: kas_user_access introduced_by_url: "https://gitlab.com/gitlab-org/gitlab/-/merge_requests/104504" -rollout_issue_url: -milestone: '15.8' +rollout_issue_url: "https://gitlab.com/gitlab-org/gitlab/-/issues/391201" +milestone: '15.9' type: development group: group::configure default_enabled: false diff --git a/config/feature_flags/development/kas_user_access_project.yml b/config/feature_flags/development/kas_user_access_project.yml index b23949ca75b6da..ac66ed932a9c68 100644 --- a/config/feature_flags/development/kas_user_access_project.yml +++ b/config/feature_flags/development/kas_user_access_project.yml @@ -1,8 +1,8 @@ --- name: kas_user_access_project introduced_by_url: "https://gitlab.com/gitlab-org/gitlab/-/merge_requests/104504" -rollout_issue_url: -milestone: '15.8' +rollout_issue_url: "https://gitlab.com/gitlab-org/gitlab/-/issues/391211" +milestone: '15.9' type: development group: group::configure default_enabled: false -- GitLab From c517406d4e64ba16295a81b013b06662437b6fb3 Mon Sep 17 00:00:00 2001 From: Hordur Freyr Yngvason Date: Fri, 10 Feb 2023 13:02:08 +0000 Subject: [PATCH 03/18] Add KAS domain to content security policy --- lib/gitlab/content_security_policy/config_loader.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/gitlab/content_security_policy/config_loader.rb b/lib/gitlab/content_security_policy/config_loader.rb index ceca206b084063..477877e6a7ce2d 100644 --- a/lib/gitlab/content_security_policy/config_loader.rb +++ b/lib/gitlab/content_security_policy/config_loader.rb @@ -50,6 +50,7 @@ def self.default_directives allow_sentry(directives) if Gitlab::CurrentSettings.try(:sentry_enabled) && Gitlab::CurrentSettings.try(:sentry_clientside_dsn) allow_framed_gitlab_paths(directives) allow_customersdot(directives) if ENV['CUSTOMER_PORTAL_URL'].present? + allow_kas(directives) allow_review_apps(directives) if ENV['REVIEW_APPS_ENABLED'] # The follow section contains workarounds to patch Safari's lack of support for CSP Level 3 @@ -147,6 +148,17 @@ def self.allow_customersdot(directives) append_to_directive(directives, 'frame_src', customersdot_host) end + def self.allow_kas(directives) + return unless ::Gitlab::Kas::UserAccess.enabled? + + kas_url = ::Gitlab::Kas.tunnel_url + return if URI(kas_url).host == ::Gitlab.config.gitlab.host # already allowed, no need for exception + + kas_url += '/' unless kas_url.end_with?('/') + + append_to_directive(directives, 'connect_src', kas_url) + end + def self.allow_legacy_sentry(directives) # Support for Sentry setup via configuration files will be removed in 16.0 # in favor of Gitlab::CurrentSettings. -- GitLab From eaab6e3f22564109c0f6c1b9be1486d252d153cd Mon Sep 17 00:00:00 2001 From: Hordur Freyr Yngvason Date: Fri, 10 Feb 2023 13:14:31 +0000 Subject: [PATCH 04/18] Remove from ApplicationController; extract concern --- app/controllers/application_controller.rb | 13 ----- app/controllers/concerns/kas_cookie.rb | 16 ++++++ .../application_controller_spec.rb | 40 --------------- spec/controllers/concerns/kas_cookie_spec.rb | 49 +++++++++++++++++++ 4 files changed, 65 insertions(+), 53 deletions(-) create mode 100644 app/controllers/concerns/kas_cookie.rb create mode 100644 spec/controllers/concerns/kas_cookie_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index fc398e05c175ac..ff888cf9d72e81 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -55,8 +55,6 @@ class ApplicationController < ActionController::Base after_action :set_page_title_header, if: :json_request? after_action :ensure_authenticated_session_time, if: -> { current_user } - after_action :set_kas_cookie, if: -> { current_user } - protect_from_forgery with: :exception, prepend: true helper_method :can? @@ -590,17 +588,6 @@ def required_signup_info redirect_to users_sign_up_welcome_path end - - def set_kas_cookie - return unless ::Gitlab::Kas::UserAccess.enabled? - - public_session_id = Gitlab::Session.current&.id&.public_id - return unless public_session_id - - cookie_data = ::Gitlab::Kas::UserAccess.cookie_data(public_session_id) - - cookies[::Gitlab::Kas::UserAccess.cookie_key] = cookie_data - end end ApplicationController.prepend_mod diff --git a/app/controllers/concerns/kas_cookie.rb b/app/controllers/concerns/kas_cookie.rb new file mode 100644 index 00000000000000..01e55362ee4956 --- /dev/null +++ b/app/controllers/concerns/kas_cookie.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module KasCookie + extend ActiveSupport::Concern + + def set_kas_cookie + return unless ::Gitlab::Kas::UserAccess.enabled? + + public_session_id = Gitlab::Session.current&.id&.public_id + return unless public_session_id + + cookie_data = ::Gitlab::Kas::UserAccess.cookie_data(public_session_id) + + cookies[::Gitlab::Kas::UserAccess.cookie_key] = cookie_data + end +end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index b3fd02cc22e68d..35e374d3b7fe89 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -1117,44 +1117,4 @@ def redirect_to_example end end end - - describe '#set_kas_cookie', feature_category: :kubernetes_management do - controller do - def index - render json: {}, status: :ok - end - end - - before do - allow(::Gitlab::Kas).to receive(:enabled?).and_return(true) - end - - subject(:kas_cookie) do - get :index - - request.env['action_dispatch.cookies'][Gitlab::Kas::UserAccess.cookie_key] - end - - context 'when user is signed out' do - it { is_expected.to be_blank } - end - - context 'when user is signed in' do - let_it_be(:user) { create(:user) } - - before do - sign_in(user) - end - - it { is_expected.to be_present } - - context 'when feature flag is disabled' do - before do - stub_feature_flags(kas_user_access: false) - end - - it { is_expected.to be_blank } - end - end - end end diff --git a/spec/controllers/concerns/kas_cookie_spec.rb b/spec/controllers/concerns/kas_cookie_spec.rb new file mode 100644 index 00000000000000..f42037cb020ff3 --- /dev/null +++ b/spec/controllers/concerns/kas_cookie_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe KasCookie, feature_category: :kubernetes_management do + describe '#set_kas_cookie' do + controller(ApplicationController) do + include KasCookie + + def index + set_kas_cookie + + render json: {}, status: :ok + end + end + + before do + allow(::Gitlab::Kas).to receive(:enabled?).and_return(true) + end + + subject(:kas_cookie) do + get :index + + request.env['action_dispatch.cookies'][Gitlab::Kas::UserAccess.cookie_key] + end + + context 'when user is signed out' do + it { is_expected.to be_blank } + end + + context 'when user is signed in' do + let_it_be(:user) { create(:user) } + + before do + sign_in(user) + end + + it { is_expected.to be_present } + + context 'when feature flag is disabled' do + before do + stub_feature_flags(kas_user_access: false) + end + + it { is_expected.to be_blank } + end + end + end +end -- GitLab From b58d6d68409827aad7b003d6cb62e730c3970e30 Mon Sep 17 00:00:00 2001 From: Hordur Freyr Yngvason Date: Fri, 10 Feb 2023 14:58:55 +0000 Subject: [PATCH 05/18] Add KasCookie to the agents controller for testing --- app/controllers/projects/cluster_agents_controller.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/controllers/projects/cluster_agents_controller.rb b/app/controllers/projects/cluster_agents_controller.rb index 3f759e5c18c446..e0c9763abb6995 100644 --- a/app/controllers/projects/cluster_agents_controller.rb +++ b/app/controllers/projects/cluster_agents_controller.rb @@ -1,7 +1,10 @@ # frozen_string_literal: true class Projects::ClusterAgentsController < Projects::ApplicationController + include KasCookie + before_action :authorize_can_read_cluster_agent! + before_action :set_kas_cookie, only: [:show], if: -> { current_user } feature_category :kubernetes_management urgency :low -- GitLab From 071b2280e9ab8403851c7651b59354222fec88ae Mon Sep 17 00:00:00 2001 From: Timo Furrer Date: Mon, 20 Feb 2023 16:19:14 +0100 Subject: [PATCH 06/18] Move KAS User Access FFs to 15.10 --- config/feature_flags/development/kas_user_access.yml | 6 +++--- .../feature_flags/development/kas_user_access_project.yml | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/config/feature_flags/development/kas_user_access.yml b/config/feature_flags/development/kas_user_access.yml index ac5954dcfd6a9f..efcf0c15227b50 100644 --- a/config/feature_flags/development/kas_user_access.yml +++ b/config/feature_flags/development/kas_user_access.yml @@ -1,8 +1,8 @@ --- name: kas_user_access -introduced_by_url: "https://gitlab.com/gitlab-org/gitlab/-/merge_requests/104504" -rollout_issue_url: "https://gitlab.com/gitlab-org/gitlab/-/issues/391201" -milestone: '15.9' +introduced_by_url: 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/104504' +rollout_issue_url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/391201' +milestone: '15.10' type: development group: group::configure default_enabled: false diff --git a/config/feature_flags/development/kas_user_access_project.yml b/config/feature_flags/development/kas_user_access_project.yml index ac66ed932a9c68..34a4ac1271ac3d 100644 --- a/config/feature_flags/development/kas_user_access_project.yml +++ b/config/feature_flags/development/kas_user_access_project.yml @@ -1,8 +1,8 @@ --- name: kas_user_access_project -introduced_by_url: "https://gitlab.com/gitlab-org/gitlab/-/merge_requests/104504" -rollout_issue_url: "https://gitlab.com/gitlab-org/gitlab/-/issues/391211" -milestone: '15.9' +introduced_by_url: 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/104504' +rollout_issue_url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/391211' +milestone: '15.10' type: development group: group::configure default_enabled: false -- GitLab From 391af3ba51123636ea12af3d8a77e345d10f77f8 Mon Sep 17 00:00:00 2001 From: Timo Furrer Date: Mon, 20 Feb 2023 16:13:06 +0100 Subject: [PATCH 07/18] Add spec for config loader --- .../config_loader_spec.rb | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/spec/lib/gitlab/content_security_policy/config_loader_spec.rb b/spec/lib/gitlab/content_security_policy/config_loader_spec.rb index b40829d72a0db7..ffb651fe23c5b2 100644 --- a/spec/lib/gitlab/content_security_policy/config_loader_spec.rb +++ b/spec/lib/gitlab/content_security_policy/config_loader_spec.rb @@ -178,6 +178,53 @@ end end + context 'when KAS is configured' do + before do + stub_config_setting(host: 'gitlab.example.com') + allow(::Gitlab::Kas).to receive(:enabled?).and_return true + end + + context 'when user access feature flag is disabled' do + before do + stub_feature_flags(kas_user_access: false) + end + + it 'does not add KAS url to CSP' do + expect(directives['connect_src']).not_to eq("'self' ws://gitlab.example.com #{::Gitlab::Kas.tunnel_url}") + end + end + + context 'when user access feature flag is enabled' do + before do + stub_feature_flags(kas_user_access: true) + end + + context 'when KAS is on same domain as rails' do + let_it_be(:kas_tunnel_url) { "ws://gitlab.example.com/-/k8s-proxy/" } + + before do + allow(::Gitlab::Kas).to receive(:tunnel_url).and_return(kas_tunnel_url) + end + + it 'does not add KAS url to CSP' do + expect(directives['connect_src']).not_to eq("'self' ws://gitlab.example.com #{::Gitlab::Kas.tunnel_url}") + end + end + + context 'when KAS is on subdomain' do + let_it_be(:kas_tunnel_url) { "ws://kas.gitlab.example.com/k8s-proxy/" } + + before do + allow(::Gitlab::Kas).to receive(:tunnel_url).and_return(kas_tunnel_url) + end + + it 'does add KAS url to CSP' do + expect(directives['connect_src']).to eq("'self' ws://gitlab.example.com #{kas_tunnel_url}") + end + end + end + end + context 'when CUSTOMER_PORTAL_URL is set' do let(:customer_portal_url) { 'https://customers.example.com' } -- GitLab From 28751ad61cb9f0282f392ed87e685ab7db9bcf2f Mon Sep 17 00:00:00 2001 From: Timo Furrer Date: Thu, 23 Feb 2023 16:09:59 +0100 Subject: [PATCH 08/18] Respond with unauthorized instead of not found for invalid sessions in kubernetes proxy --- lib/api/internal/kubernetes.rb | 2 +- spec/requests/api/internal/kubernetes_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/api/internal/kubernetes.rb b/lib/api/internal/kubernetes.rb index 6e227d85b32f29..b17a7c51e53dd2 100644 --- a/lib/api/internal/kubernetes.rb +++ b/lib/api/internal/kubernetes.rb @@ -156,7 +156,7 @@ def increment_count_events session_id = Rack::Session::SessionId.new(public_session_id_string) session = ActiveSession.sessions_from_ids([session_id.private_id]).first&.symbolize_keys - not_found!('ActiveSession') unless session + unauthorized!('Invalid session') unless session # CSRF check unless ::Gitlab::Kas::UserAccess.valid_authenticity_token?(session, params[:csrf_token]) diff --git a/spec/requests/api/internal/kubernetes_spec.rb b/spec/requests/api/internal/kubernetes_spec.rb index ce14720d50365d..8102ef31393e02 100644 --- a/spec/requests/api/internal/kubernetes_spec.rb +++ b/spec/requests/api/internal/kubernetes_spec.rb @@ -368,11 +368,11 @@ def new_token expect(response).to have_gitlab_http_status(:bad_request) end - it 'returns 404 when session is not found' do + it 'returns 401 when session is not found' do access_key = Gitlab::Kas::UserAccess.encrypt_public_session_id('abc') send_request(params: { agent_id: agent.id, access_type: 'session_cookie', access_key: access_key, csrf_token: mask_token(new_token) }) - expect(response).to have_gitlab_http_status(:not_found) + expect(response).to have_gitlab_http_status(:unauthorized) end it 'returns 401 when CSRF token does not match' do -- GitLab From c95ce396b3c0293222abacc03d7100e5179b5977 Mon Sep 17 00:00:00 2001 From: Timo Furrer Date: Fri, 24 Feb 2023 14:19:27 +0100 Subject: [PATCH 09/18] Add more test coverage --- spec/controllers/concerns/kas_cookie_spec.rb | 10 +++++++++- spec/requests/api/internal/kubernetes_spec.rb | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/spec/controllers/concerns/kas_cookie_spec.rb b/spec/controllers/concerns/kas_cookie_spec.rb index f42037cb020ff3..f35d760e8a53e7 100644 --- a/spec/controllers/concerns/kas_cookie_spec.rb +++ b/spec/controllers/concerns/kas_cookie_spec.rb @@ -35,7 +35,15 @@ def index sign_in(user) end - it { is_expected.to be_present } + it 'sets the KAS cookie', :aggregate_failures do + allow(::Gitlab::Kas::UserAccess).to receive(:cookie_data).and_return('foobar') + + actual_cookie = subject + + expect(actual_cookie).to be_present + expect(actual_cookie).to eq('foobar') + expect(::Gitlab::Kas::UserAccess).to have_received(:cookie_data) + end context 'when feature flag is disabled' do before do diff --git a/spec/requests/api/internal/kubernetes_spec.rb b/spec/requests/api/internal/kubernetes_spec.rb index 8102ef31393e02..a2c09abf22facd 100644 --- a/spec/requests/api/internal/kubernetes_spec.rb +++ b/spec/requests/api/internal/kubernetes_spec.rb @@ -323,6 +323,15 @@ def stub_user_session(user, csrf_token) ) end + def stub_user_session_with_no_user_id(user, csrf_token) + stub_session( + { + 'warden.user.user.key' => [[nil], user.authenticatable_salt], + '_csrf_token' => csrf_token + } + ) + end + def mask_token(encoded_token) controller = ActionController::Base.new raw_token = controller.send(:decode_csrf_token, encoded_token) @@ -422,5 +431,15 @@ def new_token expect(response).to have_gitlab_http_status(:unauthorized) end + + it 'returns 500 when user id is not found in session' do + project.add_member(user, :developer) + token = new_token + public_id = stub_user_session_with_no_user_id(user, token) + access_key = Gitlab::Kas::UserAccess.encrypt_public_session_id(public_id) + send_request(params: { agent_id: agent.id, access_type: 'session_cookie', access_key: access_key, csrf_token: mask_token(token) }) + + expect(response).to have_gitlab_http_status(:internal_server_error) + end end end -- GitLab From eeceb7fa1cb86e8b45554095f8d1beb48c9e7b7d Mon Sep 17 00:00:00 2001 From: Timo Furrer Date: Tue, 28 Feb 2023 10:47:46 +0100 Subject: [PATCH 10/18] Rename `authorizations` to `access_as` in KAS k8s API --- app/services/clusters/agents/authorize_proxy_user_service.rb | 2 +- .../services/ee/clusters/agents/authorize_proxy_user_service.rb | 2 +- .../ee/clusters/agents/authorize_proxy_user_service_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/services/clusters/agents/authorize_proxy_user_service.rb b/app/services/clusters/agents/authorize_proxy_user_service.rb index 5e38ed8d068e3f..5390041dae9e07 100644 --- a/app/services/clusters/agents/authorize_proxy_user_service.rb +++ b/app/services/clusters/agents/authorize_proxy_user_service.rb @@ -51,7 +51,7 @@ def access_as_agent(user_access) groups = authorized_groups(user_access) return unless projects.size + groups.size > 0 - response_base.merge(authorizations: { agent: {} }) + response_base.merge(access_as: { agent: {} }) end def authorized_projects(user_access) diff --git a/ee/app/services/ee/clusters/agents/authorize_proxy_user_service.rb b/ee/app/services/ee/clusters/agents/authorize_proxy_user_service.rb index ce976903ac5b41..8e51bbd64554c2 100644 --- a/ee/app/services/ee/clusters/agents/authorize_proxy_user_service.rb +++ b/ee/app/services/ee/clusters/agents/authorize_proxy_user_service.rb @@ -17,7 +17,7 @@ def access_as_user(user_access) return unless projects.size + groups.size > 0 response_base.merge( - authorizations: { + access_as: { user: { # FIXME: N+1 queries on projects and groups projects: projects.map { |p| { id: p.id, roles: project_roles(p) } }, diff --git a/ee/spec/services/ee/clusters/agents/authorize_proxy_user_service_spec.rb b/ee/spec/services/ee/clusters/agents/authorize_proxy_user_service_spec.rb index 6e5382a5971e94..491ce56bba35b2 100644 --- a/ee/spec/services/ee/clusters/agents/authorize_proxy_user_service_spec.rb +++ b/ee/spec/services/ee/clusters/agents/authorize_proxy_user_service_spec.rb @@ -41,7 +41,7 @@ group.add_member(user, :maintainer) expect(service_response).to be_success - expect(service_response.payload[:authorizations]).to eq({ + expect(service_response.payload[:access_as]).to eq({ user: { projects: [{ id: project.id, roles: %i[guest reporter developer] }], groups: [{ id: group.id, roles: %i[guest reporter developer maintainer] }] -- GitLab From e25fa2540f41615f6f5f4bcc055c09d87e70bfbd Mon Sep 17 00:00:00 2001 From: Timo Furrer Date: Tue, 7 Mar 2023 09:36:00 +0100 Subject: [PATCH 11/18] Make KAS cookie key a constant --- app/controllers/concerns/kas_cookie.rb | 2 +- lib/gitlab/kas/user_access.rb | 7 +++---- spec/controllers/concerns/kas_cookie_spec.rb | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/app/controllers/concerns/kas_cookie.rb b/app/controllers/concerns/kas_cookie.rb index 01e55362ee4956..ef58ab1972be16 100644 --- a/app/controllers/concerns/kas_cookie.rb +++ b/app/controllers/concerns/kas_cookie.rb @@ -11,6 +11,6 @@ def set_kas_cookie cookie_data = ::Gitlab::Kas::UserAccess.cookie_data(public_session_id) - cookies[::Gitlab::Kas::UserAccess.cookie_key] = cookie_data + cookies[::Gitlab::Kas::COOKIE_KEY] = cookie_data end end diff --git a/lib/gitlab/kas/user_access.rb b/lib/gitlab/kas/user_access.rb index fd235d688976c2..bcb0cf7f1dc7b8 100644 --- a/lib/gitlab/kas/user_access.rb +++ b/lib/gitlab/kas/user_access.rb @@ -2,6 +2,9 @@ module Gitlab module Kas + # The name of the cookie that will be used for the KAS cookie + COOKIE_KEY = '_gitlab_kas' + class UserAccess class << self def enabled? @@ -27,10 +30,6 @@ def valid_authenticity_token?(session, masked_authenticity_token) # rubocop:enable GitlabSecurity/PublicSend end - def cookie_key - '_gitlab_kas' - end - def cookie_data(public_session_id) uri = URI(::Gitlab::Kas.tunnel_url) diff --git a/spec/controllers/concerns/kas_cookie_spec.rb b/spec/controllers/concerns/kas_cookie_spec.rb index f35d760e8a53e7..8051202e71632d 100644 --- a/spec/controllers/concerns/kas_cookie_spec.rb +++ b/spec/controllers/concerns/kas_cookie_spec.rb @@ -21,7 +21,7 @@ def index subject(:kas_cookie) do get :index - request.env['action_dispatch.cookies'][Gitlab::Kas::UserAccess.cookie_key] + request.env['action_dispatch.cookies'][Gitlab::Kas::COOKIE_KEY] end context 'when user is signed out' do -- GitLab From 7c69c83899daa089b61c994e5ca1007838bc185f Mon Sep 17 00:00:00 2001 From: Timo Furrer Date: Tue, 7 Mar 2023 09:47:33 +0100 Subject: [PATCH 12/18] Reference N+1 issue for KAS project and group access --- .../services/ee/clusters/agents/authorize_proxy_user_service.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/ee/app/services/ee/clusters/agents/authorize_proxy_user_service.rb b/ee/app/services/ee/clusters/agents/authorize_proxy_user_service.rb index 8e51bbd64554c2..a9bcd0fe919896 100644 --- a/ee/app/services/ee/clusters/agents/authorize_proxy_user_service.rb +++ b/ee/app/services/ee/clusters/agents/authorize_proxy_user_service.rb @@ -20,6 +20,7 @@ def access_as_user(user_access) access_as: { user: { # FIXME: N+1 queries on projects and groups + # see https://gitlab.com/gitlab-org/gitlab/-/issues/393336 projects: projects.map { |p| { id: p.id, roles: project_roles(p) } }, groups: groups.map { |g| { id: g.id, roles: group_roles(g) } } } -- GitLab From 20a827271224792daf22d4007a36370f2d4e92eb Mon Sep 17 00:00:00 2001 From: Timo Furrer Date: Tue, 7 Mar 2023 10:23:13 +0100 Subject: [PATCH 13/18] Use GitLab HTTPS config for KAS cookie secure setting --- lib/gitlab/kas/user_access.rb | 2 +- spec/lib/gitlab/kas/user_access_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/kas/user_access.rb b/lib/gitlab/kas/user_access.rb index bcb0cf7f1dc7b8..9e4cb479961554 100644 --- a/lib/gitlab/kas/user_access.rb +++ b/lib/gitlab/kas/user_access.rb @@ -38,7 +38,7 @@ def cookie_data(public_session_id) expires: 1.day, httponly: true, path: uri.path.presence || '/', - secure: true # the tunnel only works over HTTPS + secure: Gitlab.config.gitlab.https } # Only set domain attribute if KAS is on a subdomain. # When on the same domain, we can omit the attribute. diff --git a/spec/lib/gitlab/kas/user_access_spec.rb b/spec/lib/gitlab/kas/user_access_spec.rb index 106cf268842826..8795ad565d0ab5 100644 --- a/spec/lib/gitlab/kas/user_access_spec.rb +++ b/spec/lib/gitlab/kas/user_access_spec.rb @@ -66,7 +66,7 @@ before do stub_config( - gitlab: { host: 'example.com' }, + gitlab: { host: 'example.com', https: true }, gitlab_kas: { external_k8s_proxy_url: external_k8s_proxy_url } ) end -- GitLab From 9e008b5d3d5d0fcba09c1743a9448912fc184811 Mon Sep 17 00:00:00 2001 From: Timo Furrer Date: Tue, 7 Mar 2023 20:03:45 +0100 Subject: [PATCH 14/18] Use explicit default cookie cipher for KAS cookie encryptor --- lib/gitlab/kas/user_access.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/kas/user_access.rb b/lib/gitlab/kas/user_access.rb index 9e4cb479961554..65ae399d82650e 100644 --- a/lib/gitlab/kas/user_access.rb +++ b/lib/gitlab/kas/user_access.rb @@ -4,6 +4,7 @@ module Gitlab module Kas # The name of the cookie that will be used for the KAS cookie COOKIE_KEY = '_gitlab_kas' + DEFAULT_ENCRYPTED_COOKIE_CIPHER = 'aes-256-gcm' class UserAccess class << self @@ -55,9 +56,9 @@ def encryptor serializer = ActiveSupport::MessageEncryptor::NullSerializer key_generator = ::Gitlab::Application.key_generator - cipher = action_dispatch_config.encrypted_cookie_cipher + cipher = action_dispatch_config.encrypted_cookie_cipher || DEFAULT_ENCRYPTED_COOKIE_CIPHER salt = action_dispatch_config.authenticated_encrypted_cookie_salt - key_len = ActiveSupport::MessageEncryptor.key_len + key_len = ActiveSupport::MessageEncryptor.key_len(cipher) secret = key_generator.generate_key(salt, key_len) ActiveSupport::MessageEncryptor.new(secret, cipher: cipher, serializer: serializer) -- GitLab From c283340ebb4f9484993ef52b55bf7b47a8d50524 Mon Sep 17 00:00:00 2001 From: Timo Furrer Date: Wed, 8 Mar 2023 09:02:53 +0100 Subject: [PATCH 15/18] Use Warden session serializer instead of session dig --- lib/api/internal/kubernetes.rb | 10 ++++------ spec/requests/api/internal/kubernetes_spec.rb | 4 ++-- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/lib/api/internal/kubernetes.rb b/lib/api/internal/kubernetes.rb index b17a7c51e53dd2..bf9612db6bf537 100644 --- a/lib/api/internal/kubernetes.rb +++ b/lib/api/internal/kubernetes.rb @@ -155,19 +155,17 @@ def increment_count_events end session_id = Rack::Session::SessionId.new(public_session_id_string) - session = ActiveSession.sessions_from_ids([session_id.private_id]).first&.symbolize_keys + session = ActiveSession.sessions_from_ids([session_id.private_id]).first unauthorized!('Invalid session') unless session # CSRF check - unless ::Gitlab::Kas::UserAccess.valid_authenticity_token?(session, params[:csrf_token]) + unless ::Gitlab::Kas::UserAccess.valid_authenticity_token?(session.symbolize_keys, params[:csrf_token]) unauthorized!('CSRF token does not match') end # Load user - user_id = session.dig(:'warden.user.user.key', 0, 0) - render_api_error!('500 Internal server error - Unexpected session data', 500) unless user_id - - user = User.find(user_id) + user = Warden::SessionSerializer.new('rack.session' => session).fetch(:user) + unauthorized!('Invalid user in session') unless user # Load agent agent = ::Clusters::Agent.find(params[:agent_id]) diff --git a/spec/requests/api/internal/kubernetes_spec.rb b/spec/requests/api/internal/kubernetes_spec.rb index a2c09abf22facd..6099db5c0fe95c 100644 --- a/spec/requests/api/internal/kubernetes_spec.rb +++ b/spec/requests/api/internal/kubernetes_spec.rb @@ -432,14 +432,14 @@ def new_token expect(response).to have_gitlab_http_status(:unauthorized) end - it 'returns 500 when user id is not found in session' do + it 'returns 401 when user id is not found in session' do project.add_member(user, :developer) token = new_token public_id = stub_user_session_with_no_user_id(user, token) access_key = Gitlab::Kas::UserAccess.encrypt_public_session_id(public_id) send_request(params: { agent_id: agent.id, access_type: 'session_cookie', access_key: access_key, csrf_token: mask_token(token) }) - expect(response).to have_gitlab_http_status(:internal_server_error) + expect(response).to have_gitlab_http_status(:unauthorized) end end end -- GitLab From 45e42f22846966fbf69c2e7bf40e83568674baef Mon Sep 17 00:00:00 2001 From: Timo Furrer Date: Wed, 8 Mar 2023 11:01:25 +0100 Subject: [PATCH 16/18] Use named subject instead of separate variable --- spec/controllers/concerns/kas_cookie_spec.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/spec/controllers/concerns/kas_cookie_spec.rb b/spec/controllers/concerns/kas_cookie_spec.rb index 8051202e71632d..e2ca19457ffc5d 100644 --- a/spec/controllers/concerns/kas_cookie_spec.rb +++ b/spec/controllers/concerns/kas_cookie_spec.rb @@ -38,10 +38,8 @@ def index it 'sets the KAS cookie', :aggregate_failures do allow(::Gitlab::Kas::UserAccess).to receive(:cookie_data).and_return('foobar') - actual_cookie = subject - - expect(actual_cookie).to be_present - expect(actual_cookie).to eq('foobar') + expect(kas_cookie).to be_present + expect(kas_cookie).to eq('foobar') expect(::Gitlab::Kas::UserAccess).to have_received(:cookie_data) end -- GitLab From 6aee249bb0a07f3c19fa22c1fe6e8e8f8c7ee68f Mon Sep 17 00:00:00 2001 From: Timo Furrer Date: Wed, 8 Mar 2023 11:13:38 +0100 Subject: [PATCH 17/18] Add spec to test forbidden access to foreign agent --- spec/requests/api/internal/kubernetes_spec.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/spec/requests/api/internal/kubernetes_spec.rb b/spec/requests/api/internal/kubernetes_spec.rb index 6099db5c0fe95c..547b9071f94899 100644 --- a/spec/requests/api/internal/kubernetes_spec.rb +++ b/spec/requests/api/internal/kubernetes_spec.rb @@ -364,6 +364,7 @@ def new_token end let_it_be(:agent) { create(:cluster_agent, name: 'the-agent', project: configuration_project) } + let_it_be(:another_agent) { create(:cluster_agent) } let(:user) { create(:user) } @@ -420,6 +421,16 @@ def new_token expect(response).to have_gitlab_http_status(:success) end + it 'returns 401 when user has valid KAS cookie and CSRF token but has no access to requested agent' do + project.add_member(user, :developer) + token = new_token + public_id = stub_user_session(user, token) + access_key = Gitlab::Kas::UserAccess.encrypt_public_session_id(public_id) + send_request(params: { agent_id: another_agent.id, access_type: 'session_cookie', access_key: access_key, csrf_token: mask_token(token) }) + + expect(response).to have_gitlab_http_status(:forbidden) + end + it 'returns 401 when global flag is disabled' do stub_feature_flags(kas_user_access: false) -- GitLab From 6372f8fc367468a30076c7c849614c056992d222 Mon Sep 17 00:00:00 2001 From: Timo Furrer Date: Wed, 8 Mar 2023 15:40:50 +0100 Subject: [PATCH 18/18] Memoize authorized projects and groups calls --- .../agents/authorize_proxy_user_service.rb | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/app/services/clusters/agents/authorize_proxy_user_service.rb b/app/services/clusters/agents/authorize_proxy_user_service.rb index 5390041dae9e07..ec6645b2db4b52 100644 --- a/app/services/clusters/agents/authorize_proxy_user_service.rb +++ b/app/services/clusters/agents/authorize_proxy_user_service.rb @@ -55,17 +55,21 @@ def access_as_agent(user_access) end def authorized_projects(user_access) - user_access.fetch(:projects, []) - .first(::Clusters::Agents::RefreshAuthorizationService::AUTHORIZED_ENTITY_LIMIT) - .map { |project| ::Project.find_by_full_path(project[:id]) } - .select { |project| current_user.can?(:use_k8s_proxies, project) } + strong_memoize_with(:authorized_projects, user_access) do + user_access.fetch(:projects, []) + .first(::Clusters::Agents::RefreshAuthorizationService::AUTHORIZED_ENTITY_LIMIT) + .map { |project| ::Project.find_by_full_path(project[:id]) } + .select { |project| current_user.can?(:use_k8s_proxies, project) } + end end def authorized_groups(user_access) - user_access.fetch(:groups, []) - .first(::Clusters::Agents::RefreshAuthorizationService::AUTHORIZED_ENTITY_LIMIT) - .map { |group| ::Group.find_by_full_path(group[:id]) } - .select { |group| current_user.can?(:use_k8s_proxies, group) } + strong_memoize_with(:authorized_groups, user_access) do + user_access.fetch(:groups, []) + .first(::Clusters::Agents::RefreshAuthorizationService::AUTHORIZED_ENTITY_LIMIT) + .map { |group| ::Group.find_by_full_path(group[:id]) } + .select { |group| current_user.can?(:use_k8s_proxies, group) } + end end def user_access_config -- GitLab