Exposes additional configuration knobs for TLS and updates docker-config so they can be set using env variables #869
Conversation
| @@ -77,57 +92,142 @@ func (s *localStoreCertProvider) FetchServerCertificate() (*tls.Certificate, err | |||
| return s.serverCert, nil | |||
| } | |||
|
|
|||
| func (s *localStoreCertProvider) fetchServerCertificateFromInline() (*tls.Certificate, error) { | |||
| if s.tlsSettings.Server.CertData == "" { | |||
There was a problem hiding this comment.
Shouldn't we return an error here?
| if !s.tlsSettings.Server.InlineData && len(s.tlsSettings.Server.ClientCAFiles) == 0 { | ||
| return nil, nil | ||
| } | ||
| if s.tlsSettings.Server.InlineData && len(s.tlsSettings.Server.ClientCaData) == 0 { | ||
| return nil, nil |
There was a problem hiding this comment.
Shouldn't we return an error here?
There was a problem hiding this comment.
we were not returning an error before my change, but am not opposed to returnong one here given that a user who requires mutual authentication and doesn't set this has a broken environment
There was a problem hiding this comment.
Disregard. My mistake, sorry.
| for _, ca := range caFiles { | ||
| if ca == "" { |
There was a problem hiding this comment.
did this so that we are not forced to create pass redundant variables as we did in the tls sample
| s.RLock() | ||
| if s.serverCAs != nil { | ||
| defer s.RUnlock() | ||
| return s.clientCAs, nil | ||
| return s.serverCAs, nil |
There was a problem hiding this comment.
this looks like this was an existing bug?
common/cassandra/cassandraCluster.go
Outdated
| certBytes, err = ioutil.ReadFile(cfg.TLS.CertFile) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error reading client certificate file: %w", err) | ||
| } | ||
| } | ||
|
|
||
| if cfg.TLS.CertData != "" { |
There was a problem hiding this comment.
Nit: Would putting this inside an else from the previous if increase readability?
There was a problem hiding this comment.
sure, i can do else if.
| } | ||
|
|
||
| if cfg.TLS.CaData != "" && cfg.TLS.CaFile != "" { | ||
| return nil, errors.New("Cannot specify both caData and caFile properties") |
There was a problem hiding this comment.
Maybe this should be additive though? Because users may want multiple CA with some being intermediate and some specified in files with others in bytes?
There was a problem hiding this comment.
talked offline. we'll keep it simple for now (also removed merging behavior for the regular TLS specifications), but if there is user interest, then we can add in merging fairly easily
Tested via unit tests and a private deployment