Refactor authn-k8s-client to be authentication flow generic #425

tzheleznyak · 2021-12-15T12:13:18Z

Desired Outcome

As part of the authn-jwt side car in K8S this is first stage where we do refactor, that will allow adding more authentication methods.

Defining interface for Config And Authenticator and factories creating instances of them.

Folder structure change

Connected Issue/Story

ONYX-14719

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

pkg/log/log_messages.go

nessiLahav

LGTM

pkg/log/log_messages.go

shulifink · 2021-12-20T16:15:33Z

pkg/log/log_messages.go

@@ -75,3 +75,5 @@ const CAKC059 string = "CAKC059 Path exists but does not contain regular file: %
 const CAKC060 string = "CAKC060 Setting %s given invalid value %s"
 const CAKC061 string = "CAKC061 Failed to validate setting for Authenticator configuration"
 const CAKC062 string = "CAKC062 Required Authenticator setting %s not provided"
+const CAKC063 string = "CAKC063 Can't find configuration for URL : %s"
+const CAKC064 string = "CAKC064 Can't find Authenticator for this configuration"


@tzheleznyak - What is the use-case for this?

We are going to add support to authn-jwt to authn-k8s-client.
And this is the error message when the authn-client can't decide which authentication flow should run (authn-k8s or authn-jwt)

How about "No authenticator configured. "

or even better: "No authentication configured. "

diverdane · 2021-12-21T05:34:06Z

@tzheleznyak,

Thanks for taking on this challenge! I think the interfaces that you've defined are a good start at making this client more pluggable. I'd like to make some suggestions that might simplify things a little bit. This will take a bit of rearranging, so we can chat about this tomorrow, and maybe pull some other folks in to get some consensus (Kumbi and Johah might be good to include).

First, some high-level comments:

With the proposed directory structure, there seems to be a "mix of concerns" in the common
and creators directory. For example, each of these directories contains code that does
configuration work as well as code that does authentication work. Another way of looking at it,
the configuration code is split out between the common and creators directories. (I'm
thinking that this mix of concerns might have caused the cyclic dependency references???)
I think it might be cleaner to organize things similar to the original directory structure:
```
-pkg/
   - authenticator/     # Common authentication code
      - config/               # Common config code
      - k8s/                   # "Concrete" instantiations of config/authen interfaces for K8s
      - jwt/                     # "Concrete" instantiations of config/authen interfaces for JWT
```
I see that for each authenticator type (authn-k8s or authn-jwt), you're defining a config structure and an
authenticator structure, with each structure satisfying the ConfigurationInterface and the
AuthenticatorInterface interfaces respectively. I'm thinking that it might simplify things to have one combined
"Authenticator" structure for each authenticator type. That structure can satisfy both
ConfigurationInterface and AuthenticatorInterface interfaces. Going even further, we could
potentially combine both interfaces into a single "Authenticator" interface, where each concrete
realization of that interface knows e.g. what config variables are required, what defaults to use for
config variables, and how to initialize and authenticate with Conjur. This would essentially combine
everything we need to know about an authenticator. Once we create a structure that satisfies
the single, "Authenticator" interface, then it's pretty easy and clean to pass it around to do
configuration and authentication actions. We wouldn't have to do type switches/assertions like the following:
```
var cfg = (*config).(*Config)
```
I think there are some methods that can be dropped from the interface definitions, so that a
combined interface might look like this:
```
type Authenticator interface {                                         
  LoadConfig(settings map[string]string)                                                                                 
  GetEnvVariables() []string                                                  
  GetRequiredVariables() []string                                             
  GetDefaultValues() map[string]string                                        
  Init(config *ConfigurationInterface) (AuthenticatorInterface, error)                                                                                                                    
  Authenticate() error                                                                                          
```

}

pkg/authenticator/common/configuration_interface.go

pkg/authenticator/creators/authenticator_factory.go

pkg/authenticator/creators/configuration_factory.go

tzheleznyak · 2021-12-21T09:48:46Z

@diverdane
Thanks for the detailed feedback

About 1:

During development i came across dependency loop when i tried putting interfaces and factories in the same package. It what made me split factories to one separate and the interfaces to the common package.

The common package is all code relevant to the main authenticator package that don't uses other packages inside authenticator and used by multiple packages inside authenticator.

The name creators can be discussed and i can remove this directory and move the factories one step back.

About 2:
I don't think we need to combine to one entity the ConfigInterface and AuthenticatorInterface. They have separate resnponsibilits . Config responsible for:

Collecting Config Data - Currently from env but it future may be from future sources like DB/network and authenticator shouldn't care where it comes from
Validating the config data
Storing the config data

These are not responsibilits of the authenticator that should only support two things:

Authenticate
Providing access token

doodlesbykumbi

Some thoughts

doodlesbykumbi · 2021-12-22T14:25:24Z

pkg/log/log_messages.go

@@ -75,3 +75,5 @@ const CAKC059 string = "CAKC059 Path exists but does not contain regular file: %
 const CAKC060 string = "CAKC060 Setting %s given invalid value %s"
 const CAKC061 string = "CAKC061 Failed to validate setting for Authenticator configuration"
 const CAKC062 string = "CAKC062 Required Authenticator setting %s not provided"
+const CAKC063 string = "CAKC063 Can't find configuration for URL: %s"


In other repos we have been standardising on Unable. So these would all become Unable to ...

doodlesbykumbi · 2021-12-22T14:28:03Z

pkg/authenticator/k8s/config.go

+func (config *Config) GetAuthenticationType() string {
+	return AuthnType
+}
+
+func (config *Config) GetEnvVariables() []string {
+	return envVariables
+}
+
+func (config *Config) GetRequiredVariables() []string {
+	return requiredEnvVariables
+}
+
+func (config *Config) GetDefaultValues() map[string]string {
+	return defaultValues
+}


These methods are a bit strange because they don't make use of the struct at all, and so could be static.

But the usage inside Validate() and GatherSettings()
Is config.GetDefaultValues() so it will go to the config of the correct flow

doodlesbykumbi · 2021-12-22T14:29:43Z

pkg/authenticator/k8s/authenticator.go

 // New creates a new authenticator instance from a token file
