Adds support for token based authentication #23

jvanderhoof · 2018-08-24T19:31:05Z

What does this PR do?

Adds support for using a Conjur authentication token for authorization. This allows the library to leverage Kubernetes authentication, removing the need for host and API keys.

Any background context you want to provide?

The code for this PR comes from the USAA engineering team.

What ticket does this PR close?

Closes #22

Link to build in Jenkins (if appropriate)

https://jenkins.conjur.net/job/cyberark--conjur-api-java/job/authn-kubernetes/ is green, but note that no additional tests have been added.

Has the Version and Changelog been updated?

Yes.

This code was contributed by a customer. It provides support for loading the token provided by the authn-k8s authenticator.

This commit includes an updated README and JavaDocs

This commit includes a minor level version change as well as a Changelog entry.

rafis3

Hi @jvanderhoof,
I left some comments. Mainly regarding standardizing the feature to our other APIs that already have this capability. Please let me know what you think about them.
Thanks,
Rafi

rafis3 · 2018-08-26T07:54:41Z

src/main/java/net/conjur/api/clients/ResourceClient.java

@@ -26,6 +27,13 @@ public ResourceClient(final String username, final String password, final Endpoi
        init(username, password);
 	}

+	// Build ResourceClient using a K8S auth token


I think that according to the official standard, you can either say K8s, k8s or Kubernetes but never K8S.

I don't think this should reference K8s at all, this is just a normal Conjur access token at this point, correct?

rafis3 · 2018-08-26T07:59:01Z

src/main/java/net/conjur/api/clients/AuthnK8sClient.java

