Extend Account to support token and user/password auth

[vavsab]

Also moved repeated parts into shared controller to avoid code duplication.

Why not env var injection like initially proposed?

Injecting as env vars into server URL will leak secrets in logs. To avoid this we need to pass sensitive params into nats client as separate options.

Related to

nats-io/k8s#876
#76

Usage example (token)

apiVersion: v1
kind: Secret
metadata:
  name: nats-token
type: Opaque
data:
  # token. required to be in base64 in Secret.
  token: dG9rZW4=
---
apiVersion: jetstream.nats.io/v1beta2
kind: Account
metadata:
  name: token-acc
spec:
  name: TOKEN_ACCOUNT
  servers:
    - 'nats://localhost:4222'
  token:
    secret:
      name: nats-token
    token: token
---
apiVersion: jetstream.nats.io/v1beta2
kind: Stream
metadata:
  name: js-token
spec:
  account: token-acc
  name: js-token
  storage: file
  subjects:
    - data.>

Usage example (user with password)

apiVersion: v1
kind: Secret
metadata:
  name: nats-user-with-pass
type: Opaque
data:
  # admin/admin. required to be in base64 in Secret.
  user: YWRtaW4=
  password: YWRtaW4=
---
apiVersion: jetstream.nats.io/v1beta2
kind: Account
metadata:
  name: user-pass-acc
spec:
  name: USER_PASS_ACCOUNT
  servers:
    - 'nats://localhost:4222'
  user:
    secret:
      name: nats-user-with-pass
    user: user
    password: password
---
apiVersion: jetstream.nats.io/v1beta2
kind: Stream
metadata:
  name: js-user-pass
spec:
  account: user-pass-acc
  name: js-user-pass
  storage: file
  subjects:
    - data.>

[vavsab]

@samuelattwood @adriandieter @ramonberrutti @Jarema @johnweldon @vsinger
Could you please review and merge it?

UPD: I will really appreciate if you create a release as well so I could get rid of using a forked docker image.

[Jarema]

We will take a look at this one shortly.

[Jarema]

@samuelattwood could you please provide feedback for this PR?

[samuelattwood]

Thanks for the contribution! Apologies as well for the delay. A few comments but nothing major.

I do have some mixed feelings on using the Account CRD for all types of shared auth. My concern is that it creates conceptual confusion with user/password authentication, as defining an Account resource in NACK would not inherently correspond to a NATS account - as it generally does now (except maybe in the case of TLS user mapping.)

I don't know if that would be sufficient justification to add a dedicated User CRD or if we want to just leave it in Account. @Jarema perhaps you have thoughts on this.

[vavsab]

@samuelattwood ✅ Fixed all your notes. Please take a look

[vavsab]

Thanks for the contribution! Apologies as well for the delay. A few comments but nothing major.

I do have some mixed feelings on using the Account CRD for all types of shared auth. My concern is that it creates conceptual confusion with user/password authentication, as defining an Account resource in NACK would not inherently correspond to a NATS account - as it generally does now (except maybe in the case of TLS user mapping.)

I don't know if that would be sufficient justification to add a dedicated User CRD or if we want to just leave it in Account. @Jarema perhaps you have thoughts on this.

When I started using NACK I had a confusion with Account resource. It was not obvious why we need it. Then I renamed it in my head to NatsConnectionConfig and it became clear. It's just a config. It's not a real resource. Actually having connection config only in this resource would be enough. Duplicating auth logic in all other resources creates complicated code and more confusion for new NACK users. I would completely remove all connection related things from Stream and Consumer.

[mstaicu]

Thanks for the contribution! Apologies as well for the delay. A few comments but nothing major.
I do have some mixed feelings on using the Account CRD for all types of shared auth. My concern is that it creates conceptual confusion with user/password authentication, as defining an Account resource in NACK would not inherently correspond to a NATS account - as it generally does now (except maybe in the case of TLS user mapping.)
I don't know if that would be sufficient justification to add a dedicated User CRD or if we want to just leave it in Account. @Jarema perhaps you have thoughts on this.

When I started using NACK I had a confusion with Account resource. It was not obvious why we need it. Then I renamed it in my head to NatsConnectionConfig and it became clear. It's just a config. It's not a real resource. Actually having connection config only in this resource would be enough. Duplicating auth logic in all other resources creates complicated code and more confusion for new NACK users. I would completely remove all connection related things from Stream and Consumer.

I had the same issue when reading about the Account resource, this clears things up a bit

[Jarema]

I also agree with the confusion that Account crd can cause. Intuitevely it feels like it should create NATS Account (which is not possible via API call), but in reality, It's just credentials for a specific user, which can of course connect to a specific account.

