Add JWT flow #426

tzheleznyak · 2021-12-23T15:44:15Z

Desired Outcome

Added JWT flow to the authenticator

Connected Issue/Story

ONYX-14722

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

doodlesbykumbi

Some comments

pkg/authenticator/authenticator_factory.go

pkg/authenticator/jwt/authenticator.go

pkg/authenticator/config/validate_test.go

sashaCher · 2021-12-26T09:14:12Z

pkg/authenticator/jwt/requests.go

+	req.Header.Set("Content-Type", "text/plain")
+	req.Header.Set("Content-Length", string(len(formattedJwt)))
+	req.Header.Set("User-Agent", "k8s")


Do we want to add support for accept-encoding header?
Conjur is able to return access token already encoded base64 instead of plain json (1, 2)

Currently the components that use authn-k8s-client do the encode themselves . I think this is good idea but not in scope. I wanted to be aligned with the auhtn-k8s flow

tzheleznyak · 2021-12-26T14:47:59Z

CHANGELOG.md

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

 ### Added
 - Add support for tracing with OpenTelemetry. This adds a new function to the authenticator, `AuthenticateWithContext`. The existing funtion, `Authenticate()` is deprecated and will be removed in a future upddate. [cyberark/conjur-authn-k8s-client#423](https://github.com/cyberark/conjur-authn-k8s-client/pull/423)
+- Authn JWT flow support [cyberark/conjur-authn-k8s-client#426](https://github.com/cyberark/conjur-authn-k8s-client/pull/426)


@szamir1 @shulifink
May you review my code

pkg/authenticator/jwt/requests.go

nessiLahav · 2021-12-27T08:55:44Z

pkg/authenticator/jwt/authenticator.go

+		authenticatingIdentity = ""
+	}
+
+	req, err := AuthenticateRequest(


i think we should align the context handling with this code

This what we talked about and i fixed

Oh i see i done it in all error handling in the function

nessiLahav

Small comments, great job

szh

Looking good, left a few comments

pkg/authenticator/jwt/authenticator.go

pkg/authenticator/jwt/tests/authenticator_test.go

pkg/authenticator/jwt/tests/authenticator_test_server.go

szh

LGTM! 👍

diverdane

Looks good!!!
Some editorial comments and a possible missing strconv.Itoa().

diverdane · 2021-12-27T20:11:50Z

pkg/authenticator/jwt/config.go

+	config.Common = common.Config{}
+	config.Common.LoadConfig(settings)
+
+	for key, value := range settings {


Instead of looping through the range of settings, couldn't we see if the "JWT_TOKEN_PATH" entry exists and then use it? Something like this?:

if path, exists := settings["JWT_TOKEN_PATH"]; exists { config.JWTTokenFilePath = path }

Good point thanks

diverdane · 2021-12-27T20:52:39Z

pkg/authenticator/jwt/requests.go

+	}
+
+	req.Header.Set("Content-Type", "text/plain")
+	req.Header.Set("Content-Length", string(len(formattedJwt)))


When an int is typecast using string(), the int value is interpreted as Unicode. I believe that what we need here is:

req.Header.Set("Content-Length", strconv.Itoa(len(formattedJwt)))

diverdane · 2021-12-27T20:57:03Z

pkg/authenticator/jwt/requests.go

+func createUrl(authnURL string, account string, username string) string {
+	if len(username) > 0 {
+		return fmt.Sprintf("%s/%s/%s/authenticate", authnURL, account, url.QueryEscape(username))
+	} else {


The if block ends with a return, so the } else { isn't needed. That is, this should be:

if len(username) > 0 { return fmt.Sprintf("%s/%s/%s/authenticate", authnURL, account, url.QueryEscape(username)) } return fmt.Sprintf("%s/%s/authenticate", authnURL, account)

diverdane · 2021-12-27T21:30:29Z

pkg/authenticator/config/configuration_factory.go

@@ -44,6 +45,8 @@ func ConfigFromEnv(readFileFunc common.ReadFileFunc) (Configuration, error) {
 func getConfiguration(url string) (Configuration, error) {
 	if strings.Contains(url, k8sAuthenticator.AuthnType) {
 		return &k8sAuthenticator.Config{}, nil
+	} else if strings.Contains(url, jwtAuthenticator.AuthnType) {


Instead of using an if/else block here, you could optionally use switch without a statement (i.e. evaluating expressions in the case statements). I think this makes it a little more obvious that there are 3 possible results for which we're looking. For example:

switch { case strings.Contains(url, k8sAuthenticator.AuthnType): return &k8sAuthenticator.Config{}, nil case strings.Contains(url, jwtAuthenticator.AuthnType): return &jwtAuthenticator.Config{}, nil default: return nil, fmt.Errorf(log.CAKC063, url) }

diverdane · 2021-12-27T21:35:29Z

pkg/authenticator/jwt/config.go

+
+func configureLogLevel(level string) {
+	validVal := "true"
+	if level == validVal {


You could also use a switch statement here. I think this makes it a little more obvious that there are three possible outcomes (2 good, 1 bad):

switch level { case validVal: log.EnableDebugMode() case "": // Log level not configured break default: // Log level is configured but it's invalid log.Warn(log.CAKC034, level, validVal) }

Good point. Moved it to ConfigFactory to be in one place

nessiLahav

LGTM

I fixed your comments so i will merge. If there anything else let me know and i will open separate PR :)

tzheleznyak requested review from szh, doodlesbykumbi, diverdane and a team December 23, 2021 15:44

doodlesbykumbi reviewed Dec 23, 2021

View reviewed changes

pkg/authenticator/authenticator_factory.go Outdated Show resolved Hide resolved

pkg/authenticator/jwt/authenticator.go Outdated Show resolved Hide resolved

tzheleznyak force-pushed the add-jwt-flow branch 3 times, most recently from 810c3fa to ec99e32 Compare December 26, 2021 08:39

tzheleznyak commented Dec 26, 2021

View reviewed changes

pkg/authenticator/config/validate_test.go Show resolved Hide resolved

tzheleznyak force-pushed the add-jwt-flow branch from ec99e32 to 1acaa3e Compare December 26, 2021 09:02

sashaCher reviewed Dec 26, 2021

View reviewed changes

tzheleznyak force-pushed the add-jwt-flow branch 3 times, most recently from 235938e to 82cfcc7 Compare December 26, 2021 14:45

tzheleznyak marked this pull request as ready for review December 26, 2021 14:46

tzheleznyak requested review from a team as code owners December 26, 2021 14:46

tzheleznyak commented Dec 26, 2021

View reviewed changes

tzheleznyak force-pushed the add-jwt-flow branch 2 times, most recently from e9d0cd1 to 8adadfe Compare December 27, 2021 08:13

nessiLahav reviewed Dec 27, 2021

View reviewed changes

pkg/authenticator/jwt/requests.go Outdated Show resolved Hide resolved

nessiLahav reviewed Dec 27, 2021

View reviewed changes

tzheleznyak force-pushed the add-jwt-flow branch from 8adadfe to 076654d Compare December 27, 2021 08:56

nessiLahav suggested changes Dec 27, 2021

View reviewed changes

tzheleznyak force-pushed the add-jwt-flow branch 3 times, most recently from 86f070f to 6a81934 Compare December 27, 2021 11:02

tzheleznyak requested a review from nessiLahav December 27, 2021 11:12

szh suggested changes Dec 27, 2021

View reviewed changes

tzheleznyak force-pushed the add-jwt-flow branch from 6a81934 to 5cf4125 Compare December 27, 2021 16:13

szh previously approved these changes Dec 27, 2021

View reviewed changes

diverdane previously requested changes Dec 27, 2021

View reviewed changes

nessiLahav previously approved these changes Dec 28, 2021

View reviewed changes

Add JWT flow

25a3282

tzheleznyak dismissed stale reviews from nessiLahav and szh via 25a3282 December 28, 2021 09:05

tzheleznyak force-pushed the add-jwt-flow branch from 5cf4125 to 25a3282 Compare December 28, 2021 09:05

nessiLahav approved these changes Dec 28, 2021

View reviewed changes

tzheleznyak merged commit 6e8549a into master Dec 28, 2021

tzheleznyak deleted the add-jwt-flow branch December 28, 2021 09:52

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

Add JWT flow #426

Add JWT flow #426

tzheleznyak commented Dec 23, 2021

doodlesbykumbi left a comment

sashaCher Dec 26, 2021

tzheleznyak Dec 26, 2021

tzheleznyak Dec 26, 2021

nessiLahav Dec 27, 2021

tzheleznyak Dec 27, 2021

tzheleznyak Dec 27, 2021

nessiLahav left a comment

szh left a comment

szh left a comment

diverdane left a comment

diverdane Dec 27, 2021

tzheleznyak Dec 28, 2021

diverdane Dec 27, 2021

diverdane Dec 27, 2021

diverdane Dec 27, 2021

diverdane Dec 27, 2021 •

edited

Loading

tzheleznyak Dec 28, 2021

nessiLahav left a comment

Add JWT flow #426

Add JWT flow #426

Conversation

tzheleznyak commented Dec 23, 2021

Desired Outcome

Connected Issue/Story

Changelog

Test coverage

Documentation

Behavior

Security

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

nessiLahav left a comment

Choose a reason for hiding this comment

szh left a comment

Choose a reason for hiding this comment

szh left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diverdane Dec 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nessiLahav left a comment

Choose a reason for hiding this comment

diverdane Dec 27, 2021 •

edited

Loading