NATS Auth: user/password or token

[ghstahl]

nats: add user/password and token authentication
#3010

[CLAassistant]

All committers have signed the CLA.

[mmatczuk]

Code review

Found 6 issues:

1. Output test uses input constructor (tester: "Tests must exercise the code under test")

   The "Missing password" and "Missing user" subtests in output_jetstream_test.go call newJetStreamReaderFromConfig (input constructor) instead of newJetStreamWriterFromConfig. Copy-paste from input_jetstream_test.go — the variable is also named inputConfig. The auth validation is shared code so this accidentally passes, but the output code path is never tested.

   connect/internal/impl/nats/output_jetstream_test.go

   Lines 91 to 93 in c0d3830

   	_, err = newJetStreamReaderFromConfig(conf, service.MockResources())
   	require.Error(t, err)

   
   

   connect/internal/impl/nats/output_jetstream_test.go

   Lines 105 to 107 in c0d3830

   	_, err = newJetStreamReaderFromConfig(conf, service.MockResources())
   	require.Error(t, err)

2. user field marked as Secret() (godev: "Certification Standards — Observability")

   Usernames are identifiers, not secrets. Marking user as Secret() causes it to be redacted in logs and config dumps, making auth debugging harder. The docs also show a misleading "sensitive information" warning for the username. Only password and token should be Secret().

   connect/internal/impl/nats/auth.go

   Lines 90 to 93 in c0d3830

   	service.NewStringField("user").
   	Description("An optional plain text user name (given along with the corresponding user password).").
   	Secret().
   	Optional(),

3. No mutual exclusion between auth methods (godev: "Certification Standards — UX validation")

   A user can set token + user/password + nkey_file + user_credentials_file + user_jwt/user_nkey_seed simultaneously. All options are appended to the NATS connection, and the server silently picks one based on internal precedence. The existing auth methods already lack mutual exclusion (pre-existing), but this PR adds two more methods without validation, expanding the space of silent misconfigurations. A check at the end of AuthFromParsedConfig that counts configured auth methods and errors if >1 would fix this.

   connect/internal/impl/nats/auth.go

   Lines 159 to 167 in c0d3830

   	if auth.Token != "" {
   	opts = append(opts, nats.Token(
   	auth.Token,
   	))
   	}
   	if auth.User != "" && auth.Password != "" {
   	opts = append(opts, nats.UserInfo(auth.User, auth.Password))
   	}

   
   

   connect/internal/impl/nats/auth.go

   Lines 204 to 226 in c0d3830

   	if p.Contains("token") {
   	if c.Token, err = p.FieldString("token"); err != nil {
   	return
   	}
   	}
   	if p.Contains("user") || p.Contains("password") {
   	if !p.Contains("user") {
   	err = errors.New("missing auth.user config field")
   	return
   	}
   	if !p.Contains("password") {
   	err = errors.New("missing auth.password config field")
   	return
   	}
   	if c.User, err = p.FieldString("user"); err != nil {
   	return
   	}
   	if c.Password, err = p.FieldString("password"); err != nil {
   	return
   	}
   	}
   	return

4. Error assertions don't verify error message (tester: "require.ErrorContains preferred over require.Error")

   All 6 validation error tests across 3 files use bare require.Error(t, err). These pass on any error rather than confirming the specific validation fired. Should use require.ErrorContains(t, err, "missing auth.").

   Files: input_jetstream_test.go, input_kv_test.go, output_jetstream_test.go — all "Missing password" and "Missing user" subtests.

5. Commit message is a bare URL (commit policy: "Message format")

   Commit 67fdd35 message is https://github.com/redpanda-data/connect/issues/3010. Should follow system: message format, e.g., nats: add user/password and token authentication.

6. Commit bundles unrelated changes (commit policy: "Granularity")

   Commit 67fdd35 combines NATS auth additions, ockam platform-specific build files, and dependency bumps in a single commit. These are three unrelated changes and should be separate commits.

Generated with Claude Code

[ghstahl]

Addressed all the Code Review Issues.

[mmatczuk]

Code review

Found 3 issues:

