Merge branch '294018-dependency-proxy-sso' into 'master'

Dependency Proxy SSO access

See merge request gitlab-org/gitlab!67373

app/policies/group_policy.rb

@@ -52,7 +52,7 @@ class GroupPolicy < BasePolicy
 
   condition(:dependency_proxy_access_allowed) do
     if Feature.enabled?(:dependency_proxy_for_private_groups, default_enabled: true)
-      access_level >= GroupMember::GUEST || valid_dependency_proxy_deploy_token
+      access_level(for_any_session: true) >= GroupMember::GUEST || valid_dependency_proxy_deploy_token
     else
       can?(:read_group)
     end
@@ -240,14 +240,14 @@ class GroupPolicy < BasePolicy
     enable :read_label
   end
 
-  def access_level
+  def access_level(for_any_session: false)
     return GroupMember::NO_ACCESS if @user.nil?
     return GroupMember::NO_ACCESS unless user_is_user?
 
-    @access_level ||= lookup_access_level!
+    @access_level ||= lookup_access_level!(for_any_session: for_any_session)
   end
 
-  def lookup_access_level!
+  def lookup_access_level!(for_any_session: false)
     @subject.max_member_access_for_user(@user)
   end
 

doc/user/group/saml_sso/index.md

@@ -117,6 +117,7 @@ SSO has the following effects when enabled:
   even if the project is forked.
 - For a Git activity, users must be signed-in through SSO before they can push to or
   pull from a GitLab repository.
+- Users must be signed-in through SSO before they can pull images using the [Dependency Proxy](../../packages/dependency_proxy/index.md).
 <!-- Add bullet for API activity when https://gitlab.com/gitlab-org/gitlab/-/issues/9152 is complete -->
 
 ## Providers

doc/user/packages/dependency_proxy/index.md

