From 76ced6e9b09199e5f6bc0a650ace1999cdd582ad Mon Sep 17 00:00:00 2001
From: Micah Lee <micah.lee@cyberark.com>
Date: Mon, 6 Feb 2023 13:13:01 -0500
Subject: [PATCH] Log when Conjur config permissions are incorrect

---
 CHANGELOG.md                                  |   7 +
 app/domain/logs.rb                            |  30 +++
 app/domain/util/trackable_error_class.rb      |   2 +
 .../util/trackable_log_message_class.rb       |   2 +
 bin/conjur-cli.rb                             |  10 +-
 config/initializers/conjur_config.rb          |   4 +-
 lib/conjur/conjur_config.rb                   | 110 +++++++++-
 spec/conjurctl/configuration_show_spec.rb     |   8 +-
 spec/lib/conjur/conjur_config_spec.rb         | 202 +++++++++++++++---
 spec/spec_helper.rb                           |  12 +-
 10 files changed, 347 insertions(+), 40 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1954a8b5c2..f6a47e31e4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## [1.19.3] - 2023-01-26
 
+### Added
+- Conjur now logs when it detects that the Conjur configuration file
+  (conjur.yml) or directory permissions prevent the Conjur server from
+  successfully reading it. Conjur also now logs at the DEBUG level when it
+  detects that either the directory or file do not exist.
+  [cyberark/conjur#2715](https://github.com/cyberark/conjur/pull/2715)
+
 ## [1.19.2] - 2022-01-13
 
 ### Fixed
diff --git a/app/domain/logs.rb b/app/domain/logs.rb
index 92a9694c82..a49d897f2a 100644
--- a/app/domain/logs.rb
+++ b/app/domain/logs.rb
@@ -1,5 +1,8 @@
 # frozen_string_literal: true
 
+require_relative 'util/trackable_log_message_class'
+require_relative 'util/trackable_error_class'
+
 # This file maintains the collection of possible logs emitted by Conjur using
 # the standard numbering scheme.
 #
@@ -794,4 +797,31 @@ module Util
     )
 
   end
+
+  module Config
+    DirectoryDoesNotExist = ::Util::TrackableLogMessageClass.new(
+      msg: "Conjur config directory doesn't exist or has " \
+           "insufficient permission to list it: {0-config-directory}",
+      code: "CONJ00147D"
+    )
+
+    DirectoryInvalidPermissions = ::Util::TrackableLogMessageClass.new(
+      msg: "Conjur config directory exists but is missing " \
+           "search/execute permission required to list the config file: " \
+           "{0-config-directory}",
+      code: "CONJ00148W"
+    )
+
+    FileDoesNotExist = ::Util::TrackableLogMessageClass.new(
+      msg: "Conjur config file doesn't exist or has insufficient " \
+           "permission to list it: {0-config-path}",
+      code: "CONJ00149D"
+    )
+
+    FileInvalidPermissions = ::Util::TrackableLogMessageClass.new(
+      msg: "Conjur config file exists but has insufficient permission to " \
+           "read it: {0-config-path}",
+      code: "CONJ00150W"
+    )
+  end
 end
diff --git a/app/domain/util/trackable_error_class.rb b/app/domain/util/trackable_error_class.rb
index 3285b42978..78341baca7 100644
--- a/app/domain/util/trackable_error_class.rb
+++ b/app/domain/util/trackable_error_class.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require_relative 'error_class'
+
 # A factory for creating an ErrorClass with an error code prefix
 #
 module Util
diff --git a/app/domain/util/trackable_log_message_class.rb b/app/domain/util/trackable_log_message_class.rb
index 7a9581c7b1..775f838e0c 100644
--- a/app/domain/util/trackable_log_message_class.rb
+++ b/app/domain/util/trackable_log_message_class.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require_relative 'log_message_class'
+
 # A factory for creating a LogMessage with a code prefix
 #
 module Util
diff --git a/bin/conjur-cli.rb b/bin/conjur-cli.rb
index 1cf84c6381..8e898b7399 100644
--- a/bin/conjur-cli.rb
+++ b/bin/conjur-cli.rb
@@ -7,11 +7,17 @@
 require 'uri'
 require 'open3'
 require 'conjur/conjur_config'
+require 'logger'
 
 require_relative './conjur-cli/commands'
 
 include GLI::App
 
+# Create a logger instance that may be passed to classes that take injected
+# dependencies. Because many commands output to stdout, we log to stderr.
+cli_logger = Logger.new($stderr)
+cli_logger.level = ENV['CONJUR_LOG_LEVEL'] || :info
+
 program_desc "Command and control application for Conjur"
 version File.read(File.expand_path("../VERSION", File.dirname(__FILE__)))
 arguments :strict
@@ -241,7 +247,9 @@
   Anyway::Settings.default_config_path = "/etc/conjur/config"
 
   begin
-    conjur_config = Conjur::ConjurConfig.new
+    conjur_config = Conjur::ConjurConfig.new(
+      logger: cli_logger
+    )
   rescue Conjur::ConfigValidationError => e
     $stderr.puts e
     exit 1
diff --git a/config/initializers/conjur_config.rb b/config/initializers/conjur_config.rb
index 989343fc49..4feb76b883 100644
--- a/config/initializers/conjur_config.rb
+++ b/config/initializers/conjur_config.rb
@@ -6,5 +6,7 @@
   #
   # We create this in application.rb instead of an initializer so that it's
   # guaranteed to be available for other initializers to use.
-  config.conjur_config = Conjur::ConjurConfig.new
+  config.conjur_config = Conjur::ConjurConfig.new(
+    logger: Rails.logger
+  )
 end
diff --git a/lib/conjur/conjur_config.rb b/lib/conjur/conjur_config.rb
index a92f47ce16..df9d3f7cc3 100644
--- a/lib/conjur/conjur_config.rb
+++ b/lib/conjur/conjur_config.rb
@@ -2,6 +2,14 @@
 # rely on Rails autoloading to make the `Anyway::Config` constant available.
 require 'anyway_config'
 
+# It is a design smell that we have to require files from `app/domain` to use
+# in `/lib` (particularly that we traverse up to a common root directory). This
+# suggests that, if we want to continue using well-defined classes for logs and
+# errors, we should move those classes into `/lib` from `app/domain`. However,
+# that change greatly expands the scope of this PR and so, instead, I used a
+# relative require to include the log definitions here.
+require_relative '../../app/domain/logs'
+
 module Conjur
   # We are temporarily avoiding hooking into the application error system
   # because using it means you have to require about five classes when loading
@@ -33,8 +41,21 @@ class ConjurConfig < Anyway::Config
       extensions: []
     )
 
-    def initialize(*args)
-      super(*args)
+    def initialize(
+      *args,
+      logger: Rails.logger,
+      **kwargs
+    )
+      # The permissions checks emit log messages, so we need to initialize the
+      # logger before verifying permissions.
+      @logger = logger
+
+      # First verify that we have the permissions necessary to read the config
+      # file.
+      verify_config_is_readable
+
+      # Initialize Anyway::Config
+      super(*args, **kwargs)
 
     # If the config file is not a valid YAML document, we want
     # to raise a user-friendly ConfigValidationError rather than
@@ -101,6 +122,91 @@ def extensions=(val)
 
     private
 
+    def verify_config_is_readable
+      return unless verify_config_directory_exists
+
+      return unless verify_config_directory_permissions
+
+      return unless verify_config_file_exists
+
+      return unless verify_config_file_permissions
+
+      # If no issues are detected, log where the config file is being read from.
+      @logger.info("Loading Conjur config file: #{config_path}")
+    end
+
+    def verify_config_directory_exists
+      return true if File.directory?(config_directory)
+
+      # It may be expected that the config directory not exist, so this is
+      # a log message for debugging the Conjur config, rather than alerting the
+      # user to an issue.
+      @logger.debug(
+        LogMessages::Config::DirectoryDoesNotExist.new(config_directory).to_s
+      )
+      false
+    end
+
+    def verify_config_directory_permissions
+      return true if File.executable?(config_directory)
+
+      # require 'pry'
+      # binding.pry
+
+      # If the config direct does exist, we want to alert the user if the
+      # permissions will prevent Conjur from reading a config file in that
+      # directory.
+      @logger.warn(
+        LogMessages::Config::DirectoryInvalidPermissions.new(
+          config_directory
+        ).to_s
+      )
+
+      false
+    end
+
+    def verify_config_file_exists
+      return true if File.file?(config_path)
+
+      # Similar to the directory, if the config file doesn't exist, this becomes
+      # debugging information.
+      @logger.debug(
+        LogMessages::Config::FileDoesNotExist.new(config_path).to_s
+      )
+
+      false
+    end
+
+    def verify_config_file_permissions
+      return true if File.readable?(config_path)
+
+      # If the file exists but we can detect it's not readable, let the user
+      # know. Conjur will also fail to start in this configuration.
+      @logger.warn(
+        LogMessages::Config::FileInvalidPermissions.new(config_path).to_s
+      )
+
+      false
+    end
+
+    def config_directory
+      File.dirname(config_path)
+    end
+
+    # Because we call this before the base class initialized, we need to move the
+    # implementation here.
+    #
+    # Derived from:
+    # https://github.com/palkan/anyway_config/blob/83d79ccaf5619889c07c8ecdf8d66dcb22c9dc05/lib/anyway/config.rb#L358
+    #
+    # :reek:DuplicateMethodCall due to `self.class`
+    def config_path
+      @config_path ||= resolve_config_path(
+        self.class.config_name,
+        self.class.env_prefix
+      )
+    end
+
     def str_to_list(val)
       val.is_a?(String) ? val.split(',') : val
     end
diff --git a/spec/conjurctl/configuration_show_spec.rb b/spec/conjurctl/configuration_show_spec.rb
index 63d4fffd7c..cad662e0b3 100644
--- a/spec/conjurctl/configuration_show_spec.rb
+++ b/spec/conjurctl/configuration_show_spec.rb
@@ -33,8 +33,8 @@
       "conjurctl configuration show --output invalid"
     )
 
-    expect(stderr).to eq(
-      "error: Unknown configuration output format 'invalid'\n"
+    expect(stderr).to include(
+      "error: Unknown configuration output format 'invalid'"
     )
   end
 
@@ -43,8 +43,8 @@
       "CONJUR_TRUSTED_PROXIES=boop conjurctl configuration show"
     )
 
-    expect(stderr).to eq(
-      "Invalid values for configured attributes: trusted_proxies\n"
+    expect(stderr).to include(
+      "Invalid values for configured attributes: trusted_proxies"
     )
 
     expect(status.exitstatus).to eq(1)
diff --git a/spec/lib/conjur/conjur_config_spec.rb b/spec/lib/conjur/conjur_config_spec.rb
index 27174c6cba..1621b30b7c 100644
--- a/spec/lib/conjur/conjur_config_spec.rb
+++ b/spec/lib/conjur/conjur_config_spec.rb
@@ -1,19 +1,33 @@
 require 'spec_helper'
 
 describe Conjur::ConjurConfig do
+  let(:config_folder) { @config_folder || '/etc/conjur/config' }
+  let(:config_file) { "#{config_folder}/conjur.yml" }
+
+  let(:logger_double) { Logger.new(log_output) }
+  let(:log_output) { StringIO.new }
+
+  let(:config_args) { [] }
+  let(:config_kwargs) { {} }
+
+  subject do
+    Conjur::ConjurConfig.new(
+      *config_args,
+      logger: logger_double,
+      **config_kwargs
+    )
+  end
+
   it "uses default value if not set by environment variable or config file" do
-    expect(Conjur::ConjurConfig.new.trusted_proxies).to eq([])
+    expect(subject.trusted_proxies).to eq([])
   end
 
   it "reports the attribute source as :defaults" do
-    expect(Conjur::ConjurConfig.new.attribute_sources[:trusted_proxies]).
+    expect(subject.attribute_sources[:trusted_proxies]).
       to eq(:defaults)
   end
 
   context "with config file" do
-    let(:config_folder) { "/etc/conjur/config" }
-    let(:config_file) { "#{config_folder}/conjur.yml" }
-
     let(:config_file_contents) do
       <<~YAML
         trusted_proxies:
@@ -21,24 +35,33 @@
       YAML
     end
 
-    before do
-      FileUtils.mkdir_p(config_folder)
+    around do |example|
+      with_temp_config_directory do |dir|
+        @config_folder = dir
+
+        # Create config file
+        File.open(config_file, 'w') do |f|
+          f.puts(config_file_contents)
+        end
 
-      File.open(config_file, 'w') do |f|
-        f.puts(config_file_contents)
+        # Run the example
+        example.run
       end
     end
 
-    after do
-      FileUtils.remove_dir(config_folder)
+    it "logs that the config file exists" do
+      subject
+      expect(log_output.string).to include(
+        "Loading Conjur config file: #{@config_folder}/conjur.yml"
+      )
     end
 
     it "reads config value from file" do
-      expect(Conjur::ConjurConfig.new.trusted_proxies).to eq(["1.2.3.4"])
+      expect(subject.trusted_proxies).to eq(["1.2.3.4"])
     end
 
     it "reports the attribute source as :yml" do
-      expect(Conjur::ConjurConfig.new.attribute_sources[:trusted_proxies]).
+      expect(subject.attribute_sources[:trusted_proxies]).
         to eq(:yml)
     end
 
@@ -50,7 +73,7 @@
       end
 
       it "fails validation" do
-        expect { Conjur::ConjurConfig.new }.
+        expect { subject }.
           to raise_error(Conjur::ConfigValidationError)
       end
     end
@@ -64,7 +87,7 @@
       end
 
       it "fails validation" do
-        expect { Conjur::ConjurConfig.new }.
+        expect { subject }.
           to raise_error(Conjur::ConfigValidationError)
       end
     end
@@ -77,7 +100,7 @@
       end
 
       it "fails validation" do
-        expect { Conjur::ConjurConfig.new }.
+        expect { subject }.
           to raise_error(Conjur::ConfigValidationError)
       end
     end
@@ -90,7 +113,7 @@
       end
 
       it "fails validation" do
-        expect { Conjur::ConjurConfig.new }.
+        expect { subject }.
           to raise_error(Conjur::ConfigValidationError, /syntax error/)
       end
     end
@@ -112,11 +135,11 @@
       end
 
       it "overrides the config file value" do
-        expect(Conjur::ConjurConfig.new.trusted_proxies).to eq(["5.6.7.8"])
+        expect(subject.trusted_proxies).to eq(["5.6.7.8"])
       end
 
       it "reports the attribute source as :env" do
-        expect(Conjur::ConjurConfig.new.attribute_sources[:trusted_proxies]).
+        expect(subject.attribute_sources[:trusted_proxies]).
           to eq(:env)
       end
     end
@@ -138,35 +161,36 @@
       end
 
       it "overrides the config file value" do
-        expect(Conjur::ConjurConfig.new.trusted_proxies).
+        expect(subject.trusted_proxies).
           to eq(["5.6.7.8", "9.10.11.12"])
       end
     end
   end
 
   describe "validation" do
-    let(:invalid_config) {
-      Conjur::ConjurConfig.new(
-        authenticators: "invalid-authn", trusted_proxies: "boop"
-      )
-    }
+    let(:config_kwargs) do
+      {
+        authenticators: "invalid-authn",
+        trusted_proxies: "boop"
+      }
+    end
 
     it "raises error when validation fails" do
-      expect { invalid_config }.
+      expect { subject }.
         to raise_error(Conjur::ConfigValidationError)
     end
 
     it "includes the attributes that failed validation" do
-      expect { invalid_config }.
+      expect { subject }.
         to raise_error(/trusted_proxies/)
-      expect { invalid_config }.
+      expect { subject }.
         to raise_error(/authenticators/)
     end
 
     it "does not include the value that failed validation" do
-      expect { invalid_config }.
+      expect { subject }.
         to_not raise_error(/boop/)
-      expect { invalid_config }.
+      expect { subject }.
         to_not raise_error(/invalid-authn/)
     end
   end
@@ -193,7 +217,123 @@
     end
 
     it "reads value from TRUSTED_PROXIES env var" do
-      expect(Conjur::ConjurConfig.new.trusted_proxies).to eq(["5.6.7.8"])
+      expect(subject.trusted_proxies).to eq(["5.6.7.8"])
+    end
+  end
+
+  describe "file and directory permissions" do
+    context "when the directory doesn't exist" do
+      around do |example|
+        with_temp_config_directory do |dir|
+          @config_folder = dir
+
+          FileUtils.rm_rf(config_folder)
+
+          # Run the example
+          example.run
+        end
+      end
+
+      it "logs a warning" do
+        subject
+        expect(log_output.string).to include(
+          "Conjur config directory doesn't exist or has " \
+          "insufficient permission to list it:"
+        )
+      end
+    end
+
+    context "when the directory lacks execute/search permissions" do
+      around do |example|
+        with_temp_config_directory do |dir|
+          @config_folder = dir
+          FileUtils.chmod(0444, @config_folder)
+
+          # The tests run as root, so we must drop privilege to test permission
+          # checks.
+          as_user 'nobody' do
+            example.run
+          end
+        end
+      end
+
+      it "logs a warning" do
+        subject
+        expect(log_output.string).to include(
+          "Conjur config directory exists but is missing " \
+          "search/execute permission required to list the config file:"
+        )
+      end
+    end
+
+    context "when the file doesn't exist" do
+      around do |example|
+        with_temp_config_directory do |dir|
+          @config_folder = dir
+
+          FileUtils.chmod(0111, config_folder)
+          FileUtils.rm_rf(config_file)
+
+          example.run
+        end
+      end
+
+      it "logs a warning" do
+        subject
+        expect(log_output.string).to include(
+          "Conjur config file doesn't exist or has insufficient " \
+          "permission to list it:"
+        )
+      end
+    end
+
+    context "when the file lacks read permissions" do
+      around do |example|
+        with_temp_config_directory do |dir|
+          @config_folder = dir
+          FileUtils.chmod(0555, @config_folder)
+          FileUtils.touch(config_file)
+          FileUtils.chmod(0000, config_file)
+
+          # The tests run as root, so we must drop privilege to test permission
+          # checks.
+          as_user 'nobody' do
+            example.run
+          end
+        end
+      end
+
+      it "logs a warning" do
+        # This last scenario will also raise an exception, but in this case, we
+        # are only interested in the log output
+        begin
+          subject
+        rescue
+        end
+        expect(log_output.string).to include(
+          "Conjur config file exists but has insufficient " \
+          "permission to read it:"
+        )
+      end
     end
   end
 end
+
+# Helper method for the config file tests to create a temporary directory for
+# testing file and directory permissions behavior
+def with_temp_config_directory
+  # Configure a temporary config directory
+  prev_config_dir = Anyway::Settings.default_config_path
+
+  config_dir = Dir.mktmpdir
+  Anyway::Settings.default_config_path = config_dir
+
+  # Call the block
+  yield config_dir
+ensure
+  # Resete the config directory
+  Anyway::Settings.default_config_path = prev_config_dir
+
+  # remove the temporary config directory
+  FileUtils.rm_rf(config_folder)
+end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index c5e89a273d..73c0fe888c 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -96,7 +96,6 @@ def secret_logged?(secret)
 
   # Remaining possibilities are 0 and 1, secret found or not found.
   exit_status == 0
-
 end
 
 # Creates valid access token for the given username.
@@ -150,3 +149,14 @@ def conjur_server_dir
   # Navigate from its directory (/bin) to the root Conjur server directory
   Pathname.new(File.join(File.dirname(conjurctl_path), '..')).cleanpath
 end
+
+# Allows running a block of test code as another user.
+# For example, to run a block without root privileges.
+def as_user(user, &block)
+  prev = Process.uid
+  u = Etc.getpwnam(user)
+  Process.uid = Process.euid = u.uid
+  block.call
+ensure
+  Process.uid = Process.euid = prev
+end