Merge branch 'if-6990-enforce_smartcard_session_for_git_and_api' into 'master'

Require session with smartcard login for Git access

See merge request gitlab-org/gitlab-ee!14368

config/gitlab.yml.example

@@ -664,6 +664,9 @@ production: &base
     # Port where the client side certificate is requested by the webserver (NGINX/Apache)
     # client_certificate_required_port: 3444
 
+    # Browser session with smartcard sign-in is required for Git access
+    # required_for_git_access: false
+
   ## Kerberos settings
   kerberos:
     # Allow the HTTP Negotiate authentication method for Git clients

config/initializers/1_settings.rb

@@ -76,6 +76,7 @@ Gitlab.ee do
   Settings['smartcard'] ||= Settingslogic.new({})
   Settings.smartcard['enabled'] = false if Settings.smartcard['enabled'].nil?
   Settings.smartcard['client_certificate_required_port'] = 3444 if Settings.smartcard['client_certificate_required_port'].nil?
+  Settings.smartcard['required_for_git_access'] = false if Settings.smartcard['required_for_git_access'].nil?
 end
 
 Settings['omniauth'] ||= Settingslogic.new({})

ee/app/controllers/smartcard_controller.rb

@@ -28,6 +28,7 @@ class SmartcardController < ApplicationController
       return
     end
 
+    store_active_session
     log_audit_event(user, with: certificate.auth_method)
     sign_in_and_redirect(user)
   end
@@ -43,6 +44,10 @@ class SmartcardController < ApplicationController
     end
   end
 
+  def store_active_session
+    Gitlab::Auth::Smartcard::SessionEnforcer.new.update_session
+  end
+
   def log_audit_event(user, options = {})
     AuditEventService.new(user, user, options).for_authentication.security_event
   end

ee/changelogs/unreleased/ee-6990-enforce_smartcard_session_for_git.yml

+---
+title: Require session with smartcard login for Git access
+merge_request: 14368
+author:
+type: added

ee/lib/ee/gitlab/git_access.rb

@@ -11,6 +11,7 @@ module EE
       override :check
       def check(cmd, changes)
         check_geo_license!
+        check_smartcard_access!
 
         super
       end
@@ -64,6 +65,18 @@ module EE
         end
       end
 
+      def check_smartcard_access!
+        unless can_access_without_new_smartcard_login?
+          raise ::Gitlab::GitAccess::UnauthorizedError, 'Project requires smartcard login. Please login to GitLab using a smartcard.'
+        end
+      end
+
+      def can_access_without_new_smartcard_login?
+        return true unless user
+
+        !::Gitlab::Auth::Smartcard::SessionEnforcer.new.access_restricted?(user)
+      end
+
       def geo?
         actor == :geo
       end

ee/lib/gitlab/auth/smartcard.rb

@@ -8,6 +8,10 @@ module Gitlab
       def enabled?
         ::License.feature_available?(:smartcard_auth) && ::Gitlab.config.smartcard.enabled
       end
+
+      def required_for_git_access?
+        self.enabled? && ::Gitlab.config.smartcard.required_for_git_access
+      end
     end
   end
 end

ee/lib/gitlab/auth/smartcard/session.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Auth
+    module Smartcard
+      class Session
+        SESSION_STORE_KEY = :smartcard_signins
+
+        def active?(user)
+          sessions = ActiveSession.list_sessions(user)
+          sessions.any? do |session|
+            Gitlab::NamespacedSessionStore.new(SESSION_STORE_KEY, session.with_indifferent_access )['last_signin_at']
+          end
+        end
+
+        def update_active(value)
+          current_session_data['last_signin_at'] = value
+        end
+
+        private
+
+        def current_session_data
+          Gitlab::NamespacedSessionStore.new(SESSION_STORE_KEY)
+        end
+      end
+    end
+  end
+end

