ldap: fixed a couple of bugs around SSL support

This commit fixes a couple of bugs present in both master and 2.3:

1. We didn't implement some options that needed to be passed to the LDAP
   backend to fully support SSL connections. This has been addressed
   also in the configuration, but without breaking existing
   installations (e.g. the `method` attribute from 2.3 has been left
   untouched). This will be addressed in later commits of the master
   branch (so in 2.4 users should adapt to this change).
2. We were relying on Devise's translations for failures, but some of
   them were not available. This has been addressed and improved: the
   error message will be more on point and more informative to end
   users.

There is still room for improvement, but we can do it in later commits:
let's keep this commit to the point so it can be cherry-picked into the
2.3 branch.

Fixes #1746
Fixes #1774

bsc#1073232

Signed-off-by: Miquel Sabaté Solà <[email protected]>

config/config.yml

@@ -46,10 +46,22 @@ ldap:
   hostname: "ldap_hostname"
   port: 389
 
-  # Available options: "plain", "simple_tls" and "starttls". The default is
-  # "plain", the recommended is "starttls".
+  # Available options: "plain", "simple_tls" and "start_tls".
+  # TODO: deprecated in favor of `encryption.method`.
   method: "plain"
 
+  # Encryption options
+  encryption:
+    # Available methods: "plain", "simple_tls" and "start_tls".
+    method: ""
+    options:
+      # The CA file to be accepted by the LDAP server. If none is provided, then
+      # the default parameters from the host will be sent.
+      ca_file: ""
+
+      # Protocol version.
+      ssl_version: "TLSv1_2"
+
   # The base where users are located (e.g. "ou=users,dc=example,dc=com").
   base: ""
 

lib/portus/ldap.rb

@@ -26,6 +26,7 @@ module Portus
   class LDAP < Devise::Strategies::Authenticatable
     # Re-implemented from Devise::Strategies::Authenticatable to authenticate
     # the user.
+    # rubocop:disable Metrics/PerceivedComplexity
     def authenticate!
       @ldap = load_configuration
 
@@ -37,14 +38,29 @@ def authenticate!
         # authentication process will fail without going to the any other
         # strategy. Otherwise, login the current user into Portus (and create
         # it if it doesn't exist).
-        @ldap.bind_as(bind_options) ? portus_login! : fail!(:ldap_bind_failed)
+        res = @ldap.bind_as(bind_options)
+        if res
+          portus_login!
+        else
+          msg = if @ldap.get_operation_result.code.zero?
+                  "Could not find user '#{username}'"
+                else
+                  "#{@ldap.get_operation_result.message} (code #{@ldap.get_operation_result.code})"
+                end
+          Rails.logger.tagged(:ldap) { Rails.logger.error("#{msg}.") }
+          fail!(msg)
+        end
       else
         # rubocop:disable Style/SignalException
         fail(:ldap_failed)
         # rubocop:enable Style/SignalException
       end
       # rubocop:enable Style/GuardClause
+    rescue Net::LDAP::Error => e
+      Rails.logger.tagged(:ldap) { Rails.logger.error("#{e.message}.") }
+      fail!(e.message)
     end
+    # rubocop:enable Metrics/PerceivedComplexity
 
     # Returns true if LDAP has been enabled in the application, false
     # otherwise.
@@ -73,6 +89,7 @@ def authentication?
     end
 
     def adapter_options
