KAFKA-9577: SaslClientAuthenticator incorrectly negotiates SASL_HANDSHAKE version by lbradstreet · Pull Request #8142 · apache/kafka

lbradstreet · 2020-02-20T19:13:23Z

The SaslClientAuthenticator incorrectly negotiates supported SaslHandshakeRequest version and uses the maximum version supported by the broker whether or not the client supports it.

This bug was exposed by a recent version bump in 0a2569e.

This PR rolls back the recent SaslHandshake[Request,Response] bump, fixes the version negotiation, and adds a test to prevent anyone from accidentally bumping the version without a workaround such as a new ApiKey. The existing key will be difficult to support for clients < 2.5 due to the incorrect negotiation.

Tests:

Prevent SASL_HANDSHAKE schema version bump
Add test to return ApiVersions unsupported by client

SaslHandshakeRequest versions and uses the max versions supported by the broker whether or not the client supports it. This PR rolls back the SaslHandshake[Request,Response] bump, fixes the version negotiation, and adds a test to prevent anyone from accidentally bumping the version without a workaround (e.g. a new ApiKey).

lbradstreet · 2020-02-20T19:17:47Z

                    break;
                else {
-                    saslAuthenticateVersion(apiVersionsResponse);
+                    saslApiVersions(apiVersionsResponse);


Set the versions for both auth and handshake the same way.

nit: saslAuthenticateAndHandshakeVersions() is more intuitive to me (100% subjective, of course; I mention it only in case it didn't occur to you)

ijuma · 2020-02-20T19:18:01Z

cc @dajac

lbradstreet · 2020-02-20T19:18:04Z

-  // Version 2 adds flexible version support
-  "validVersions": "0-2",
-  "flexibleVersions": "2+",
+  // NOTE: Version cannot be easily bumped due to incorrect


Rollback version with comment

lbradstreet · 2020-02-20T19:18:45Z

     */
    @Test
-    public void testApiVersionsRequestWithUnsupportedVersion() throws Exception {
+    public void testApiVersionsRequestWithServerUnsupportedVersion() throws Exception {


Rename as this only tested part of the version support.

lbradstreet · 2020-02-20T19:18:56Z


+
+    @Test
+    public void testForBrokenSaslHandshakeVersionBump() {


Block anyone from bumping the schema without realizing.

rondagostino

The fact that this lives on the client indeed makes it tough to bump the SaslHandshake version -- older clients without this fix will run into trouble. Releasing patch versions (2.4.1, etc.) gives an easier client upgrade path, but I'm wondering if there is there a way to make the broker respond intelligently about its max version for SaslHandshake? Does it know the version of the client making the ApiVersionsRequest request, and if so, it could downgrade its response for SaslHandshake for older clients? It feels like an awful solution, but I figured I would suggest it anyway.

rondagostino · 2020-02-20T19:37:53Z

                    break;
                else {
-                    saslAuthenticateVersion(apiVersionsResponse);
+                    saslApiVersions(apiVersionsResponse);


nit: saslAuthenticateAndHandshakeVersions() is more intuitive to me (100% subjective, of course; I mention it only in case it didn't occur to you)

failure message.

lbradstreet · 2020-02-20T20:59:19Z

The fact that this lives on the client indeed makes it tough to bump the SaslHandshake version -- older clients without this fix will run into trouble. Releasing patch versions (2.4.1, etc.) gives an easier client upgrade path, but I'm wondering if there is there a way to make the broker respond intelligently about its max version for SaslHandshake? Does it know the version of the client making the ApiVersionsRequest request, and if so, it could downgrade its response for SaslHandshake for older clients? It feels like an awful solution, but I figured I would suggest it anyway.

Client version detection is one possibility as we do get supplied with client versions nowadays. I haven't thought too deeply about potential solutions though.

rajinisivaram · 2020-02-21T10:57:34Z

One alternative would be to update SaslServerAuthenticator to return 1 as the maximum handshake version when ApiVersions request version is zero. Clients always send ApiVersionsRequest with version 0 as their first request when using SASL because they don't know what the broker supports. At this time, broker doesn't know what the client supports either. As long as we don't return handshake version > 1 for this first request, I think we should be fine.

When we do want to use handshake requests with version > 1 in the future because we want to make an actual change to handshake, we can update clients which know about this updated handshake protocol to send a second ApiVersions request with the latest supported ApiVersions request version derived from the broker's first ApiVersions response in order to get the broker's actual max handshake version. We could add this flow now to enable testing, but we don't have to because handshake hasn't really changed.

lbradstreet · 2020-02-21T19:46:55Z

One alternative would be to update SaslServerAuthenticator to return 1 as the maximum handshake version when ApiVersions request version is zero. Clients always send ApiVersionsRequest with version 0 as their first request when using SASL because they don't know what the broker supports. At this time, broker doesn't know what the client supports either. As long as we don't return handshake version > 1 for this first request, I think we should be fine.

When we do want to use handshake requests with version > 1 in the future because we want to make an actual change to handshake, we can update clients which know about this updated handshake protocol to send a second ApiVersions request with the latest supported ApiVersions request version derived from the broker's first ApiVersions response in order to get the broker's actual max handshake version. We could add this flow now to enable testing, but we don't have to because handshake hasn't really changed.

That makes sense as an option. For this PR I think we should avoid any further changes as trunk is currently broken and this is a blocker for 2.5.

cmccabe · 2020-02-21T21:09:50Z

-  "validVersions": "0-2",
-  "flexibleVersions": "2+",
+  // NOTE: Version cannot be easily bumped due to incorrect
+  // client negotiation. See https://issues.apache.org/jira/browse/KAFKA-9577


Can you specify the client versions with the incorrect negotiation? 2.4 and earlier, right?

That's right.

cmccabe · 2020-02-21T21:10:49Z

LGTM pending one comment

hachikuji

LGTM. I think we have some options to bump this protocol again in the future, but there seems to be no pressing need for it now.

hachikuji · 2020-02-21T21:47:35Z

ok to test

hachikuji · 2020-02-21T21:47:44Z

retest this please

…HAKE version (#8142) The SaslClientAuthenticator incorrectly negotiates supported SaslHandshakeRequest version and uses the maximum version supported by the broker whether or not the client supports it. This bug was exposed by a recent version bump in 0a2569e. This PR rolls back the recent SaslHandshake[Request,Response] bump, fixes the version negotiation, and adds a test to prevent anyone from accidentally bumping the version without a workaround such as a new ApiKey. The existing key will be difficult to support for clients < 2.5 due to the incorrect negotiation. Reviewers: Ron Dagostino <rdagostino@confluent.io>, Rajini Sivaram <rajinisivaram@googlemail.com>, Colin P. McCabe <cmccabe@apache.org>, Jason Gustafson <jason@confluent.io>

@test

* apache-github/trunk: (23 commits) KAFKA-9530; Fix flaky test `testDescribeGroupWithShortInitializationTimeout` (apache#8154) HOTFIX: fix NPE in Kafka Streams IQ (apache#8158) MINOR: set scala version automatically based on gradle.properties KAFKA-9577; SaslClientAuthenticator incorrectly negotiates SASL_HANDSHAKE version (apache#8142) KAFKA-9441: Add internal TransactionManager (apache#8105) MINOR: Document endpoints for connector topic tracking (KIP-558) MINOR: Standby task commit needed when offsets updated (apache#8146) KAFKA-9206; Throw KafkaException on CORRUPT_MESSAGE error in Fetch response (apache#8111) MINOR: Remove unwanted regexReplace on tests/kafkatest/__init__.py KAFKA-9586: Fix errored json filename in ops documentation KAFKA-9575: Mention ZooKeeper 3.5.7 upgrade KAFKA-9481: Graceful handling TaskMigrated and TaskCorrupted (apache#8058) HOTFIX: don't try to remove uninitialized changelogs from assignment & don't prematurely mark task closed (apache#8140) MINOR: Fix javadoc at org.apache.kafka.clients.producer.KafkaProducer.InterceptorCallback#onCompletion (apache#7337) MINOR: Improve EOS example exception handling (apache#8052) MINOR: Fix a number of warnings in clients test (apache#8073) MINOR: Update shell scripts to support z/OS system (apache#7913) MINOR: Wording fix in Streams DSL docs (apache#5692) MINOR: Add missing @test annotation to MetadataTest#testMetadataMerge (apache#8141) KAFKA-9533: ValueTransform forwards `null` values (apache#8108) ...

@test

…etrics-common * confluent/master: (76 commits) KAFKA-9530; Fix flaky test `testDescribeGroupWithShortInitializationTimeout` (apache#8154) HOTFIX: fix NPE in Kafka Streams IQ (apache#8158) MINOR: set scala version automatically based on gradle.properties KAFKA-9577; SaslClientAuthenticator incorrectly negotiates SASL_HANDSHAKE version (apache#8142) KAFKA-9441: Add internal TransactionManager (apache#8105) MINOR: Document endpoints for connector topic tracking (KIP-558) MINOR: Standby task commit needed when offsets updated (apache#8146) Changes to migrate to Artifactory (#263) KAFKA-9206; Throw KafkaException on CORRUPT_MESSAGE error in Fetch response (apache#8111) MINOR: Remove unwanted regexReplace on tests/kafkatest/__init__.py KAFKA-9586: Fix errored json filename in ops documentation KAFKA-9575: Mention ZooKeeper 3.5.7 upgrade KAFKA-9481: Graceful handling TaskMigrated and TaskCorrupted (apache#8058) HOTFIX: don't try to remove uninitialized changelogs from assignment & don't prematurely mark task closed (apache#8140) MINOR: Fix javadoc at org.apache.kafka.clients.producer.KafkaProducer.InterceptorCallback#onCompletion (apache#7337) MINOR: Improve EOS example exception handling (apache#8052) MINOR: Fix a number of warnings in clients test (apache#8073) MINOR: Update shell scripts to support z/OS system (apache#7913) MINOR: Wording fix in Streams DSL docs (apache#5692) MINOR: Add missing @test annotation to MetadataTest#testMetadataMerge (apache#8141) ...

…HAKE version (apache#8142) The SaslClientAuthenticator incorrectly negotiates supported SaslHandshakeRequest version and uses the maximum version supported by the broker whether or not the client supports it. This bug was exposed by a recent version bump in apache@0a2569e. This PR rolls back the recent SaslHandshake[Request,Response] bump, fixes the version negotiation, and adds a test to prevent anyone from accidentally bumping the version without a workaround such as a new ApiKey. The existing key will be difficult to support for clients < 2.5 due to the incorrect negotiation. Reviewers: Ron Dagostino <rdagostino@confluent.io>, Rajini Sivaram <rajinisivaram@googlemail.com>, Colin P. McCabe <cmccabe@apache.org>, Jason Gustafson <jason@confluent.io>

apache#8142) * KENGINE-283 Topic ID Mismatch Issue - Exception added to disallow Alter Topic with Zookeeper Flag * Addressed PR Comments and added a check to see if TopicID still exists after the command is stopped * Addressed PR Comments and added a check to see if TopicID still exists after the command is stopped * Changed exception message * Changed exception message * Changed exception message

…er Topic with Zookeeper Flag (#844) * KENGINE-283 Topic ID Mismatch Issue - Exception added to disallow Alt… (apache#8142) * KENGINE-283 Topic ID Mismatch Issue - Exception added to disallow Alter Topic with Zookeeper Flag * Addressed PR Comments and added a check to see if TopicID still exists after the command is stopped * Addressed PR Comments and added a check to see if TopicID still exists after the command is stopped * Changed exception message * Changed exception message * Changed exception message * Don't need assignment * Nit change

…er Topic with Zookeeper Flag (#844) (#851) * KENGINE-283 Topic ID Mismatch Issue - Exception added to disallow Alt… (apache#8142) * KENGINE-283 Topic ID Mismatch Issue - Exception added to disallow Alter Topic with Zookeeper Flag * Addressed PR Comments and added a check to see if TopicID still exists after the command is stopped * Addressed PR Comments and added a check to see if TopicID still exists after the command is stopped * Changed exception message * Changed exception message * Changed exception message * Don't need assignment * Nit change

…er Topic with Topic ID present (#852) * KENGINE-283 Topic ID Mismatch Issue - Exception added to disallow Alter Topic with Zookeeper Flag (#844) * KENGINE-283 Topic ID Mismatch Issue - Exception added to disallow Alt… (apache#8142) * KENGINE-283 Topic ID Mismatch Issue - Exception added to disallow Alter Topic with Zookeeper Flag * Addressed PR Comments and added a check to see if TopicID still exists after the command is stopped * Addressed PR Comments and added a check to see if TopicID still exists after the command is stopped * Changed exception message * Changed exception message * Changed exception message * Don't need assignment * Nit change * Nit change

…er Topic with Topic ID present (#852) (#854) * KENGINE-283 Topic ID Mismatch Issue - Exception added to disallow Alter Topic with Zookeeper Flag (#844) * KENGINE-283 Topic ID Mismatch Issue - Exception added to disallow Alt… (apache#8142) * KENGINE-283 Topic ID Mismatch Issue - Exception added to disallow Alter Topic with Zookeeper Flag * Addressed PR Comments and added a check to see if TopicID still exists after the command is stopped * Addressed PR Comments and added a check to see if TopicID still exists after the command is stopped * Changed exception message * Changed exception message * Changed exception message * Don't need assignment * Nit change * Nit change

lbradstreet changed the title ~~KAFKA-9577: SaslClientAuthenticator incorrectly negotiates supported SaslHandshakeRequest version~~ KAFKA-9577: SaslClientAuthenticator incorrectly negotiates SASL_HANDSHAKE version Feb 20, 2020

ijuma requested review from cmccabe, hachikuji and rajinisivaram February 20, 2020 19:17

lbradstreet commented Feb 20, 2020

View reviewed changes

rondagostino reviewed Feb 20, 2020

View reviewed changes

saslApiVersions -> setSaslAuthenticateAndHandshakeVersions. Improve test

de7274f

failure message.

cmccabe reviewed Feb 21, 2020

View reviewed changes

Better document bad negotiation

fb7ce37

hachikuji approved these changes Feb 21, 2020

View reviewed changes

hachikuji merged commit 1a8dcff into apache:trunk Feb 22, 2020

Conversation

lbradstreet commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Feb 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rondagostino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lbradstreet commented Feb 20, 2020

Uh oh!

rajinisivaram commented Feb 21, 2020

Uh oh!

lbradstreet commented Feb 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmccabe commented Feb 21, 2020

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

hachikuji commented Feb 21, 2020

Uh oh!

hachikuji commented Feb 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lbradstreet commented Feb 20, 2020 •

edited

Loading