From 438475d7483af2b8406899f57f57d35a064b4f59 Mon Sep 17 00:00:00 2001
From: Igor Drozdov <idrozdov@gitlab.com>
Date: Thu, 9 Nov 2023 18:42:33 +0100
Subject: [PATCH] Enfoce SSH Certificates via Settings
A new boolean group setting is introduced in order to control
whether SSH certificates are enforced or not.
The previous approach can be safely removed later because
it hasn't been released yet.
A customer has noticed that with the current approach deploy
key won't be accepted as well. It shows the limitation of
treating SSH certificates as a separate Git protocol.
Changelog: added
---
.../groups/settings/_permissions.html.haml | 1 +
.../enforce_ssh_certificates_via_settings.yml | 8 ++
..._ssh_certificates_to_namespace_settings.rb | 11 +++
db/schema_migrations/20231109165512 | 1 +
db/structure.sql | 1 +
ee/app/controllers/ee/groups_controller.rb | 1 +
ee/app/models/ee/namespace.rb | 9 ++
.../_enforce_ssh_certificates.html.haml | 7 ++
ee/lib/ee/gitlab/git_access_project.rb | 20 ++--
ee/spec/models/ee/namespace_spec.rb | 54 ++++++++++
ee/spec/requests/api/internal/base_spec.rb | 99 ++++++++++++++-----
ee/spec/requests/groups_controller_spec.rb | 29 ++++++
...enforce_ssh_certificates.html.haml_spec.rb | 37 +++++++
locale/gitlab.pot | 3 +
14 files changed, 248 insertions(+), 33 deletions(-)
create mode 100644 config/feature_flags/development/enforce_ssh_certificates_via_settings.yml
create mode 100644 db/migrate/20231109165512_add_enforce_ssh_certificates_to_namespace_settings.rb
create mode 100644 db/schema_migrations/20231109165512
create mode 100644 ee/app/views/groups/settings/_enforce_ssh_certificates.html.haml
create mode 100644 ee/spec/views/groups/settings/_enforce_ssh_certificates.html.haml_spec.rb
diff --git a/app/views/groups/settings/_permissions.html.haml b/app/views/groups/settings/_permissions.html.haml
index 8ea807003407f..b1fa63dbf5601 100644
--- a/app/views/groups/settings/_permissions.html.haml
+++ b/app/views/groups/settings/_permissions.html.haml
@@ -44,6 +44,7 @@
= render 'groups/settings/subgroup_creation_level', f: f, group: @group
= render_if_exists 'groups/settings/prevent_forking', f: f, group: @group
= render_if_exists 'groups/settings/service_access_tokens_expiration_enforced', f: f, group: @group
+ = render_if_exists 'groups/settings/enforce_ssh_certificates', f: f, group: @group
= render 'groups/settings/two_factor_auth', f: f, group: @group
= render_if_exists 'groups/personal_access_token_expiration_policy', f: f, group: @group
= render 'groups/settings/membership', f: f, group: @group
diff --git a/config/feature_flags/development/enforce_ssh_certificates_via_settings.yml b/config/feature_flags/development/enforce_ssh_certificates_via_settings.yml
new file mode 100644
index 0000000000000..e721c2afe8c8a
--- /dev/null
+++ b/config/feature_flags/development/enforce_ssh_certificates_via_settings.yml
@@ -0,0 +1,8 @@
+---
+name: enforce_ssh_certificates_via_settings
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/136498
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/426235
+milestone: '16.7'
+type: development
+group: group::source code
+default_enabled: false
diff --git a/db/migrate/20231109165512_add_enforce_ssh_certificates_to_namespace_settings.rb b/db/migrate/20231109165512_add_enforce_ssh_certificates_to_namespace_settings.rb
new file mode 100644
index 0000000000000..98c4de0cb6a57
--- /dev/null
+++ b/db/migrate/20231109165512_add_enforce_ssh_certificates_to_namespace_settings.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class AddEnforceSshCertificatesToNamespaceSettings < Gitlab::Database::Migration[2.2]
+ enable_lock_retries!
+
+ milestone '16.7'
+
+ def change
+ add_column :namespace_settings, :enforce_ssh_certificates, :boolean, default: false, null: false
+ end
+end
diff --git a/db/schema_migrations/20231109165512 b/db/schema_migrations/20231109165512
new file mode 100644
index 0000000000000..1e3a229c9d1c3
--- /dev/null
+++ b/db/schema_migrations/20231109165512
@@ -0,0 +1 @@
+2d3abd070d856db04eea298bbbe82681ca01912e19f978de876fce68ed2ada26
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index 5ea1c69abc563..6116bb5688f81 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -19472,6 +19472,7 @@ CREATE TABLE namespace_settings (
service_access_tokens_expiration_enforced boolean DEFAULT true NOT NULL,
product_analytics_enabled boolean DEFAULT false NOT NULL,
allow_merge_without_pipeline boolean DEFAULT false NOT NULL,
+ enforce_ssh_certificates boolean DEFAULT false NOT NULL,
CONSTRAINT check_0ba93c78c7 CHECK ((char_length(default_branch_name) <= 255)),
CONSTRAINT namespace_settings_unique_project_download_limit_alertlist_size CHECK ((cardinality(unique_project_download_limit_alertlist) <= 100)),
CONSTRAINT namespace_settings_unique_project_download_limit_allowlist_size CHECK ((cardinality(unique_project_download_limit_allowlist) <= 100))
diff --git a/ee/app/controllers/ee/groups_controller.rb b/ee/app/controllers/ee/groups_controller.rb
index 786e1d630b921..4b4d96f485c1f 100644
--- a/ee/app/controllers/ee/groups_controller.rb
+++ b/ee/app/controllers/ee/groups_controller.rb
@@ -98,6 +98,7 @@ def group_params_ee
params_ee << :max_personal_access_token_lifetime if current_group&.personal_access_token_expiration_policy_available?
params_ee << :prevent_forking_outside_group if can_change_prevent_forking?(current_user, current_group)
params_ee << :service_access_tokens_expiration_enforced if can_change_service_access_tokens_expiration?(current_user, current_group)
+ params_ee << :enforce_ssh_certificates if current_group&.ssh_certificates_available?
params_ee << :code_suggestions if ai_assist_ui_enabled?
params_ee << { value_stream_dashboard_aggregation_attributes: [:enabled] } if can?(current_user, :modify_value_stream_dashboard_settings, current_group)
params_ee << :product_analytics_enabled
diff --git a/ee/app/models/ee/namespace.rb b/ee/app/models/ee/namespace.rb
index b1948f7190509..c770ef222bec5 100644
--- a/ee/app/models/ee/namespace.rb
+++ b/ee/app/models/ee/namespace.rb
@@ -114,6 +114,7 @@ module Namespace
:additional_purchased_storage_ends_on, :additional_purchased_storage_ends_on=,
:temporary_storage_increase_ends_on, :temporary_storage_increase_ends_on=,
to: :namespace_limit, allow_nil: true
+ delegate :enforce_ssh_certificates=, to: :namespace_settings
# `eligible_additional_purchased_storage_size` uses a FF to start checking `additional_purchased_storage_ends_on`
# if the FF is enabled before returning `additional_purchased_storage_size`
@@ -548,6 +549,14 @@ def domain_verification_available?
::Gitlab.com? && root? && licensed_feature_available?(:domain_verification)
end
+ def enforce_ssh_certificates?
+ root? && namespace_settings.enforce_ssh_certificates?
+ end
+
+ def ssh_certificates_available?
+ root? && licensed_feature_available?(:ssh_certificates)
+ end
+
def custom_roles_enabled?
root_ancestor.licensed_feature_available?(:custom_roles)
end
diff --git a/ee/app/views/groups/settings/_enforce_ssh_certificates.html.haml b/ee/app/views/groups/settings/_enforce_ssh_certificates.html.haml
new file mode 100644
index 0000000000000..74fb495b01e70
--- /dev/null
+++ b/ee/app/views/groups/settings/_enforce_ssh_certificates.html.haml
@@ -0,0 +1,7 @@
+- if group.ssh_certificates_available?
+ %h5= s_('GroupSettings|Enforce SSH Certificates')
+
+ .form-group.gl-mb-3
+ = f.gitlab_ui_checkbox_component :enforce_ssh_certificates,
+ s_('GroupSettings|Enforce SSH Certificates'),
+ checkbox_options: { disabled: !can?(current_user, :admin_group, group), checked: group.enforce_ssh_certificates? }
diff --git a/ee/lib/ee/gitlab/git_access_project.rb b/ee/lib/ee/gitlab/git_access_project.rb
index 7adcac2635d18..adc816afcd2cc 100644
--- a/ee/lib/ee/gitlab/git_access_project.rb
+++ b/ee/lib/ee/gitlab/git_access_project.rb
@@ -34,14 +34,22 @@ def allowed_access_namespace?
# It may happen, when a user authenticates via SSH certificate and tries accessing to personal namespace
return allowed_namespace_path.blank? unless namespace&.licensed_feature_available?(:ssh_certificates)
- root_namespace = namespace.root_ancestor
-
- # When allowed_namespace_path is not specified, it's checked whether SSH certificates are not enforced
- return true if allowed_namespace_path.blank? && ::Feature.disabled?(:enforce_ssh_certificates, root_namespace)
- return root_namespace.enabled_git_access_protocol != 'ssh_certificates' if allowed_namespace_path.blank?
+ # When allowed_namespace_path is not specified, it's checked whether SSH certificates are enforced
+ return !enforced_ssh_certificates? if allowed_namespace_path.blank?
allowed_namespace = ::Namespace.find_by_full_path(allowed_namespace_path)
- allowed_namespace.present? && root_namespace.id == allowed_namespace.id
+ allowed_namespace.present? && namespace.root_ancestor.id == allowed_namespace.id
+ end
+
+ # Verify that enabled_git_access_protocol is ssh_certificates and the
+ # actor is either User or Key
+ # Deploy Keys are allowed anyway
+ def enforced_ssh_certificates?
+ return false if ::Feature.disabled?(:enforce_ssh_certificates_via_settings, namespace.root_ancestor)
+ return false unless namespace.root_ancestor.enforce_ssh_certificates?
+ return false unless actor.is_a?(User) || actor.instance_of?(::Key)
+
+ user.human?
end
end
end
diff --git a/ee/spec/models/ee/namespace_spec.rb b/ee/spec/models/ee/namespace_spec.rb
index 9def58b82a85f..6c7c6ff2c30f3 100644
--- a/ee/spec/models/ee/namespace_spec.rb
+++ b/ee/spec/models/ee/namespace_spec.rb
@@ -1971,6 +1971,60 @@
end
end
+ describe '#enforce_ssh_certificates?' do
+ let(:namespace) { create(:group) }
+
+ before do
+ namespace.enforce_ssh_certificates = true
+ end
+
+ it 'delegates the field to namespace settings' do
+ expect(namespace.namespace_settings).to receive(:enforce_ssh_certificates?).and_call_original
+
+ expect(namespace.enforce_ssh_certificates?).to eq(true)
+ end
+
+ context 'with a subgroup' do
+ let(:namespace) { create(:group, :nested) }
+
+ it 'returns false' do
+ expect(namespace.enforce_ssh_certificates?).to eq(false)
+ end
+ end
+ end
+
+ describe '#ssh_certificates_available?' do
+ let(:namespace) { create(:group) }
+
+ context 'when the feature is not licensed' do
+ before do
+ stub_licensed_features(ssh_certificates: false)
+ end
+
+ it 'is not available' do
+ expect(namespace.ssh_certificates_available?).to eq(false)
+ end
+ end
+
+ context 'when the feature is licensed' do
+ before do
+ stub_licensed_features(ssh_certificates: true)
+ end
+
+ it 'is available' do
+ expect(namespace.ssh_certificates_available?).to eq(true)
+ end
+
+ context 'with a subgroup' do
+ let(:subgroup) { create(:group, :nested) }
+
+ it 'is not available' do
+ expect(subgroup.ssh_certificates_available?).to eq(false)
+ end
+ end
+ end
+ end
+
describe '#custom_roles_enabled?', feature_category: :system_access do
let_it_be(:namespace) { create(:group) }
diff --git a/ee/spec/requests/api/internal/base_spec.rb b/ee/spec/requests/api/internal/base_spec.rb
index a913d7a0be95c..bd2711aecae19 100644
--- a/ee/spec/requests/api/internal/base_spec.rb
+++ b/ee/spec/requests/api/internal/base_spec.rb
@@ -525,20 +525,26 @@ def check_access_by_alias(alias_name)
end
context 'when authenticated via an SSH certificate' do
- let_it_be(:root_group) { create(:group) }
+ let_it_be_with_refind(:root_group) { create(:group) }
let_it_be(:group) { create(:group, parent: root_group) }
let_it_be(:project) { create(:project, :public, :repository, group: group) }
- def check_allowed(namespace_path)
+ let(:namespace_path) { nil }
+
+ let(:params) do
+ {
+ action: "git-upload-pack",
+ user_id: user.id,
+ project: project.full_path,
+ protocol: 'ssh',
+ namespace_path: namespace_path
+ }
+ end
+
+ def check_allowed
post(
api("/internal/allowed"),
- params: {
- action: "git-upload-pack",
- user_id: user.id,
- project: project.full_path,
- protocol: 'ssh_certificates',
- namespace_path: namespace_path
- },
+ params: params,
headers: gitlab_shell_internal_api_request_header
)
end
@@ -548,32 +554,67 @@ def check_allowed(namespace_path)
end
context 'when group is not specified' do
- it 'is successful' do
- check_allowed(nil)
+ it 'returns success response' do
+ check_allowed
expect(response).to have_gitlab_http_status(:ok)
end
context 'when auth via SSH certificates is enforced' do
- it 'is forbidden' do
- root_group.namespace_settings.enabled_git_access_protocol = 'ssh_certificates'
+ before do
+ root_group.namespace_settings.enforce_ssh_certificates = true
root_group.save!
+ end
- check_allowed(nil)
+ it 'returns an unauthorized error response' do
+ check_allowed
expect(response).to have_gitlab_http_status(:unauthorized)
expect(json_response['status']).to eq(false)
expect(json_response['message']).to eq('You are not allowed to access projects in this namespace.')
end
- context 'when enforce_ssh_certificates feature flag is disabled' do
- it 'is successful' do
- root_group.namespace_settings.enabled_git_access_protocol = 'ssh_certificates'
- root_group.save!
+ context 'when enforce_ssh_certificates_via_settings feature flag is disabled' do
+ before do
+ stub_feature_flags(enforce_ssh_certificates_via_settings: false)
+ end
- stub_feature_flags(enforce_ssh_certificates: false)
+ it 'returns success response' do
+ check_allowed
- check_allowed(nil)
+ expect(response).to have_gitlab_http_status(:ok)
+ end
+ end
+
+ context 'when service account is used' do
+ let(:params) do
+ {
+ action: "git-upload-pack",
+ project: project.full_path,
+ namespace_path: namespace_path,
+ user_id: create(:user, :service_account).id
+ }
+ end
+
+ it 'returns success response' do
+ check_allowed
+
+ expect(response).to have_gitlab_http_status(:ok)
+ end
+ end
+
+ context 'when deploy key is used' do
+ let(:params) do
+ {
+ action: "git-upload-pack",
+ project: project.full_path,
+ namespace_path: namespace_path,
+ key_id: create(:deploy_key).id
+ }
+ end
+
+ it 'returns success response' do
+ check_allowed
expect(response).to have_gitlab_http_status(:ok)
end
@@ -582,8 +623,10 @@ def check_allowed(namespace_path)
end
context 'when non-root group is specified' do
- it 'is forbidden' do
- check_allowed(group.full_path)
+ let(:namespace_path) { group.full_path }
+
+ it 'returns an unauthorized error response' do
+ check_allowed
expect(response).to have_gitlab_http_status(:unauthorized)
expect(json_response['status']).to eq(false)
@@ -592,17 +635,19 @@ def check_allowed(namespace_path)
end
context 'when root group is specified' do
+ let(:namespace_path) { root_group.full_path }
+
it 'is successful' do
- check_allowed(root_group.full_path)
+ check_allowed
expect(response).to have_gitlab_http_status(:ok)
end
context 'when ssh_certificates licensed feature is not available' do
- it 'is forbidden' do
+ it 'returns an unauthorized error response' do
stub_licensed_features(ssh_certificates: false)
- check_allowed(root_group.full_path)
+ check_allowed
expect(response).to have_gitlab_http_status(:unauthorized)
end
@@ -611,8 +656,8 @@ def check_allowed(namespace_path)
context 'when personal project is accessed' do
let_it_be(:project) { create(:project, :public, :repository, namespace: user.namespace) }
- it 'is forbidden' do
- check_allowed(root_group.full_path)
+ it 'returns an unauthorized error response' do
+ check_allowed
expect(response).to have_gitlab_http_status(:unauthorized)
end
diff --git a/ee/spec/requests/groups_controller_spec.rb b/ee/spec/requests/groups_controller_spec.rb
index faadd8fc4a2a9..949eaf546add5 100644
--- a/ee/spec/requests/groups_controller_spec.rb
+++ b/ee/spec/requests/groups_controller_spec.rb
@@ -333,6 +333,35 @@
end
end
end
+
+ context 'setting enforce_ssh_certificates' do
+ let(:params) { { group: { enforce_ssh_certificates: true } } }
+
+ it 'does not change the column' do
+ expect { subject }.not_to change { group.reload.enforce_ssh_certificates? }
+ expect(response).to have_gitlab_http_status(:found)
+ end
+
+ context 'when ssh_certificates licensed feature is available' do
+ before do
+ stub_licensed_features(ssh_certificates: true)
+ end
+
+ it 'successfully changes the column' do
+ expect { subject }.to change { group.reload.enforce_ssh_certificates? }
+ expect(response).to have_gitlab_http_status(:found)
+ end
+
+ context 'when a group is not a top-level group' do
+ let(:group) { create(:group, :nested) }
+
+ it 'does not change the column' do
+ expect { subject }.not_to change { group.reload.enforce_ssh_certificates? }
+ expect(response).to have_gitlab_http_status(:found)
+ end
+ end
+ end
+ end
end
describe 'PUT #transfer', :saas do
diff --git a/ee/spec/views/groups/settings/_enforce_ssh_certificates.html.haml_spec.rb b/ee/spec/views/groups/settings/_enforce_ssh_certificates.html.haml_spec.rb
new file mode 100644
index 0000000000000..51884c94df32a
--- /dev/null
+++ b/ee/spec/views/groups/settings/_enforce_ssh_certificates.html.haml_spec.rb
@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'groups/settings/_enforce_ssh_certificates.html.haml', feature_category: :source_code_management do
+ let_it_be(:group) { build_stubbed(:group, namespace_settings: build_stubbed(:namespace_settings)) }
+
+ before do
+ allow(view).to receive(:group).and_return(group)
+ end
+
+ context 'when ssh certificates feature is unavailable' do
+ it 'does not render enforce SSH certificates settings' do
+ render
+
+ expect(rendered).to be_empty
+ end
+ end
+
+ context 'when ssh certificates feature is available' do
+ it 'renders enforce SSH certificates settings' do
+ form = instance_double('Gitlab::FormBuilders::GitlabUiFormBuilder')
+ allow(view).to receive(:can?).and_return(true)
+ allow(view).to receive(:f).and_return(form)
+ allow(group).to receive(:ssh_certificates_available?).and_return(true)
+
+ expect(form).to receive(:gitlab_ui_checkbox_component).with(
+ :enforce_ssh_certificates, "Enforce SSH Certificates", anything
+ )
+
+ render
+
+ expect(rendered).to render_template('groups/settings/_enforce_ssh_certificates')
+ expect(rendered).to have_content('Enforce SSH Certificates')
+ end
+ end
+end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 5c827b241365d..3fcd83490f7f5 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -23266,6 +23266,9 @@ msgstr ""
msgid "GroupSettings|Enabling these features is your acceptance of the %{link_start}GitLab Testing Agreement%{link_end}."
msgstr ""
+msgid "GroupSettings|Enforce SSH Certificates"
+msgstr ""
+
msgid "GroupSettings|Experiment and Beta features"
msgstr ""
--
GitLab