Skip to content

Conversation

@alex-berger
Copy link
Contributor

@alex-berger alex-berger commented Oct 20, 2020

Add support to define the SASL mechanism for Kafka Changefeed DSNs as well as disabling TLS server certificate verification, which is important for debugging connectivity issue to Kafka. This PR is a result of support desk request 6569. This PR depends on cockroachdb/vendored#38.

Release note (ccl change): Add parameter to Kafka Changefeed DSNs to specify SASL mechanism.

Release note (ccl change): Add parameter to Kafka Changefeed DSNs to disable TLS server certificate and hostname verification.

Important: I am by no means an expert in go dependency management, so it might well be that I totally messed up the vendor submodule :-), Please feel free create a clean PR on you own based on the changes in this one.

@alex-berger alex-berger requested review from a team and pbardea and removed request for a team October 20, 2020 19:29
@blathers-crl
Copy link

blathers-crl bot commented Oct 20, 2020

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added the O-community Originated from the community label Oct 20, 2020
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Add support to define the SASL mechanism for Kafka Changefeed DSNs as well as disabling TLS server certificate verification, which is important for debugging connectivity issue to Kafka. This commit is a result of support desk request 6569.

Release note (ccl change): Add parameter to Kafka Changefeed DSNs to specify SASL mechanism.

Release note (ccl change): Add parameter to Kafka Changefeed DSNs to disable TLS server certificate and hostname verification.
@blathers-crl
Copy link

blathers-crl bot commented Oct 20, 2020

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@knz
Copy link
Contributor

knz commented Oct 20, 2020

@bdarnell @aaron-crl I would like you to review the vendoring changes and the overall extension in the security surface area in this PR as well as related cockroachdb/vendored#38

@knz knz added the A-security label Oct 20, 2020
@dt dt requested a review from ajwerner October 20, 2020 22:19
@knz
Copy link
Contributor

knz commented Oct 20, 2020

Hi Alex,
thank you for your contribution.
According to our contribution guidelines at https://wiki.crdb.io/wiki/spaces/CRDB/pages/181829940/Evaluate+the+complexity+of+your+project

it seems like the complexity of your project is "medium" or "high".

Can you point us to the design document / discussion that you'd have carried out first, where the reviewres can get a general idea of the approach you've taken prior to looking at the code?

@ajwerner
Copy link
Contributor

Thanks for this PR. I have some questions regarding the ability to disable SSL which we'll get to. In general, we are huge advocates of SSL and are not really inclined to make it easier to not use it, even if it may simplify some workflows.

I do agree it's critical that we add support for the modern password authentication mechanisms to kafka. We should work to ensure that that gets done and tested so that future cockroach releases lead to a better out-of-the-box experience. One thing we're very much missing currently is testing of the various authentication and connection options already supported. We'll need to address that before feeling comfortable merging new functionality.

@ajwerner
Copy link
Contributor

I've created #55788 to track the issue irrespective of this PR.

@alex-berger
Copy link
Contributor Author

@knz there is no design document for this rather straight forward and small addition. I am just a customer trying to point-out how this could be solved. Please check-in with CockroachCloud support request 6569 for more information on our use case.

@ajwerner I am also an advocate of TLS (SSL). In reality you can often leverage TLS for encryption, but you have to give-up on MTM protection by disabling hostname verification. There are several reasons for this:

  • if there sits a public loadbalancer (TCP, non TLS terminating) between the client and the TLS terminating server. In this case the DNS name of the loadbalancer will not match the DNS name in the X.509 certificate of the server.
  • ...

@mwang1026 mwang1026 removed the request for review from pbardea October 27, 2020 19:56
@HonoreDB HonoreDB added the A-cdc Change Data Capture label Jan 19, 2021
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heads up, we've got somebody who's just started who's going to be working to get this across the finish line. The big missing piece is automated testing. I say that not really just about this PR but about all of the Kafka connection stuff. There's lots of space to improve here. Thanks for pushing on us to do it!

cc @stevendanna

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@stevendanna
Copy link
Collaborator

#59797 adds some basic tests for authentication.

Once we get that in shape and merged, we should be able to rebase this and write a test for it. I've done the start of that here:

master...stevendanna:kafka-scram-support-with-tests

@stevendanna
Copy link
Collaborator

I've opened this #60150 which takes this change, rebases it against master, and adds some tests. I'm going to close this PR out to avoid any confusion. Thanks a ton @alex-berger for implementing this.

@stevendanna stevendanna closed this Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cdc Change Data Capture A-security O-community Originated from the community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants