ONYX-16576: Add Authn-JWT support to helm #431

tzheleznyak · 2022-01-30T16:02:43Z

Changed helm chart to support token projection for JWT cases using this values.yaml

environment:
  k8sSecrets:
  conjur: 
    account:
    applianceUrl:
    authnUrl:
    sslCertificate: 
      value:
    authnJWT:
      projectToken:
      projectedFilename:
      audience:
      expiration:

sashaCher

LGTM

diverdane · 2022-01-31T17:32:28Z

helm/secrets-provider/templates/secrets-provider.yaml

        name: {{ .Values.secretsProvider.name }}
        env:
+        {{- if .Values.environment.conjur.authnJWT.projectToken}}
+        - name: JWT_TOKEN_PATH


Using an environment variable here for the Secrets Provider "standalone" or "app" mode Helm chart is okay, but when the Secrets Provider is run in any other mode, this should be configured as a Pod Annotation. Is there a plan to add support to Secrets Provider so that this can be configured as a Pod Annotation?

Actually, I thought we wouldn't need this setting, and the token would be hard-coded in the Secrets Provider code?

It will be hardcoded to defaults in documentation and helm charts but if client for any reason want to change he can

We talked about not adding it as annotation not?

Our discussion was in the context of setting the token path via Pod Annotations, but I assumed that this extended more generally to not having a configuration for this in env variables, either.

