[scottf]

One SSL context serves every server of a connection. When the options asked for both the default context and the trust-all context, the trust-all context won, so the certificate verification the other side asked for was silently removed. That happened for secure() together with opentls(), for the secure and opentls properties both true, and for a bootstrap list with an opentls:// server next to a tls:// or wss:// server, where one opentls:// entry made the whole connection, discovered servers included, trust all certificates.

Why a trust-all context matters

opentls without a context builds an SSLContext whose trust manager accepts any server certificate: self-signed, expired, revoked, issued by anyone, for any name. Supplying that context by hand, with sslContext(SSLUtils.createTrustAllTlsContext()), sslContext(SSLUtils.createOpenTLSContext()), or any trust manager that does not throw, is the same thing; the client cannot tell the difference and does not warn.

TLS gives encryption and proof of who is at the other end. The proof comes entirely from checking the certificate against something the client already trusts. A trust-all context keeps the encryption and discards the proof: the connection is encrypted to whoever answered. Anything on the path that can answer the TCP connection, a compromised host on the same network, a wrong proxy, a wrong DNS answer, a pod that took over an address, presents its own certificate and the client completes the handshake. The CONNECT then hands it the credentials: a password or token outright; for NKey and credentials-file authentication, a relaying impersonator passes the real server's nonce through and forwards the signature, so signed authentication does not prevent it either. From then on it reads everything the client publishes, delivers anything to the client's subscriptions, and can relay to the real server so nothing looks wrong. No error, event or log line is raised. Every client has this switch, and Go's documentation says InsecureSkipVerify "should NOT be used in a production setting"; it is for development and tests only.

This PR does not remove opentls. It stops the case where a configuration asked for verification and got trust-all anyway, and it documents the rest.

Change

• Options.build() throws IllegalStateException when both contexts are asked for, by builder, by properties, or by a server list that mixes the schemes. The message names both sources and says to choose one or provide an SSLContext.
• One explicit choice is unchanged: opentls() or secure() with any mix of schemes builds as before, because an explicit flag stops the schemes from being consulted; sslContext(ctx) is used whatever the flags and schemes are; plain nats:// servers still mix with either secure scheme.
• The comment that documented "trust all wins when both are provided" is gone with the behavior.

Documentation

• docs/TLS_CONFIGURATION.md: how the context is chosen and in what order; what the default context trusts; the trust-all context, opentls or hand-supplied, what it removes, what an impersonated server can do with it, when it is acceptable and what to use otherwise; this rule with the exact error text; client certificates; TLS-first; the server-side checks.
• README: one sentence in the SSLContext section pointing at the document.

Tests

• OptionsTests.testSslContextIsProvided: rejected for tls+opentls, opentls+tls, wss+opentls, a three-entry list with nats, tls and opentls, secure() with opentls(), and both properties true; accepted for nats+opentls, two opentls entries, opentls() with tls urls, secure() with opentls urls, opentls() over a mixed list, and a provided context over a mixed list. 69 of 69 pass.

[synadia-claude-reviewer]

Claude finished @scottf's task in 2m 25s —— View job

---

Reviewing PR #1639

• Fetch PR metadata and diff
• Read Options.java changes and surrounding context
• Read OptionsTests.java changes
• Read docs/TLS_CONFIGURATION.md and README diff
• Check for correctness/security/edge cases
• Post review summary

Review

Traced the new check in Options.Builder.build() (Options.java:2426-2446) against the full set of priority branches (sslContext → factory → keystore/truststore → opentls → secure → none) and against the updated test cases. The logic is correct and self-consistent:

• useDefaultTls/useTrustAllTls are only inferred from bootstrap URI schemes when neither was already set explicitly (by builder or property), so an explicit opentls()/secure() correctly takes precedence over a mixed URL list, matching the documented "explicit flag wins" rule.
• The new conflict check (useDefaultTls && useTrustAllTls) runs after URI inference but before either context is actually built, so it correctly catches explicit+explicit, explicit+scheme, and scheme+scheme conflicts alike.
• The for (int i = 0; sslContext == null && ...) → plain for-each simplification is safe: sslContext is never reassigned inside the loop body in either version, so the removed condition was dead.
• One edge case worth noting (pre-existing, not introduced by this PR): when keystore/truststore is set, the build takes that branch entirely and skips the new conflict check, so opentls() + secure() set alongside a keystore wouldn't throw. This matches the documented context-selection priority (keystore wins before opentls/secure are even considered), so it's consistent rather than a bug, just worth confirming that's the intended scope.

Tests look sound: assertThrows cases cover scheme-pair conflicts, explicit-flag conflicts, and the three-entry mixed list; the "accepted" cases correctly verify that an explicit flag or a provided SSLContext overrides scheme inference. Imports (java.util.*, org.junit.jupiter.api.Assertions.*) already cover Properties/assertThrows/assertEquals, so no missing-import issue.

Minor, non-blocking: the build() javadoc's @throws IllegalStateException line (Options.java:2379) still only mentions the token/user-pass conflict example; could mention the new SSL context conflict too, but this doesn't affect correctness.

No correctness, security, or thread-safety issues found.

[Jarema]

LGTM