Merge remote-tracking branch 'dev/master'

# Conflicts:
#	Gemfile
#	Gemfile.lock

Gemfile

@@ -62,6 +62,7 @@ gem 'browser', '~> 2.2'
 # GitLab fork with several improvements to original library. For full list of changes
 # see https://github.com/intridea/omniauth-ldap/compare/master...gitlabhq:master
 gem 'gitlab_omniauth-ldap', '~> 2.0.3', require: 'omniauth-ldap'
+gem 'net-ldap'
 
 # Git Wiki
 # Required manually in config/initializers/gollum.rb to control load order

Gemfile.lock

@@ -1013,6 +1013,7 @@ DEPENDENCIES
   minitest (~> 5.7.0)
   mousetrap-rails (~> 1.4.6)
   mysql2 (~> 0.4.5)
+  net-ldap
   nokogiri (~> 1.6.7, >= 1.6.7.2)
   oauth2 (~> 1.4)
   octokit (~> 4.6.2)

changelogs/unreleased/mk-add-ldap-ssl-certificate-verification.yml

+---
+title: Add LDAP SSL certificate verification option
+merge_request:
+author:

config/gitlab.yml.example

@@ -228,7 +228,8 @@ production: &base
   # ==========================
 
   ## LDAP settings
-  # You can inspect a sample of the LDAP users with login access by running:
+  # You can test connections and inspect a sample of the LDAP users with login
+  # access by running:
   #   bundle exec rake gitlab:ldap:check RAILS_ENV=production
   ldap:
     enabled: false
@@ -251,13 +252,45 @@ production: &base
         # Example: 'Paris' or 'Acme, Ltd.'
         label: 'LDAP'
 
+        # Example: 'ldap.mydomain.com'
         host: '_your_ldap_server'
-        port: 389
-        uid: 'sAMAccountName'
-        method: 'plain' # "tls" or "ssl" or "plain"
+        # This port is an example, it is sometimes different but it is always an integer and not a string
+        port: 389 # usually 636 for SSL
+        uid: 'sAMAccountName' # This should be the attribute, not the value that maps to uid.
+
+        # Examples: 'america\\momo' or 'CN=Gitlab Git,CN=Users,DC=mydomain,DC=com'
         bind_dn: '_the_full_dn_of_the_user_you_will_bind_with'
         password: '_the_password_of_the_bind_user'
 
+        # Encryption method. The "method" key is deprecated in favor of
+        # "encryption".
+        #
+        #   Examples: "start_tls" or "simple_tls" or "plain"
+        #
+        #   Deprecated values: "tls" was replaced with "start_tls" and "ssl" was
+        #   replaced with "simple_tls".
+        #
+        encryption: 'plain'
+
+        # Enables SSL certificate verification if encryption method is
+        # "start_tls" or "simple_tls". (Defaults to false for backward-
+        # compatibility)
+        verify_certificates: false
+
+        # Specifies the path to a file containing a PEM-format CA certificate,
+        # e.g. if you need to use an internal CA.
+        #
+        #   Example: '/etc/ca.pem'
+        #
+        ca_cert: ''
+
+        # Specifies the SSL version for OpenSSL to use, if the OpenSSL default
+        # is not appropriate.
+        #
+        #   Example: 'TLSv1_1'
+        #
+        ssl_version: ''
+
         # Set a timeout, in seconds, for LDAP queries. This helps avoid blocking
         # a request if the LDAP server becomes unresponsive.
         # A value of 0 means there is no timeout.
@@ -286,17 +319,20 @@ production: &base
 
         # Base where we can search for users
         #
-        #   Ex. ou=People,dc=gitlab,dc=example
+        #   Ex. 'ou=People,dc=gitlab,dc=example' or 'DC=mydomain,DC=com'
         #
         base: ''
 
         # Filter LDAP users
         #
-        #   Format: RFC 4515 http://tools.ietf.org/search/rfc4515
+        #   Format: RFC 4515 https://tools.ietf.org/search/rfc4515
         #   Ex. (employeeType=developer)
         #
         #   Note: GitLab does not support omniauth-ldap's custom filter syntax.
         #
+        #   Example for getting only specific users:
+        #   '(&(objectclass=user)(|(samaccountname=momo)(samaccountname=toto)))'
+        #
         user_filter: ''
 
         # LDAP attributes that GitLab will use to create an account for the LDAP user.
@@ -674,7 +710,7 @@ test:
         host: 127.0.0.1
         port: 3890
         uid: 'uid'
