diff --git a/app/services/auth/dependency_proxy_authentication_service.rb b/app/services/auth/dependency_proxy_authentication_service.rb
index 1224a86ce6df9b588fc61127d7f4313651ddcd19..d090a62229f2ddaf4a813e47712830ecf0886c8b 100644
--- a/app/services/auth/dependency_proxy_authentication_service.rb
+++ b/app/services/auth/dependency_proxy_authentication_service.rb
@@ -5,17 +5,19 @@ class DependencyProxyAuthenticationService < BaseService
AUDIENCE = 'dependency_proxy'
HMAC_KEY = 'gitlab-dependency-proxy'
DEFAULT_EXPIRE_TIME = 1.minute
- REQUIRED_ABILITIES = %i[read_container_image create_container_image].freeze
+ REQUIRED_CI_ABILITIES = %i[build_read_container_image build_create_container_image].freeze
+ REQUIRED_USER_ABILITIES = %i[read_container_image create_container_image].freeze
MISSING_ABILITIES_MESSAGE = 'Dependency proxy missing authentication abilities'
def execute(authentication_abilities:)
+ @authentication_abilities = authentication_abilities
+
return error('dependency proxy not enabled', 404) unless ::Gitlab.config.dependency_proxy.enabled
- return error('access forbidden', 403) unless valid_user_actor?(authentication_abilities)
+ return error('access forbidden', 403) unless valid_user_actor?
- unless deploy_token || has_required_abilities?(authentication_abilities)
- log_missing_authentication_abilities(authentication_abilities)
- end
+ # TODO: Remove this when enforce_abilities_check_for_dependency_proxy is permanently enabled
+ log_missing_authentication_abilities unless deploy_token || has_required_abilities?
{ token: authorized_token.encoded }
end
@@ -40,23 +42,35 @@ def token_expire_at
private
- def valid_user_actor?(authentication_abilities)
+ attr_reader :authentication_abilities
+
+ def valid_user_actor?
feature_user = deploy_token&.user || current_user
+ # TODO: Cleanup code related to packages_dependency_proxy_containers_scope_check
+ # https://gitlab.com/gitlab-org/gitlab/-/issues/520321
if Feature.enabled?(:packages_dependency_proxy_containers_scope_check, feature_user)
- if deploy_token
- deploy_token.valid_for_dependency_proxy?
- elsif current_user&.project_bot?
- group_access_token&.active? && has_required_abilities?(authentication_abilities)
- else
- current_user
- end
+ dependency_proxy_containers_scope_check
+ elsif Feature.enabled?(:enforce_abilities_check_for_dependency_proxy, feature_user)
+ has_required_abilities?
else
current_user || valid_deploy_token?
end
end
- def has_required_abilities?(authentication_abilities)
- (REQUIRED_ABILITIES & authentication_abilities).size == REQUIRED_ABILITIES.size
+ def dependency_proxy_containers_scope_check
+ if deploy_token
+ deploy_token.valid_for_dependency_proxy?
+ elsif current_user&.project_bot?
+ group_access_token&.active? && has_required_abilities?
+ else
+ current_user
+ end
+ end
+
+ def has_required_abilities?
+ [REQUIRED_CI_ABILITIES, REQUIRED_USER_ABILITIES].any? do |required_abilities|
+ (required_abilities & authentication_abilities).size == required_abilities.size
+ end
end
def group_access_token
@@ -95,7 +109,7 @@ def personal_access_token_user?
raw_token && current_user && (current_user.human? || current_user.service_account?)
end
- def log_missing_authentication_abilities(authentication_abilities)
+ def log_missing_authentication_abilities
log_info = {
message: MISSING_ABILITIES_MESSAGE,
authentication_abilities: authentication_abilities,
diff --git a/config/feature_flags/wip/enforce_abilities_check_for_dependency_proxy.yml b/config/feature_flags/wip/enforce_abilities_check_for_dependency_proxy.yml
new file mode 100644
index 0000000000000000000000000000000000000000..ce3b0254b09bf170bebd5c23fcac08e48b37105c
--- /dev/null
+++ b/config/feature_flags/wip/enforce_abilities_check_for_dependency_proxy.yml
@@ -0,0 +1,9 @@
+---
+name: enforce_abilities_check_for_dependency_proxy
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/520313
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/182559
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/521193
+milestone: '17.10'
+group: group::container registry
+type: wip
+default_enabled: false
diff --git a/spec/controllers/groups/dependency_proxy_for_containers_controller_spec.rb b/spec/controllers/groups/dependency_proxy_for_containers_controller_spec.rb
index 9c81e5cb1b5d1a8891147edb5b463b652478ca1e..7dd970edc91dbadba299a4c0fa848cae1b279d88 100644
--- a/spec/controllers/groups/dependency_proxy_for_containers_controller_spec.rb
+++ b/spec/controllers/groups/dependency_proxy_for_containers_controller_spec.rb
@@ -86,6 +86,8 @@
context 'with insufficient scopes' do
it { is_expected.to have_gitlab_http_status(:not_found) }
+ # TODO: Cleanup code related to packages_dependency_proxy_containers_scope_check
+ # https://gitlab.com/gitlab-org/gitlab/-/issues/520321
context 'packages_dependency_proxy_containers_scope_check disabled' do
before do
stub_feature_flags(packages_dependency_proxy_containers_scope_check: false)
diff --git a/spec/requests/jwt_controller_spec.rb b/spec/requests/jwt_controller_spec.rb
index bd4d3d8055becb08eda8f450e13f3ca4b8204b42..20a218cd1f9f52a4ae36db56677bbdba67fe59b7 100644
--- a/spec/requests/jwt_controller_spec.rb
+++ b/spec/requests/jwt_controller_spec.rb
@@ -304,6 +304,10 @@ def credentials(login, password)
context 'authenticating against dependency proxy' do
let_it_be(:user) { create(:user) }
let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
+ let_it_be(:personal_access_token_without_required_scopes) do
+ create(:personal_access_token, user: user, scopes: ['read_user'])
+ end
+
let_it_be(:group) { create(:group) }
let_it_be(:project) { create(:project, :private, group: group) }
let_it_be(:bot_user) { create(:user, :project_bot) }
@@ -332,12 +336,49 @@ def credentials(login, password)
end
end
+ shared_examples 'enforce_abilities_check_for_dependency_proxy disabled' do
+ before do
+ stub_feature_flags(enforce_abilities_check_for_dependency_proxy: false)
+ end
+
+ it_behaves_like 'with valid credentials'
+ end
+
context 'with personal access token' do
let(:credential_user) { nil }
let(:credential_password) { personal_access_token.token }
it_behaves_like 'with valid credentials'
it_behaves_like 'a token that expires today'
+ it_behaves_like 'enforce_abilities_check_for_dependency_proxy disabled'
+
+ # TODO: Cleanup code related to packages_dependency_proxy_containers_scope_check
+ # https://gitlab.com/gitlab-org/gitlab/-/issues/520321
+ context 'packages_dependency_proxy_containers_scope_check disabled' do
+ before do
+ stub_feature_flags(packages_dependency_proxy_containers_scope_check: false)
+ end
+
+ it_behaves_like 'with valid credentials'
+ it_behaves_like 'enforce_abilities_check_for_dependency_proxy disabled'
+ end
+
+ context 'without the required scopes' do
+ let(:credential_password) { personal_access_token_without_required_scopes.token }
+
+ # TODO: Cleanup code related to packages_dependency_proxy_containers_scope_check
+ # https://gitlab.com/gitlab-org/gitlab/-/issues/520321
+ context 'packages_dependency_proxy_containers_scope_check disabled' do
+ before do
+ stub_feature_flags(packages_dependency_proxy_containers_scope_check: false)
+ end
+
+ it_behaves_like 'returning response status', :forbidden
+ it_behaves_like 'enforce_abilities_check_for_dependency_proxy disabled'
+ end
+
+ it_behaves_like 'enforce_abilities_check_for_dependency_proxy disabled'
+ end
end
context 'with user credentials token' do
@@ -355,6 +396,17 @@ def credentials(login, password)
it_behaves_like 'with valid credentials'
it_behaves_like 'a token that expires today'
+ # TODO: Cleanup code related to packages_dependency_proxy_containers_scope_check
+ # https://gitlab.com/gitlab-org/gitlab/-/issues/520321
+ context 'packages_dependency_proxy_containers_scope_check disabled' do
+ before do
+ stub_feature_flags(packages_dependency_proxy_containers_scope_check: false)
+ end
+
+ it_behaves_like 'with valid credentials'
+ it_behaves_like 'enforce_abilities_check_for_dependency_proxy disabled'
+ end
+
context 'revoked' do
before do
group_access_token.update!(revoked: true)
@@ -370,6 +422,8 @@ def credentials(login, password)
it_behaves_like 'returning response status', :unauthorized
end
+
+ it_behaves_like 'enforce_abilities_check_for_dependency_proxy disabled'
end
context 'without the required scopes' do
@@ -379,6 +433,8 @@ def credentials(login, password)
it_behaves_like 'returning response status', :forbidden
+ # TODO: Cleanup code related to packages_dependency_proxy_containers_scope_check
+ # https://gitlab.com/gitlab-org/gitlab/-/issues/520321
context 'packages_dependency_proxy_containers_scope_check disabled' do
before do
stub_feature_flags(packages_dependency_proxy_containers_scope_check: false)
@@ -386,6 +442,8 @@ def credentials(login, password)
it_behaves_like 'with valid credentials'
end
+
+ it_behaves_like 'enforce_abilities_check_for_dependency_proxy disabled'
end
end
@@ -403,6 +461,17 @@ def credentials(login, password)
let(:credential_password) { job.token }
it_behaves_like 'with valid credentials'
+
+ # TODO: Cleanup code related to packages_dependency_proxy_containers_scope_check
+ # https://gitlab.com/gitlab-org/gitlab/-/issues/520321
+ context 'packages_dependency_proxy_containers_scope_check disabled' do
+ before do
+ stub_feature_flags(packages_dependency_proxy_containers_scope_check: false)
+ end
+
+ it_behaves_like 'with valid credentials'
+ it_behaves_like 'enforce_abilities_check_for_dependency_proxy disabled'
+ end
end
context 'with project deploy token' do
diff --git a/spec/services/auth/dependency_proxy_authentication_service_spec.rb b/spec/services/auth/dependency_proxy_authentication_service_spec.rb
index c2d37b537fb12ea7f4f0d4d8d3466ba6df6b064c..7084da48c4e75324c30ba69e09cf2b7dfa52e9c2 100644
--- a/spec/services/auth/dependency_proxy_authentication_service_spec.rb
+++ b/spec/services/auth/dependency_proxy_authentication_service_spec.rb
@@ -4,8 +4,8 @@
RSpec.describe Auth::DependencyProxyAuthenticationService, feature_category: :virtual_registry do
let_it_be(:user) { create(:user) }
- let_it_be(:params) { {} }
+ let(:params) { {} }
let(:authentication_abilities) { [] }
let(:service) { described_class.new(nil, user, params) }
@@ -73,20 +73,23 @@
end
context 'with a deploy token' do
- let(:user) { nil }
- let_it_be(:deploy_token) { create(:deploy_token, :group, :dependency_proxy_scopes) }
- let_it_be(:params) { { deploy_token: deploy_token } }
+ let_it_be(:deploy_token_with_dependency_proxy_scopes) { create(:deploy_token, :group, :dependency_proxy_scopes) }
+ let_it_be(:deploy_token_with_missing_scopes) { create(:deploy_token, :group, read_registry: false) }
- it_behaves_like 'returning a token with an encoded field', 'deploy_token'
+ let(:user) { nil }
- context 'with packages_dependency_proxy_containers_scope_check disabled' do
- before do
- stub_feature_flags(packages_dependency_proxy_containers_scope_check: false)
- end
+ context 'with sufficient scopes' do
+ let(:params) { { deploy_token: deploy_token_with_dependency_proxy_scopes } }
it_behaves_like 'returning a token with an encoded field', 'deploy_token'
end
+ context 'with insufficient scopes' do
+ let(:params) { { deploy_token: deploy_token_with_missing_scopes } }
+
+ it_behaves_like 'returning', status: 403, message: 'access forbidden'
+ end
+
context 'when the the deploy token is restricted with external_authorization' do
before do
allow(Gitlab::ExternalAuthorization).to receive(:allow_deploy_tokens_and_deploy_keys?).and_return(false)
@@ -110,18 +113,49 @@
context 'with a personal access token user' do
let_it_be_with_reload(:token) { create(:personal_access_token, user: user) }
- let_it_be(:params) { { raw_token: token.token } }
+ let(:params) { { raw_token: token.token } }
it_behaves_like 'returning a token with an encoded field', 'personal_access_token'
context 'with insufficient authentication abilities' do
it_behaves_like 'logs missing authentication abilities'
+
+ # TODO: Cleanup code related to packages_dependency_proxy_containers_scope_check
+ # https://gitlab.com/gitlab-org/gitlab/-/issues/520321
+ context 'with packages_dependency_proxy_containers_scope_check disabled' do
+ before do
+ stub_feature_flags(packages_dependency_proxy_containers_scope_check: false)
+ end
+
+ it_behaves_like 'returning', status: 403, message: 'access forbidden'
+
+ context 'with enforce_abilities_check_for_dependency_proxy disabled' do
+ before do
+ stub_feature_flags(enforce_abilities_check_for_dependency_proxy: false)
+ end
+
+ it_behaves_like 'returning a token with an encoded field', 'personal_access_token'
+ end
+ end
end
context 'with sufficient authentication abilities' do
- let_it_be(:authentication_abilities) { Auth::DependencyProxyAuthenticationService::REQUIRED_ABILITIES }
+ [described_class::REQUIRED_USER_ABILITIES,
+ described_class::REQUIRED_CI_ABILITIES].each do |abilities|
+ context "with #{abilities}" do
+ let(:authentication_abilities) { abilities }
+
+ it_behaves_like 'does not log missing authentication abilities'
- it_behaves_like 'does not log missing authentication abilities'
+ context 'with enforce_abilities_check_for_dependency_proxy disabled' do
+ before do
+ stub_feature_flags(enforce_abilities_check_for_dependency_proxy: false)
+ end
+
+ it_behaves_like 'returning a token with an encoded field', 'personal_access_token'
+ end
+ end
+ end
end
end
@@ -129,7 +163,8 @@
let_it_be(:user) { create(:user, :project_bot) }
let_it_be(:group) { create(:group) }
let_it_be_with_reload(:token) { create(:personal_access_token, user: user) }
- let_it_be(:params) { { raw_token: token.token } }
+
+ let(:params) { { raw_token: token.token } }
before_all do
group.add_guest(user)
@@ -138,39 +173,61 @@
context 'with insufficient authentication abilities' do
it_behaves_like 'returning', status: 403, message: 'access forbidden'
+ # TODO: Cleanup code related to packages_dependency_proxy_containers_scope_check
+ # https://gitlab.com/gitlab-org/gitlab/-/issues/520321
context 'packages_dependency_proxy_containers_scope_check disabled' do
before do
stub_feature_flags(packages_dependency_proxy_containers_scope_check: false)
end
- it_behaves_like 'returning a token with an encoded field', 'group_access_token'
- it_behaves_like 'logs missing authentication abilities'
+ it_behaves_like 'returning', status: 403, message: 'access forbidden'
+
+ context 'with enforce_abilities_check_for_dependency_proxy disabled' do
+ before do
+ stub_feature_flags(enforce_abilities_check_for_dependency_proxy: false)
+ end
+
+ it_behaves_like 'returning a token with an encoded field', 'group_access_token'
+ end
end
end
context 'with sufficient authentication abilities' do
- let_it_be(:authentication_abilities) { Auth::DependencyProxyAuthenticationService::REQUIRED_ABILITIES }
- let_it_be(:params) { { raw_token: token.token } }
+ [described_class::REQUIRED_USER_ABILITIES,
+ described_class::REQUIRED_CI_ABILITIES].each do |abilities|
+ context "with #{abilities}" do
+ let(:authentication_abilities) { abilities }
+ let(:params) { { raw_token: token.token } }
- subject { service.execute(authentication_abilities: authentication_abilities) }
+ subject { service.execute(authentication_abilities: authentication_abilities) }
- it_behaves_like 'returning a token with an encoded field', 'group_access_token'
- it_behaves_like 'does not log missing authentication abilities'
+ it_behaves_like 'returning a token with an encoded field', 'group_access_token'
+ it_behaves_like 'does not log missing authentication abilities'
- context 'revoked' do
- before do
- token.revoke!
- end
+ context 'with enforce_abilities_check_for_dependency_proxy disabled' do
+ before do
+ stub_feature_flags(enforce_abilities_check_for_dependency_proxy: false)
+ end
- it_behaves_like 'returning', status: 403, message: 'access forbidden'
- end
+ it_behaves_like 'returning a token with an encoded field', 'group_access_token'
+ end
- context 'expired' do
- before do
- token.update_column(:expires_at, 1.day.ago)
- end
+ context 'revoked' do
+ before do
+ token.revoke!
+ end
- it_behaves_like 'returning', status: 403, message: 'access forbidden'
+ it_behaves_like 'returning', status: 403, message: 'access forbidden'
+ end
+
+ context 'expired' do
+ before do
+ token.update_column(:expires_at, 1.day.ago)
+ end
+
+ it_behaves_like 'returning', status: 403, message: 'access forbidden'
+ end
+ end
end
end
end
@@ -183,6 +240,20 @@
end
it_behaves_like 'returning a token with an encoded field', 'user_id'
+
+ context 'packages_dependency_proxy_containers_scope_check disabled' do
+ before do
+ stub_feature_flags(packages_dependency_proxy_containers_scope_check: false)
+ end
+
+ context 'with enforce_abilities_check_for_dependency_proxy disabled' do
+ before do
+ stub_feature_flags(enforce_abilities_check_for_dependency_proxy: false)
+ end
+
+ it_behaves_like 'returning a token with an encoded field', 'user_id'
+ end
+ end
end
end
end