Add annotation based user search #2565

mFelgate · 2022-06-01T19:12:21Z

Desired Outcome

Please describe the desired outcome for this PR. Said another way, what was
the original request that resulted in these code changes? Feel free to copy
this information from the connected issue.

Implemented Changes

Describe how the desired outcome above has been achieved with this PR. In
particular, consider:

What's changed? Why were these changes made?
How should the reviewer approach this PR, especially if manual tests are required?
Are there relevant screenshots you can add to the PR description?

Connected Issue/Story

Resolves #[relevant GitHub issue(s), e.g. 76]

CyberArk internal issue link: insert issue ID

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

micahlee · 2022-06-03T13:10:59Z

app/controllers/concerns/current_user.rb

+    annotation = Annotation.fetch_annotation(id: id.split(':')[-1].underscore.to_sym)
+    if annotation
+      id = annotation.resource_id
+    end


It is not at all obvious why annotations are involved here. Mind adding more comments through these code changes explaining what they're for?

micahlee · 2022-06-03T13:15:14Z

app/controllers/concerns/current_user.rb

+
+    Role[role_id: id] || raise(ApplicationController::Forbidden)
  end
 end


There is a VS Code setting that will make sure the final newline is present automatically. I would recommend configuring that so you don't have to think about these.

files.insertFinalNewline

More info here: https://stackoverflow.com/a/44704969

micahlee · 2022-06-03T13:28:25Z

app/models/annotation.rb

+    query =  "value = '#{id}'"
+    return query unless account && service_id
+
+    "#{query} and name = 'authn-oidc/#{service_id}' and resource_id LIKE  '%#{account}%'"


Seeing 'authn-oidc` in this file is a big red flag to me. The annotation model shouldn't know anything about OIDC. Particularly because the reference is in a string, this introduces a hidden dependency and coupling that is hard to discover and will almost certainly cause future pain.

I'm still trying to digest the changeset as a whole, but on this point in particular, I would expect to see something more like an OIDCAnnotation class or similar that wraps Annotation to provide the OIDC domain-specific methods.

This probably means that there need to be variants of a generic Role and an OIDCRole. This would cascade further to the find_current_user method to use the authenticator plugin interface to allow each plugin to attempt to resolve the current user, rather than hardcode a special case for OIDC.

Let me know what makes sense and what I should elaborate on further. In general, special cases are a code smell, and we should instead design our code to follow single, generic workflows with pluggable interfaces to supply domain specific logic that implement the generic flow in a specific way (e.g. finding the current user by OIDC username).

micahlee · 2022-06-06T18:26:14Z

app/domain/authentication/handler/authentication_handler.rb

+        annotation = Annotation.fetch_annotation(
+          account: authenticator.account,
+          value: identity,
+          name: authenticator.authenticator_name
+        ).first
+        return @role_repository_class.from_username(authenticator.account, identity) unless annotation
+
+        @role_repository_class[role_id: annotation.resource_id]


How come the default/base implementation for fetching a role is using annotations? I would expect this to be solely based on the ID, and I'm not sure why this change is necessary

I agree with the order of selection concern. Let's use the following for the resolution order:

Find user by ID - essentially, this requires users to be in the root

Find user by annotation (authn-oidc/identity) - allows users to exist outside the root policy, but runs the risk of two users with the same OIDC identity

Defaulting to User ID as the default lookup maintains a similar behavior as other authenticators, but still allows for annotation based lookup.

@andytinkham would love your thoughts on the find user by annotation scenario where multiple users have same annotation and how to handle this from a security best practice.

I was also wondering if a user could have multiple annotations - for example email and username and are we searching both??

micahlee · 2022-06-06T18:29:01Z

app/domain/authentication/handler/oidc_authentication_handler.rb

+        annotation = Annotation.fetch_annotation(account: account, value: id).first
+        return id unless annotation


This definitely needs a good comment describing what's going on here.

This is still giving precedence to annotations over role ID's, correct?

from what i understood that is the intention. i asked while talking about this feature with jason if we wanted to default to role_id, then use annotation if it didn't match and Jason said it was to be the other way around. This wasn't about a specific section of code tho but more of a high level discussion

Hey @jvanderhoof , can you give any additional context to why we would give precedence for annotation values over resource IDs when resolving OIDC identities?

The risk I'm considering is that this allows a host to set an annotation and impersonate an existing role with the host ID used for the mapping. Also, it's generally been an implied design principle that annotations are a weaker security control than IDs or variables. So taking an annotation as the higher precedent over the ID doesn't seem to jive with that either.

I'm probably missing some background or context that's important here, though.

CC: @andytinkham

@micahlee, as mentioned above, with annotations, I'm hoping to provide a path for getting users with email addresses (common OIDC pattern) mapped to Conjur users (where we use the "unfortunate" @ to delineate policy scope for a user - my.user@my-policy).

Annotations have emerged as a common pattern for attributing external ID to internal ID. I agree it's less secure. We should absolutely keep pushing on this to understand how best to manage to "Users in root" requirement and tie internal/external mapping.

jvanderhoof · 2022-06-07T20:41:48Z

app/models/annotation.rb

    end
  end

+  def self.fetch_annotation(account: nil, value:, name: nil)


@mFelgate, what's the scenario where an annotation doesn't have a value? This feels problematic for looking up a resources as it may find an annotation related to a resource which is unrelated to OIDC (or the target annotation).

jvanderhoof · 2022-06-07T20:45:57Z

app/models/authenticator/authenticator.rb

    end

+    def authenticator_name
+      return self.service_id


In Ruby the last line is returned, so the return is redundant here.

Alternatively, we can just add an alias:

module Authenticator class Authenticator alias authenticator_name, service_id ... end end

jvanderhoof · 2022-06-07T20:50:09Z

app/domain/authentication/handler/authentication_handler.rb


      def fetch_conjur_role(authenticator, identity)
-        return @role_repository_class.from_username(authenticator.account, identity)
+        annotation = Annotation.fetch_annotation(


Instead of including the Annotation dependency here, I'd recommend injecting it into the Authentication::Handler::AuthenticationHandler initialization method.

This will allow you to write tests for this handler without loading anything into the database.

jvanderhoof · 2022-06-07T22:38:00Z

app/domain/authentication/handler/authentication_handler.rb

@@ -130,7 +130,14 @@ def validate_client_ip(client_ip_address, conjur_role)
      end

      def fetch_conjur_role(authenticator, identity)


I think we should consider moving this into either a Role or a User repository. This allows us to decouple the process of finding the relevant user from the authentication process (which this class does).

We could do something like this (warning pseudocode) :

module DB module Repository class RoleRepository def initialize(role: ::Role, annotation: ::Annotation) @role = role @annotation = annotation end def find(account:, identifier:, annotation: nil, type: 'user') role = find_by_id(account: account, identifier: identifier, type: type) if role.blank? && annotation.present? role = find_by_annotation(account: account, identifier: identifier, type: type, annotation: annotation) end role end # The methods below are public to simplify testing, but only `find` should be part of the primary interface. def find_by_id(account:, identifier:, type: 'user') @role[[account, type, identifier].join(':')] end def find_by_annotation(account:, identifier:, annotation:, type: 'user') resource_id = @annotation .where(name: annotation) # exact match on annotation name .ilike(value: identifier) # case insensitive match on annotation value .like(resource_id: "#{account}:#{type}:%") # verify account and type .first .try(:resource_id) return nil unless resource_id @role[resource_id] end end end end

Alternatively, we can extend Annotation to include a method to handle finding a resource given an annotation name and value. In this case, the Annotation method should return the resource_id to align with the Role#roleid_from_username convention.

@jvanderhoof, do you want a role repo or would this be fine to just add to the role model?

The problem is Annotation. Adding find_by_annotation to Role feels a bit weird as it forces Annotation as a dependency on Role.

It might make more sense to add a find_by_key_and_value type method to Annotation.

in that case should it be more like the original, but just have role_id be the default

jvanderhoof · 2022-06-07T22:42:46Z

cucumber/authenticators_oidc/features/authn_oidc.feature

+    - !user
+      id: alice
+      annotations:
+       authn-oidc/keycloak: alice.somebody@cyberark.com


I was thinking about the annotation more along the lines of:

- !user id: alice annotations: authn-oidc/identity: alice.somebody@cyberark.com

which would allow the user to be selected given a JWT with the subject alice.somebody@cyberark.com. This lines up with the way annotations are used for mapping in other authenticators: Azure (checkout the "Define Azure Authenticator policy" policy sample).

adamouamani

I have a few questions:

can this annotation be anything (ie: not just email) so long as it matches user defined in JWT token
what happens if duplicate annotations for different users - if these are not unique this could be security concern
can hosts have this annotation?

micahlee · 2022-06-16T13:48:18Z

spec/app/db/repository/role_repository.spec.rb

@@ -0,0 +1,74 @@
+# frozen_string_literal: true


Nice work on these specs, @mFelgate!

I read back through https://www.betterspecs.org/ and https://martinfowler.com/articles/mocksArentStubs.html and I think what you've done is a perfectly reasonably approach:

You've mocked out the RoleRepository dependencies

Those get passed to a real RoleRepository object

None of the internals of RoleRepository are mocked

micahlee · 2022-06-16T13:48:57Z

spec/app/db/repository/role_repository.spec.rb

+    )
+  end
+
+  describe('.find') do


