Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ccl: add support for additional sasl mechanisms (SCRAM-SHA-256 and SCRAM-SHA-512) #60150

Merged
merged 2 commits into from
Feb 17, 2021

Conversation

stevendanna
Copy link
Collaborator

This add support to specify the SASL mechanism for Kafka Changefeed
DSNs via the sasl_mechanism option. Further, we add support for
SCRAM-SHA-256 and SCRAM-SHA-512 in addition to the previously
supported PLAIN mechanism.

Release note (ccl change): Add sasl_mechanism parameter to Kafka
Changefeed DSNs to specify SASL mechanism. CockroachDB supports the
PLAIN, SCRAM-SHA-256, and SCRAM-SHA-512 SASL mechanisms.

@stevendanna stevendanna requested review from a team and dt and removed request for a team February 9, 2021 16:20
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@stevendanna stevendanna changed the title ccl: add support for additional sasl mechanism (SCRAM-SHA-256 and SCRAM-SHA-512) ccl: add support for additional sasl mechanisms (SCRAM-SHA-256 and SCRAM-SHA-512) Feb 9, 2021
Copy link
Contributor

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dt and @stevendanna)


pkg/cmd/roachtest/cdc.go, line 550 at r2 (raw file):

	if err := db.QueryRow(
		`CREATE CHANGEFEED FOR auth_test_table INTO $1`,
		fmt.Sprintf("%s?tls_enabled=true&ca_cert=%s&sasl_enabled=true&sasl_user=plain&sasl_password=plain-secret", kafka.sinkURLSASL(ctx), testCerts.CACertBase64()),

Are we losing all our tests of non-SCRAM logins here?


pkg/cmd/roachtest/cdc.go, line 921 at r2 (raw file):

   org.apache.kafka.common.security.plain.PlainLoginModule required
   username="admin"
   password="admin-secret"

We should probably use different usernames/passwords for the different login methods so the test makes sure that we're not succeeding on the plaintext path and all the SASL stuff is getting ignored.

Copy link
Collaborator Author

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell and @dt)


pkg/cmd/roachtest/cdc.go, line 550 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Are we losing all our tests of non-SCRAM logins here?

D'oh. Thanks for catching this. We can test all three, I just was too aggressive when fixing a rebase conflict.


pkg/cmd/roachtest/cdc.go, line 921 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

We should probably use different usernames/passwords for the different login methods so the test makes sure that we're not succeeding on the plaintext path and all the SASL stuff is getting ignored.

We do use different usernames for each login method that is used from cockroach. I've kept the "admin" password the same, but that should only be used by kafka internally for inter-broker communication.

Copy link
Collaborator Author

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell and @dt)


pkg/cmd/roachtest/cdc.go, line 550 at r2 (raw file):

Previously, stevendanna (Steven Danna) wrote…

D'oh. Thanks for catching this. We can test all three, I just was too aggressive when fixing a rebase conflict.

Done.

@stevendanna stevendanna force-pushed the kafka-scram-support-with-tests branch 2 times, most recently from 792aae5 to bb7a238 Compare February 16, 2021 13:35
alex-berger and others added 2 commits February 16, 2021 13:43
Add support to specify the SASL mechanism for Kafka Changefeed
DSNs via the `sasl_mechanism` option. Further, we add support for
SCRAM-SHA-256 and SCRAM-SHA-512 in addition to the previously
supported PLAIN mechanism.

Co-authored-by: Steven Danna <danna@cockroachlabs.com>

Release note (ccl change): Add `sasl_mechanism` parameter to Kafka
Changefeed DSNs to specify SASL mechanism. CockroachDB supports the
PLAIN, SCRAM-SHA-256, and SCRAM-SHA-512 SASL mechanisms.
@stevendanna
Copy link
Collaborator Author

bors r=bdarnell

@craig
Copy link
Contributor

craig bot commented Feb 17, 2021

Build succeeded:

@Neustradamus
Copy link

@stevendanna: Thanks a lot for all about SCRAM.

Linked to:

@Neustradamus

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants