Set SkipVerify when VerifyCA is false #182
Conversation
| remoteCAPath: "" | ||
| caServerName: "" | ||
| verifyCA: false | ||
| skipCAVerification: true |
There was a problem hiding this comment.
does it mean customer will need to set skipCAVerification to true now?
There was a problem hiding this comment.
Yes. In the new config, customers will have to proactively set this to true
| if serverConfig.VerifyCA { | ||
| tlsConfig.ClientAuth = tls.RequireAndVerifyClientCert | ||
| if !serverConfig.SkipCAVerification { | ||
| tlsConfig.ClientAuth = tls.RequireAnyClientCert |
There was a problem hiding this comment.
what is difference between RequireAndVerifyClientCert and RequireAnyClientCert?
There was a problem hiding this comment.
RequireAndVerifyClientCert checks the cert against the local CA chain. We don't need it, because we're already using a certificate allowlist.
encryption/tls.go
Outdated
| if !clientConfig.SkipCAVerification { | ||
| if clientConfig.CAServerName == "" || clientConfig.RemoteCAPath == "" { | ||
| return nil, errors.New("CAServerName and RemoteCAPath must be set when VerifyCA is true") | ||
| return nil, errors.New("CAServerName and RemoteCAPath must be set when SkipCAVerification is true") |
There was a problem hiding this comment.
SkipCAVerification is false?
| case config.ConnTypeMuxClient, config.ConnTypeMuxServer: | ||
| observer := NewReplicationStreamObserver(c.loggers.Get(LogStreamObserver)) | ||
| grpcServer, err := buildProxyServer(c, c.clusterDefinition.Connection.MuxAddressInfo.TLSConfig, observer.ReportStreamValue, lifetime) | ||
| grpcServer, err := buildProxyServer(c, encryption.TLSConfig{}, observer.ReportStreamValue, lifetime) |
There was a problem hiding this comment.
we are passing an empty encryption.TLSConfig{} here?
There was a problem hiding this comment.
The internal mux connection is already TLS, so the hosted grpc server should have no TLSConfig set. This accidental repeating of the TLS config is what actually broke main.
encryption/tls.go
Outdated
| func GetServerTLSConfig(serverConfig TLSConfig, logger log.Logger) (tlsConfig *tls.Config, err error) { | ||
| func GetServerTLSConfig(name string, serverConfig TLSConfig, logger log.Logger) (tlsConfig *tls.Config, err error) { | ||
| if !serverConfig.IsEnabled() { | ||
| logger.Info(fmt.Sprintf("TLS disabled for %s", name)) |
There was a problem hiding this comment.
if name just used for logging, can we set a label in logger instead of passing as an input parameter?
There was a problem hiding this comment.
Fair point, will update
…ork properly with TLS
What was changed
Why?
Fix local TLS auth for the main branch