Merge branch 'if-10526-smartcard_support_different_hostname' into 'master'

Make hostname configurable for smartcard authentication

See merge request gitlab-org/gitlab!26411

changelogs/unreleased/10526-smartcard_support_different_hostname.yml

+---
+title: Make hostname configurable for smartcard authentication
+merge_request: 26411
+author:
+type: added

config/gitlab.yml.example

@@ -752,7 +752,9 @@ production: &base
     # Path to a file containing a CA certificate
     ca_file: '/etc/ssl/certs/CA.pem'
 
-    # Port where the client side certificate is requested by the webserver (NGINX/Apache)
+    # Host and port where the client side certificate is requested by the
+    # webserver (NGINX/Apache)
+    # client_certificate_required_host: smartcard.gitlab.example.com
     # client_certificate_required_port: 3444
 
     # Browser session with smartcard sign-in is required for Git access

config/initializers/1_settings.rb

@@ -77,6 +77,7 @@ end
 Gitlab.ee do
   Settings['smartcard'] ||= Settingslogic.new({})
   Settings.smartcard['enabled'] = false if Settings.smartcard['enabled'].nil?
+  Settings.smartcard['client_certificate_required_host'] = Settings.gitlab['host'] if Settings.smartcard['client_certificate_required_host'].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?
   Settings.smartcard['san_extensions'] = false if Settings.smartcard['san_extensions'].nil?

ee/app/controllers/smartcard_controller.rb

@@ -5,25 +5,58 @@ class SmartcardController < ApplicationController
   skip_before_action :verify_authenticity_token
 
   before_action :check_feature_availability
-  before_action :check_certificate_headers
+  before_action :check_certificate_required_host_and_port, only: :extract_certificate
+  before_action :check_ngingx_certificate_header, only: :extract_certificate
+  before_action :check_certificate_param, only: :verify_certificate
 
   def auth
-    certificate = Gitlab::Auth::Smartcard::Certificate.new(certificate_header)
-    sign_in_with(certificate)
+    redirect_to extract_certificate_smartcard_url(extract_certificate_url_options)
   end
 
-  def ldap_auth
-    certificate = Gitlab::Auth::Smartcard::LdapCertificate.new(params[:provider], certificate_header)
-    sign_in_with(certificate)
+  def extract_certificate
+    redirect_to verify_certificate_smartcard_url(verify_certificate_url_options)
+  end
+
+  def verify_certificate
+    sign_in_with(client_certificate)
   end
 
   private
 
+  def extract_certificate_url_options
+    {
+      host: ::Gitlab.config.smartcard.client_certificate_required_host,
+      port: ::Gitlab.config.smartcard.client_certificate_required_port,
+      provider: params[:provider]
+    }.compact
+  end
+
+  def verify_certificate_url_options
+    {
+      host: ::Gitlab.config.gitlab.host,
+      port: ::Gitlab.config.gitlab.port,
+      provider: params[:provider],
+      client_certificate: request.headers['HTTP_X_SSL_CLIENT_CERTIFICATE']
+    }.compact
+  end
+
+  def client_certificate
+    if ldap_provider?
+      Gitlab::Auth::Smartcard::LdapCertificate.new(params[:provider], certificate_param)
+    else
+      Gitlab::Auth::Smartcard::Certificate.new(certificate_param)
+    end
+  end
+
+  def ldap_provider?
+    params[:provider].present?
+  end
+
   def sign_in_with(certificate)
     user = certificate.find_or_create_user
     unless user&.persisted?
       flash[:alert] = _('Failed to signing using smartcard authentication')
-      redirect_to new_user_session_path(port: Gitlab.config.gitlab.port)
+      redirect_to new_user_session_path
 
       return
     end
@@ -33,13 +66,43 @@ class SmartcardController < ApplicationController
     sign_in_and_redirect(user)
   end
 
+  def nginx_certificate_header
+    request.headers['HTTP_X_SSL_CLIENT_CERTIFICATE']
+  end
+
+  def certificate_param
+    param = params[:client_certificate]
+    return unless param
+
+    unescaped_param = CGI.unescape(param)
+    if unescaped_param.include?("\n")
+      # NGINX forwarding the $ssl_client_escaped_cert variable
+      unescaped_param
+    else
+      # older version of NGINX forwarding the now deprecated $ssl_client_cert variable
+      param.gsub(/ (?!CERTIFICATE)/, "\n")
+    end
+  end
+
   def check_feature_availability
     render_404 unless ::Gitlab::Auth::Smartcard.enabled?
   end
 