The only nit from reading better spec again is that this should follow the Ruby conventions for referring to a method:

Suggested change

describe('.find') do

describe('#find') do

…pository

…earer token)

mFelgate requested a review from a team as a code owner June 1, 2022 19:12

micahlee reviewed Jun 3, 2022

View reviewed changes

mFelgate force-pushed the Search-user-by-annotation branch 4 times, most recently from 6e3f27d to e332389 Compare June 3, 2022 21:01

micahlee reviewed Jun 6, 2022

View reviewed changes

jvanderhoof reviewed Jun 7, 2022

View reviewed changes

mFelgate force-pushed the feature/ONYX-17902 branch from 11006b6 to 0ce2084 Compare June 8, 2022 13:36

mFelgate force-pushed the Search-user-by-annotation branch 8 times, most recently from e1e1dc7 to f028d38 Compare June 10, 2022 17:13

mFelgate force-pushed the Search-user-by-annotation branch from 6ebb0ec to dcaa5b4 Compare June 15, 2022 18:32

adamouamani reviewed Jun 16, 2022

View reviewed changes

mFelgate force-pushed the feature/ONYX-17902 branch from d210521 to 9536b8e Compare June 16, 2022 13:01

micahlee reviewed Jun 16, 2022

View reviewed changes

mFelgate force-pushed the Search-user-by-annotation branch from dcaa5b4 to 7f0f7f1 Compare June 16, 2022 14:46

micahlee mentioned this pull request Jun 16, 2022

Remove redhat-rpm-config package from UBI image #2577

Merged

13 tasks

mFelgate force-pushed the feature/ONYX-17902 branch from bceeb65 to 16f9d72 Compare June 16, 2022 17:03

mFelgate force-pushed the Search-user-by-annotation branch from 7f0f7f1 to d57f23d Compare June 16, 2022 17:12

mFelgate force-pushed the Search-user-by-annotation branch from d57f23d to 17aaf89 Compare June 16, 2022 17:21

micahlee mentioned this pull request Jun 23, 2022

Allow k8s test to be run locally and clean up test issues #2582

Merged

13 tasks

mFelgate and others added 21 commits June 30, 2022 16:49

Add annotation based user search

6a37612

initial effort to refactor oidc

e33971d

move functionality into appropriate classes

beeef4d

beginning of a handler class

abc4d15

add cache expiration to strategy

f161c85

Add handler to the refactor

619b4da

add handler to the refactor

adaeccf

Small refactor to allow the desired DataObject to be passed to the re…

c2c5630

…pository

Use the data object as defined by the authenticator

3823d98

Remove some block commments

d9851c7

Refactor to support dynamic authentication provider endpoints

3e4ede6

Fix paths to handler

f1538c1

Optionally use access token (which is what Keycloak provides in its b…

3d0a633

…earer token)

adds Keycloak as a valid authenticator

5465806

Fix the failing cucumber tests

b8d22df

Update origonal cucumber and rspec test

1e7f8bd

Adds rspec tests for namespace selector

57df77e

Add unit test for provider_context

36bf815

Return error for multiple roles having same annotation

ac65d7e

Add unit tests for ResolveIdentity and Strategy

2eaaf37

Add status test fix

97eb2f9

mFelgate force-pushed the Search-user-by-annotation branch from 6369cf1 to 97eb2f9 Compare July 7, 2022 16:12

		annotation = Annotation.fetch_annotation(account: account, value: id).first
		return id unless annotation

		@@ -130,7 +130,14 @@ def validate_client_ip(client_ip_address, conjur_role)
		end

		def fetch_conjur_role(authenticator, identity)

Add annotation based user search #2565

Are you sure you want to change the base?

Add annotation based user search #2565

Uh oh!

Conversation

mFelgate commented Jun 1, 2022

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

Uh oh!

micahlee Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mFelgate Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micahlee Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mFelgate Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvanderhoof Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamouamani left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

micahlee Jun 3, 2022 •

edited

Loading

mFelgate Jun 7, 2022 •

edited

Loading

micahlee Jun 7, 2022 •

edited

Loading

mFelgate Jun 8, 2022 •

edited

Loading

jvanderhoof Jun 7, 2022 •

edited

Loading

adamouamani left a comment •

edited

Loading