Add ability to talk to Kafka over TLS rather than plaintext #21
Conversation
|
@aeijdenberg thanks for the PR! A few early feedbacks:
|
kafka.go
Outdated
| @@ -27,6 +30,38 @@ func NewKafkaProducer(logger *log.Logger, stats *Stats, config *Config) (NozzleP | |||
| // TODO (tcnksm): Enable to configure more properties. | |||
| producerConfig := sarama.NewConfig() | |||
|
|
|||
| if !config.Kafka.InsecurePlainText { | |||
| if len(config.Kafka.CACerts) == 0 { | |||
| return nil, errors.New("please specify at least one ca_certificate, else set insecure_use_plaintext") | |||
There was a problem hiding this comment.
is specifying additional CA certs really required? what if the kafka cert is already signed by a trusted CA?
There was a problem hiding this comment.
I agree it would be sensible to use the system trusted roots if no CA certs are specified - I'll update the PR to do so (am on holiday break now, so will be likely be in a few weeks).
There was a problem hiding this comment.
Changed to default to system CA pool if no CAs are specified.
kafka.go
Outdated
| return nil, errors.New("please specify at least one ca_certificate, else set insecure_use_plaintext") | ||
| } | ||
| if config.Kafka.ClientCert == "" { | ||
| return nil, errors.New("please specify client_certificate, else set insecure_use_plaintext") |
There was a problem hiding this comment.
I don't know if strictly required by Kafka or not - but I'm not sure it would be sensible not to. ie the client cert is used to authenticate the client to Kafka - so if not presented I guess at the least the connection is encrypted, but the server would still be wide open. (I may be missing a use-case?)
|
Apparently |
|
Upstream Have rebased on current Note that I contributed some of those same tests into |
@aeijdenberg no, feel free to bump to 1.9.x+tip |
|
@CAFxX - is there anything else that you're waiting for me on? I think I fixed the last tests (failing on Go 1.6 - by simply excluding Go 1.6 from testing). |
|
Should be OK to merge, I'm just waiting for internal approval to proceed (sorry!) |
|
@aeijdenberg merged and closed, thanks! |
|
@CAFxX - you are most welcome! Now I need to go and finish work on the other side - kafka-boshrelease... |
(and make this the default unless opt-in to no TLS)
To see an example of this being used, see https://github.com/govau/kafka-boshrelease/tree/moartls (particularly the example operator files that add it to a
cf-deployment) - I'm still working out how best to make a PR there.I'm guessing that in order to accept this PR, you might want to flip the default back to current state (ie plaintext), but I thought I'd start with "secure by default", as that seems to match best the "insecure_skip_ssl_verify" and similar flags used elsewhere.