Skip to content

Fix crash when LDAP CA file set outside tls_options

Stan Hu requested to merge sh-fix-options-logging into master

What does this MR do and why?

Previously if ldap_servers.ca_file were set instead of ldap_servers.tls_options.ca_file, the changes in !122888 (merged) would flag the call to Gitlab::Options#delete.

Unfortunately, the introduction of Gitlab::ErrorTracking.track_and_raise_for_dev_exception in !124396 (merged) caused the process to crash with an undefined devise method in the User module. This happened because the method attempted to load the User for the application context, but since Devise is initialized in 8_devise.rb, this came too late for the LDAP config parsing in 1_settings.rb.

To avoid this error, avoid loading the ApplicationContext by using Gitlab::AppJsonLogger.

Relates to #419485 (closed)

How to set up and validate locally

  1. In config/gitlab.yml insert a ldap.servers.ca_file option:
        # Enables SSL certificate verification if encryption method is
        # "start_tls" or "simple_tls". Defaults to true.
        verify_certificates: true

        ca_file: 'asdf'
  1. Run bin/rails console.
  2. In the GDK, this should crash.
  3. Disable the raise and repeat the test. This should NOT crash:
diff --git a/lib/gitlab_settings/options.rb b/lib/gitlab_settings/options.rb
index 3338c2d216ba..eed5013b52dd 100644
--- a/lib/gitlab_settings/options.rb
+++ b/lib/gitlab_settings/options.rb
@@ -172,7 +172,7 @@ def respond_to_missing?(name, include_all = false)
     # fail to load User since the Devise is not yet set up in
     # `config/initialiers/8_devise.rb`.
     def log_and_raise_dev_exception(message, extra = {})
-      raise message unless Rails.env.production?
+#      raise message unless Rails.env.production?
 
       # Gitlab::BacktraceCleaner drops config/initializers, so we just limit the
       # backtrace to the first 10 lines.

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Stan Hu

Merge request reports