My thinking is that if the token path, and/or the token filename, is something that we expect the user would never care about (i.e. it's only ever used inside the SP container, which we own and control), then it's probably best to not provide a way for a user to configure it (one less thing that can be misconfigured, and it's one less thing we need to document and test).

If this is being added to this Helm chart as a "just-in-case" setting, then we should already have support for this environment variable built into the SP container image. We could exclude this setting from user-facing Helm charts (e.g. this one) and user-facing documentation for now. If the situation does arise where a customer for some unforeseen reason needs to change this, we can always offer the customer a workaround (they can just modify their manifests or Helm charts).

But if this is a setting that we still want to make available to the user in this Helm chart, then we'll also want to add support for this into SP as an Annotation (so that it's available when SP is running as an init or sidecar container).

diverdane · 2022-01-31T17:43:58Z

helm/secrets-provider/tests/test-schema

@@ -216,10 +216,6 @@ function main() {
    conjur_authn_login_test ""
    update_results "$?" "$EXPECT_FAILURE"

-    announce "Missing Conjur authn login causes failure"


For non-JWT functionality, the value .Values.environment.conjur.authnLogin is still required, and we want the user notified (as a "fail fast" mechanism) if this is missing, and it's not configured for authn-jwt.

We'll probably need to add an authnJWT.enabled boolean chart value to this Helm chart, and default it to "false". Then, either in the Helm chart secrets-provider.yaml template, or better yet, in the values.schema.json, we need to indicate that either authnJWT.enabled needs to be set to "true", or authnLogin needs to be provided.

With the above changes, then the missing_conjur_authn_login_test should still be called here.

I thought about authnJWT.enabled but because token projection is mandatory for authn-jwt then
authnJWT.projectToken.

How can i do this test thing with values.schema.json?

diverdane · 2022-01-31T17:44:59Z

helm/secrets-provider/values.yaml

@@ -84,3 +84,8 @@ environment:
    # This setting is required.


If you add an authnJWT.enabled setting, then this comment should say:

# This setting is required if `authnJWT.enabled` is set to false.

Or something like that.

diverdane · 2022-01-31T17:47:29Z

helm/secrets-provider/templates/secrets-provider.yaml

        name: {{ .Values.secretsProvider.name }}
        env:
+        {{- if .Values.environment.conjur.authnJWT.projectToken}}
+        - name: JWT_TOKEN_PATH
+          value: /var/run/secrets/tokens/{{ .Values.environment.conjur.authnJWT.projectedFilname }}


Typo here and below: projectedFilname should be projectedFilename.

diverdane

We should keep the check for missing authnLogin for the non-JWT case, if possible.
Also, I thought we would hard-code the token path in Secrets Provider code?

diverdane · 2022-02-02T00:19:38Z

@tzheleznyak ,

Please take a look at the changes that I made on this branch: f5703bb

On this branch, I've implemented the following:

Helm chart schema to make authnLogin a required Helm chart value if authn-jwt authentication is not being used.
Added back the schema test for missing authnLogin.
Helm chart schema for the chart values that you're adding. This will cause Helm install to fail fast if users supply invalid values for authn-jwt.
Helm chart unittest unit tests for the changes that you're adding to the secrets-provider.yaml template.
Helm chart schema tests for the chart values that you're adding.
Fixed the typo for Filename.

(I figured that it's probably quicker to just add the schema/unittest code, since I've done it before.)

I've set the minimum JWT token interval to 1 (second). I don't know if this is appropriate.
I haven't set a maximum on the JWT token interval... if there's an appropriate value, we can add that to the schema.

Please feel free to copy the changes from that branch/commit into your PR branch.

codeclimate · 2022-02-02T17:41:10Z

Code Climate has analyzed commit ca062f1 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 92.6% (0.0% change).

View more on Code Climate.

Talked about it. Tell me if i missed something and i will open new PR. I will create a PR for annotation add for JWT token path and add it to the automation

tzheleznyak requested review from a team January 30, 2022 16:02

tzheleznyak self-assigned this Jan 30, 2022

tzheleznyak force-pushed the add-authn-jwt-params-to-helm branch from 567bd41 to 555e363 Compare January 30, 2022 16:08

sashaCher approved these changes Jan 31, 2022

View reviewed changes

diverdane reviewed Jan 31, 2022

View reviewed changes

diverdane previously requested changes Jan 31, 2022

View reviewed changes

tzheleznyak force-pushed the add-authn-jwt-params-to-helm branch from 555e363 to c6b6ca8 Compare February 1, 2022 09:57

tzheleznyak force-pushed the add-authn-jwt-params-to-helm branch 2 times, most recently from a071fea to 0e70d8e Compare February 2, 2022 15:52

ONYX-16576: Add Authn-JWT support to helm

ca062f1

tzheleznyak force-pushed the add-authn-jwt-params-to-helm branch from 0e70d8e to ca062f1 Compare February 2, 2022 16:38

tzheleznyak merged commit 0d1eb58 into main Feb 3, 2022

tzheleznyak deleted the add-authn-jwt-params-to-helm branch February 3, 2022 08:23

tzheleznyak mentioned this pull request Feb 3, 2022

ONYX-16669: Update Secrets Provider Helm To 1.4.0 cyberark/helm-charts#32

Merged

john-odonnell mentioned this pull request May 17, 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

ONYX-16576: Add Authn-JWT support to helm #431

ONYX-16576: Add Authn-JWT support to helm #431

tzheleznyak commented Jan 30, 2022

sashaCher left a comment

diverdane Jan 31, 2022

diverdane Jan 31, 2022

tzheleznyak Jan 31, 2022

tzheleznyak Jan 31, 2022

diverdane Jan 31, 2022 •

edited

Loading

diverdane Jan 31, 2022 •

edited

Loading

tzheleznyak Feb 1, 2022

tzheleznyak Feb 1, 2022

diverdane Jan 31, 2022

diverdane Jan 31, 2022

diverdane left a comment

diverdane commented Feb 2, 2022 •

edited

Loading

codeclimate bot commented Feb 2, 2022

ONYX-16576: Add Authn-JWT support to helm #431

ONYX-16576: Add Authn-JWT support to helm #431

Conversation

tzheleznyak commented Jan 30, 2022

sashaCher 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 Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

diverdane Jan 31, 2022 • edited Loading

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

diverdane commented Feb 2, 2022 • edited Loading

codeclimate bot commented Feb 2, 2022

diverdane Jan 31, 2022 •

edited

Loading

diverdane Jan 31, 2022 •

edited

Loading

diverdane commented Feb 2, 2022 •

edited

Loading