-  def check_certificate_headers
-    # Failing on requests coming from the port not requiring client side certificate
-    unless certificate_header.present?
+  def check_certificate_required_host_and_port
+    unless request.host == ::Gitlab.config.smartcard.client_certificate_required_host &&
+      request.port == ::Gitlab.config.smartcard.client_certificate_required_port
+      render_404
+    end
+  end
+
+  def check_ngingx_certificate_header
+    unless nginx_certificate_header.present?
+      access_denied!(_('Smartcard authentication failed: client certificate header is missing.'), 401)
+    end
+  end
+
+  def check_certificate_param
+    unless certificate_param.present?
       access_denied!(_('Smartcard authentication failed: client certificate header is missing.'), 401)
     end
   end
@@ -52,20 +115,6 @@ class SmartcardController < ApplicationController
     AuditEventService.new(user, user, options).for_authentication.security_event
   end
 
-  def certificate_header
-    header = request.headers['HTTP_X_SSL_CLIENT_CERTIFICATE']
-    return unless header
-
-    unescaped_header = CGI.unescape(header)
-    if unescaped_header.include?("\n")
-      # NGINX forwarding the $ssl_client_escaped_cert variable
-      unescaped_header
-    else
-      # older version of NGINX forwarding the now deprecated $ssl_client_cert variable
-      header.gsub(/ (?!CERTIFICATE)/, "\n")
-    end
-  end
-
   def after_sign_in_path_for(resource)
     stored_location_for(:redirect) || stored_location_for(resource) || root_url(port: Gitlab.config.gitlab.port)
   end

ee/app/views/devise/sessions/_new_smartcard.html.haml

 - if smartcard_enabled?
   .login-box.tab-pane{ id: 'smartcard', role: 'tabpanel', class: active_when(form_based_auth_provider_has_active_class?(:smartcard)) }
     .login-body
-      = form_tag(smartcard_auth_url(port: smartcard_config_port), html: { 'aria-live' => 'assertive'}) do
+      = form_tag(auth_smartcard_url, html: { 'aria-live' => 'assertive'}) do
         .submit-container
           = submit_tag _('Login with smartcard'), class: 'btn btn-success'

ee/app/views/devise/sessions/_new_smartcard_ldap.html.haml

@@ -9,8 +9,7 @@
       = _('Use your smart card to authenticate with the LDAP server.')
 
     .login-body
-      = form_tag(smartcard_ldap_auth_url(provider: server['provider_name'],
-                                         port: smartcard_config_port),
+      = form_tag(auth_smartcard_url(provider: server['provider_name']),
                  html: { 'aria-live' => 'assertive'}) do
         .submit-container
           = submit_tag(_('Sign in with smart card'),

ee/config/routes/smartcard.rb

 # frozen_string_literal: true
 
-post 'smartcard/auth' => 'smartcard#auth'
-post 'smartcard/ldap_auth' => 'smartcard#ldap_auth'
+resource :smartcard, only: [], controller: :smartcard do
+  collection do
+    post :auth
+    get :extract_certificate
+    get :verify_certificate
+  end
+end

ee/spec/requests/smartcard_controller_spec.rb

@@ -5,11 +5,150 @@ require 'spec_helper'
 describe SmartcardController, type: :request do
   include LdapHelpers
 
-  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) }
+  let(:smartcard_host) { 'smartcard.example.com' }
+  let(:smartcard_port) { 3443 }
 