-        method: 'plain' # "tls" or "ssl" or "plain"
+        encryption: 'plain' # "start_tls" or "simple_tls" or "plain"
         base: 'dc=example,dc=com'
         user_filter: ''
         group_base: 'ou=groups,dc=example,dc=com'

config/initializers/1_settings.rb

@@ -145,6 +145,24 @@ if Settings.ldap['enabled'] || Rails.env.test?
     server['attributes'] = {} if server['attributes'].nil?
     server['provider_name'] ||= "ldap#{key}".downcase
     server['provider_class'] = OmniAuth::Utils.camelize(server['provider_name'])
+
+    # For backwards compatibility
+    server['encryption'] ||= server['method']
+    server['encryption'] = 'simple_tls' if server['encryption'] == 'ssl'
+    server['encryption'] = 'start_tls' if server['encryption'] == 'tls'
+
+    # Certificates are not verified for backwards compatibility.
+    # This default should be flipped to true in 9.5.
+    if server['verify_certificates'].nil?
+      server['verify_certificates'] = false
+
+      message = <<-MSG.strip_heredoc
+        LDAP SSL certificate verification is disabled for backwards-compatibility.
+        Please add the "verify_certificates" option to gitlab.yml for each LDAP
+        server. Certificate verification will be enabled by default in GitLab 9.5.
+      MSG
+      Rails.logger.warn(message)
+    end
   end
 end
 

doc/administration/auth/ldap.md

@@ -69,14 +69,42 @@ main: # 'main' is the GitLab 'provider ID' of this LDAP server
   # Example: 'ldap.mydomain.com'
   host: '_your_ldap_server'
   # This port is an example, it is sometimes different but it is always an integer and not a string
-  port: 389
+  port: 389 # usually 636 for SSL
   uid: 'sAMAccountName' # This should be the attribute, not the value that maps to uid.
-  method: 'plain' # "tls" or "ssl" or "plain"
 
   # Examples: 'america\\momo' or 'CN=Gitlab Git,CN=Users,DC=mydomain,DC=com'
   bind_dn: '_the_full_dn_of_the_user_you_will_bind_with'
   password: '_the_password_of_the_bind_user'
 
+  # Encryption method. The "method" key is deprecated in favor of
+  # "encryption".
+  #
+  #   Examples: "start_tls" or "simple_tls" or "plain"
+  #
+  #   Deprecated values: "tls" was replaced with "start_tls" and "ssl" was
+  #   replaced with "simple_tls".
+  #
+  encryption: 'plain'
+
+  # Enables SSL certificate verification if encryption method is
+  # "start_tls" or "simple_tls". (Defaults to false for backward-
+  # compatibility)
+  verify_certificates: false
+
+  # Specifies the path to a file containing a PEM-format CA certificate,
+  # e.g. if you need to use an internal CA.
+  #
+  #   Example: '/etc/ca.pem'
+  #
+  ca_cert: ''
+
+  # Specifies the SSL version for OpenSSL to use, if the OpenSSL default
+  # is not appropriate.
+  #
+  #   Example: 'TLSv1_1'
+  #
+  ssl_version: ''
+
   # Set a timeout, in seconds, for LDAP queries. This helps avoid blocking
   # a request if the LDAP server becomes unresponsive.
   # A value of 0 means there is no timeout.
@@ -116,8 +144,8 @@ main: # 'main' is the GitLab 'provider ID' of this LDAP server
   #
   #   Note: GitLab does not support omniauth-ldap's custom filter syntax.
   #
-  #   Below an example for get only specific users
-  #   Example: '(&(objectclass=user)(|(samaccountname=momo)(samaccountname=toto)))'
+  #   Example for getting only specific users:
+  #   '(&(objectclass=user)(|(samaccountname=momo)(samaccountname=toto)))'
   #
   user_filter: ''
 
@@ -250,6 +278,19 @@ In other words, if an existing GitLab user wants to enable LDAP sign-in for
 themselves, they should check that their GitLab email address matches their
 LDAP email address, and then sign into GitLab via their LDAP credentials.
 
+## Encryption
+
+### TLS Server Authentication
+
+There are two encryption methods, `simple_tls` and `start_tls`.
+
+For either encryption method, if setting `validate_certificates: false`, TLS
+encryption is established with the LDAP server before any LDAP-protocol data is
+exchanged but no validation of the LDAP server's SSL certificate is performed.
+
+>**Note**: Before GitLab 9.5, `validate_certificates: false` is the default if
+unspecified.
+
 ## Limitations
 
 ### TLS Client Authentication
