From 5ffed27ee7262ee8045ab1680935fcfe0d7ee77a Mon Sep 17 00:00:00 2001 From: John ODonnell Date: Wed, 11 Oct 2023 17:45:21 -0400 Subject: [PATCH 1/4] AuthnOIDC V2: write custom certs to non-default directory --- CHANGELOG.md | 7 ++ .../authentication/authn_oidc/v2/client.rb | 63 ++++++++++++++-- .../authn-oidc/v2/client_spec.rb | 72 +++++++++++++++++++ 3 files changed, 138 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad15365cfd..7c25fa7afc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,13 @@ 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) + ## [1.20.0] - 2023-09-21 ### Fixed diff --git a/app/domain/authentication/authn_oidc/v2/client.rb b/app/domain/authentication/authn_oidc/v2/client.rb index 660daa7e3f..e15ab6d655 100644 --- a/app/domain/authentication/authn_oidc/v2/client.rb +++ b/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) } diff --git a/spec/app/domain/authentication/authn-oidc/v2/client_spec.rb b/spec/app/domain/authentication/authn-oidc/v2/client_spec.rb index aadf80ba94..22e8456f65 100644 --- a/spec/app/domain/authentication/authn-oidc/v2/client_spec.rb +++ b/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 From 2b65a73d67a3a5cf1bf2266ccc8dc16101027a74 Mon Sep 17 00:00:00 2001 From: John ODonnell Date: Thu, 12 Oct 2023 23:27:44 -0400 Subject: [PATCH 2/4] Fix OpenSSL X509 hash commands --- ci/test_suites/authenticators_oidc/test | 2 +- dev/start | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/test_suites/authenticators_oidc/test b/ci/test_suites/authenticators_oidc/test index e56bf8e909..079b6c8080 100755 --- a/ci/test_suites/authenticators_oidc/test +++ b/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 diff --git a/dev/start b/dev/start index 7d3ab88da2..d0817759ee 100755 --- a/dev/start +++ b/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 From 158b283e6d9bdf7a4ea89e4956adbd2dff5b49f9 Mon Sep 17 00:00:00 2001 From: John ODonnell Date: Thu, 12 Oct 2023 23:28:45 -0400 Subject: [PATCH 3/4] Cukes: remove Keycloak cert from Conjur container --- ci/shared.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/ci/shared.sh b/ci/shared.sh index 081fb0b4f6..6c7fac0fde 100644 --- a/ci/shared.sh +++ b/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\" \ From 1d079cdb538ebca4cb49fefebd17364026d5e06b Mon Sep 17 00:00:00 2001 From: John ODonnell Date: Fri, 13 Oct 2023 10:17:12 -0400 Subject: [PATCH 4/4] Fix CHANGELOG entries for 1.20.1 --- CHANGELOG.md | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c25fa7afc..03e3dd58aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. 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 @@ -41,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 @@ -58,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