1. Unrelated change breaks filepath_join/filepath_split documentation examples (bugs/security: "invalid JSON in code examples")

   The diff replaces forward slashes with backslashes in the filepath_join and filepath_split examples. Backslashes in JSON are escape characters: \f becomes a form feed, and \" escapes the closing quote, producing syntactically invalid JSON. This looks like an accidental editor or OS-level substitution.

   connect/docs/modules/guides/pages/bloblang/methods.adoc

   Lines 180 to 183 in 139e2e5

   	# In: {"path_elements":["\foo\","bar.txt"]}
   	# Out: {"path":"\foo\bar.txt"}
   	```

   connect/docs/modules/guides/pages/bloblang/methods.adoc

   Lines 194 to 197 in 139e2e5

   	# In: {"path":"\foo\bar.txt"}
   	# Out: {"path_sep":["\foo\","bar.txt"]}

2. Commit history needs cleanup (commit policy: "granularity", "message format", "message quality")

   • 67fdd35: Commit message is a bare URL (https://github.com/redpanda-data/connect/issues/3010). Must follow system: message format. Also mixes NATS auth, ockam platform files, and dependency bumps in one commit.
   • ed7fdc5: Message (Commit bundles unrelated changes (commit policy: "Granularity")) is meta-commentary about the previous commit, not a change description. Does not follow any accepted format.
   • 469e738 overlaps with 67fdd35 — both touch NATS auth files without the first being reverted.
   • c0d3830, 94da9da, 139e2e5: Three merge commits. Branch should be rebased onto main.

   The branch needs a rebase: squash the NATS auth work into one commit with a message like nats: add user/password and token authentication, put the ockam change in its own commit with a proper message, and remove merge commits.

3. authConfToOptions uses stricter check than AuthFromParsedConfig, creating a silent auth bypass path (bugs/security: "inconsistent validation between parsing and option construction")

   AuthFromParsedConfig validates that both user and password keys are present using p.Contains() (presence-based), but the mutual exclusion check and authConfToOptions use emptiness checks (c.User != "", auth.User != "" && auth.Password != ""). A config with user: "alice" and password: "" (e.g., from unset env var password: "${NATS_PASSWORD}") passes parsing and mutual exclusion but silently gets no authentication applied — authConfToOptions skips the nats.UserInfo() call because auth.Password != "" is false.

   connect/internal/impl/nats/auth.go

   Lines 160 to 165 in 139e2e5

   	auth.Token,
   	))
   	}
   	if auth.User != "" && auth.Password != "" {
   	opts = append(opts, nats.UserInfo(auth.User, auth.Password))

Areas for human review

The following areas had observations where the automated review was not confident enough to flag as issues. A human reviewer should verify these.

1. Test coverage of legacy auth methods reduced — field-mapping assertions removed, not augmented

   The existing happy-path tests in input_jetstream_test.go, input_kv_test.go, and output_jetstream_test.go that asserted NKeyFile, UserCredentialsFile, UserJWT, and UserNkeySeed struct values were replaced with tests that only assert User and Password. The mutual exclusion enforcement correctly makes the old multi-method config invalid, but no new per-method happy-path test was added to verify legacy field-mapping correctness. auth_test.go covers parsing success for each method but does not assert struct field values.

   connect/internal/impl/nats/input_jetstream_test.go

   Lines 35 to 48 in 139e2e5

   	subject: testsubject
   	max_reconnects: -1
   	auth:
   	user: test auth inline user name
   	password: test auth inline user password
   	tls_handshake_first: true
   	`
   	conf, err := spec.ParseYAML(inputConfig, env)
   	require.NoError(t, err)
   	e, err := newJetStreamReaderFromConfig(conf, service.MockResources())
   	require.NoError(t, err)

Generated with Claude Code

[mmatczuk]

Code review

Found 2 issues:

1. Field ordering splits user_jwt/user_nkey_seed logical pair (godev: "field name constants, ConfigSpec construction")

The new fields user, password, token are inserted between user_jwt (L89) and user_nkey_seed (L101) in authFieldSpec(). These two fields must be used together and appear as a pair in both the config spec and generated documentation. Splitting them with three unrelated fields makes the config harder to understand for users.