@@ -259,14 +300,6 @@ You should disable anonymous LDAP authentication and enable simple or SASL
 authentication. The TLS client authentication setting in your LDAP server cannot
 be mandatory and clients cannot be authenticated with the TLS protocol.
 
-### TLS Server Authentication
-
-Not supported by GitLab's configuration options.
-When setting `method: ssl`, the underlying authentication method used by
-`omniauth-ldap` is `simple_tls`.  This method establishes TLS encryption with
-the LDAP server before any LDAP-protocol data is exchanged but no validation of
-the LDAP server's SSL certificate is performed.
-
 ## Troubleshooting
 
 ### Debug LDAP user filter with ldapsearch
@@ -306,9 +339,9 @@ tree and traverse it.
 ### Connection Refused
 
 If you are getting 'Connection Refused' errors when trying to connect to the
-LDAP server please double-check the LDAP `port` and `method` settings used by
-GitLab. Common combinations are `method: 'plain'` and `port: 389`, OR
-`method: 'ssl'` and `port: 636`.
+LDAP server please double-check the LDAP `port` and `encryption` settings used by
+GitLab. Common combinations are `encryption: 'plain'` and `port: 389`, OR
+`encryption: 'simple_tls'` and `port: 636`.
 
 ### Troubleshooting
 

doc/articles/how_to_configure_ldap_gitlab_ce/index.md

@@ -120,7 +120,8 @@ gitlab_rails['ldap_servers'] = {
   'host' =>  'ad.example.org',
   'port' => 636,
   'uid' => 'sAMAccountName',
-  'method' => 'ssl',
+  'encryption' => 'simple_tls',
+  'verify_certificates' => true,
   'bind_dn' => 'CN=GitLabSRV,CN=Users,DC=GitLab,DC=org',
   'password' => 'Password1',
   'active_directory' => true,
@@ -255,7 +256,7 @@ If `allow_username_or_email_login` is enabled in the LDAP configuration, GitLab
 
 ## LDAP extended features on GitLab EE
 
-With [GitLab Enterprise Edition (EE)](https://about.gitlab.com/giltab-ee/), besides everything we just described, you'll
+With [GitLab Enterprise Edition (EE)](https://about.gitlab.com/gitlab-ee/), besides everything we just described, you'll
 have extended functionalities with LDAP, such as:
 
 - Group sync

lib/gitlab/ldap/config.rb

@@ -2,6 +2,12 @@
 module Gitlab
   module LDAP
     class Config
+      NET_LDAP_ENCRYPTION_METHOD = {
+        simple_tls: :simple_tls,
+        start_tls:  :start_tls,
+        plain:      nil
+      }.freeze
+
       attr_accessor :provider, :options
 
       def self.enabled?
@@ -39,7 +45,7 @@ module Gitlab
 
       def adapter_options
         opts = base_options.merge(
-          encryption: encryption
+          encryption: encryption_options
         )
 
         opts.merge!(auth_options) if has_auth?
@@ -50,9 +56,10 @@ module Gitlab
       def omniauth_options
         opts = base_options.merge(
           base: base,
-          method: options['method'],
+          encryption: options['encryption'],
           filter: omniauth_user_filter,
-          name_proc: name_proc
+          name_proc: name_proc,
+          disable_verify_certificates: !options['verify_certificates']
         )
 
         if has_auth?
@@ -62,6 +69,9 @@ module Gitlab
           )
         end
 
+        opts[:ca_file] = options['ca_file'] if options['ca_file'].present?
+        opts[:ssl_version] = options['ssl_version'] if options['ssl_version'].present?
+
         opts
       end
 
@@ -157,15 +167,37 @@ module Gitlab
         base_config.servers.values.find { |server| server['provider_name'] == provider }
       end
 
-      def encryption
-        case options['method'].to_s
-        when 'ssl'
-          :simple_tls
-        when 'tls'
-          :start_tls
+      def encryption_options
+        method = translate_method(options['encryption'])
+        return nil unless method
+
+        {
+          method: method,
+          tls_options: tls_options(method)
+        }
+      end
+
+      def translate_method(method_from_config)
+        NET_LDAP_ENCRYPTION_METHOD[method_from_config.to_sym]
+      end
+
+      def tls_options(method)
+        return { verify_mode: OpenSSL::SSL::VERIFY_NONE } unless method
+
+        opts = if options['verify_certificates']
+                 OpenSSL::SSL::SSLContext::DEFAULT_PARAMS
                else
-          nil
+                 # It is important to explicitly set verify_mode for two reasons:
+                 # 1. The behavior of OpenSSL is undefined when verify_mode is not set.
+                 # 2. The net-ldap gem implementation verifies the certificate hostname
+                 #    unless verify_mode is set to VERIFY_NONE.
+                 { verify_mode: OpenSSL::SSL::VERIFY_NONE }
                end
+
+        opts[:ca_file] = options['ca_file'] if options['ca_file'].present?
+        opts[:ssl_version] = options['ssl_version'] if options['ssl_version'].present?
+
+        opts
       end
 
       def auth_options

spec/lib/gitlab/ldap/config_spec.rb

@@ -5,7 +5,7 @@ describe Gitlab::LDAP::Config, lib: true do
 
   let(:config) { Gitlab::LDAP::Config.new('ldapmain') }
 
-  describe '#initalize' do
+  describe '#initialize' do
     it 'requires a provider' do
       expect{ Gitlab::LDAP::Config.new }.to raise_error ArgumentError
     end
@@ -25,7 +25,7 @@ describe Gitlab::LDAP::Config, lib: true do
         options: {
           'host'       => 'ldap.example.com',
           'port'       => 386,
-          'method'  => 'plain'
+          'encryption' => 'plain'
         }
       )
 
@@ -41,22 +41,138 @@ describe Gitlab::LDAP::Config, lib: true do
         options: {
           'host'                => 'ldap.example.com',
           'port'                => 686,
-          'method'    => 'ssl',
+          'encryption'          => 'simple_tls',
+          'verify_certificates' => true,
           'bind_dn'             => 'uid=admin,dc=example,dc=com',
           'password'            => 'super_secret'
         }
       )
 