+      # TODO: add connect_timeout
       cfg = APP_CONFIG["ldap"]
       {
         host:       cfg["hostname"],
@@ -94,14 +111,37 @@ def load_configuration
       adapter.new(adapter_options)
     end
 
-    # Returns the encryption method to be used. Invalid encryption methods will
-    # be mapped to "plain".
+    # Returns the encryption hash to be used. If no encryption is being used,
+    # then nil is returned.
     def encryption(config)
-      case config["method"]
+      method = encryption_method(config)
+      return nil if method.blank?
+
+      {
+        method:      method,
+        tls_options: encryption_options(config)
+      }
+    end
+
+    # Returns the encryption method as a symbol or nil if none was provided.
+    def encryption_method(config)
+      method = config.fetch("encryption", {})["method"] || config["method"]
+      case method.to_s
+      when "start_tls", "simple_tls"
+        method.to_sym
       when "starttls"
         :start_tls
-      when "simple_tls"
-        :simple_tls
+      end
+    end
+
+    # Returns the encryption options to be used. If none was specified, then the
+    # default parameters will be returned (default CA from the host).
+    def encryption_options(config)
+      options = config.fetch("encryption", {})["options"]
+      return OpenSSL::SSL::SSLContext::DEFAULT_PARAMS if options.blank? || options["ca_file"].blank?
+
+      { ca_file: options["ca_file"] }.tap do |opt|
+        opt[:ssl_version] = options["ssl_version"] if options["ssl_version"].present?
       end
     end
 

spec/lib/portus/ldap_spec.rb

@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "ostruct"
 require "rails_helper"
 
 class LdapMockAdapter
@@ -16,8 +17,20 @@ def bind_as(_)
 
 class LdapFailedBindAdapter < LdapMockAdapter
   def bind_as(_)
+    raise Net::LDAP::Error, "Net::LDAP::Error exception" if ENV["LDAP_RAISE_EXCEPTION"] == "true"
     false
   end
+
+  # rubocop:disable Naming/AccessorMethodName
+  def get_operation_result
+    code = ENV["LDAP_OPERATION_CODE"] || 1
+
+    OpenStruct.new(
+      message: "a message",
+      code:    code.to_i
+    )
+  end
+  # rubocop:enable Naming/AccessorMethodName
 end
 
 class LdapSearchAdapter
@@ -168,7 +181,63 @@ def load_configuration_test
     [["starttls", :start_tls], ["simple_tls", :simple_tls], ["lala", nil]].each do |e|
       APP_CONFIG["ldap"]["method"] = e[0]
       cfg = lm.load_configuration_test
-      expect(cfg.opts[:encryption]).to eq e[1]
+
+      enc = cfg.opts.fetch(:encryption, {})
+      expect(enc ? enc[:method] : enc).to eq e[1]
+    end
+  end
+
+  context "encryption" do
+    before do
+      APP_CONFIG["ldap"] = ldap_config
+    end
+
+    it "returns nil on plain" do
+      APP_CONFIG["ldap"]["encryption"] = {
+        "method" => "plain"
+      }
+
+      lm = LdapMock.new(username: "name", password: "1234")
+      cfg = lm.load_configuration_test
+
+      expect(cfg.opts[:encryption]).to be_nil
+    end
+
+    it "returns some default parameters when options are not given" do
+      APP_CONFIG["ldap"]["encryption"] = {
+        "method" => "start_tls"
+      }
+
+      lm = LdapMock.new(username: "name", password: "1234")
+      cfg = lm.load_configuration_test
+      expect(cfg.opts[:encryption][:method]).to eq :start_tls
+      expect(cfg.opts[:encryption][:tls_options]).to eq OpenSSL::SSL::SSLContext::DEFAULT_PARAMS
+    end
+
+    it "adds the CA file" do
+      APP_CONFIG["ldap"]["encryption"] = {
+        "method"  => "start_tls",
+        "options" => { "ca_file" => "/my/pem/file" }
+      }
+
+      lm = LdapMock.new(username: "name", password: "1234")
+      cfg = lm.load_configuration_test
+      expect(cfg.opts[:encryption][:method]).to eq :start_tls
+      expect(cfg.opts[:encryption][:tls_options][:ca_file]).to eq "/my/pem/file"
+      expect(cfg.opts[:encryption][:tls_options][:ssl_version]).to be_nil
+    end
+
+    it "adds the CA file and the SSL version" do
+      APP_CONFIG["ldap"]["encryption"] = {
+        "method"  => "start_tls",
+        "options" => { "ca_file" => "/my/pem/file", "ssl_version" => "TLSv1_1" }
+      }
+
+      lm = LdapMock.new(username: "name", password: "1234")
+      cfg = lm.load_configuration_test
+      expect(cfg.opts[:encryption][:method]).to eq :start_tls
+      expect(cfg.opts[:encryption][:tls_options][:ca_file]).to eq "/my/pem/file"
+      expect(cfg.opts[:encryption][:tls_options][:ssl_version]).to eq "TLSv1_1"
     end
   end
 
@@ -296,6 +365,11 @@ def load_configuration_test
   end
 
   describe "#authenticate!" do
+    before do
+      ENV["LDAP_OPERATION_CODE"] = nil
+      ENV["LDAP_RAISE_EXCEPTION"] = nil
+    end
+
     it "raises an exception if ldap is not supported" do
       lm = LdapMock.new(username: "name", password: "1234")
       lm.authenticate!
@@ -307,7 +381,27 @@ def load_configuration_test
       lm = LdapMock.new(username: "name", password: "12341234")
       lm.bind_result = false
       lm.authenticate!
-      expect(lm.last_symbol).to be :ldap_bind_failed
+      expect(lm.last_symbol).to eq "a message (code 1)"
+    end
+
+    it "fails if the user was not found" do
+      APP_CONFIG["ldap"] = { "enabled" => true, "base" => "" }
+      ENV["LDAP_OPERATION_CODE"] = "0"
+
+      lm = LdapMock.new(username: "name", password: "12341234")
+      lm.bind_result = false
+      lm.authenticate!
+      expect(lm.last_symbol).to eq "Could not find user 'name'"
+    end
+
+    it "can rescue Net::LDAP::Error exceptions" do
+      APP_CONFIG["ldap"] = { "enabled" => true, "base" => "" }
+      ENV["LDAP_RAISE_EXCEPTION"] = "true"
+
+      lm = LdapMock.new(username: "name", password: "12341234")
+      lm.bind_result = false
+      lm.authenticate!
+      expect(lm.last_symbol).to eq "Net::LDAP::Error exception"
     end
 
     it "raises an exception if the user could not created" do