@@ -68,11 +68,6 @@ The requirement to authenticate is a breaking change added in 13.7. An [administ
 disable it](../../../administration/packages/dependency_proxy.md#disabling-authentication) if it
 has disrupted your existing Dependency Proxy usage.
 
-WARNING:
-If [SSO enforcement](../../group/saml_sso/index.md#sso-enforcement)
-is enabled for your Group, requests to the dependency proxy will fail. This bug is being tracked in
-[this issue](https://gitlab.com/gitlab-org/gitlab/-/issues/294018).
-
 Because the Dependency Proxy is storing Docker images in a space associated with your group,
 you must authenticate against the Dependency Proxy.
 
@@ -91,6 +86,12 @@ You can authenticate using:
 - A [personal access token](../../../user/profile/personal_access_tokens.md) with the scope set to `read_registry` and `write_registry`.
 - A [group deploy token](../../../user/project/deploy_tokens/index.md#group-deploy-token) with the scope set to `read_registry` and `write_registry`.
 
+#### SAML SSO
+
+When [SSO enforcement](../../group/saml_sso/index.md#sso-enforcement)
+is enabled, users must be signed-in through SSO before they can pull images through the Dependency
+Proxy.
+
 #### Authenticate within CI/CD
 
 > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/280582) in GitLab 13.7.

ee/app/policies/ee/group_policy.rb

@@ -77,6 +77,10 @@ module EE
         sso_enforcement_prevents_access?
       end
 
+      condition(:no_active_sso_session) do
+        sso_session_prevents_access?
+      end
+
       condition(:ip_enforcement_prevents_access) do
         !::Gitlab::IpRestriction::Enforcer.new(subject).allows_current_ip?
       end
@@ -376,8 +380,12 @@ module EE
     end
 
     override :lookup_access_level!
-    def lookup_access_level!
-      return ::GroupMember::NO_ACCESS if needs_new_sso_session?
+    def lookup_access_level!(for_any_session: false)
+      if for_any_session
+        return ::GroupMember::NO_ACCESS if no_active_sso_session?
+      else
+        return ::GroupMember::NO_ACCESS if needs_new_sso_session?
+      end
 
       super
     end
@@ -397,6 +405,13 @@ module EE
       ::Gitlab::Auth::GroupSaml::SsoEnforcer.group_access_restricted?(subject, user: user)
     end
 
+    def sso_session_prevents_access?
+      return false unless subject.persisted?
+      return false if user&.auditor? || user&.can_read_all_resources?
+
+      ::Gitlab::Auth::GroupSaml::SessionEnforcer.new(user, subject).access_restricted?
+    end
+
     # Available in Core for self-managed but only paid, non-trial for .com to prevent abuse
     override :resource_access_token_feature_available?
     def resource_access_token_feature_available?

ee/app/views/groups/saml_providers/_form.html.haml

@@ -20,7 +20,7 @@
       .gl-form-checkbox.custom-control.custom-checkbox
         = f.check_box :git_check_enforced, { class: 'custom-control-input js-group-saml-enforced-git-check-input' }
         = f.label :git_check_enforced, { class: 'custom-control-label' } do
-          = s_('GroupSAML|Enforce SSO-only authentication for Git activity for this group')
+          = s_('GroupSAML|Enforce SSO-only authentication for Git and Dependency Proxy activity for this group')
           %p.help-text.js-helper-text{ class: saml_provider.enabled? && 'gl-display-none' }
             = s_('GroupSAML|Before enforcing SSO-only authentication for Git activity, enable SSO-only authentication for web activity.')
     - if Feature.enabled?(:group_managed_accounts, group)

ee/spec/controllers/groups/dependency_proxy_for_containers_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Groups::DependencyProxyForContainersController do
+  include HttpBasicAuthHelpers
+  include DependencyProxyHelpers
+
+  let_it_be(:user) { create(:user) }
+
+  let_it_be_with_reload(:saml_provider) { create(:saml_provider, enforced_sso: true) }
+  let_it_be_with_reload(:group) { saml_provider.group }
+  let_it_be_with_reload(:identity) { create(:group_saml_identity, user: user, saml_provider: saml_provider) }
+
+  let(:token_response) { { status: :success, token: 'abcd1234' } }
+  let(:jwt) { build_jwt(user) }
+  let(:token_header) { "Bearer #{jwt.encoded}" }
+
+  shared_examples 'when sso is enabled for the group' do |successful_example|
+    before do
+      stub_licensed_features(group_saml: true)
+    end
+
+    context 'group owner' do
+      before do
+        group.add_owner(user)
+      end
+
+      it_behaves_like successful_example
+    end
+
+    context 'group reporter' do
+      before do
+        group.add_reporter(user)
+      end
+
+      context 'when git check is enforced' do
+        before do
+          saml_provider.update_column(:git_check_enforced, true)
+        end
+
+        it 'returns not found' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+
+        context 'with an active session', :clean_gitlab_redis_shared_state do
+          let(:session_id) { '42' }
+          let(:session_time) { 5.minutes.ago }
+          let(:stored_session) do
+            { 'active_group_sso_sign_ins' => { saml_provider.id => session_time } }
+          end
+
+          before do
+            Gitlab::Redis::SharedState.with do |redis|
+              redis.set("session:gitlab:#{session_id}", Marshal.dump(stored_session))
+              redis.sadd("session:lookup:user:gitlab:#{user.id}", [session_id])
+            end
+          end
+
+          it_behaves_like successful_example
+        end
+      end
+
+      context 'when git check is not enforced' do
+        it_behaves_like successful_example
+      end
+    end
+  end
+
+  before do
+    allow(Gitlab.config.dependency_proxy)
+      .to receive(:enabled).and_return(true)
+
+    allow_next_instance_of(DependencyProxy::RequestTokenService) do |instance|
+      allow(instance).to receive(:execute).and_return(token_response)
+    end
+
+    request.headers['HTTP_AUTHORIZATION'] = token_header
+  end
+
+  describe 'GET #manifest' do
+    let_it_be(:manifest) { create(:dependency_proxy_manifest) }
+
+    let(:pull_response) { { status: :success, manifest: manifest, from_cache: false } }
+
+    subject(:get_manifest) do
+      get :manifest, params: { group_id: group.to_param, image: 'alpine', tag: '3.9.2' }
+    end
+
+    before do
+      allow_next_instance_of(DependencyProxy::FindOrCreateManifestService) do |instance|
+        allow(instance).to receive(:execute).and_return(pull_response)
+      end
+    end
+
+    it_behaves_like 'when sso is enabled for the group', 'a successful manifest pull'
+  end
+
+  describe 'GET #blob' do
+    let_it_be(:blob) { create(:dependency_proxy_blob) }
+
+    let(:blob_sha) { blob.file_name.sub('.gz', '') }
+    let(:blob_response) { { status: :success, blob: blob, from_cache: false } }
+
+    subject(:get_blob) do
+      get :blob, params: { group_id: group.to_param, image: 'alpine', sha: blob_sha }
+    end
+
+    before do
+      allow_next_instance_of(DependencyProxy::FindOrCreateBlobService) do |instance|
+        allow(instance).to receive(:execute).and_return(blob_response)
+      end
+    end
+
+    it_behaves_like 'when sso is enabled for the group', 'a successful blob pull'
+  end
+end

locale/gitlab.pot

@@ -15892,7 +15892,7 @@ msgstr ""
 msgid "GroupSAML|Enable SAML authentication for this group"
 msgstr ""
 
-msgid "GroupSAML|Enforce SSO-only authentication for Git activity for this group"
+msgid "GroupSAML|Enforce SSO-only authentication for Git and Dependency Proxy activity for this group"
 msgstr ""
 
 msgid "GroupSAML|Enforce SSO-only authentication for web activity for this group"

spec/controllers/groups/dependency_proxy_for_containers_controller_spec.rb

@@ -147,25 +147,6 @@ RSpec.describe Groups::DependencyProxyForContainersController do
 
     subject { get_manifest }
 
-    shared_examples 'a successful manifest pull' do
-      it 'sends a file' do
-        expect(controller).to receive(:send_file).with(manifest.file.path, type: manifest.content_type)
-
-        subject
-      end
-
-      it 'returns Content-Disposition: attachment', :aggregate_failures do
-        subject
-
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(response.headers['Docker-Content-Digest']).to eq(manifest.digest)
-        expect(response.headers['Content-Length']).to eq(manifest.size)
-        expect(response.headers['Docker-Distribution-Api-Version']).to eq(DependencyProxy::DISTRIBUTION_API_VERSION)
-        expect(response.headers['Etag']).to eq("\"#{manifest.digest}\"")
-        expect(response.headers['Content-Disposition']).to match(/^attachment/)
-      end
-    end
-
     context 'feature enabled' do
       before do
         enable_dependency_proxy
@@ -272,21 +253,6 @@ RSpec.describe Groups::DependencyProxyForContainersController do
       end
     end
 
-    shared_examples 'a successful blob pull' do
-      it 'sends a file' do
-        expect(controller).to receive(:send_file).with(blob.file.path, {})
-
-        subject
-      end
-
-      it 'returns Content-Disposition: attachment', :aggregate_failures do
-        subject
-
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(response.headers['Content-Disposition']).to match(/^attachment/)
-      end
-    end
-
     subject { get_blob }
 
     context 'feature enabled' do

spec/support/shared_examples/features/dependency_proxy_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'a successful blob pull' do
+  it 'sends a file' do
+    expect(controller).to receive(:send_file).with(blob.file.path, {})
+
+    subject
+  end
+
+  it 'returns Content-Disposition: attachment', :aggregate_failures do
+    subject
+
+    expect(response).to have_gitlab_http_status(:ok)
+    expect(response.headers['Content-Disposition']).to match(/^attachment/)
+  end
+end
+
+RSpec.shared_examples 'a successful manifest pull' do
+  it 'sends a file' do
+    expect(controller).to receive(:send_file).with(manifest.file.path, type: manifest.content_type)
+
+    subject
+  end
+
+  it 'returns Content-Disposition: attachment', :aggregate_failures do
+    subject
+
+    expect(response).to have_gitlab_http_status(:ok)
+    expect(response.headers['Docker-Content-Digest']).to eq(manifest.digest)
+    expect(response.headers['Content-Length']).to eq(manifest.size)
+    expect(response.headers['Docker-Distribution-Api-Version']).to eq(DependencyProxy::DISTRIBUTION_API_VERSION)
+    expect(response.headers['Etag']).to eq("\"#{manifest.digest}\"")
+    expect(response.headers['Content-Disposition']).to match(/^attachment/)
+  end
+end