if host does not exist raise RoleNotFound #2018

mbenita-Cyberark · 2021-02-01T15:16:19Z

What does this PR do?

in k8s authenticator, raise RoleNotFound Exception if host does not exist

What ticket does this PR close?

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

codeclimate · 2021-02-01T15:39:06Z

app/domain/authentication/authn_k8s/inject_client_cert.rb

+
+        @host = @resource_class[host_id]
+        unless @host
+          raise Errors::Authentication::Security::RoleNotFound.new(


Provide an exception class and message as arguments to raise.

izgeri

If we're changing the functional behavior of the server, we should definitely have a CHANGELOG message for this change. You can see our guidelines for writing good changelog messages here. We try to write the changelog messages so they speak to the user-facing impact of the change - basically, try to explain, "If I am the user, why do I care?"

You should probably also try to write a simple test case for this change - something that would have failed before you implemented it, but would be passing now. The PR checklist is a good guide for the things to consider when submitting PRs.

Note: the PR will also need a rebase.

andytinkham · 2021-02-19T20:28:48Z

I'm not sure which file changed to require quality-architects as codeowner here, but my concerns are the same as those @izgeri brought up. I have no further changes to suggest..

orenbm · 2021-02-21T14:26:31Z

CHANGELOG.md

@@ -5,6 +5,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

 ## [Unreleased]
+### Changed
+- Conjur now Throw RoleNotFound error in k8s authenticator 


Conjur now raises a RoleNotFound when trying to authenticate a non-existing host in authn-k8s

codeclimate · 2021-02-21T15:00:20Z

app/domain/authentication/authn_k8s/inject_client_cert.rb

@@ -170,16 +176,17 @@ def audit_success
      end

      def audit_failure(err)
-        @audit_log.log(
-          Audit::Event::Authn::InjectClientCert.new(
+


Extra empty line detected at method body beginning.

codeclimate · 2021-02-21T15:00:20Z

app/domain/authentication/authn_k8s/inject_client_cert.rb

            client_ip:          @client_ip,
            success:            false,
            error_message:      err.message
          )
        )
+


Extra empty line detected at method body end.

codeclimate · 2021-02-21T15:00:20Z

app/domain/authentication/authn_k8s/inject_client_cert.rb

            authenticator_name: AUTHENTICATOR_NAME,
            service:            webservice,
-            role_id:            host.id,
+            role_id:            err.class == Errors::Authentication::Security::RoleNotFound ? "" : host.id,


Align the keys of a hash literal if they span more than one line.

codeclimate · 2021-02-21T15:00:20Z

app/domain/authentication/authn_k8s/inject_client_cert.rb

            authenticator_name: AUTHENTICATOR_NAME,
            service:            webservice,
-            role_id:            host.id,
+            role_id:            err.class == Errors::Authentication::Security::RoleNotFound ? "" : host.id,


Use instance_of?(Errors::Authentication::Security::RoleNotFound) instead of comparing classes.

codeclimate · 2021-02-21T15:53:52Z

app/domain/authentication/authn_k8s/inject_client_cert.rb

+        @audit_log.log(Audit::Event::Authn::InjectClientCert.new(
+          authenticator_name: AUTHENTICATOR_NAME,
+          service:            webservice,
+          role_id:            err.instance_of?(Errors::Authentication::Security::RoleNotFound) ? "" : host.id,


Align the keys of a hash literal if they span more than one line.

orenbm · 2021-02-21T16:24:01Z

CHANGELOG.md

@@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

 ## [Unreleased]
+### Changed
+- Conjur now raises a RoleNotFound when trying to authenticate a non-existing host in authn-k8s


mnissing link to issue

codeclimate · 2021-02-21T17:16:04Z

app/domain/authentication/authn_k8s/inject_client_cert.rb

+      # Masking role if it doesn't exist to avoid audit pollution
+      # Checking @success as well to save DB call on success
+      def sanitized_role_id
+        return "not-found" unless @resource_class[host_id]


Add empty line after guard clause.

codeclimate · 2021-02-21T17:16:04Z

app/domain/authentication/authn_k8s/inject_client_cert.rb

@@ -161,7 +167,7 @@ def audit_success
          Audit::Event::Authn::InjectClientCert.new(
            authenticator_name: AUTHENTICATOR_NAME,
            service:            webservice,
-            role_id:            host.id,
+            role_id:            sanitized_role_id,


Align the keys of a hash literal if they span more than one line.

codeclimate · 2021-02-21T17:16:04Z

app/domain/authentication/authn_k8s/inject_client_cert.rb

@@ -174,13 +180,22 @@ def audit_failure(err)
          Audit::Event::Authn::InjectClientCert.new(
            authenticator_name: AUTHENTICATOR_NAME,
            service:            webservice,
-            role_id:            host.id,
+            role_id:            sanitized_role_id,


Align the keys of a hash literal if they span more than one line.

orenbm · 2021-02-22T08:44:20Z

app/domain/authentication/authn_k8s/inject_client_cert.rb

+      # TODO: this logic is taken from app/models/audit/event/authn.rb.
+      # We should use that logic here.
+      # Masking role if it doesn't exist to avoid audit pollution
+      # Checking @success as well to save DB call on success


you actually don't check @success here. Please fix comment.

- Added RoleNotFound error message in case host not exist for hostId - Fixed bug when creating InjectClientCert inside log write command - Added UT

orenbm · 2021-02-22T09:11:55Z

CHANGELOG.md

@@ -5,6 +5,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

 ## [Unreleased]
+### Changed
+- Conjur now raises a RoleNotFound when trying to authenticate a non-existing host in authn-k8s
+  [conjurinc/appliance#1546]https://github.com/conjurinc/appliance/issues/1546


this should be [conjurinc/appliance#1546](https://github.com/conjurinc/appliance/issues/1546)` so it is linkable in a MD file

this is not the correct issue. The issue you are fixing is that we now raise a NilError instead of an informative one. please add an issue if it's not present

why are we linking to an issue filed in a private repo?

nm - looks like this was updated later, and the changelog is current now.

- Added RoleNotFound error message in case host not exist for hostId - Fixed bug when creating InjectClientCert inside log write command - Added UT

codeclimate · 2021-02-22T09:53:25Z

Code Climate has analyzed commit 27193bb and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Style	2

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 89.3% (0.0% change).

View more on Code Climate.

I implemented requested UTs and Changelog with Oren's PR approval

alexkalish · 2021-03-24T19:03:35Z

This is connected to #2046, yes? Please be sure to link PRs to issues, as it helps trace changes when building a release. Thank you!

alexkalish · 2021-03-24T19:04:59Z

@mbenita-Cyberark @orenbm: Does this have user facing behavior change? How will users be affected? Thanks.

orenbm · 2021-03-25T08:02:25Z

@mbenita-Cyberark @orenbm: Does this have user facing behavior change? How will users be affected? Thanks.

the change is written in the CHANGELOG: Conjur now raises a RoleNotFound when trying to authenticate a non-existing host in authn-k8s

alexkalish · 2021-03-25T13:59:45Z

@orenbm: Yup, I saw that entry. However, customers typically interact with authn-k8s through the client, not the API directly. How would this change manifest in the client?

orenbm · 2021-03-25T14:29:19Z

it won't. they will see it if they have a failure and will check the logs to find it. do instead of seeing an unhelpful nil pointer message they will see the new message.

mbenita-Cyberark requested a review from orenbm February 1, 2021 15:16

mbenita-Cyberark requested a review from a team as a code owner February 1, 2021 15:16

codeclimate bot reviewed Feb 1, 2021

View reviewed changes

jonahx previously approved these changes Feb 1, 2021

View reviewed changes

izgeri previously requested changes Feb 1, 2021

View reviewed changes

mbenita-Cyberark dismissed jonahx’s stale review via 02e874a February 14, 2021 10:01

mbenita-Cyberark requested a review from a team as a code owner February 14, 2021 10:35

mbenita-Cyberark force-pushed the k8s_hostId_not_exist_error_log_improvement branch from 6e0e956 to 020b26d Compare February 14, 2021 11:39

orenbm reviewed Feb 21, 2021

View reviewed changes

codeclimate bot reviewed Feb 21, 2021

View reviewed changes

mbenita-Cyberark force-pushed the k8s_hostId_not_exist_error_log_improvement branch 3 times, most recently from bf82190 to 4428823 Compare February 21, 2021 15:30

codeclimate bot reviewed Feb 21, 2021

View reviewed changes

mbenita-Cyberark force-pushed the k8s_hostId_not_exist_error_log_improvement branch from 4428823 to cdbbfaa Compare February 21, 2021 15:54

orenbm reviewed Feb 21, 2021

View reviewed changes

mbenita-Cyberark force-pushed the k8s_hostId_not_exist_error_log_improvement branch from cdbbfaa to d5ad4de Compare February 21, 2021 16:52

codeclimate bot reviewed Feb 21, 2021

View reviewed changes

orenbm reviewed Feb 22, 2021

View reviewed changes

Raise proper error in case k8s host not found

4359d63

- Added RoleNotFound error message in case host not exist for hostId - Fixed bug when creating InjectClientCert inside log write command - Added UT

mbenita-Cyberark force-pushed the k8s_hostId_not_exist_error_log_improvement branch from d5ad4de to 4359d63 Compare February 22, 2021 08:56

orenbm reviewed Feb 22, 2021

View reviewed changes

Raise proper error in case k8s host not found

27193bb

- Added RoleNotFound error message in case host not exist for hostId - Fixed bug when creating InjectClientCert inside log write command - Added UT

mbenita-Cyberark force-pushed the k8s_hostId_not_exist_error_log_improvement branch from 4b966db to 27193bb Compare February 22, 2021 09:29

orenbm approved these changes Feb 22, 2021

View reviewed changes

mbenita-Cyberark merged commit 9f06ebd into master Feb 22, 2021

mbenita-Cyberark deleted the k8s_hostId_not_exist_error_log_improvement branch February 22, 2021 10:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

if host does not exist raise RoleNotFound #2018

if host does not exist raise RoleNotFound #2018

mbenita-Cyberark commented Feb 1, 2021 •

edited

Loading

codeclimate bot Feb 1, 2021

izgeri left a comment •

edited

Loading

andytinkham commented Feb 19, 2021

orenbm Feb 21, 2021

codeclimate bot Feb 21, 2021

codeclimate bot Feb 21, 2021

codeclimate bot Feb 21, 2021

codeclimate bot Feb 21, 2021

codeclimate bot Feb 21, 2021

orenbm Feb 21, 2021

codeclimate bot Feb 21, 2021

codeclimate bot Feb 21, 2021

codeclimate bot Feb 21, 2021

orenbm Feb 22, 2021 •

edited

Loading

orenbm Feb 22, 2021

orenbm Feb 22, 2021

izgeri Feb 26, 2021

izgeri Feb 26, 2021

codeclimate bot commented Feb 22, 2021

alexkalish commented Mar 24, 2021

alexkalish commented Mar 24, 2021

orenbm commented Mar 25, 2021

alexkalish commented Mar 25, 2021

orenbm commented Mar 25, 2021

if host does not exist raise RoleNotFound #2018

if host does not exist raise RoleNotFound #2018

Conversation

mbenita-Cyberark commented Feb 1, 2021 • edited Loading

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

codeclimate bot Feb 1, 2021

Choose a reason for hiding this comment

izgeri left a comment • edited Loading

Choose a reason for hiding this comment

andytinkham commented Feb 19, 2021

orenbm Feb 21, 2021

Choose a reason for hiding this comment

codeclimate bot Feb 21, 2021

Choose a reason for hiding this comment

codeclimate bot Feb 21, 2021

Choose a reason for hiding this comment

codeclimate bot Feb 21, 2021

Choose a reason for hiding this comment

codeclimate bot Feb 21, 2021

Choose a reason for hiding this comment

codeclimate bot Feb 21, 2021

Choose a reason for hiding this comment

orenbm Feb 21, 2021

Choose a reason for hiding this comment

codeclimate bot Feb 21, 2021

Choose a reason for hiding this comment

codeclimate bot Feb 21, 2021

Choose a reason for hiding this comment

codeclimate bot Feb 21, 2021

Choose a reason for hiding this comment

orenbm Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

orenbm Feb 22, 2021

Choose a reason for hiding this comment

orenbm Feb 22, 2021

Choose a reason for hiding this comment

izgeri Feb 26, 2021

Choose a reason for hiding this comment

izgeri Feb 26, 2021

Choose a reason for hiding this comment

codeclimate bot commented Feb 22, 2021

alexkalish commented Mar 24, 2021

alexkalish commented Mar 24, 2021

orenbm commented Mar 25, 2021

alexkalish commented Mar 25, 2021

orenbm commented Mar 25, 2021

mbenita-Cyberark commented Feb 1, 2021 •

edited

Loading

izgeri left a comment •

edited

Loading

orenbm Feb 22, 2021 •

edited

Loading