Enabled authenticators can be configured using configuration file #2217

h-artzi · 2021-05-26T20:41:57Z

What does this PR do?

Add authenticators to Conjur Config

What ticket does this PR close?

Resolves #2173

Checklists

Change log

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, and/or there is a follow-on issue to update docs, or
This PR does not require updating any documentation

API Changes

The OpenAPI spec has been updated to meet new API changes (or an issue has been opened), or
The changes in this PR do not affect the Conjur API

codeclimate · 2021-05-26T20:43:39Z

CHANGELOG.md

@@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

 ## [Unreleased]
 ### Added
+- Enabled authenticators can now be configured via a configuration file, the 


Trailing spaces

We should clean this up.

codeclimate · 2021-05-26T20:43:39Z

CHANGELOG.md

@@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

 ## [Unreleased]
 ### Added
+- Enabled authenticators can now be configured via a configuration file, the 
+  CONJUR_AUTHENTICATORS environment variable, or the authenticator allowlist 


Trailing spaces

We should clean this up.

codeclimate · 2021-05-26T20:43:39Z

CHANGELOG.md

@@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

 ## [Unreleased]
 ### Added
+- Enabled authenticators can now be configured via a configuration file, the 


Lists should be surrounded by blank lines

micahlee

Looks good! I left a couple of small comments for you. Would you also mind updating the PR title to something descriptive?

Thanks!

lib/conjur/conjur_config.rb

micahlee · 2021-05-26T20:48:33Z

CHANGELOG.md

@@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

 ## [Unreleased]
 ### Added
+- Enabled authenticators can now be configured via a configuration file, the 


We should clean this up.

micahlee · 2021-05-26T20:48:41Z

CHANGELOG.md

@@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

 ## [Unreleased]
 ### Added
+- Enabled authenticators can now be configured via a configuration file, the 
+  CONJUR_AUTHENTICATORS environment variable, or the authenticator allowlist 


We should clean this up.

jtuttle · 2021-05-26T20:53:19Z

app/domain/authentication/installed_authenticators.rb

-      def env_enabled_authenticators(env)
-        authenticators = env["CONJUR_AUTHENTICATORS"]
-        authenticators.present? ? authenticators.split(',') : nil
+      def env_enabled_authenticators


I would call this configured_authenticators now.

configured_authenticators is already used

Hah, okay. In that case (and knowing that we're going to tear out the DB-configured authenticators sooni-ish) I would get rid of this helper method and just build the logic into the enabled_authenticators method.

jtuttle · 2021-05-26T20:54:39Z

app/domain/authentication/installed_authenticators.rb

-        authenticators = env["CONJUR_AUTHENTICATORS"]
-        authenticators.present? ? authenticators.split(',') : nil
+      def env_enabled_authenticators
+        authenticators = Conjur::ConjurConfig.new.authenticators.uniq


Please use Rails.application.config.conjur_config.authenticators within the application code. It gets loaded once when the app starts instead of loading configuration fresh each time ConjurConfig is instantiated.

jtuttle · 2021-05-26T20:55:11Z

lib/conjur/conjur_config.rb

@@ -32,7 +34,7 @@ class ConjurConfig < Anyway::Config

    # Get attribute sources without including attribute values
    def attribute_sources
-      to_source_trace.map { |k,v| [ k.to_sym, v[:source][:type] ] }.to_h
+      to_source_trace.map { |k, v| [ k.to_sym, v[:source][:type] ] }.to_h


Weird, I had it like this originally but CodeClimate complained.

jtuttle · 2021-05-26T20:57:57Z

spec/lib/conjur/conjur_config_spec.rb

-    expect(Conjur::ConjurConfig.new.attribute_sources[:trusted_proxies]).
-      to eq(:defaults)
+    expect(Conjur::ConjurConfig.new.attribute_sources[:trusted_proxies])
+      .to eq(:defaults)


I think we use trailing dots more often than leading so can we change these back?

@jtuttle Dissenting vote :)

Consider:

Resource .where(identifier.like("#{AUTHN_RESOURCE_PREFIX}%")) .where(kind => "webservice") .select_map(identifier) .map { |id| id[%r{^conjur/(authn(?:-[^/]+)?(?:/[^/]+)?)$}, 1] } # filter out nested status webservice .compact .push(::Authentication::Common.default_authenticator_name)

leading dot does two things:

