Add ability to talk to Kafka over TLS rather than plaintext

rakutentech / kafka-firehose-nozzle

Forward logs from the Cloud Foundry Firehose to Apache Kafka

MIT License

13 stars 8 forks source link

Add ability to talk to Kafka over TLS rather than plaintext #21

Closed aeijdenberg closed 6 years ago

aeijdenberg commented 6 years ago

(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.

CAFxX commented 6 years ago

@aeijdenberg thanks for the PR! A few early feedbacks:

There seem to be no tests that cover TLS... can you add some, e.g. TLS enabled and cert match, TLS enabled and cert mismatch, incoherent options? No need to run all existing tests in all configurations (TLS enabled and disabled) but at least some tests should cover this, since refactorings may lead to security issues.
(cosmetic) I would prefer to maintain the Sarama option names, since they map one-to-one. Most of them already do so this would mean mostly just renaming InsecurePlainText to EnableTLS or something like that, and default it to false (as you suggested in the PR description)
I'll add other feedback inline

aeijdenberg commented 6 years ago

Apparently x509.SystemCertPool() was added in in Golang 1.7, and I guess the CI is on an older version? Let me know if it's important to support older versions.

aeijdenberg commented 6 years ago

Upstream sarama has now merged the PR I sent to add a function needed for testing.

Have rebased on current master with a commit to update the vendored version of sarama, and then squashed the other commits for this change into one, including some basic tests.

Note that I contributed some of those same tests into sarama as well, so I don't know if we should repeat them here or not.

CAFxX commented 6 years ago

Apparently x509.SystemCertPool() was added in in Golang 1.7, and I guess the CI is on an older version? Let me know if it's important to support older versions.

@aeijdenberg no, feel free to bump to 1.9.x+tip

aeijdenberg commented 6 years ago

@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).

CAFxX commented 6 years ago

Should be OK to merge, I'm just waiting for internal approval to proceed (sorry!)

CAFxX commented 6 years ago

@aeijdenberg merged and closed, thanks!

aeijdenberg commented 6 years ago

@CAFxX - you are most welcome! Now I need to go and finish work on the other side - kafka-boshrelease...