+import net.conjur.api.AuthnProvider;
+import net.conjur.api.Token;
+
+public class AuthnK8sClient implements AuthnProvider {


I think this class should be called AuthnFileClient.
Even though reading a token from a file is currently used in K8s, it could easily be used in other flows as well and therefore I think that a generic name is better. We did the same in the other APIs, such as in Go: https://github.com/cyberark/conjur-api-go/blob/b75a31bafc95e06780b81df18ba5dba8dd0dc32f/conjurapi/authn/token_file_authenticator.go

I agree with @rafis3, that we should get this to line up with our Go API client. I think we can do this iteratively though, lining it up with the TokenAuthenticator [1], rather than the TokenFileAuthenticator. I would suggest we rename this to AuthnTokenProvider. We can then add a ticket to add a AuthnTokenProvider that includes the file watch support.

[1] https://github.com/cyberark/conjur-api-go/blob/b75a31bafc95e06780b81df18ba5dba8dd0dc32f/conjurapi/authn/token_authenticator.go

I will add that I think the should be updated to include the environment variable, but I would suggest we use CONJUR_AUTHN_TOKEN rather than the file path for this PR.

rafis3 · 2018-08-26T08:07:56Z

README.md


+#### Authorization Token
+```java
+String authTokenString = new String(Files.readAllBytes(Paths.get('path/to/conjur/authentication/token.json')));


I think that this should be encapsulated inside the library and not given by the developer. So the developer will only state that he/she is using a token from a file. The file should be given in the CONJUR_AUTHN_TOKEN_FILE environment variable or in the Conjur class constructor (instead of getting a token).
Once this is encapsulated, we need to also add a wait mechanism, in case the application is trying to use the token file before it got created by the sidecar. See similar mechanism here: https://github.com/cyberark/conjur-api-go/blob/b75a31bafc95e06780b81df18ba5dba8dd0dc32f/conjurapi/authn/token_file_authenticator.go#L23

micahlee

I agree with @rafis3 that we should bring this in line a little more with our Go API client in naming and parameter passing (e.g. environment variables).

I think we should keep this at the token string level though, rather than add the token file support now. I feel like that could be adding as a future iteration if and when it is desired.

micahlee · 2018-08-27T14:42:51Z

src/main/java/net/conjur/api/clients/AuthnK8sClient.java

+import net.conjur.api.AuthnProvider;
+import net.conjur.api.Token;
+
+public class AuthnK8sClient implements AuthnProvider {


I agree with @rafis3, that we should get this to line up with our Go API client. I think we can do this iteratively though, lining it up with the TokenAuthenticator [1], rather than the TokenFileAuthenticator. I would suggest we rename this to AuthnTokenProvider. We can then add a ticket to add a AuthnTokenProvider that includes the file watch support.

[1] https://github.com/cyberark/conjur-api-go/blob/b75a31bafc95e06780b81df18ba5dba8dd0dc32f/conjurapi/authn/token_authenticator.go

micahlee · 2018-08-27T14:44:08Z

src/main/java/net/conjur/api/clients/AuthnK8sClient.java

+import net.conjur.api.AuthnProvider;
+import net.conjur.api.Token;
+
+public class AuthnK8sClient implements AuthnProvider {


I will add that I think the should be updated to include the environment variable, but I would suggest we use CONJUR_AUTHN_TOKEN rather than the file path for this PR.

micahlee · 2018-08-27T14:46:06Z

src/main/java/net/conjur/api/clients/ResourceClient.java

@@ -26,6 +27,13 @@ public ResourceClient(final String username, final String password, final Endpoi
        init(username, password);
 	}

+	// Build ResourceClient using a K8S auth token


I don't think this should reference K8s at all, this is just a normal Conjur access token at this point, correct?

Switching to contributor, so I am no longer a reviewer

micahlee · 2018-12-21T16:00:42Z

@rafis3, I have picked up the coding work for this PR from Jason. I'm working on adding some additional CI tests for this change, but in case that doesn't get finished today before the holiday break, I wanted to go ahead and mention that the code changes themselves are ready for another review.

I have deferred adding to this PR for the file watching functionality that is present in the Go API. But I will file a new issue to continue enhancing the Java API with that capability. Since the bulk of this PR was a customer contribution, I'd rather get it merged as close to the original form as is acceptable and then continue to build on it as need or priority dictates.

Let me know what you think! Thanks!

rafis3

@micahlee I went over the changes and they look good to me. Just added one small rephrasing of a text in the README.
Thanks!

README.md

Co-Authored-By: micahlee <[email protected]>

jvanderhoof added 3 commits August 24, 2018 11:22

Authentication using Kubernetes Authentication

ac73e0f

This code was contributed by a customer. It provides support for loading the token provided by the authn-k8s authenticator.

Added documentation

98651dc

This commit includes an updated README and JavaDocs

Bump version and update Changelog

b3b2146

This commit includes a minor level version change as well as a Changelog entry.

ghost assigned jvanderhoof Aug 24, 2018

ghost added the in progress label Aug 24, 2018

rafis3 requested changes Aug 26, 2018

View reviewed changes

micahlee previously requested changes Aug 27, 2018

View reviewed changes

Rename classes to represent more generic access token.

abc8820

ghost assigned micahlee Dec 21, 2018

ghost added the implementing label Dec 21, 2018

Add support for loading token by filepath or environment var

aed8032

micahlee force-pushed the authn-kubernetes branch from fb7a22c to aed8032 Compare December 21, 2018 15:32

micahlee unassigned jvanderhoof Dec 21, 2018

rafis3 approved these changes Dec 23, 2018

View reviewed changes

README.md Outdated Show resolved Hide resolved

Update README.md

9d081f3

Co-Authored-By: micahlee <[email protected]>

micahlee merged commit 530db48 into master Jan 2, 2019

ghost removed implementing in progress labels Jan 2, 2019

micahlee deleted the authn-kubernetes branch January 2, 2019 14:11

micahlee mentioned this pull request Jan 2, 2019

Support for watching authentication token file #25

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for token based authentication #23

Adds support for token based authentication #23

jvanderhoof commented Aug 24, 2018

rafis3 left a comment

rafis3 Aug 26, 2018

micahlee Aug 27, 2018

rafis3 Aug 26, 2018

micahlee Aug 27, 2018

micahlee Aug 27, 2018

rafis3 Aug 26, 2018

micahlee left a comment

micahlee Aug 27, 2018

micahlee Aug 27, 2018

micahlee Aug 27, 2018

micahlee commented Dec 21, 2018

rafis3 left a comment

Adds support for token based authentication #23

Adds support for token based authentication #23

Conversation

jvanderhoof commented Aug 24, 2018

What does this PR do?

Any background context you want to provide?

What ticket does this PR close?

Link to build in Jenkins (if appropriate)

Has the Version and Changelog been updated?

rafis3 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

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

micahlee commented Dec 21, 2018

rafis3 left a comment

Choose a reason for hiding this comment