Makes clear at a glance that we're dealing with a continuation (otherwise, if you don't notice the dot at the end of the previous line, it could be a new method call on self)

Makes cut and paste more ergonomic (otherwise the final item is a special case).

Yeah this is a whole thing (rubocop/ruby-style-guide#176). There's a reason the ruby style guide doesn't push one way or the other. 😂

I prefer having my continuation marker at the end of the line. The indentation of subsequent lines is enough of a signal to show that we're dealing with indentation.

Sure, but this doesn't happen often and isn't that big a deal when you need to fix it.

app/domain/authentication/installed_authenticators.rb

orenbm · 2021-05-27T05:41:07Z

CHANGELOG.md

+- Enabled authenticators can now be configured via a configuration file, the
+  CONJUR_AUTHENTICATORS environment variable, or the authenticator allowlist
+  API.
+  [cyberark/conjur##2173](https://github.com/cyberark/conjur/issues/#2173)


@h-artzi i'm probably missing something but i couldn't find in the code how authenticators can still be enabled by the CONJUR_AUTHENTICATORS environment variable. can you point me to it?

also - i am not sure that the authenticator allowlist API is GA so not sure that it should be in the changelog. @alexkalish wdyt?

The anyway_config gem takes care of the usage of the environment variable CONJUR_AUTHENTICATORS.

Yeah, I agree with Oren. Please drop the reference to the API from the message. It's not a GA feature. Otherwise, LGTM!

jvanderhoof · 2021-05-27T14:14:05Z

app/domain/authentication/installed_authenticators.rb

        # Enabling via environment overrides enabling via CLI
-        env_enabled_authenticators(env) || db_enabled_authenticators
+        authenticators = 
+          Rails.application.config.conjur_config.authenticators.uniq


What do you think about moving the uniq call to the authenticators method in the config? (We may need to create a new method that extends the parent class behaviour). To me, it feels out of place here to "fix-up" config.

We might also want to strip empty/nil items from the array.

jvanderhoof · 2021-05-27T14:33:00Z

app/domain/authentication/installed_authenticators.rb

-        env_enabled_authenticators(env) || db_enabled_authenticators
+        authenticators = 
+          Rails.application.config.conjur_config.authenticators.uniq
+        authenticators.empty? ? db_enabled_authenticators : authenticators


I rarely feel like the ternary operator reads well. What do you think about something like:

def enabled_authenticators Rails.application.config.conjur_config.authenticators.select { |e| !e.to_s.empty? } || db_enabled_authenticators end

I think I prefer the ternary in this case. Having a negated select that calls several methods on the elements it's selecting hurts my head more than the ternary does. 😄

Also note that we are hopefully going to tear out db_enabled_authenticators soon-ish so whatever we choose here is going to be a temporary inconvenience.

I personally think the ternary is clear here, but here's a non-ternary alternative which does the same thing:

authenticators = Rails.application.config.conjur_config.authenticators.uniq return authenticators if authenticators db_enabled_authenticators

jvanderhoof · 2021-05-27T14:39:35Z

lib/conjur/conjur_config.rb

@@ -63,5 +71,13 @@ def trusted_proxies_valid?
    rescue
      false
    end
+
+    def authenticators_valid?


What do you think about checking for a valid authenticator (authn-k8s, authn-ldap, authn-oidc, etc.) rather than just the form of a possible authenticator?

codeclimate · 2021-05-27T18:01:12Z

lib/conjur/conjur_config.rb

@@ -63,5 +71,18 @@ def trusted_proxies_valid?
    rescue
      false
    end
+
+    def authenticators_valid?
+      # TODO: Ideally we would check against the enabled authenticators


codeclimate · 2021-05-27T18:01:12Z

CHANGELOG.md

@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

 ## [Unreleased]
 ### Added
+- Enabled authenticators can now be configured via a configuration file, or the


Lists should be surrounded by blank lines

jtuttle · 2021-05-27T19:38:53Z

lib/conjur/conjur_config.rb

+      # in the DB. However, we need to figure out how to use code from the
+      # application without introducing warnings.
+      authenticators_regex = 
+        %r{^(authn|authn-(k8s|oidc|iam|ldap|gcp|azure|config)(/.+)?)$}


You can remove config from here now, right?

great catch!

There is a ticket that is currently filed in order to understand/address the current behavior of anyway_config gem. palkan/anyway_config#82

codeclimate · 2021-05-27T20:05:21Z

Code Climate has analyzed commit 1a00282 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Bug Risk	1
Style	1

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

This pull request will bring the total coverage in the repository to 89.2%.

View more on Code Climate.

h-artzi requested a review from a team as a code owner May 26, 2021 20:41

codeclimate bot reviewed May 26, 2021

View reviewed changes

micahlee reviewed May 26, 2021

View reviewed changes

h-artzi changed the title ~~2173 authenticators~~ Enabled authenticators can be configured using configuration file May 26, 2021

jtuttle suggested changes May 26, 2021

View reviewed changes

h-artzi force-pushed the 2173-authenticators branch 2 times, most recently from d5e5485 to 394e076 Compare May 26, 2021 21:52

h-artzi requested a review from jtuttle May 26, 2021 21:52

codeclimate bot reviewed May 26, 2021

View reviewed changes

app/domain/authentication/installed_authenticators.rb Outdated Show resolved Hide resolved

h-artzi force-pushed the 2173-authenticators branch from 394e076 to fb4d465 Compare May 27, 2021 01:15

orenbm reviewed May 27, 2021

View reviewed changes

h-artzi requested a review from micahlee May 27, 2021 13:18

h-artzi force-pushed the 2173-authenticators branch 2 times, most recently from 7e58c23 to 2a0b719 Compare May 27, 2021 13:52

jvanderhoof reviewed May 27, 2021

View reviewed changes

h-artzi force-pushed the 2173-authenticators branch 5 times, most recently from 6ca3e86 to b6e3840 Compare May 27, 2021 17:36

codeclimate bot reviewed May 27, 2021

View reviewed changes

h-artzi force-pushed the 2173-authenticators branch from b6e3840 to 92504f7 Compare May 27, 2021 18:56

jtuttle suggested changes May 27, 2021

View reviewed changes

h-artzi added 5 commits May 27, 2021 15:42

Add authenticators to Conjur config

8591a62

There is a ticket that is currently filed in order to understand/address the current behavior of anyway_config gem. palkan/anyway_config#82

Authenticators can be set via Conjur config file

118b024

Update CHANGELOG(#2173)

79b62e3

Add Conjur config tests for authenticators

d4b4bd3

Rename authenticator with valid name

1a00282

h-artzi force-pushed the 2173-authenticators branch from 92504f7 to 1a00282 Compare May 27, 2021 19:42

h-artzi requested a review from jtuttle May 27, 2021 19:42

jtuttle approved these changes May 27, 2021

View reviewed changes

h-artzi merged commit a48be72 into master May 27, 2021

h-artzi deleted the 2173-authenticators branch May 27, 2021 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabled authenticators can be configured using configuration file #2217

Enabled authenticators can be configured using configuration file #2217

h-artzi commented May 26, 2021

codeclimate bot May 26, 2021

micahlee May 26, 2021

codeclimate bot May 26, 2021

micahlee May 26, 2021

codeclimate bot May 26, 2021

micahlee left a comment

micahlee May 26, 2021

micahlee May 26, 2021

jtuttle May 26, 2021

h-artzi May 26, 2021

jtuttle May 26, 2021

jtuttle May 26, 2021

jtuttle May 26, 2021

jtuttle May 26, 2021

jonahx May 27, 2021 •

edited

Loading

jtuttle May 28, 2021 •

edited

Loading

orenbm May 27, 2021

h-artzi May 27, 2021

alexkalish May 27, 2021

jvanderhoof May 27, 2021

jvanderhoof May 27, 2021

jvanderhoof May 27, 2021

jtuttle May 27, 2021

jtuttle May 27, 2021

jonahx May 27, 2021

jvanderhoof May 27, 2021

codeclimate bot May 27, 2021

codeclimate bot May 27, 2021

jtuttle May 27, 2021

h-artzi May 27, 2021

codeclimate bot commented May 27, 2021

Enabled authenticators can be configured using configuration file #2217

Enabled authenticators can be configured using configuration file #2217

Conversation

h-artzi commented May 26, 2021

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

API Changes

codeclimate bot May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot May 26, 2021

Choose a reason for hiding this comment

micahlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahx May 27, 2021 • edited Loading

Choose a reason for hiding this comment

jtuttle May 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot May 27, 2021

Choose a reason for hiding this comment

codeclimate bot May 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot commented May 27, 2021

jonahx May 27, 2021 •

edited

Loading

jtuttle May 28, 2021 •

edited

Loading