-      expect(config.adapter_options).to eq(
-        host: 'ldap.example.com',
-        port: 686,
-        encryption: :simple_tls,
+      expect(config.adapter_options).to include({
         auth: {
           method: :simple,
           username: 'uid=admin,dc=example,dc=com',
           password: 'super_secret'
         }
+      })
+    end
+
+    it 'sets encryption method to simple_tls when configured as simple_tls' do
+      stub_ldap_config(
+        options: {
+          'host'                => 'ldap.example.com',
+          'port'                => 686,
+          'encryption'          => 'simple_tls'
+        }
+      )
+
+      expect(config.adapter_options[:encryption]).to include({ method: :simple_tls })
+    end
+
+    it 'sets encryption method to start_tls when configured as start_tls' do
+      stub_ldap_config(
+        options: {
+          'host'                => 'ldap.example.com',
+          'port'                => 686,
+          'encryption'          => 'start_tls'
+        }
+      )
+
+      expect(config.adapter_options[:encryption]).to include({ method: :start_tls })
+    end
+
+    context 'when verify_certificates is enabled' do
+      it 'sets tls_options to OpenSSL defaults' do
+        stub_ldap_config(
+          options: {
+            'host'                => 'ldap.example.com',
+            'port'                => 686,
+            'encryption'          => 'simple_tls',
+            'verify_certificates' => true
+          }
+        )
+
+        expect(config.adapter_options[:encryption]).to include({ tls_options: OpenSSL::SSL::SSLContext::DEFAULT_PARAMS })
+      end
+    end
+
+    context 'when verify_certificates is disabled' do
+      it 'sets verify_mode to OpenSSL VERIFY_NONE' do
+        stub_ldap_config(
+          options: {
+            'host'                => 'ldap.example.com',
+            'port'                => 686,
+            'encryption'          => 'simple_tls',
+            'verify_certificates' => false
+          }
+        )
+
+        expect(config.adapter_options[:encryption]).to include({
+          tls_options: {
+            verify_mode: OpenSSL::SSL::VERIFY_NONE
+          }
+        })
+      end
+    end
+
+    context 'when ca_file is specified' do
+      it 'passes it through in tls_options' do
+        stub_ldap_config(
+          options: {
+            'host'                => 'ldap.example.com',
+            'port'                => 686,
+            'encryption'          => 'simple_tls',
+            'ca_file'             => '/etc/ca.pem'
+          }
+        )
+
+        expect(config.adapter_options[:encryption][:tls_options]).to include({ ca_file: '/etc/ca.pem' })
+      end
+    end
+
+    context 'when ca_file is a blank string' do
+      it 'does not add the ca_file key to tls_options' do
+        stub_ldap_config(
+          options: {
+            'host'                => 'ldap.example.com',
+            'port'                => 686,
+            'encryption'          => 'simple_tls',
+            'ca_file'             => ' '
+          }
+        )
+
+        expect(config.adapter_options[:encryption][:tls_options]).not_to have_key(:ca_file)
+      end
+    end
+
+    context 'when ssl_version is specified' do
+      it 'passes it through in tls_options' do
+        stub_ldap_config(
+          options: {
+            'host'                => 'ldap.example.com',
+            'port'                => 686,
+            'encryption'          => 'simple_tls',
+            'ssl_version'         => 'TLSv1_2'
+          }
+        )
+
+        expect(config.adapter_options[:encryption][:tls_options]).to include({ ssl_version: 'TLSv1_2' })
+      end
+    end
+
+    context 'when ssl_version is a blank string' do
+      it 'does not add the ssl_version key to tls_options' do
+        stub_ldap_config(
+          options: {
+            'host'                => 'ldap.example.com',
+            'port'                => 686,
+            'encryption'          => 'simple_tls',
+            'ssl_version'         => ' '
+          }
         )
+
+        expect(config.adapter_options[:encryption][:tls_options]).not_to have_key(:ssl_version)
+      end
     end
   end
 
@@ -67,7 +183,7 @@ describe Gitlab::LDAP::Config, lib: true do
           'host'       => 'ldap.example.com',
           'port'       => 386,
           'base'       => 'ou=users,dc=example,dc=com',
-          'method'  => 'plain',
+          'encryption' => 'plain',
           'uid'        => 'uid'
         }
       )
