Enhance NATS provider to support multiple authentication methods

[latchmihay]

Enhance NATS provider to support multiple authentication methods including JWT, NKey, and Username/Password.

This would allow the notification-controller to support the recommended NATS 2.0+ authentication paradigms.

[latchmihay]

@cappyzawa fixed it, its pointing to main now

[cappyzawa]

Thanks for the contribution!

Please run go mod tidy to update go.mod. The github.com/nats-io/nkeys package is currently listed as an indirect dependency, but since it's now directly imported in nats.go, it should be a direct dependency.

[cappyzawa]

One suggestion: the auth decision in publish() could be moved to NewNATS. Since we already know which auth method to use at construction time, we can capture it as a closure and just call it in publish().

This keeps publish() focused on "connect and send" rather than "figure out auth and connect and send".

Something like:

type natsClient struct {
    server string
    authFn func() (nats.Option, func(), error)
}

func NewNATS(...) (*NATS, error) {
    client := &natsClient{server: server}
    
    if len(credsData) > 0 {
        client.authFn = func() (nats.Option, func(), error) {
            return nats.UserCredentialBytes(credsData), nil, nil
        }
    } else if len(nkeySeed) > 0 {
        client.authFn = func() (nats.Option, func(), error) {
            kp, err := nkeys.FromSeed(nkeySeed)
            if err != nil {
                return nil, nil, fmt.Errorf("error parsing nkey seed: %w", err)
            }
            pubKey, err := kp.PublicKey()
            if err != nil {
                kp.Wipe()
                return nil, nil, fmt.Errorf("error getting public key: %w", err)
            }
            sigCB := func(nonce []byte) ([]byte, error) {
                return kp.Sign(nonce)
            }
            return nats.Nkey(pubKey, sigCB), kp.Wipe, nil
        }
    } else if username != "" && password != "" {
        client.authFn = func() (nats.Option, func(), error) {
            return nats.UserInfo(username, password), nil, nil
        }
    }
    // ...
}

func (n *natsClient) publish(...) error {
    opts := []nats.Option{nats.Name("NATS Provider Publisher")}
    
    if n.authFn != nil {
        authOpt, cleanup, err := n.authFn()
        if err != nil {
            return err
        }
        if cleanup != nil {
            defer cleanup()
        }
        opts = append(opts, authOpt)
    }
    
    nc, err := nats.Connect(n.server, opts...)
    // ...
}

The cleanup func handles kp.Wipe() for nkey auth - it gets deferred and runs after the connection is done.

Not a blocker, just a suggestion for cleaner separation.

[cappyzawa]

The authFn refactoring looks clean! 👍

One thing I noticed: the current test checks authFn != nil, but doesn't actually call authFn() to verify it returns the correct auth type. Since we moved away from storing fields directly, it'd be nice to confirm the returned nats.Option actually sets the right authentication.

Also, the "credentials file data is stored" and "nkey seed data is stored" cases could double as priority tests by adding the other auth params - this way we can verify creds > nkey > userpass priority works correctly.

Here's a minimal diff that adds this verification:

+	"github.com/nats-io/nats.go"

+		// Priority tests: creds > nkey > userpass
 		{
-			name:            "credentials file data is stored",
+			name:            "creds takes priority over nkey and userpass",
 			subject:         "test",
 			server:          "nats",
+			username:        "user",
+			password:        "pass",
 			credsData:       []byte("credentials-content"),
+			nkeySeed:        []byte("SUAGMJH5XLGZKQQWAWKRZJIGMOU4HPFUYLXJMXOO5NLFEO2OOQJ5LPRDPM"),
 			expectedSubject: "test",
 			expectedCreds:   true,
 		},
 		{
-			name:            "nkey seed data is stored",
+			name:            "nkey takes priority over userpass",
 			subject:         "test",
 			server:          "nats",
+			username:        "user",
+			password:        "pass",
 			nkeySeed:        []byte("SUAGMJH5XLGZKQQWAWKRZJIGMOU4HPFUYLXJMXOO5NLFEO2OOQJ5LPRDPM"),
 			expectedSubject: "test",
 			expectedNkey:    true,
 		},