ee/lib/gitlab/auth/smartcard/session_enforcer.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Auth
+    module Smartcard
+      class SessionEnforcer
+        def update_session
+          session.update_active(DateTime.now)
+        end
+
+        def access_restricted?(user)
+          return false unless ::Gitlab::Auth::Smartcard.required_for_git_access?
+
+          !active_session?(user)
+        end
+
+        private
+
+        def session
+          @session ||= Smartcard::Session.new
+        end
+
+        def active_session?(user)
+          session.active?(user)
+        end
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/auth/smartcard/session_enforcer_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Auth::Smartcard::SessionEnforcer do
+  describe '#update_session' do
+    let(:session) { {} }
+
+    around do |example|
+      Gitlab::Session.with_session(session) do
+        example.run
+      end
+    end
+
+    it 'stores the time of last sign-in in session' do
+      expect { subject.update_session }.to change { session[:smartcard_signins] }
+      expect(session[:smartcard_signins]).to have_key('last_signin_at')
+      expect(session[:smartcard_signins]['last_signin_at']).not_to be_nil
+    end
+  end
+
+  describe '#access_restricted?' do
+    let(:user) { create(:user) }
+
+    subject { described_class.new.access_restricted?(user) }
+
+    before do
+      stub_licensed_features(smartcard_auth: true)
+      stub_smartcard_setting(enabled: true, required_for_git_access: true)
+    end
+
+    context 'with a smartcard session', :clean_gitlab_redis_shared_state do
+      let(:session_id) { '42' }
+      let(:stored_session) do
+        { 'smartcard_signins' => { 'last_signin_at' => 5.minutes.ago } }
+      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 { is_expected.to be_falsey }
+    end
+
+    context 'without any session' do
+      it { is_expected.to be_truthy }
+    end
+
+    context 'with the setting off' do
+      before do
+        stub_smartcard_setting(required_for_git_access: false)
+      end
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'with smartcard auth disabled' do
+      before do
+        stub_smartcard_setting(enabled: false)
+      end
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'without a license' do
+      before do
+        stub_licensed_features(smartcard_auth: false)
+      end
+
+      it { is_expected.to be_falsey }
+    end
+  end
+end

ee/spec/lib/gitlab/auth/smartcard/session_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Auth::Smartcard::Session do
+  describe '#active?' do
+    let(:user) { create(:user) }
+
+    subject { described_class.new.active?(user) }
+
+    context 'with a smartcard session', :clean_gitlab_redis_shared_state do
+      let(:session_id) { '42' }
+      let(:stored_session) do
+        { 'smartcard_signins' => { 'last_signin_at' => 5.minutes.ago } }
+      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 { is_expected.to be_truthy }
+    end
+
+    context 'without any session' do
+      it { is_expected.to be_falsey }
+    end
+  end
+
+  describe '#update_active' do
+    let(:now) { Time.now }
+
+    around do |example|
+      Gitlab::Session.with_session({}) do
+        example.run
+      end
+    end
+
+    it 'stores the time of last sign-in' do
+      subject.update_active(now)
+
+      expect(Gitlab::Session.current[:smartcard_signins]).to eq({ 'last_signin_at' => now })
+    end
+  end
+end

ee/spec/lib/gitlab/git_access_spec.rb

@@ -613,6 +613,61 @@ describe Gitlab::GitAccess do
     end
   end
 