connect/internal/impl/nats/auth.go

Lines 86 to 104 in 36eec05

	service.NewStringField("user_jwt").
	Description("An optional plain text user JWT (given along with the corresponding user NKey Seed).").
	Secret().
	Optional(),
	service.NewStringField("user").
	Description("An optional plain text user name (given along with the corresponding user password).").
	Optional(),
	service.NewStringField("password").
	Description("An optional plain text password (given along with the corresponding user name).").
	Secret().
	Optional(),
	service.NewStringField("token").
	Description("An optional plain text token.").
	Secret().
	Optional(),
	service.NewStringField("user_nkey_seed").
	Description("An optional plain text user NKey Seed (given along with the corresponding user JWT).").
	Secret().
	Optional(),

2. Token and user/password docs appended inside "User credentials" section (godev: "ConfigSpec construction")

The two new Alternatively, ... lines at L63-65 appear inside the === User credentials section of authDescription(), after the user_jwt/user_nkey_seed alternative. Token auth and user/password auth are distinct auth methods, not alternatives to user credentials. They should be under their own === subsections, following the existing structure (=== NKey file, === User credentials).

connect/internal/impl/nats/auth.go

Lines 62 to 67 in 36eec05

	Alternatively, the ` + "`token`" + ` field can contain a plain text random string.
	Alternatively, the ` + "`user`" + ` and ` + "`password`" + ` fields can contain plain text user and password.
	https://docs.nats.io/using-nats/developer/connecting/creds[More details^].`

Areas for human review

The following areas had observations where the automated review was not confident enough to flag as issues. A human reviewer should verify these.

1. Empty user with non-empty password silently drops auth — follows the pre-existing value-emptiness pattern across all auth fields, but more likely to occur in practice for user/password (e.g., user: ${NATS_USER} where env var is empty string)

If user: "" is explicitly set alongside password: "secret", the pairing validation at L204 passes (p.Contains checks presence, not value), both fields are extracted, but the mutual exclusion check at L238 (c.User != "") and authConfToOptions at L157 (auth.User != "") both skip it. The password is silently discarded and the connection proceeds unauthenticated. All other auth fields have the same structural pattern (value-emptiness gating), so this may be by design.

connect/internal/impl/nats/auth.go

Lines 204 to 219 in 36eec05

	if p.Contains("user") || p.Contains("password") {
	if !p.Contains("user") {
	err = errors.New("missing auth.user config field")
	return
	}
	if !p.Contains("password") {
	err = errors.New("missing auth.password config field")
	return
	}
	if c.User, err = p.FieldString("user"); err != nil {
	return
	}
	if c.Password, err = p.FieldString("password"); err != nil {
	return
	}
	}

Generated with Claude Code

[mmatczuk]

This looks good! Two things:

• Please fix the linting issues — you can check with task lint
• It would be great if you could address the docs colocation as well

[ghstahl]

A small logic change was also added. I have a custom NATS auth service where I decide what can be passed.
Which means that user == "", password == "something" is allowed.

Any combination as long as user or password has something it passes validation. Not something I would do in practice but since NATS allows it then we should support it.

[mmatczuk]

Review of NATS user/password and token authentication changes.

[mmatczuk]

Commits

1. 8b09a598 ("addressed the docs colocation as well") — format violation. Must match system: message pattern (e.g., nats: fix docs colocation). Message is also vague — doesn't describe what changed. This looks like a fixup to the first commit and should be squashed into 36eec05 (nats: add user/password and token authentication).

Review
Auth implementation is clean: mutual exclusion validation is correct, authConfToOptions switch is consistent with the validation guarantees, password and token fields are properly marked .Secret(). Test coverage for the new auth methods is thorough (field mapping, mutual exclusion, missing field errors). The bugfix in output_jetstream_test.go (was calling newJetStreamReaderFromConfig instead of newJetStreamWriterFromConfig) is a good catch.

1. Three test/build artifacts accidentally committed — nats.test.exe (binary), compile_out.txt (empty), test_output.txt (empty). See inline comment.