And add verification logic after checking client.authFn:

				// Verify authFn returns valid option and correct auth type
				if client.authFn != nil {
					opt, cleanup, err := client.authFn()
					g.Expect(err).To(BeNil(), "authFn should not return error")
					g.Expect(opt).NotTo(BeNil(), "authFn should return a valid option")
					if cleanup != nil {
						defer cleanup()
					}

					// Apply option to verify which auth method was selected
					var opts nats.Options
					g.Expect(opt(&opts)).To(Succeed())

					if tt.expectedCreds {
						g.Expect(opts.UserJWT).NotTo(BeNil(), "creds auth should set UserJWT")
					} else if tt.expectedNkey {
						g.Expect(opts.Nkey).NotTo(BeEmpty(), "nkey auth should set Nkey")
						g.Expect(opts.SignatureCB).NotTo(BeNil(), "nkey auth should set SignatureCB")
					} else if tt.expectedUsername != "" {
						g.Expect(opts.User).To(Equal(tt.expectedUsername), "userpass auth should set User")
						g.Expect(opts.Password).To(Equal(tt.expectedPassword), "userpass auth should set Password")
					}
				}

This keeps the existing test structure while adding actual auth type verification. What do you think?

[latchmihay]

This keeps the existing test structure while adding actual auth type verification. What do you think?

Thank you for the suggestions, they have been added :)

[cappyzawa]

LGTM! Thanks for implementing the test improvements - the priority tests and authFn verification look great. 🎉

[matheuscscp]

Thanks for this PR, @latchmihay!

We should strive to use the standard fields when possible:

type notifierOptions struct {
	Context  context.Context
	URL      string
	ProxyURL string
	Username string
	Channel  string
	Token    string
	Headers  map[string]string
	// CertPool is kept for Git platform providers (GitHub, GitLab, etc.) that use third-party SDKs.
	// TODO: Remove this field once all notifiers support client certificate authentication via TLSConfig.
	CertPool           *x509.CertPool
	TLSConfig          *tls.Config
	Password           string
	CommitStatus       string
	ProviderName       string
	ProviderNamespace  string
	SecretData         map[string][]byte
	ServiceAccountName string
	TokenCache         *cache.TokenCache
	TokenClient        client.Client
}

From what I understand, what you are adding here in creds could be a good fit for Token?

We will release Flux 2.8 at the end of the month, let's try to work on this PR so it can make it to the release 🙏

[matheuscscp]

Also, please rebase 🙏

[matheuscscp]

No merge commits please. Use rebase and force-push. Also, please squash, this PR should have a single commit

[latchmihay]

We should strive to use the standard fields when possible:
...
From what I understand, what you are adding here in creds could be a good fit for Token?

@matheuscscp I agree that we should use the standard fields when possible, in this particular case its not a typical token.

.creds are more of a blob of text, hence it makes the most sense for users to inject that into a secret and for the notification controller to read out of it. It looks like this.

apiVersion: v1
kind: Secret
metadata:
  name: nats-provider-creds
  namespace: desired-namespace
stringData:
  creds: |
    -----BEGIN NATS USER JWT-----
    eyJ0eXAiOiJqd3QiLCJhbGciOiJlZDI1NTE5...
    ------END NATS USER JWT------

    ************************* IMPORTANT *************************
    NKEY Seed printed below can be used to sign and prove identity.
    NKEYs are sensitive and should be treated as secrets.
    
    -----BEGIN USER NKEY SEED-----
    SUAGMJH5XLGZKQQWAWKRZJIGMOU4HPFUYLXJMXOO5NLFEO2OOQJ5LPRDPM
    ------END USER NKEY SEED------

[matheuscscp]

Some tests are failing: https://github.com/fluxcd/notification-controller/actions/runs/21757579738/job/62779341794?pr=1222