+  describe '#check_smartcard_access!' do
+    before do
+      stub_licensed_features(smartcard_auth: true)
+      stub_smartcard_setting(enabled: true, required_for_git_access: true)
+
+      project.add_developer(user)
+    end
+
+    context 'user with a smartcard session', :clean_gitlab_redis_shared_state do
+      let(:session_id) { '42' }
+      let(:stored_session) do
+        { 'smartcard_signins' => { 'last_signin_at' => 5.minutes.ago } }
+      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 'allows pull changes' do
+        expect { pull_changes }.not_to raise_error
+      end
+
+      it 'allows push changes' do
+        expect { push_changes }.not_to raise_error
+      end
+    end
+
+    context 'user without a smartcard session' do
+      it 'does not allow pull changes' do
+        expect { pull_changes }.to raise_error(Gitlab::GitAccess::UnauthorizedError)
+      end
+
+      it 'does not allow push changes' do
+        expect { push_changes }.to raise_error(Gitlab::GitAccess::UnauthorizedError)
+      end
+    end
+
+    context 'with the setting off' do
+      before do
+        stub_smartcard_setting(required_for_git_access: false)
+      end
+
+      it 'allows pull changes' do
+        expect { pull_changes }.not_to raise_error
+      end
+
+      it 'allows push changes' do
+        expect { push_changes }.not_to raise_error
+      end
+    end
+  end
+
   private
 
   def access

ee/spec/requests/api/internal_spec.rb

@@ -80,5 +80,68 @@ describe API::Internal do
         end
       end
     end
+
+    context 'smartcard session required' do
+      set(:project) { create(:project, :repository, :wiki_repo) }
+
+      subject do
+        post(
+          api("/internal/allowed"),
+          params: { key_id: key.id,
+                    project: project.full_path,
+                    gl_repository: "project-#{project.id}",
+                    action: 'git-upload-pack',
+                    secret_token: secret_token,
+                    protocol: 'ssh' })
+      end
+
+      before do
+        stub_licensed_features(smartcard_auth: true)
+        stub_smartcard_setting(enabled: true, required_for_git_access: true)
+
+        project.add_developer(user)
+      end
+
+      context 'user with a smartcard session', :clean_gitlab_redis_shared_state do
+        let(:session_id) { '42' }
+        let(:stored_session) do
+          { 'smartcard_signins' => { 'last_signin_at' => 5.minutes.ago } }
+        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 "allows access" do
+          subject
+
+          expect(response).to have_gitlab_http_status(200)
+        end
+      end
+
+      context 'user without a smartcard session' do
+        it "does not allow access" do
+          subject
+
+          expect(response).to have_gitlab_http_status(401)
+          expect(json_response['message']).to eql('Project requires smartcard login. Please login to GitLab using a smartcard.')
+        end
+      end
+
+      context 'with the setting off' do
+        before do
+          stub_smartcard_setting(required_for_git_access: false)
+        end
+
+        it "allows access" do
+          subject
+
+          expect(response).to have_gitlab_http_status(200)
+        end
+      end
+    end
   end
 end

ee/spec/requests/projects/git_http_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::GitHttpController, type: :request do
+  include GitHttpHelpers
+
+  describe 'GET #info_refs' do
+    set(:user) { create(:user) }
+    set(:project) { create(:project, :repository, :private) }
+    let(:path) { "#{project.full_path}.git" }
+    let(:env) { { user: user.username, password: user.password } }
+
+    context 'smartcard session required' do
+      subject { clone_get(path, env) }
+
+      before do
+        stub_licensed_features(smartcard_auth: true)
+        stub_smartcard_setting(enabled: true, required_for_git_access: true)
+
+        project.add_developer(user)
+      end
+
+      context 'user with a smartcard session', :clean_gitlab_redis_shared_state do
+        let(:session_id) { '42' }
+        let(:stored_session) do
+          { 'smartcard_signins' => { 'last_signin_at' => 5.minutes.ago } }
+        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 "allows access" do
+          subject
+
+          expect(response).to have_gitlab_http_status(200)
+        end
+      end
+
+      context 'user without a smartcard session' do
+        it "does not allow access" do
+          subject
+
+          expect(response).to have_gitlab_http_status(403)
+          expect(response.body).to eq('Project requires smartcard login. Please login to GitLab using a smartcard.')
+        end
+      end
+
+      context 'with the setting off' do
+        before do
+          stub_smartcard_setting(required_for_git_access: false)
+        end
+
+        it "allows access" do
+          subject
+
+          expect(response).to have_gitlab_http_status(200)
+        end
+      end
+    end
+  end
+end

