cyberark / conjur-authn-k8s-client

Authentication sidecar for Conjur Kubernetes integration.
https://www.conjur.org
Apache License 2.0
11 stars 14 forks source link

No default behavior for CONJUR_VERSION parameter #70

Closed JfcAtCyberArk closed 4 years ago

JfcAtCyberArk commented 4 years ago

Hi there, There is no default behavior if the CONJUR_VERSION is not set, or not 4 or 5. Code reference: https://github.com/cyberark/conjur-authn-k8s-client/blob/master/pkg/authenticator/authenticator.go#L230

In that case, the authenticator fails with the following entry in the logs: CAKC011I Login request to:

It's really not intuitive.

Actual version of DAP is 11.2.1 - to be aligned with CyberArk suite, version 4 is really far now. I think a default behavior would make sense.

Following DockerHub links may also be required an update: https://hub.docker.com/r/cyberark/conjur-kubernetes-authenticator/ https://hub.docker.com/r/cyberark/conjur-authn-k8s-client

Many thanks!

JFC

izgeri commented 4 years ago

Thanks for adding this issue @JfcAtCyberArk - I 100% agree and generally think most of our integrations should have this behavior (default to 5, but allow specifying 4 if using an older version of Conjur Enterprise v4.x)

To be totally clear, CONJUR_VERSION should be set to 5 unless you're using an older version of Conjur Enterprise v4.x (in which case, CONJUR_VERSION=4). The CONJUR_VERSION variable really refers to the Conjur API version being used by the server, which is 5 for DAP 10.x+ and Conjur OSS.

I'll see what we can do about getting a fix for this into our queue to address soon.

izgeri commented 4 years ago

Note:

AC for making this update:

izgeri commented 4 years ago

These changes are available in v0.16.1 (I actually think this functionality was available as early as v0.13.0), but we'll aim to propagate them to the downstream projects with PRs now (cc @BradleyBoutcher)

izgeri commented 4 years ago

version bump PRs:

Doc update issues:

JfcAtCyberArk commented 4 years ago

Hi @izgeri @BradleyBoutcher, I think the current implementation of the switch is confusing to me.

What I meant by a "Default Behavior" was that any other value than '4' would use '5' behind the scenes but it is not the case.

I just ran into an issue because my authenticator was failing when I provided it CONJUR_VERSION=11.4.0 and I had to come back to the source code to understand why.

I feel like I did not explain enough what I meant by a default behavior, sorry for that. Still, WDYT? Is there a point refusing a value different from 4 or 5?

Please let me know!

JFC

JfcAtCyberArk commented 4 years ago

I also think dockerhub doc is not really clear. Maybe it should mention that the variable is optional - and that the only reason to use it is to set it to 4.

izgeri commented 4 years ago

@JfcAtCyberArk all of our clients and tools expect the value to be either 4 or 5, and no other value.

At current, the authn-k8s client defaults to 5 if no value is specified, and errors if the value is not 4 or 5.

I've updated both DockerHub pages for this project to list CONJUR_VERSION at the bottom of the table, be clear that it's optional, and that '4' should only be used for Conjur Enterprise v4. I'll plan to remove these entries altogether once we deprecate authn-k8s for Conjur Enterprise v4, which I expect will happen soon.

Hope this makes sense, but please feel free to weigh in if you have any other suggestions.

JfcAtCyberArk commented 4 years ago

@izgeri It totally makes sense! I had something in mind where whatever is set for CONJUR_VERSION, "5" will be used except if "4" was set. Then, any Conjur/DAP user seeing a script or a plugin where there is a "CONJUR_VERSION" variable to set could set it to the version he is actually using. I understand it is not the best approach because the "CONJUR_VERSION" variable is intended to disappear and at one point, users will have to stop using it. The sooner the better.
Thanks a lot for updating the DockerHub pages!

JFC

izgeri commented 4 years ago

@JfcAtCyberArk you will be glad to hear we are trying to default this to 5 everywhere possible, and to remove it from the DAP / Conjur OSS documentation once we do. If there is a project where this is especially valuable for you and you don't see an existing gh issue for that change, please file one and let us know so we can update it soon!