-func New(config authnConfig.Config) (*Authenticator, error) {
-	accessToken, err := file.NewAccessToken(config.TokenFilePath)
+func New(config Config) (*Authenticator, error) {


I think it would be good for us to update this to NewAuthenticator to make it explicit what this method is creating.

doodlesbykumbi · 2021-12-22T14:30:54Z

pkg/authenticator/k8s/authenticator.go

+func (auth *Authenticator) Init(config *common.ConfigurationInterface) (common.AuthenticatorInterface, error) {
+	var cfg = (*config).(*Config)
+	authn, err := New(*cfg)
+	if err != nil {
+		return nil, fmt.Errorf(log.CAKC019)
+	}
+
+	return authn, nil
+}
+
+func (auth *Authenticator) InitWithAccessToken(config *common.ConfigurationInterface, token access_token.AccessToken) (common.AuthenticatorInterface, error) {


I'm not sure about the name of the method. I think we need something other than Init that better captures what this method is doing.

This should also probably be a free standing function (not a method) since it doesn't use the auth struct at all.

Lets discuss in the meeting

diverdane

This looks FANTASTIC!!! Thanks again for your patience on this! I really like how you broke out things into config and common.
I just have a few very minor editorial nits, otherwise this looks good-to-go.

diverdane · 2021-12-22T21:43:38Z

CONTRIBUTING.md

@@ -151,7 +151,7 @@ follow the instructions in this section.
 ### Update the version, changelog, and notices
 1. Create a new branch for the version bump.
 1. Based on the unreleased content, determine the new version number and update
-   the [version.go](pkg/authenticator/version.go) file.
+   the [version.go](pkg/authenticator/common/version.go) file.


Looks like this moved? Should be pkg/authenticator/version.go?

diverdane · 2021-12-22T21:48:54Z

pkg/authenticator/authenticator_factory.go

+	"github.com/cyberark/conjur-authn-k8s-client/pkg/access_token"
+	"github.com/cyberark/conjur-authn-k8s-client/pkg/access_token/file"
+	"github.com/cyberark/conjur-authn-k8s-client/pkg/authenticator/config"
+	k8sAuthenitcator "github.com/cyberark/conjur-authn-k8s-client/pkg/authenticator/k8s"


Oops. Typo, should be k8sAuthenticator.

diverdane · 2021-12-22T22:32:50Z

pkg/authenticator/authenticator_factory.go

+	"github.com/cyberark/conjur-authn-k8s-client/pkg/log"
+)
+
+func NewAuthenticator(conf config.Configuration) (Authenticator, error) {


I don't want to hold up the merge on this, but Go style guides (e.g. https://go.dev/doc/effective_go#commentary) recommend that any exported names should have a preceding doc comment that begins with that name, e.g.

// NewAuthenticator creates an instance of the Authenticator interface based on configured authenticator type.

(My IDE is running golint automatically, so when I write a file I see warnings pop up. BTW golint is deprecated, but there are replacements, e.g. golangci-lint.)

diverdane

LGTM!!!

nessiLahav

LGTM

alexkalish · 2022-02-14T15:45:59Z

Internal issue: ONYX-14722.

tzheleznyak requested review from szh, doodlesbykumbi, abrahamko and a team December 15, 2021 12:13

tzheleznyak force-pushed the refactor-generic-configuration branch 2 times, most recently from 8ad3b37 to 3dc627a Compare December 16, 2021 08:40

sashaCher reviewed Dec 16, 2021

View reviewed changes

pkg/log/log_messages.go Outdated Show resolved Hide resolved

tzheleznyak force-pushed the refactor-generic-configuration branch 4 times, most recently from 73d3808 to 469d7d2 Compare December 19, 2021 15:38

nessiLahav previously approved these changes Dec 19, 2021

View reviewed changes

pkg/log/log_messages.go Outdated Show resolved Hide resolved

tzheleznyak dismissed nessiLahav’s stale review via 1bba60a December 19, 2021 16:02

tzheleznyak force-pushed the refactor-generic-configuration branch 2 times, most recently from 1bba60a to 4644da9 Compare December 20, 2021 09:21

tzheleznyak commented Dec 20, 2021

View reviewed changes

pkg/log/log_messages.go Outdated Show resolved Hide resolved

shulifink suggested changes Dec 20, 2021

View reviewed changes

diverdane reviewed Dec 21, 2021

View reviewed changes

tzheleznyak force-pushed the refactor-generic-configuration branch from 4644da9 to c5ec2e1 Compare December 21, 2021 10:20

tzheleznyak marked this pull request as ready for review December 21, 2021 11:39

tzheleznyak requested review from a team as code owners December 21, 2021 11:39

tzheleznyak force-pushed the refactor-generic-configuration branch 2 times, most recently from aa10a51 to 37b32ed Compare December 22, 2021 10:29

doodlesbykumbi reviewed Dec 22, 2021

View reviewed changes

tzheleznyak force-pushed the refactor-generic-configuration branch 3 times, most recently from 6745595 to 2b9a857 Compare December 22, 2021 17:43

tzheleznyak force-pushed the refactor-generic-configuration branch from 2b9a857 to 05b78aa Compare December 22, 2021 18:21

diverdane reviewed Dec 22, 2021

View reviewed changes

Refactor authn-k8s-client to be authentication flow generic

320a410

tzheleznyak force-pushed the refactor-generic-configuration branch from 05b78aa to 320a410 Compare December 23, 2021 08:25

diverdane approved these changes Dec 23, 2021

View reviewed changes

nessiLahav approved these changes Dec 23, 2021

View reviewed changes

tzheleznyak merged commit b5ff748 into master Dec 23, 2021

tzheleznyak deleted the refactor-generic-configuration branch December 23, 2021 13:18

szh mentioned this pull request Jan 10, 2022

Bump to version 0.23.0 #427

Merged

john-odonnell mentioned this pull request Jul 8, 2022

Update suite.yml for v1.17.6+suite.1 release cyberark/conjur-oss-suite-release#247

Merged

gl-johnson mentioned this pull request Aug 23, 2022

Update suite.yml for v18.0.0+suite1 release cyberark/conjur-oss-suite-release#250

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor authn-k8s-client to be authentication flow generic #425

Refactor authn-k8s-client to be authentication flow generic #425

tzheleznyak commented Dec 15, 2021 •

edited

Loading

nessiLahav left a comment

shulifink Dec 20, 2021

tzheleznyak Dec 21, 2021

shulifink Dec 21, 2021

shulifink Dec 21, 2021

diverdane commented Dec 21, 2021

tzheleznyak commented Dec 21, 2021 •

edited

Loading

doodlesbykumbi left a comment

doodlesbykumbi Dec 22, 2021

doodlesbykumbi Dec 22, 2021

tzheleznyak Dec 22, 2021

doodlesbykumbi Dec 22, 2021

tzheleznyak Dec 22, 2021

doodlesbykumbi Dec 22, 2021

tzheleznyak Dec 22, 2021

diverdane left a comment

diverdane Dec 22, 2021

diverdane Dec 22, 2021

diverdane Dec 22, 2021

diverdane left a comment

nessiLahav left a comment

alexkalish commented Feb 14, 2022

Refactor authn-k8s-client to be authentication flow generic #425

Refactor authn-k8s-client to be authentication flow generic #425

Conversation

tzheleznyak commented Dec 15, 2021 • edited Loading

Desired Outcome

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

nessiLahav 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

diverdane commented Dec 21, 2021

tzheleznyak commented Dec 21, 2021 • edited Loading

doodlesbykumbi 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

diverdane 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

diverdane left a comment

Choose a reason for hiding this comment

nessiLahav left a comment

Choose a reason for hiding this comment

alexkalish commented Feb 14, 2022

tzheleznyak commented Dec 15, 2021 •

edited

Loading

tzheleznyak commented Dec 21, 2021 •

edited

Loading