@@ -76,7 +192,7 @@ describe Gitlab::LDAP::Config, lib: true do
         host: 'ldap.example.com',
         port: 386,
         base: 'ou=users,dc=example,dc=com',
-        method: 'plain',
+        encryption: 'plain',
         filter: '(uid=%{username})'
       )
       expect(config.omniauth_options.keys).not_to include(:bind_dn, :password)
@@ -98,6 +214,100 @@ describe Gitlab::LDAP::Config, lib: true do
         password: 'super_secret'
       )
     end
+
+    context 'when verify_certificates is enabled' do
+      it 'specifies disable_verify_certificates as false' do
+        stub_ldap_config(
+          options: {
+            'host'                => 'ldap.example.com',
+            'port'                => 686,
+            'encryption'          => 'simple_tls',
+            'verify_certificates' => true
+          }
+        )
+
+        expect(config.omniauth_options).to include({ disable_verify_certificates: false })
+      end
+    end
+
+    context 'when verify_certificates is disabled' do
+      it 'specifies disable_verify_certificates as true' do
+        stub_ldap_config(
+          options: {
+            'host'                => 'ldap.example.com',
+            'port'                => 686,
+            'encryption'          => 'simple_tls',
+            'verify_certificates' => false
+          }
+        )
+
+        expect(config.omniauth_options).to include({ disable_verify_certificates: true })
+      end
+    end
+
+    context 'when ca_file is present' do
+      it 'passes it through' do
+        stub_ldap_config(
+          options: {
+            'host'                => 'ldap.example.com',
+            'port'                => 686,
+            'encryption'          => 'simple_tls',
+            'verify_certificates' => true,
+            'ca_file'             => '/etc/ca.pem'
+          }
+        )
+
+        expect(config.omniauth_options).to include({ ca_file: '/etc/ca.pem' })
+      end
+    end
+
+    context 'when ca_file is blank' do
+      it 'does not include the ca_file option' do
+        stub_ldap_config(
+          options: {
+            'host'                => 'ldap.example.com',
+            'port'                => 686,
+            'encryption'          => 'simple_tls',
+            'verify_certificates' => true,
+            'ca_file'             => ' '
+          }
+        )
+
+        expect(config.omniauth_options).not_to have_key(:ca_file)
+      end
+    end
+
+    context 'when ssl_version is present' do
+      it 'passes it through' do
+        stub_ldap_config(
+          options: {
+            'host'                => 'ldap.example.com',
+            'port'                => 686,
+            'encryption'          => 'simple_tls',
+            'verify_certificates' => true,
+            'ssl_version'         => 'TLSv1_2'
+          }
+        )
+
+        expect(config.omniauth_options).to include({ ssl_version: 'TLSv1_2' })
+      end
+    end
+
+    context 'when ssl_version is blank' do
+      it 'does not include the ssl_version option' do
+        stub_ldap_config(
+          options: {
+            'host'                => 'ldap.example.com',
+            'port'                => 686,
+            'encryption'          => 'simple_tls',
+            'verify_certificates' => true,
+            'ssl_version'         => ' '
+          }
+        )
+
+        expect(config.omniauth_options).not_to have_key(:ssl_version)
+      end
+    end
   end
 
   describe '#has_auth?' do