Skip to content

Commit

Permalink
Merge pull request #2988 from cyberark/cert-writing-bug
Browse files Browse the repository at this point in the history
AuthnOIDC V2: write custom certs to non-default directory
  • Loading branch information
john-odonnell authored Oct 13, 2023
2 parents f5dabb6 + 1d079cd commit 40401f4
Show file tree
Hide file tree
Showing 6 changed files with 149 additions and 12 deletions.
21 changes: 16 additions & 5 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
63 changes: 59 additions & 4 deletions app/domain/authentication/authn_oidc/v2/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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) }
Expand Down Expand Up @@ -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) }
Expand All @@ -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
Expand All @@ -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
)
Expand Down Expand Up @@ -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) }
Expand Down
1 change: 0 additions & 1 deletion ci/shared.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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\" \
Expand Down
2 changes: 1 addition & 1 deletion ci/test_suites/authenticators_oidc/test
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion dev/start
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
72 changes: 72 additions & 0 deletions spec/app/domain/authentication/authn-oidc/v2/client_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 40401f4

Please sign in to comment.