+  describe '#auth' do
+    let(:params) { {} }
+
+    subject { post auth_smartcard_path, params: params }
+
+    before do
+      stub_smartcard_config(
+        client_certificate_required_host: smartcard_host,
+        client_certificate_required_port: smartcard_port
+      )
+    end
+
+    context 'with smartcard_auth enabled' do
+      before do
+        enable_smartcard_authentication
+      end
+
+      it 'redirects to extract certificate' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:redirect)
+        expect(response.location).to(
+          eq(extract_certificate_smartcard_url(host: smartcard_host,
+                                               port: smartcard_port)))
+      end
+
+      context 'with provider param' do
+        let(:provider) { 'ldap-provider' }
+        let(:params) { { provider: provider } }
+
+        it 'forwards the provider param' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:redirect)
+          expect(response.location).to(
+            eq(extract_certificate_smartcard_url(host: smartcard_host,
+                                                 port: smartcard_port,
+                                                 provider: provider)))
+        end
+      end
+    end
+
+    context 'with smartcard_auth disabled' do
+      before do
+        allow(Gitlab::Auth::Smartcard).to receive(:enabled?).and_return(false)
+      end
+
+      it 'renders 404' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+  end
+
+  describe '#extract_certificate' do
+    let(:certificate) { 'certificate' }
+    let(:certificate_headers) { { 'X-SSL-CLIENT-CERTIFICATE': certificate } }
+    let(:params) { {} }
+
+    subject do
+      get(extract_certificate_smartcard_path, headers: certificate_headers,
+                                              params: params)
+    end
+
+    before do
+      stub_config_setting(host: 'example.com',
+                          port: 443)
+      stub_smartcard_config(
+        client_certificate_required_host: smartcard_host,
+        client_certificate_required_port: smartcard_port
+      )
+      host! "#{smartcard_host}:#{smartcard_port}"
+    end
+
+    context 'with smartcard_auth enabled' do
+      before do
+        enable_smartcard_authentication
+      end
+
+      it 'redirects to verify certificate' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:redirect)
+        expect(response.location).to(
+          eq(verify_certificate_smartcard_url(host: ::Gitlab.config.gitlab.host,
+                                              port: ::Gitlab.config.gitlab.port,
+                                              client_certificate: certificate)))
+      end
+
+      context 'with provider param' do
+        let(:provider) { 'ldap-provider' }
+        let(:params) { { provider: provider } }
+
+        it 'forwards the provider param' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:redirect)
+          expect(response.location).to(
+            eq(verify_certificate_smartcard_url(host: ::Gitlab.config.gitlab.host,
+                                                port: ::Gitlab.config.gitlab.port,
+                                                client_certificate: certificate,
+                                                provider: provider)))
+        end
+      end
+
+      context 'missing NGINX client certificate header' do
+        let(:certificate_headers) { {} }
+
+        it 'renders unauthorized' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:unauthorized)
+        end
+      end
+
+      context 'request from different host / port' do
+        it 'renders 404' do
+          host! 'another.host:42'
+
+          subject
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+    end
+
+    context 'with smartcard_auth disabled' do
+      before do
+        allow(Gitlab::Auth::Smartcard).to receive(:enabled?).and_return(false)
+      end
+
+      it 'renders 404' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+  end
+
+  describe '#verify_certificate' do
     shared_examples 'a client certificate authentication' do |auth_method|
       context 'with smartcard_auth enabled' do
         it 'allows sign in' do
@@ -25,6 +164,8 @@ describe SmartcardController, type: :request do
         end
 
         it 'logs audit event' do
