AuthnOIDC V2: write custom certs to non-default directory #2988

john-odonnell · 2023-10-11T23:17:50Z

Desired Outcome

Authn-OIDC with ca-cert variable fails on Conjur Enterprise.

Errno::EACCES: Permission denied @ rb_file_s_symlink - (/tmp/d20231006-289-9u7rno/conjur-oidc-client.0.pem, /usr/lib/ssl/certs/bb3ed97b.0)

When Authn-OIDC V2 is configured with a ca-cert variable, it writes the provided certificate content into a temporary file, and creates a symlink to the file in the Conjur container's default certificate directory, meaning it would be trusted for outbound requests to the configured OIDC provider. After a successful request, the symlinks are deleted, and the temp files destroyed.

In Conjur Enterprise, the Conjur server is running as a user conjur, which lacks write permissions on the container's default cert store.

Implemented Changes

Instead of creating symlinks in the default cert store, instead create them in the Conjur project directory tree - the Conjur process is able to write to the ${conjur_root}/etc/certs in both Conjur Open Source and Enterprise.

In the previous solution iteration, if a custom certificate was established in the ca-cert variable but connecting to the OIDC provider did not require that particular certificate, Authn-OIDC could fall back on any trusted certificate in the default cert store. No longer - if the ca-cert variable is set, it must contain the full certificate chain required for connecting to the OIDC provider. No certificates included in the system's default store will be considered.

Connected Issue/Story

CyberArk internal issue ID: CNJR-2844

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a
CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code
changes, or
The changes in this PR do not require tests

Documentation

Docs (e.g. READMEs) were updated in this PR
A follow-up issue to update official docs has been filed here: [insert issue ID]
This PR does not require updating any documentation

Behavior

This PR changes product behavior and has been reviewed by a PO, or
These changes are part of a larger initiative that will be reviewed later, or
No behavior was changed with this PR

Security

Security architect has reviewed the changes in this PR,
These changes are part of a larger initiative with a separate security review, or
There are no security aspects to these changes

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

codeclimate · 2023-10-13T04:57:37Z

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

@@ -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


Authentication::AuthnOidc::V2::Client#callback_with_temporary_cert calls 'config.ssl' 2 times

codeclimate · 2023-10-13T04:57:37Z

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

@@ -226,6 +260,27 @@
              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


Authentication::AuthnOidc::V2::Client#self.discover calls 'config.ssl' 2 times

codeclimate · 2023-10-13T04:57:37Z

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

@@ -148,6 +161,27 @@
              symlink_a << symlink
            end

+            if OpenIDConnect.http_config.nil? || OpenIDConnect.http_client.ssl.ca_path != cert_dir


Authentication::AuthnOidc::V2::Client#callback_with_temporary_cert performs a nil-check

codeclimate · 2023-10-13T04:57:37Z

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

@@ -226,6 +260,27 @@
              symlink_a << symlink
            end

+            if OpenIDConnect.http_config.nil? || OpenIDConnect.http_client.ssl.ca_path != cert_dir


Authentication::AuthnOidc::V2::Client#self.discover performs a nil-check

codeclimate · 2023-10-13T04:57:37Z

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

+              # 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)


Replace class var :@@http_config with a class instance var.

codeclimate · 2023-10-13T15:42:49Z

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

+              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)


Replace class var :@@http_config with a class instance var.

codeclimate · 2023-10-13T15:42:49Z

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

+              # 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)


Replace class var :@@http_config with a class instance var.

codeclimate · 2023-10-13T15:42:49Z

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

+              # 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)


Replace class var :@@http_config with a class instance var.

codeclimate · 2023-10-13T15:42:49Z

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

+              # 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)


Replace class var :@@http_config with a class instance var.

codeclimate · 2023-10-13T15:42:49Z

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

+              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)


Replace class var :@@http_config with a class instance var.

codeclimate · 2023-10-13T15:42:52Z

Code Climate has analyzed commit 1d079cd and detected 16 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Style	9
Complexity	7

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 88.5% (0.0% change).

View more on Code Climate.

jvanderhoof · 2023-10-13T15:46:23Z

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

+          fileutils.mkdir_p(@default_cert_dir) unless dir.exist?(@default_cert_dir.to_s)
+
+          @default_cert_dir
+        end


Any particular reason we're using instance variables instead of variables scoped to the method?

Ah, I think I understand what you're trying to accomplish here. We only want to set the @default_cert_dir if it hasn't yet been set. This is in a singleton so you can pass it into the callback_with_temporary_cert method below.

jvanderhoof

I believe this code is good enough for now. I'll work with @jtuttle, @andytinkham, and @adamouamani, and the rest of the C&I team on a refactor to cleanup the implementation and improve testability of the code.

codeclimate bot reviewed Oct 11, 2023

View reviewed changes

john-odonnell force-pushed the cert-writing-bug branch 3 times, most recently from b2d2105 to d52c8f0 Compare October 13, 2023 04:16

codeclimate bot reviewed Oct 13, 2023

View reviewed changes

john-odonnell marked this pull request as ready for review October 13, 2023 13:43

john-odonnell requested a review from a team as a code owner October 13, 2023 13:43

john-odonnell added 3 commits October 13, 2023 10:07

AuthnOIDC V2: write custom certs to non-default directory

5ffed27

Fix OpenSSL X509 hash commands

2b65a73

Cukes: remove Keycloak cert from Conjur container

158b283

john-odonnell force-pushed the cert-writing-bug branch from d52c8f0 to 158b283 Compare October 13, 2023 14:07

Fix CHANGELOG entries for 1.20.1

1d079cd

codeclimate bot reviewed Oct 13, 2023

View reviewed changes

jvanderhoof reviewed Oct 13, 2023

View reviewed changes

jvanderhoof self-requested a review October 13, 2023 18:12

jvanderhoof approved these changes Oct 13, 2023

View reviewed changes

john-odonnell merged commit 40401f4 into master Oct 13, 2023
5 of 6 checks passed

john-odonnell deleted the cert-writing-bug branch October 13, 2023 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AuthnOIDC V2: write custom certs to non-default directory #2988

AuthnOIDC V2: write custom certs to non-default directory #2988

john-odonnell commented Oct 11, 2023 •

edited

Loading

codeclimate bot Oct 13, 2023

codeclimate bot Oct 13, 2023

codeclimate bot Oct 13, 2023

codeclimate bot Oct 13, 2023

codeclimate bot Oct 13, 2023

codeclimate bot Oct 13, 2023

codeclimate bot Oct 13, 2023

codeclimate bot Oct 13, 2023

codeclimate bot Oct 13, 2023

codeclimate bot Oct 13, 2023

codeclimate bot commented Oct 13, 2023

jvanderhoof Oct 13, 2023

jvanderhoof left a comment

AuthnOIDC V2: write custom certs to non-default directory #2988

AuthnOIDC V2: write custom certs to non-default directory #2988

Conversation

john-odonnell commented Oct 11, 2023 • edited Loading

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

codeclimate bot Oct 13, 2023

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2023

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2023

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2023

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2023

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2023

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2023

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2023

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2023

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2023

Choose a reason for hiding this comment

codeclimate bot commented Oct 13, 2023

jvanderhoof Oct 13, 2023

Choose a reason for hiding this comment

jvanderhoof left a comment

Choose a reason for hiding this comment

john-odonnell commented Oct 11, 2023 •

edited

Loading