[ADDED] Additional TLS configuration Flag to allow VerifyClientCertIfGiven to be set for the TLS Connection - #8351
Conversation
…Given to be set for the TLS Connection While the verify flag sets the RequireAndVerifyClientCert for TLS The new verify_client_cert_if_given flag sets the VerifyClientCertIfGiven for TLS The verify and verify_client_cert_if_given flags can not be used together. Signed-off-by: Jamie MacPherson <jamie.macpherson@lawo.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3678fca50
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if tc.VerifyClientCertIfGiven { | ||
| config.ClientAuth = tls.VerifyClientCertIfGiven |
There was a problem hiding this comment.
Reject conflicting TLS client-auth modes in GenTLSConfig
When TLSConfigOpts is built directly, such as by embedders/tests calling the exported GenTLSConfig instead of going through parseTLS or overrideTLS, setting both Verify and VerifyClientCertIfGiven silently reaches this branch and overwrites RequireAndVerifyClientCert with VerifyClientCertIfGiven. That contradicts the new mutual-exclusion rule and can downgrade a configuration that expected client certificates to be mandatory into one where clients without certificates are accepted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Will check in another commit to add an additional check when GenTLSConfig is called directly by test cases
…Given to be set for the TLS Connection While the verify flag sets the RequireAndVerifyClientCert for TLS The new verify_client_cert_if_given flag sets the VerifyClientCertIfGiven for TLS The verify and verify_client_cert_if_given flags can not be used together. Signed-off-by: Jamie MacPherson <jamie.macpherson@lawo.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6e5e12aa9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| case "verify_client_cert_if_given": | ||
| verifyClientCertIfGiven, ok := mv.(bool) | ||
| if !ok { | ||
| return nil, &configErr{tk, "error parsing tls config, expected 'verify_client_cert_if_given' to be a boolean"} | ||
| } | ||
| tc.VerifyClientCertIfGiven = verifyClientCertIfGiven |
There was a problem hiding this comment.
Handle optional client auth in cluster/gateway TLS
When verify_client_cert_if_given is used inside cluster or gateway TLS blocks, this shared parser accepts the option, but those callers go through getTLSConfig, which unconditionally overwrites ClientAuth with tls.RequireAndVerifyClientCert. A configuration that asks for optional client certificates is therefore accepted but still rejects route/gateway peers that do not present a certificate; reject this option in those contexts or preserve VerifyClientCertIfGiven when it is set.
Useful? React with 👍 / 👎.
|
The purpose of this change is optionally supporting Client certificates to be passed through to the auth callout. This needs to be made clear. Otherwise it acts like a security downgrade (verify=false) without clear benefit. |
|
OK, the change does what it is supposed to do. This behavior should be documented in opts.go where config.ClientAuth is being set. Specifically document:
Also comment that RequireAnyClientCert and RequestClientCert are MUST not be possible to configure. There are code paths which assume that client certificates are always valid on accepted connection. Specifically the auth callout operates under the contract that client certificates presented to it are already validated. That the main purpose to configure VerifyClientCertIfGiven is optional certificate or token based authentication in a the custom auth callout. -- Analysis by Claude------ Here's what I found tracing config.ClientAuth through the codebase. GenTLSConfig (opts.go:5799-5802) — set only when tc.Verify is true, i.e. the user put verify: true in a tls { } block. This is the client-facing path. Default when verify is absent is Go's zero value tls.NoClientCert. createClient → doTLSServerHandshake(_, opts.TLSConfig, ...) (server.go:3496) → doTLSHandshake → tls.Server(c.nc, tlsConfig) then conn.Handshake() (client.go:6565, 6576). Whether a CertificateRequest is sent and whether the cert is verified is decided entirely by tlsConfig.ClientAuth inside conn.Handshake(). Everywhere else the field is only read to derive advertised/reported state, not to enforce: NewServer (server.go:702): verify := tlsReq && ClientAuth == RequireAndVerifyClientCert → sets info.TLSVerify, which is advertised to clients in the INFO protocol. So enforcement is indirect and singular: it all funnels through conn.Handshake() with whatever ClientAuth was baked into opts.TLSConfig. tls.RequireAndVerifyClientCert — always demand + verify (needs ClientCAs populated or the handshake fails). Since the handshake consumes opts.TLSConfig by reference, the cleanest injection point is doTLSHandshake (clone tlsConfig, override ClientAuth, and ensure ClientCAs is set), rather than mutating the shared config. Note the config is currently passed shared/unlocked, so mutating it in place would be racy across connections — clone it. It iterates cs.VerifiedChains into claim.TLS.VerifiedChains. Go only populates VerifiedChains when verification actually occurred — i.e. ClientAuth was VerifyClientCertIfGiven or RequireAndVerifyClientCert and the client's cert chained to a configured ClientCAs. So concretely: NoClientCert (option unset): no cert requested → both empty → callout gets only version/cipher, no certs. That means if you want the auth callout to receive a verified chain, bumping ClientAuth alone isn't enough — you also need ClientCAs set so Go can build and populate VerifiedChains; otherwise the cert lands in the unverified Certs field. |
…Given to be set for the TLS Connection While the verify flag sets the RequireAndVerifyClientCert for TLS The new verify_client_cert_if_given flag sets the VerifyClientCertIfGiven for TLS The verify and verify_client_cert_if_given flags can not be used together. Signed-off-by: Jamie MacPherson <jamie.macpherson@lawo.com>
|
@roeschter Thanks Michael , I have pushed another commit with the documentation changes you have suggested |
The NATS verify flag sets the RequireAndVerifyClientCert for TLS.
The new verify_client_cert_if_given flag sets the VerifyClientCertIfGiven for TLS.
The verify and verify_client_cert_if_given flags can not be used together.
Use case
The work is similar to the proposal in #4739 .
By introducing a new verify_client_cert_if_given flag which sets the ClientAuthMode to VerifyClientCertIfGiven
this allows clients to provide either certificates or no certificates. This means the custom ClientAuthentication interface has
available to it the client certificate information if provided for authentication or can use another form of authentication such
NKEYS.
Hence the server can support connections from clients using two different authentication mechanisms (client certificates or
NKEYS) , without the need to run up two NATS Servers with separate verify and non verify configurations.