ee/spec/requests/smartcard_controller_spec.rb

@@ -8,6 +8,7 @@ describe SmartcardController, type: :request do
   let(:certificate_headers) { { 'X-SSL-CLIENT-CERTIFICATE': 'certificate' } }
   let(:openssl_certificate_store) { instance_double(OpenSSL::X509::Store) }
   let(:audit_event_service) { instance_double(AuditEventService) }
+  let(:session_enforcer) { instance_double(Gitlab::Auth::Smartcard::SessionEnforcer) }
 
   shared_examples 'a client certificate authentication' do |auth_method|
     context 'with smartcard_auth enabled' do
@@ -33,6 +34,14 @@ describe SmartcardController, type: :request do
         subject
       end
 
+      it 'stores active session' do
+        expect(::Gitlab::Auth::Smartcard::SessionEnforcer).to(
+          receive(:new).and_return(session_enforcer))
+        expect(session_enforcer).to receive(:update_session)
+
+        subject
+      end
+
       context 'user does not exist' do
         context 'signup allowed' do
           it 'creates user' do

ee/spec/support/helpers/ee/stub_configuration.rb

@@ -29,5 +29,9 @@ module EE
     def stub_geo_setting(messages)
       allow(::Gitlab.config.geo).to receive_messages(to_settings(messages))
     end
+
+    def stub_smartcard_setting(messages)
+      allow(::Gitlab.config.smartcard).to receive_messages(to_settings(messages))
+    end
   end
 end

lib/gitlab/namespaced_session_store.rb

@@ -4,19 +4,24 @@ module Gitlab
   class NamespacedSessionStore
     delegate :[], :[]=, to: :store
 
-    def initialize(key)
+    def initialize(key, session = Session.current)
       @key = key
+      @session = session
     end
 
     def initiated?
-      !Session.current.nil?
+      !session.nil?
     end
 
     def store
-      return unless Session.current
+      return unless session
 
-      Session.current[@key] ||= {}
-      Session.current[@key]
+      session[@key] ||= {}
+      session[@key]
     end
+
+    private
+
+    attr_reader :session
   end
 end

spec/lib/gitlab/namespaced_session_store_spec.rb

@@ -4,19 +4,33 @@ require 'spec_helper'
 
 describe Gitlab::NamespacedSessionStore do
   let(:key) { :some_key }
-  subject { described_class.new(key) }
 
-  it 'stores data under the specified key' do
-    Gitlab::Session.with_session({}) do
-      subject[:new_data] = 123
+  context 'current session' do
+    subject { described_class.new(key) }
 
-      expect(Thread.current[:session_storage][key]).to eq(new_data: 123)
+    it 'stores data under the specified key' do
+      Gitlab::Session.with_session({}) do
+        subject[:new_data] = 123
+
+        expect(Thread.current[:session_storage][key]).to eq(new_data: 123)
+      end
+    end
+
+    it 'retrieves data from the given key' do
+      Thread.current[:session_storage] = { key => { existing_data: 123 } }
+
+      expect(subject[:existing_data]).to eq 123
     end
   end
 
-  it 'retrieves data from the given key' do
-    Thread.current[:session_storage] = { key => { existing_data: 123 } }
+  context 'passed in session' do
+    let(:data) { { 'data' => 42 } }
+    let(:session) { { 'some_key' => data } }
+
+    subject { described_class.new(key, session.with_indifferent_access) }
 
-    expect(subject[:existing_data]).to eq 123
+    it 'retrieves data from the given key' do
+      expect(subject['data']).to eq 42
+    end
   end
 end

spec/support/helpers/git_http_helpers.rb

 module GitHttpHelpers
+  include WorkhorseHelpers
+
   def clone_get(project, options = {})
     get "/#{project}/info/refs", params: { service: 'git-upload-pack' }, headers: auth_env(*options.values_at(:user, :password, :spnego_request_token))
   end