+          audit_event_service = instance_double(AuditEventService)
+
           expect(AuditEventService).to(
             receive(:new)
               .with(instance_of(User), instance_of(User), with: auth_method)
@@ -35,6 +176,8 @@ describe SmartcardController, type: :request do
         end
 
         it 'stores active session' do
+          session_enforcer = instance_double(Gitlab::Auth::Smartcard::SessionEnforcer)
+
           expect(::Gitlab::Auth::Smartcard::SessionEnforcer).to(
             receive(:new).and_return(session_enforcer))
           expect(session_enforcer).to receive(:update_session)
@@ -62,6 +205,17 @@ describe SmartcardController, type: :request do
             end
           end
         end
+
+        context 'missing client certificate param' do
+          let(:params) { {} }
+
+          it 'renders unauthorized' do
+            subject
+
+            expect(response).to have_gitlab_http_status(:unauthorized)
+            expect(request.env['warden']).not_to be_authenticated
+          end
+        end
       end
 
       context 'with smartcard_auth disabled' do
@@ -77,22 +231,20 @@ describe SmartcardController, type: :request do
       end
     end
 
-  describe '#auth' do
+    let(:client_certificate) { 'certificate' }
+    let(:params) { { client_certificate: client_certificate } }
+    let(:serial) { '42' }
     let(:subject_dn) { '/O=Random Corp Ltd/CN=gitlab-user/emailAddress=gitlab-user@random-corp.org' }
-    let(:issuer_dn) { '/O=Random Corp Ltd/CN=Random Corp' }
-    let(:certificate_headers) { { 'X-SSL-CLIENT-CERTIFICATE': 'certificate' } }
-    let(:openssl_certificate_store) { instance_double(OpenSSL::X509::Store) }
-    let(:openssl_certificate) { instance_double(OpenSSL::X509::Certificate, subject: subject_dn, issuer: issuer_dn) }
-    let(:audit_event_service) { instance_double(AuditEventService) }
+    let(:issuer_dn) { 'CN=Random Corp,O=Random Corp Ltd,C=US' }
 
     before do
-      allow(Gitlab::Auth::Smartcard).to receive(:enabled?).and_return(true)
-      allow(Gitlab::Auth::Smartcard::Certificate).to receive(:store).and_return(openssl_certificate_store)
-      allow(openssl_certificate_store).to receive(:verify).and_return(true)
-      allow(OpenSSL::X509::Certificate).to receive(:new).and_return(openssl_certificate)
+      enable_smartcard_authentication
+      stub_certificate_store
+      stub_certificate
     end
 
-    subject { post '/-/smartcard/auth', params: {}, headers: certificate_headers }
+    context 'Smartcard::Certificate' do
+      subject { get verify_certificate_smartcard_path, params: params }
 
       it_behaves_like 'a client certificate authentication', 'smartcard'
 
@@ -122,41 +274,22 @@ describe SmartcardController, type: :request do
         let(:expected_certificate) { "-----BEGIN CERTIFICATE-----\nrow\nrow\n-----END CERTIFICATE-----" }
 
         context 'escaped format' do
-        let(:certificate_headers) { { 'X-SSL-CLIENT-CERTIFICATE': '-----BEGIN%20CERTIFICATE-----%0Arow%0Arow%0A-----END%20CERTIFICATE-----' } }
+          let(:client_certificate) { '-----BEGIN%20CERTIFICATE-----%0Arow%0Arow%0A-----END%20CERTIFICATE-----' }
 
           it_behaves_like 'valid certificate header'
         end
 
         context 'deprecated format' do
-        let(:certificate_headers) { { 'X-SSL-CLIENT-CERTIFICATE': '-----BEGIN CERTIFICATE----- row row -----END CERTIFICATE-----' } }
+          let(:client_certificate) { '-----BEGIN CERTIFICATE----- row row -----END CERTIFICATE-----' }
 
           it_behaves_like 'valid certificate header'
         end
       end
-
-    context 'missing certificate headers' do
-      let(:certificate_headers) { nil }
-
-      it 'renders 401' do
-        subject
-
-        expect(response).to have_gitlab_http_status(:unauthorized)
-        expect(request.env['warden']).not_to be_authenticated
-      end
-    end
-  end
-
-  describe '#ldap_auth ' do
-    let(:subject_ldap_dn) { 'uid=john doe,ou=people,dc=example,dc=com' }
-    let(:issuer_dn) { 'CN=Random Corp,O=Random Corp Ltd,C=US' }
-    let(:issuer) { instance_double(OpenSSL::X509::Name, to_s: issuer_dn) }
-    let(:serial) { '42' }
-    let(:openssl_certificate) do
-      instance_double(OpenSSL::X509::Certificate,
-                      issuer: issuer, serial: serial)
     end
 
+    context 'Smartcard::LdapCertificate' do
       let(:ldap_connection) { instance_double(::Net::LDAP) }
+      let(:subject_ldap_dn) { 'uid=john doe,ou=people,dc=example,dc=com' }
       let(:ldap_email) { 'john.doe@example.com' }
       let(:ldap_entry) do
         Net::LDAP::Entry.new.tap do |entry|
@@ -176,21 +309,11 @@ describe SmartcardController, type: :request do
       end
 
       subject do
-      post('/-/smartcard/ldap_auth',
-           { params: { provider: 'ldapmain' },
-             headers: certificate_headers } )
+        get(verify_certificate_smartcard_path,
+            { params: params.merge({ provider: 'ldapmain' }) })
       end
 
       before do
-      allow(Gitlab::Auth::Smartcard).to receive(:enabled?).and_return(true)
-
-      allow(Gitlab::Auth::Smartcard::LdapCertificate).to(
-        receive(:store).and_return(openssl_certificate_store))
-      allow(openssl_certificate_store).to receive(:verify).and_return(true)
-
-      allow(OpenSSL::X509::Certificate).to(
-        receive(:new).and_return(openssl_certificate))
-
         allow(Net::LDAP).to receive(:new).and_return(ldap_connection)
         allow(ldap_connection).to(
           receive(:search).with(ldap_search_params).and_return([ldap_entry]))
@@ -218,7 +341,7 @@ describe SmartcardController, type: :request do
           expect(request.env['warden']).to be_authenticated
         end
 
-      context "user has a different identity" do
+        context 'user has a different identity' do
           let(:ldap_email) { user.email }
 
           before do
@@ -239,4 +362,25 @@ describe SmartcardController, type: :request do
         end
       end
     end
+  end
+
+  def enable_smartcard_authentication
+    allow(Gitlab::Auth::Smartcard).to receive(:enabled?).and_return(true)
+  end
+
+  def stub_smartcard_config(smartcard_settings)
+    allow(::Gitlab.config.smartcard).to(receive_messages(smartcard_settings))
+  end
+
+  def stub_certificate_store
+    openssl_certificate_store = instance_double(OpenSSL::X509::Store)
+    allow(Gitlab::Auth::Smartcard::Base).to receive(:store).and_return(openssl_certificate_store)
+    allow(openssl_certificate_store).to receive(:verify).and_return(true)
+  end
+
+  def stub_certificate
+    issuer = instance_double(OpenSSL::X509::Name, to_s: issuer_dn)
+    openssl_certificate = instance_double(OpenSSL::X509::Certificate, subject: subject_dn, issuer: issuer, serial: serial)
+    allow(OpenSSL::X509::Certificate).to receive(:new).and_return(openssl_certificate)
+  end
 end