We should at least give a clarification in readme, or maybe even break it by renaming it?

Or I'm missing something here @samuelattwood ?

[vavsab]

Something like this would be clear and easy to understand for me as a NACK user

apiVersion: v1
kind: Secret
metadata:
  name: nats-token
type: Opaque
data:
  token: dG9rZW4=
---
apiVersion: jetstream.nats.io/v1beta3 # ⚠️ New API since it's a breaking change
kind: NatsClientConfig
metadata:
  name: nats-client-config-account1
spec:
  servers:
    - 'nats://localhost:4222'
  # We allow all auth options here.
  # ⚠️ For those who does not care about security and just wants to test quickly: if I do not set secret.name then keys contain my creds in plaintext (no need to search for secret)
  token:
    secret:
      name: nats-token
    token: token
---
apiVersion: jetstream.nats.io/v1beta3
kind: Stream
metadata:
  name: js-example
spec:
  # ⚠️ No other options to configure connection - simpler NACK code and less confusion for new NACK users
  # Focus on stream itself here and let natsClientConfig handle connection & auth.
  natsClientConfig: nats-client-config-account1 
  name: js-example
  storage: file
  subjects:
    - data.>

[samuelattwood]

To clarify my original point, with adding User/Password auth to the Account resource, even if you are not using NATS accounts and are just using basic config-mode user/pass you would still create an Account resource in NACK for the config which seems conceptually odd.

I don't disagree with the comments that the Account resource can add some confusion, as it is just a store of connection config for a given NATS Account user and does not create or mutate a server-side resource.

From scratch I would approach this differently, but I have concerns about such breaking changes as renaming it and stripping connect options from consumer/stream resources. If we do want to go that route I would be inclined to support the existing config for some period into the future, with deprecation warnings.

[vavsab]

To clarify my original point, with adding User/Password auth to the Account resource, even if you are not using NATS accounts and are just using basic config-mode user/pass you would still create an Account resource in NACK for the config which seems conceptually odd.

I don't disagree with the comments that the Account resource can add some confusion, as it is just a store of connection config for a given NATS Account user and does not create or mutate a server-side resource.

From scratch I would approach this differently, but I have concerns about such breaking changes as renaming it and stripping connect options from consumer/stream resources. If we do want to go that route I would be inclined to support the existing config for some period into the future, with deprecation warnings.

I vote for quick changes. My company needs these changes for yesterday. I want to get rid of our NACK fork asap.
If you are ok with current state of PR, could you please merge it and release? It's not perfect but will absolutely cover my use case.

[vavsab]

even if you are not using NATS accounts and are just using basic config-mode user/pass you would still create an Account resource in NACK for the config which seems conceptually odd.

Even if I had no accounts I would not use Consumer and Stream for auth. They do not support secrets. This makes auth useless there. If you are using GitOps it will expose creds. For example,

1. folks at sealed-secrets just deny to handle secrets in custom resources because secrets are always expected to be stored in secret resource.
2. Argocd will not hide custom props from other engineers. It only does it for secrets.

So anyway you will need to introduce breaking changes in the future.

[samuelattwood]

The creds and nkey fields of Consumer and Stream are file paths, not raw secret values. If you were, for example, using the NACK helm chart you could map a credential secret with additionalVolumes and additionalVolumeMounts and set the value to wherever you mounted the secret. I agree that secrets should not be stored in the CRDs.

Along those lines, I think if we add the user/pass and token options to consumer and stream resources they should probably be file paths as well. You could already put the user:pass or token in the connection URL if you wanted to store them in the resource.

That being said, since it seems there is support for amending the auth config in general, I would be ok with leaving those options out of the Stream and Consumer resource types as you had it originally.

I am ok with proceeding with this merge, and doing an API revision in the near future to replace the Account type with something more appropriately named.

[vavsab]

@samuelattwood Thank you for clarifications. Applied review comments

[samuelattwood]

LGTM

[Jarema]

LGTM!

[mstaicu]

So to clarify, I no longer have to mount the .creds file stored in a secret in order to authenticate NACK with my NATS server?

      containers:
        - name: jsc
          image: natsio/jetstream-controller:latest
          # imagePullPolicy: IfNotPresent
          workingDir: /nack
          command:
            - /jetstream-controller
          args:
            - -s=nats://nats:4222
            - --creds=/nack/nack.creds
          volumeMounts:
            - name: nack-volume-creds
              mountPath: /nack
      volumes:
        - name: nack-volume-creds
          secret:
            secretName: nack-creds