Commit b146ad7b authored by Steve Abrams's avatar Steve Abrams Committed by Mayra Cabrera

Fix group IP restrictions not enforced for container registry requests

Merge branch 'security-sh-fix-container-registry-ip-restriction-14-10' into '14-10-stable-ee'

See merge request gitlab-org/security/gitlab!2552

Changelog: security
parent cf7ef970
...@@ -54,8 +54,14 @@ class ProjectPolicy < BasePolicy ...@@ -54,8 +54,14 @@ class ProjectPolicy < BasePolicy
desc "Container registry is disabled" desc "Container registry is disabled"
condition(:container_registry_disabled, scope: :subject) do condition(:container_registry_disabled, scope: :subject) do
if user.is_a?(DeployToken)
(!user.read_registry? && !user.write_registry?) ||
user.revoked? ||
!project.container_registry_enabled?
else
!access_allowed_to?(:container_registry) !access_allowed_to?(:container_registry)
end end
end
desc "Container registry is enabled for everyone with access to the project" desc "Container registry is enabled for everyone with access to the project"
condition(:container_registry_enabled_for_everyone_with_access, scope: :subject) do condition(:container_registry_enabled_for_everyone_with_access, scope: :subject) do
...@@ -83,6 +89,16 @@ class ProjectPolicy < BasePolicy ...@@ -83,6 +89,16 @@ class ProjectPolicy < BasePolicy
user.is_a?(DeployKey) && user.can_push_to?(project) user.is_a?(DeployKey) && user.can_push_to?(project)
end end
desc "Deploy token with read_container_image scope"
condition(:read_container_image_deploy_token) do
user.is_a?(DeployToken) && user.has_access_to?(project) && user.read_registry?
end
desc "Deploy token with create_container_image scope"
condition(:create_container_image_deploy_token) do
user.is_a?(DeployToken) && user.has_access_to?(project) && user.write_registry?
end
desc "Deploy token with read_package_registry scope" desc "Deploy token with read_package_registry scope"
condition(:read_package_registry_deploy_token) do condition(:read_package_registry_deploy_token) do
user.is_a?(DeployToken) && user.has_access_to?(project) && user.read_package_registry user.is_a?(DeployToken) && user.has_access_to?(project) && user.read_package_registry
...@@ -685,6 +701,14 @@ class ProjectPolicy < BasePolicy ...@@ -685,6 +701,14 @@ class ProjectPolicy < BasePolicy
enable :push_code enable :push_code
end end
rule { read_container_image_deploy_token }.policy do
enable :read_container_image
end
rule { create_container_image_deploy_token }.policy do
enable :create_container_image
end
rule { read_package_registry_deploy_token }.policy do rule { read_package_registry_deploy_token }.policy do
enable :read_package enable :read_package
enable :read_project enable :read_project
......
...@@ -215,15 +215,13 @@ module Auth ...@@ -215,15 +215,13 @@ module Auth
def deploy_token_can_pull?(requested_project) def deploy_token_can_pull?(requested_project)
has_authentication_ability?(:read_container_image) && has_authentication_ability?(:read_container_image) &&
deploy_token.present? && deploy_token.present? &&
deploy_token.has_access_to?(requested_project) && can?(deploy_token, :read_container_image, requested_project)
deploy_token.read_registry?
end end
def deploy_token_can_push?(requested_project) def deploy_token_can_push?(requested_project)
has_authentication_ability?(:create_container_image) && has_authentication_ability?(:create_container_image) &&
deploy_token.present? && deploy_token.present? &&
deploy_token.has_access_to?(requested_project) && can?(deploy_token, :create_container_image, requested_project)
deploy_token.write_registry?
end end
## ##
......
...@@ -356,6 +356,7 @@ module EE ...@@ -356,6 +356,7 @@ module EE
prevent :read_merge_request prevent :read_merge_request
prevent :read_milestone prevent :read_milestone
prevent :read_container_image prevent :read_container_image
prevent :create_container_image
end end
rule { locked_approvers_rules }.policy do rule { locked_approvers_rules }.policy do
......
...@@ -475,6 +475,7 @@ RSpec.describe ProjectPolicy do ...@@ -475,6 +475,7 @@ RSpec.describe ProjectPolicy do
it { is_expected.to be_allowed(:read_merge_request) } it { is_expected.to be_allowed(:read_merge_request) }
it { is_expected.to be_allowed(:read_milestone) } it { is_expected.to be_allowed(:read_milestone) }
it { is_expected.to be_allowed(:read_container_image) } it { is_expected.to be_allowed(:read_container_image) }
it { is_expected.to be_allowed(:create_container_image) }
end end
context 'address is outside the range' do context 'address is outside the range' do
...@@ -485,6 +486,7 @@ RSpec.describe ProjectPolicy do ...@@ -485,6 +486,7 @@ RSpec.describe ProjectPolicy do
it { is_expected.to be_disallowed(:read_merge_request) } it { is_expected.to be_disallowed(:read_merge_request) }
it { is_expected.to be_disallowed(:read_milestone) } it { is_expected.to be_disallowed(:read_milestone) }
it { is_expected.to be_disallowed(:read_container_image) } it { is_expected.to be_disallowed(:read_container_image) }
it { is_expected.to be_disallowed(:create_container_image) }
context 'with admin enabled', :enable_admin_mode do context 'with admin enabled', :enable_admin_mode do
it { is_expected.to be_allowed(:read_project) } it { is_expected.to be_allowed(:read_project) }
...@@ -492,6 +494,7 @@ RSpec.describe ProjectPolicy do ...@@ -492,6 +494,7 @@ RSpec.describe ProjectPolicy do
it { is_expected.to be_allowed(:read_merge_request) } it { is_expected.to be_allowed(:read_merge_request) }
it { is_expected.to be_allowed(:read_milestone) } it { is_expected.to be_allowed(:read_milestone) }
it { is_expected.to be_allowed(:read_container_image) } it { is_expected.to be_allowed(:read_container_image) }
it { is_expected.to be_allowed(:create_container_image) }
end end
context 'with admin disabled' do context 'with admin disabled' do
...@@ -500,6 +503,7 @@ RSpec.describe ProjectPolicy do ...@@ -500,6 +503,7 @@ RSpec.describe ProjectPolicy do
it { is_expected.to be_disallowed(:read_merge_request) } it { is_expected.to be_disallowed(:read_merge_request) }
it { is_expected.to be_disallowed(:read_milestone) } it { is_expected.to be_disallowed(:read_milestone) }
it { is_expected.to be_disallowed(:read_container_image) } it { is_expected.to be_disallowed(:read_container_image) }
it { is_expected.to be_disallowed(:create_container_image) }
end end
context 'with auditor' do context 'with auditor' do
...@@ -510,6 +514,7 @@ RSpec.describe ProjectPolicy do ...@@ -510,6 +514,7 @@ RSpec.describe ProjectPolicy do
it { is_expected.to be_allowed(:read_merge_request) } it { is_expected.to be_allowed(:read_merge_request) }
it { is_expected.to be_allowed(:read_milestone) } it { is_expected.to be_allowed(:read_milestone) }
it { is_expected.to be_allowed(:read_container_image) } it { is_expected.to be_allowed(:read_container_image) }
it { is_expected.to be_allowed(:create_container_image) }
end end
end end
end end
......
...@@ -3,34 +3,132 @@ ...@@ -3,34 +3,132 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe JwtController do RSpec.describe JwtController do
context 'authenticating against container registry' do let_it_be(:user) { create(:user) }
let(:user) { create(:user) } let_it_be(:group) { create(:group) }
let(:group) { create(:group) }
let(:project) { create(:project, :private, group: group) } let(:actions) { ['pull'] }
let(:scope) { "repository:#{project.full_path}:pull" } let(:expected_actions) { actions }
let(:scope) { "repository:#{project.full_path}:#{expected_actions.join(',')}" }
let(:service_name) { 'container_registry' } let(:service_name) { 'container_registry' }
let(:headers) { { authorization: credentials(user.username, user.password) } } let(:headers) { { authorization: credentials(user.username, user.password) } }
let(:parameters) { { account: user.username, client_id: 'docker', offline_token: true, service: service_name, scope: scope } } let(:parameters) { { account: user.username, client_id: 'docker', offline_token: true, service: service_name, scope: scope } }
before do shared_examples 'successful JWT auth' do
stub_container_registry_config(enabled: true, issuer: 'gitlab-issuer', key: 'spec/fixtures/x509_certificate_pk.key')
project.add_reporter(user)
end
context 'when Group SSO is enforced' do
let!(:saml_provider) { create(:saml_provider, enforced_sso: true, group: group) }
let!(:identity) { create(:group_saml_identity, saml_provider: saml_provider, user: user) }
it 'allows access' do it 'allows access' do
get '/jwt/auth', params: parameters, headers: headers get '/jwt/auth', params: parameters, headers: headers
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
expect(token_response['access']).to be_present expect(token_response['access']).to be_present
expect(token_access['actions']).to eq ['pull'] expect(token_access['actions']).to eq expected_actions
expect(token_access['type']).to eq 'repository' expect(token_access['type']).to eq 'repository'
expect(token_access['name']).to eq project.full_path expect(token_access['name']).to eq project.full_path
end end
end end
shared_examples 'unsuccessful JWT auth' do
it 'denies access' do
get '/jwt/auth', params: parameters, headers: headers
expect(response).to have_gitlab_http_status(:ok)
expect(token_response['access']).to eq []
end
end
context 'with IP restriction' do
let_it_be(:project) { create(:project, :private, group: group) }
let_it_be(:group_deploy_token) { create(:deploy_token, :group, groups: [group], read_registry: true, write_registry: true) }
let_it_be(:project_deploy_token) { create(:deploy_token, projects: [project], read_registry: true, write_registry: true) }
let(:actions) { %w[push pull] }
before do
project.add_developer(user)
stub_container_registry_config(enabled: true, key: 'spec/fixtures/x509_certificate_pk.key')
allow(Gitlab::IpAddressState).to receive(:current).and_return('192.168.0.2')
stub_licensed_features(group_ip_restriction: true)
end
context 'group with restriction' do
using RSpec::Parameterized::TableSyntax
before do
create(:ip_restriction, group: group, range: range)
end
shared_examples 'successful JWT auth with token' do
let(:headers) { { authorization: credentials(token.username, token.token) } }
where(:read, :write, :expected_actions) do
true | false | %w[pull]
false | true | %w[push]
true | true | %w[push pull]
end
with_them do
before do
token.update!(read_registry: read, write_registry: write)
end
it_behaves_like 'successful JWT auth'
end
end
context 'address is within the range' do
let(:range) { '192.168.0.0/24' }
it_behaves_like 'successful JWT auth'
context 'with project deploy token' do
let(:token) { project_deploy_token }
it_behaves_like 'successful JWT auth with token'
end
context 'with group deploy token' do
let(:token) { group_deploy_token }
it_behaves_like 'successful JWT auth with token'
end
end
context 'address is outside the range' do
let(:range) { '10.0.0.0/8' }
it_behaves_like 'unsuccessful JWT auth'
context 'with deploy token credentials' do
let(:headers) { { authorization: credentials(token.username, token.token) } }
context 'with project deploy token' do
let(:token) { project_deploy_token }
it_behaves_like 'unsuccessful JWT auth'
end
context 'with group deploy token' do
let(:token) { group_deploy_token }
it_behaves_like 'unsuccessful JWT auth'
end
end
end
end
end
context 'authenticating against container registry' do
let_it_be(:project) { create(:project, :private, group: group) }
before do
project.add_reporter(user)
stub_container_registry_config(enabled: true, issuer: 'gitlab-issuer', key: 'spec/fixtures/x509_certificate_pk.key')
end
context 'when Group SSO is enforced' do
let!(:saml_provider) { create(:saml_provider, enforced_sso: true, group: group) }
let!(:identity) { create(:group_saml_identity, saml_provider: saml_provider, user: user) }
it_behaves_like 'successful JWT auth'
end
end end
def credentials(login, password) def credentials(login, password)
......
...@@ -5,6 +5,48 @@ require 'spec_helper' ...@@ -5,6 +5,48 @@ require 'spec_helper'
RSpec.describe Auth::ContainerRegistryAuthenticationService do RSpec.describe Auth::ContainerRegistryAuthenticationService do
include AdminModeHelper include AdminModeHelper
describe 'with deploy keys' do
include_context 'container registry auth service context'
let_it_be_with_reload(:group) { create(:group, :public) }
let_it_be(:current_user) { nil }
let_it_be(:project) { create(:project, group: group) }
let(:deploy_token) { create(:deploy_token, projects: [project], read_registry: true, write_registry: true) }
context 'with IP restriction' do
before do
allow(Gitlab::IpAddressState).to receive(:current).and_return('192.168.0.2')
stub_licensed_features(group_ip_restriction: true)
end
context 'group with restriction' do
before do
create(:ip_restriction, group: group, range: range)
end
context 'address is within the range' do
let(:range) { '192.168.0.0/24' }
it_behaves_like 'a container registry auth service'
end
context 'address is outside the range' do
let(:range) { '10.0.0.0/8' }
let(:current_params) do
{ scopes: ["repository:#{project.full_path}:push,pull"], deploy_token: deploy_token }
end
context 'when actor is a deploy token with read access' do
it_behaves_like 'an inaccessible'
it_behaves_like 'not a container repository factory'
it_behaves_like 'logs an auth warning', %w(push pull)
end
end
end
end
end
context 'in maintenance mode' do context 'in maintenance mode' do
include_context 'container registry auth service context' include_context 'container registry auth service context'
......
...@@ -1023,18 +1023,56 @@ RSpec.describe ProjectPolicy do ...@@ -1023,18 +1023,56 @@ RSpec.describe ProjectPolicy do
subject { described_class.new(deploy_token, project) } subject { described_class.new(deploy_token, project) }
context 'private project' do
let(:project) { private_project }
context 'a deploy token with read_registry scope' do
let(:deploy_token) { create(:deploy_token, read_registry: true, write_registry: false) }
it { is_expected.to be_allowed(:read_container_image) }
it { is_expected.to be_disallowed(:create_container_image) }
context 'with registry disabled' do
include_context 'registry disabled via project features'
it { is_expected.to be_disallowed(:read_container_image) }
it { is_expected.to be_disallowed(:create_container_image) }
end
end
context 'a deploy token with write_registry scope' do
let(:deploy_token) { create(:deploy_token, read_registry: false, write_registry: true) }
it { is_expected.to be_disallowed(:read_container_image) }
it { is_expected.to be_allowed(:create_container_image) }
context 'with registry disabled' do
include_context 'registry disabled via project features'
it { is_expected.to be_disallowed(:read_container_image) }
it { is_expected.to be_disallowed(:create_container_image) }
end
end
context 'a deploy token with no registry scope' do
let(:deploy_token) { create(:deploy_token, read_registry: false, write_registry: false) }
it { is_expected.to be_disallowed(:read_container_image) }
it { is_expected.to be_disallowed(:create_container_image) }
end
context 'a deploy token with read_package_registry scope' do context 'a deploy token with read_package_registry scope' do
let(:deploy_token) { create(:deploy_token, read_package_registry: true) } let(:deploy_token) { create(:deploy_token, read_repository: false, read_registry: false, read_package_registry: true) }
it { is_expected.to be_allowed(:read_package) }
it { is_expected.to be_allowed(:read_project) } it { is_expected.to be_allowed(:read_project) }
it { is_expected.to be_allowed(:read_package) }
it { is_expected.to be_disallowed(:create_package) } it { is_expected.to be_disallowed(:create_package) }
it_behaves_like 'package access with repository disabled' it_behaves_like 'package access with repository disabled'
end end
context 'a deploy token with write_package_registry scope' do context 'a deploy token with write_package_registry scope' do
let(:deploy_token) { create(:deploy_token, write_package_registry: true) } let(:deploy_token) { create(:deploy_token, read_repository: false, read_registry: false, write_package_registry: true) }
it { is_expected.to be_allowed(:create_package) } it { is_expected.to be_allowed(:create_package) }
it { is_expected.to be_allowed(:read_package) } it { is_expected.to be_allowed(:read_package) }
...@@ -1045,6 +1083,60 @@ RSpec.describe ProjectPolicy do ...@@ -1045,6 +1083,60 @@ RSpec.describe ProjectPolicy do
end end
end end
context 'public project' do
let(:project) { public_project }
context 'a deploy token with read_registry scope' do
let(:deploy_token) { create(:deploy_token, read_registry: true, write_registry: false) }
it { is_expected.to be_allowed(:read_container_image) }
it { is_expected.to be_disallowed(:create_container_image) }
context 'with registry disabled' do
include_context 'registry disabled via project features'
it { is_expected.to be_disallowed(:read_container_image) }
it { is_expected.to be_disallowed(:create_container_image) }
end
context 'with registry private' do
include_context 'registry set to private via project features'
it { is_expected.to be_allowed(:read_container_image) }
it { is_expected.to be_disallowed(:create_container_image) }
end
end
context 'a deploy token with write_registry scope' do
let(:deploy_token) { create(:deploy_token, read_registry: false, write_registry: true) }
it { is_expected.to be_allowed(:read_container_image) }
it { is_expected.to be_allowed(:create_container_image) }
context 'with registry disabled' do
include_context 'registry disabled via project features'
it { is_expected.to be_disallowed(:read_container_image) }
it { is_expected.to be_disallowed(:create_container_image) }
end
context 'with registry private' do
include_context 'registry set to private via project features'
it { is_expected.to be_allowed(:read_container_image) }
it { is_expected.to be_allowed(:create_container_image) }
end
end
context 'a deploy token with no registry scope' do
let(:deploy_token) { create(:deploy_token, read_registry: false, write_registry: false) }
it { is_expected.to be_disallowed(:read_container_image) }
it { is_expected.to be_disallowed(:create_container_image) }
end
end
end
describe 'create_web_ide_terminal' do describe 'create_web_ide_terminal' do
context 'with admin' do context 'with admin' do
let(:current_user) { admin } let(:current_user) { admin }
......
# frozen_string_literal: true
RSpec.shared_context 'repository disabled via project features' do
before do
project.project_feature.update_columns(
# Disable merge_requests and builds as well, since merge_requests and
# builds cannot have higher visibility than repository.
merge_requests_access_level: ProjectFeature::DISABLED,
builds_access_level: ProjectFeature::DISABLED,
repository_access_level: ProjectFeature::DISABLED)
end
end
RSpec.shared_context 'registry disabled via project features' do
before do
project.project_feature.update_columns(
container_registry_access_level: ProjectFeature::DISABLED
)
end
end
RSpec.shared_context 'registry set to private via project features' do
before do
project.project_feature.update_columns(
container_registry_access_level: ProjectFeature::PRIVATE
)
end
end
...@@ -345,16 +345,7 @@ RSpec.shared_examples 'project policies as admin without admin mode' do ...@@ -345,16 +345,7 @@ RSpec.shared_examples 'project policies as admin without admin mode' do
end end
RSpec.shared_examples 'package access with repository disabled' do RSpec.shared_examples 'package access with repository disabled' do
context 'when repository is disabled' do include_context 'repository disabled via project features'
before do
project.project_feature.update!(
# Disable merge_requests and builds as well, since merge_requests and
# builds cannot have higher visibility than repository.
merge_requests_access_level: ProjectFeature::DISABLED,
builds_access_level: ProjectFeature::DISABLED,
repository_access_level: ProjectFeature::DISABLED)
end
it { is_expected.to be_allowed(:read_package) } it { is_expected.to be_allowed(:read_package) }
end
end end
...@@ -142,9 +142,9 @@ RSpec.shared_examples 'logs an auth warning' do |requested_actions| ...@@ -142,9 +142,9 @@ RSpec.shared_examples 'logs an auth warning' do |requested_actions|
requested_project_path: project.full_path, requested_project_path: project.full_path,
requested_actions: requested_actions, requested_actions: requested_actions,
authorized_actions: [], authorized_actions: [],
user_id: current_user.id, user_id: current_user&.id,
username: current_user.username username: current_user&.username
} }.compact
end end
it do it do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment