Merge pull request #2988 from cyberark/cert-writing-bug

AuthnOIDC V2: write custom certs to non-default directory

Author: john-odonnell

CHANGELOG.md

@@ -9,6 +9,22 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Nothing should go in this section, please add to the latest unreleased version
   (and update the corresponding date), or add a new version.
 
+## [1.20.1] - 2023-10-13
+
+### Fixed
+- OIDC Authenticator now writes custom certs to a non-default directory instead
+  of the system default certificate store.
+  [cyberark/conjur#2988](https://github.com/cyberark/conjur/pull/2988)
+
+### Added
+- Support for the no_proxy & NO_PROXY environment variables for the k8s authenticator.
+  [CNJR-2759](https://ca-il-jira.il.cyber-ark.com:8443/browse/CNJR-2759)
+
+### Security
+- Upgrade google/cloud-sdk in ci/test_suites/authenticators_k8s/dev/Dockerfile/test
+  to use latest version (448.0.0)
+  [cyberark/conjur#2972](https://github.com/cyberark/conjur/pull/2972)
+
 ## [1.20.0] - 2023-09-21
 
 ### Fixed
@@ -34,8 +50,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Use base images with newer Ubuntu and UBI.
   Display FIPS Mode status in the UI (requires temporary fix for OpenSSL gem).
   [cyberark/conjur#2874](https://github.com/cyberark/conjur/pull/2874)
-- Support for the no_proxy & NO_PROXY environment variables for the k8s authenticator.
-  [CNJR-2759](https://ca-il-jira.il.cyber-ark.com:8443/browse/CNJR-2759)
 
 ### Changed
 - The database thread pool max connection size is now based on the number of
@@ -51,9 +65,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
   [cyberark/conjur#2827](https://github.com/cyberark/conjur/pull/2827)
 
 ### Security
-- Upgrade google/cloud-sdk in ci/test_suites/authenticators_k8s/dev/Dockerfile/test
-  to use latest version (448.0.0)
-  [cyberark/conjur#2972](https://github.com/cyberark/conjur/pull/2972)
 - Support plural syntax for revoke and deny
   [cyberark/conjur#2901](https://github.com/cyberark/conjur/pull/2901)
 - Previously, attempting to add and remove a privilege in the same policy load

app/domain/authentication/authn_oidc/v2/client.rb

@@ -18,6 +18,19 @@ def initialize(
           @logger = logger
         end
 
+        # Writing certificates to the default system cert store requires
+        # superuser privilege. Instead, Conjur will use ${CONJUR_ROOT}/tmp/certs.
+        def self.default_cert_dir(dir: Dir, fileutils: FileUtils)
+          if @default_cert_dir.blank?
+            conjur_root = __dir__.slice(0..(__dir__.index('/app')))
+            @default_cert_dir = File.join(conjur_root, "tmp/certs")
+          end
+
+          fileutils.mkdir_p(@default_cert_dir) unless dir.exist?(@default_cert_dir.to_s)
+
+          @default_cert_dir
+        end
+
         def oidc_client
           @oidc_client ||= begin
             issuer_uri = URI(@authenticator.provider_uri)
@@ -99,7 +112,7 @@ def callback(code:, nonce:, code_verifier: nil)
 
         # callback_with_temporary_cert wraps the callback method with commands
         # to write & clean up a given certificate or cert chain in a given
-        # directory. By default, Conjur's default cert store is used.
+        # directory. By default, ${CONJUR_ROOT}/tmp/certs is used.
         #
         # The temporary certificate file name is "x.n", where x is the hash of
         # the certificate subject name, and n is incrememnted from 0 in case of
@@ -114,7 +127,7 @@ def callback_with_temporary_cert(
           code:,
           nonce:,
           code_verifier: nil,
-          cert_dir: OpenSSL::X509::DEFAULT_CERT_DIR,
+          cert_dir: Authentication::AuthnOidc::V2::Client.default_cert_dir,
           cert_string: nil
         )
           c = -> { callback(code: code, nonce: nonce, code_verifier: code_verifier) }
@@ -148,6 +161,27 @@ def callback_with_temporary_cert(
               symlink_a << symlink
             end
 
+            if OpenIDConnect.http_config.nil? || OpenIDConnect.http_client.ssl.ca_path != cert_dir
+              config_proc = proc do |config|
+                config.ssl.ca_path = cert_dir
+                config.ssl.verify_mode = OpenSSL::SSL::VERIFY_PEER
+              end
+
+              # OpenIDConnect gem only accepts a single Faraday configuration
+              # through calls to its .http_config method, and future calls to
+              # the #http_config method return the first config instance.
+              #
+              # On the first call to OpenIDConnect.http_config, it will pass the
+              # new Faraday configuration to its dependency gems that also have
+              # nil configs. We can't be certain that each gem is configured
+              # with the same Faraday config and need them synchronized, so we
+              # inject them manually.
+              OpenIDConnect.class_variable_set(:@@http_config, config_proc)
+              WebFinger.instance_variable_set(:@http_config, config_proc)
+              SWD.class_variable_set(:@@http_config, config_proc)
+              Rack::OAuth2.class_variable_set(:@@http_config, config_proc)
+            end
+
             c.call
           ensure
             symlink_a.each{ |s| File.unlink(s) if s.present? && File.symlink?(s) }
@@ -174,7 +208,7 @@ def discovery_information(invalidate: false)
 
         # discover wraps ::OpenIDConnect::Discovery::Provider::Config.discover!
         # with commands to write & clean up a given certificate or cert chain in
-        # a given directory. By default, Conjur's default cert store is used.
+        # a given directory. By default, ${CONJUR_ROOT}/tmp/certs is used.
         #
         # The temporary certificate file name is "x.n", where x is the hash of
         # the certificate subject name, and n is incremented from 0 in case of
@@ -186,7 +220,7 @@ def discovery_information(invalidate: false)
         def self.discover(
           provider_uri:,
           discovery_configuration: ::OpenIDConnect::Discovery::Provider::Config,
-          cert_dir: OpenSSL::X509::DEFAULT_CERT_DIR,
+          cert_dir: default_cert_dir,
           cert_string: nil,
           jwks: false
         )
@@ -226,6 +260,27 @@ def self.discover(
               symlink_a << symlink
             end
 
+            if OpenIDConnect.http_config.nil? || OpenIDConnect.http_client.ssl.ca_path != cert_dir
+              config_proc = proc do |config|
+                config.ssl.ca_path = cert_dir
+                config.ssl.verify_mode = OpenSSL::SSL::VERIFY_PEER
+              end
+
+              # OpenIDConnect gem only accepts a single Faraday configuration
+              # through calls to its .http_config method, and future calls to
+              # the #http_config method return the first config instance.
+              #
+              # On the first call to OpenIDConnect.http_config, it will pass the
+              # new Faraday configuration to its dependency gems that also have
+              # nil configs. We can't be certain that each gem is configured
+              # with the same Faraday config and need them synchronized, so we
+              # inject them manually.
+              OpenIDConnect.class_variable_set(:@@http_config, config_proc)
+              WebFinger.instance_variable_set(:@http_config, config_proc)
+              SWD.class_variable_set(:@@http_config, config_proc)
+              Rack::OAuth2.class_variable_set(:@@http_config, config_proc)
+            end
+
             d.call
           ensure
             symlink_a.each{ |s| File.unlink(s) if s.present? && File.symlink?(s) }

ci/shared.sh

@@ -170,7 +170,6 @@ _run_cucumber_tests() {
   # ${cucumber_tags_arg} should overwrite the profile tags in a way for @smoke to work correctly
   $COMPOSE run "${run_flags[@]}" "${env_var_flags[@]}" \
     cucumber -ec "\
-      /oauth/keycloak/scripts/fetch_certificate &&
       bundle exec parallel_cucumber . -n ${PARALLEL_PROCESSES} \
        -o '--strict --profile \"${profile}\" ${cucumber_tags_arg} \
        --format json --out \"cucumber/$profile/cucumber_results.json\" \

ci/test_suites/authenticators_oidc/test

@@ -54,7 +54,7 @@ function main() {
   local conjur_parallel_services
   read -ra conjur_parallel_services <<< "$(get_parallel_services 'conjur')"
   for parallel_service in "${conjur_parallel_services[@]}"; do
-    hash=$($COMPOSE exec "${parallel_service}" openssl x509 -hash -in /etc/ssl/certs/keycloak.pem -out /dev/null)
+    hash=$($COMPOSE exec "${parallel_service}" openssl x509 -hash -in /etc/ssl/certs/keycloak.pem --noout)
     $COMPOSE exec "${parallel_service}" rm "/etc/ssl/certs/$hash.0" || true
   done
 

dev/start

@@ -322,7 +322,7 @@ configure_oidc_v2() {
   if [ "$service_id" = "keycloak2" ]; then
     client_add_secret "conjur/authn-oidc/$service_id/ca-cert" "$($COMPOSE exec conjur cat /etc/ssl/certs/keycloak.pem)"
     # Delete the symlink so we can test with the 'ca-cert' variable
-    hash=$($COMPOSE exec conjur openssl x509 -hash -in /etc/ssl/certs/keycloak.pem -out /dev/null)
+    hash=$($COMPOSE exec conjur openssl x509 -hash -in /etc/ssl/certs/keycloak.pem --noout)
     $COMPOSE exec conjur rm "/etc/ssl/certs/$hash.0" || true
   fi
 

spec/app/domain/authentication/authn-oidc/v2/client_spec.rb

@@ -644,4 +644,76 @@ def client(config)
       end
     end
   end
+
+  describe '.default_cert_dir', type: 'unit' do
+    let(:target)      { Authentication::AuthnOidc::V2::Client }
+    let(:conjur_root) { "/src/conjur-server" }
+    let(:dir)         { double("Mock Dir") }
+    let(:fileutils)   { double("Mock FileUtils") }
+
+    context 'when @default_cert_dir is blank' do
+      before(:each) do
+        target.instance_variable_set(:@default_cert_dir, nil)
+      end
+
+      context 'when default cert dir exists in filesystem' do
+        before(:each) do
+          allow(dir).to receive(:exist?).with(String).and_return(true)
+        end
+
+        it 'returns the default path' do
+          expect(target.default_cert_dir(dir: dir, fileutils: fileutils)).to eq("#{conjur_root}/tmp/certs")
+          expect(target.instance_variable_get(:@default_cert_dir)).to eq("#{conjur_root}/tmp/certs")
+        end
+      end
+
+      context 'when the default cert dir does not exist in filesystem' do
+        before(:each) do
+          allow(dir).to receive(:exist?).with(String).and_return(false)
+          allow(fileutils).to receive(:mkdir_p).with(String) do |s|
+            [s]
+          end
+        end
+
+        it 'creates the dir and returns the default path' do
+          expect(fileutils).to receive(:mkdir_p).with(String)
+          expect(target.default_cert_dir(dir: dir, fileutils: fileutils)).to eq("#{conjur_root}/tmp/certs")
+          expect(target.instance_variable_get(:@default_cert_dir)).to eq("#{conjur_root}/tmp/certs")
+        end
+      end
+
+    end
+
+    context 'when @default_cert_dir is not blank' do
+      before(:each) do
+        target.instance_variable_set(:@default_cert_dir, "/path/to/dir")
+      end
+
+      context 'when @default_cert_dir exists in filesystem' do
+        before(:each) do
+          allow(dir).to receive(:exist?).with(String).and_return(true)
+        end
+
+        it 'returns existing path' do
+          expect(target.default_cert_dir(dir: dir, fileutils: fileutils)).to eq("/path/to/dir")
+          expect(target.instance_variable_get(:@default_cert_dir)).to eq("/path/to/dir")
+        end
+      end
+
+      context 'when @default_cert_dir does not exist in filesystem' do
+        before(:each) do
+          allow(dir).to receive(:exist?).with(String).and_return(false)
+          allow(fileutils).to receive(:mkdir_p).with(String) do |s|
+            [s]
+          end
+        end
+
+        it 'creates the dir and returns the existing path' do
+          expect(fileutils).to receive(:mkdir_p).with(String)
+          expect(target.default_cert_dir(dir: dir, fileutils: fileutils)).to eq("/path/to/dir")
+          expect(target.instance_variable_get(:@default_cert_dir)).to eq("/path/to/dir")
+        end
+      end
+    end
+  end
 end