[FEAT] Add server_name field in ClientTLS auth request claims #210
Conversation
| @@ -70,6 +70,7 @@ type ClientTLS struct { | |||
| Cipher string `json:"cipher,omitempty"` | |||
| Certs StringList `json:"certs,omitempty"` | |||
| VerifiedChains []StringList `json:"verified_chains,omitempty"` | |||
| ServerName string `json:"server_name,omitempty"` | |||
There was a problem hiding this comment.
Should this be HostName? Would that be more descriptive?
There was a problem hiding this comment.
I used the same terminology as crypto/tls: https://pkg.go.dev/crypto/tls#ConnectionState, and I though that in the context of a ClientTLS block, ServerName would have a clear meaning.
But I understand that it may be misleading due to the fact that a server also has a server_name, so I would like for you to make the choice 😅
There was a problem hiding this comment.
Good point, we can stick with ServerName or SNI.
There was a problem hiding this comment.
I think that SNI may be the best candidate here. Googling "tls sni" gives pretty relevant results, and it does not reuse an existing field name from other claims data.
If that's ok with you, I can push a commit with this modification.
There was a problem hiding this comment.
The change would be:
VerifiedChains []StringList `json:"verified_chains,omitempty"`
- ServerName string `json:"server_name,omitempty"`
+ SNI string `json:"sni,omitempty"`There was a problem hiding this comment.
Looping in @philpennock and @ripienaar for their opinions here. I could do either.
This PR introduces a new field on ClientTLS struct to hold the server name used in TLS handshake.
Discussed in nats-io/nats-server#4706