KAFKA-3618: Handle ApiVersionsRequest before SASL authentication by rajinisivaram · Pull Request #1286 · apache/kafka

rajinisivaram · 2016-04-28T21:53:39Z

Server-side implementation and tests for handling ApiVersionsRequest before SaslHandshakeRequest.

rajinisivaram · 2016-04-28T22:00:28Z

@ijuma I have rebased the PR for KAFKA-3618. Sorry I closed the old one and opened another one since the old one wasn't linked to the JIRA properly. I have addressed the comments on the other one. Thank you for the reviews.

ijuma · 2016-04-28T22:24:52Z

Shouldn't we be using SASL_PLAINTEXT as this point? Also, it may be worth including a test with SASL_SSL as well for this case.

@ijuma This test uses PLAINTEXT for the client connection rather than SASL_PLAINTEXT to bypass the authenticators which do the SASL handshake. Instead the test sends the version request and then does the authentication within the test. So even though the client protocol is set to PLAINTEXT it simulates SASL_PLAINTEXT and talks to a SASL server. I can add a similar one with SSL too.
That reminds me - I had a question about whether we need the Java client to request ApiVersions before SASL handshake. There is no real reason to do that now since we have only one version of handshake request, but just want to make sure.

@gwenshap looked into this and showed me why it doesn't work. Could we use a plain socket to send the api versions request to avoid the issue? It's a bit difficult to verify that we are doing the right thing with the current test.

For the second question, the current thinking is that we will do that after 0.10.0.0.

@ijuma Sorry, I was about to say I have added an SSL test as well. But I think I have missed the point. Can you explain? Do you mean use a blocking socket instead of a non-blocking channel?

@ijuma is on the way to London now, so I'll jump in for a bit :)

What we mean is that the whole point of the test is to show that the broker can reply to an ApiVersionRequest on the SASL port before doing the handshake. Current test doesn't really validate that.

@ijuma suggested simply opening a socket (low level java type, the kind we use in SocketServer tests) to the SASL_PLAIN / SASL_SSL port, sending an ApiVersionRequest and checking the result.

Does that make sense? We are open to other suggestions on how to validate this patch.

I saw the client doesn't use SASL, and I know that if it would, the test would fail because our current SASL client tries to authenticate before sending ApiVersionRequest.

However, the requirements for this patch were to allow clients to send ApiVersionRequest to SASL port before performing SASL authentication. So we need to test them...

Just to clarify, my main issue is that we added a code path in SaslServerAuthenticator class, but the tests don't touch the new code path because they don't connect in a way that goes through the SaslServerAuthenticator.

@gwenshap @ijuma SaslAuthenticatorTest is a unit test that doesn't use a Kafka broker. It uses an echo server that simply echoes back random data sent by the client. So it does indeed test the code path added by this PR. I have added more comments to the code in case this was not clear. I have also added a test in core that sends and validates ApiVersionsRequest on a Kafka broker that uses SASL.

@rajinisivaram Can you point to which specific parts of the PR are tested by this unit test?

I placed a breakpoint in SaslServerAuthenticator.java, line 296 (switch (apiKey), just where you added the special case for API_VERSION) and the test does not hit this particular method at all.

I will check the additional test, if it provides sufficient coverage, then I'm happy :)

@gwenshap Debug log from the SaslAuthenticatorTest.testUnauthenticatedApiVersionsRequestOverPlaintext():

[2016-04-29 16:13:59,039] DEBUG Handle Kafka request API_VERSIONS (org.apache.kafka.common.security.authenticator.SaslServerAuthenticator:295) [2016-04-29 16:13:59,051] DEBUG Handle Kafka request SASL_HANDSHAKE (org.apache.kafka.common.security.authenticator.SaslServerAuthenticator:295) [2016-04-29 16:13:59,051] DEBUG Using SASL mechanism 'PLAIN' provided by client (org.apache.kafka.common.security.authenticator.SaslServerAuthenticator:335) [2016-04-29 16:13:59,054] DEBUG Set SASL server state to AUTHENTICATE (org.apache.kafka.common.security.authenticator.SaslServerAuthenticator:264) [2016-04-29 16:13:59,055] DEBUG Set SASL server state to COMPLETE (org.apache.kafka.common.security.authenticator.SaslServerAuthenticator:264)

ijuma · 2016-04-29T14:17:35Z

+        int secondsLeft = 30;
+        while (!selector.isChannelReady(node) && secondsLeft-- > 0) {
+            selector.poll(1000L);
+        }


Shouldn't we check that the channel is ready once we exit the while loop and if it is not, throw an exception?

ijuma · 2016-04-29T14:41:55Z

@rajinisivaram, thanks for the explanation and additional tests. I left some minor comments, it looks good otherwise. I'll leave it to @gwenshap to also take a look and merge it if she's happy with the PR (hopefully you can address the minor comments before they wake up in the US).

rajinisivaram · 2016-04-29T15:40:33Z

@gwenshap @ijuma Thank you both for the reviews. I have made the changes suggested. Will wait to see if there are more changes/tests required.

gwenshap · 2016-04-29T18:14:28Z

I double checked the test and it all looks good. Thanks for your patience @rajinisivaram.

junrao · 2016-05-02T17:16:12Z

            switch (apiKey) {
+                case API_VERSIONS:
+                    handleApiVersionsRequest(requestHeader, (ApiVersionsRequest) request);
+                    break;


One impact of this implementation is that we allow a client to send any number of ApiVersion requests before the SaslHandshake request. It doesn't seem to hurt. However, it seems that requiring that a client issue a SaslHandshake request immediately after ApiVersion request probably makes the protocol simpler and easier to understand?

If there isnt a strong reason to constrain the semantics to max one request prior to handshake I dont think we should.
In my view adding constraints like that actually make the protocol less simple, but it might make the broker implementation more simple. From the client's point of view a less strict protocol is easier to work with.

Also, it will complicate the implementation quite a bit. Count requests per connection? We don't do it for any other request type...

The broker implementation is more complicated, but not too complicated, we'd have to update SaslState or something like that. However, I also think that adding this restriction doesn't necessarily make things simpler overall (I can see Jun's point, but also Magnus's), so I'm +0 on leaving as is given that any change at this stage adds some risk.

@edenhill : Is there a use case for a client to ask for ApiVersionRequest more than once before sasl handshake? I thought a more strict protocol is easier for a client to deal with since there are few code paths that a client has to test.

@gwenshap : Yes, the implementation on the server side will be a bit more complicated to restrict the number of times ApiVersionRequest is issued before SaslHandshakeRequest, but not too much. The requests before authentication are already treated specially anyway. My main concern is on documenting the protocol. It's simpler if the protocol is

ApiVersionRequest? SaslHandShakeRequest SaslTokens

instead of

ApiVersionRequest* SaslHandShakeRequest SaslTokens

@rajinisivaram : Yes, if you could submit a followup patch on this, that will be great. Thanks,

What about the case where we add a new version of ApiVersionsRequest. In that case, a client could issue an ApiVersionsRequest with a newer version and a 0.10 broker would reply with a response containing an error code. The client would then issue an ApiVersionsRequest with version 0.

We are not taking advantage of this yet, but it's something we discussed. For clients, it's fine to add this support later, but we can't do that for brokers so they need the flexibility from the start.

Or am I missing something?

Yes, very good point. To support that, we can allow multiple ApiVersionRequests before SaslHandshakeRequest as it is. However, we need to fix a few things in the code.

We handle unsupported version of ApiVersionRequest in KafkaApis.handleApiVersionsRequest(). That's too late. In RequestChannel, we have the generic logic to throw an exception on unknown version of a request which will kill the connection.

In SaslServerAuthenticator.handleKafkaRequest(), when receiving an unknown version of ApiVersionRequest, we throw an UnsupportedSaslMechanismException which kills the connection. We should send an error response in this case.

Sounds good @junrao. @rajinisivaram, maybe you can add some tests to verify the desired behaviour as described by Jun along with the code changes? Thanks!

@junrao @ijuma Implementation changes with unit tests are in the PR #1310

Server-side implementation and tests for handling ApiVersionsRequest before SaslHandshakeRequest. Author: Rajini Sivaram <rajinisivaram@googlemail.com> Reviewers: Gwen Shapira, Ismael Juma Closes apache#1286 from rajinisivaram/KAFKA-3618

…edKafka (apache#1286)

…edKafka (#1286) SupportedKafka needs to be detectable for downgrade_test and upgrade_test to use older ccs version binaries.

* Currently find share coordinator operation is accomplished by issuing FC RPC to a random node. * If the cluster is absolutely fresh, WE NEED the RPC as it will create the internal topic __share_group_state as a side effect. * However, subsequent RPCs aren't necessary since the required information is present in the metadata cache. * In this PR we have introduced the changes to facilitate this optimization. * The code checks if internal topic is created and if created whether it can locate the share coordinator in the cache. If it can then the node is returned from the cache. * Otherwise, FC RPC is issued.

ijuma reviewed Apr 28, 2016
View reviewed changes

KAFKA-3618: Handle ApiVersionsRequest before SASL authentication

5f86d6f

rajinisivaram force-pushed the KAFKA-3618 branch from d19ca95 to 5f86d6f Compare April 28, 2016 23:03

KAFKA-3618: Add more tests

20c75ac

ijuma reviewed Apr 29, 2016
View reviewed changes

KAFKA-3618: Address review comments

4132fff

asfgit closed this in 69d9a66 Apr 29, 2016

junrao reviewed May 2, 2016
View reviewed changes

lbradstreet added a commit to lbradstreet/kafka that referenced this pull request Mar 1, 2020

MINOR: kafka.py does not correctly detect pids for deprecated Support…

709bff1

…edKafka (apache#1286)

lbradstreet referenced this pull request in confluentinc/kafka Mar 1, 2020

MINOR: kafka.py does not correctly detect pids for deprecated Support…

3d1a070

…edKafka (#1286) SupportedKafka needs to be detectable for downgrade_test and upgrade_test to use older ccs version binaries.

Conversation

rajinisivaram commented Apr 28, 2016

Uh oh!

rajinisivaram commented Apr 28, 2016

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!

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!

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!

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 Apr 29, 2016

Uh oh!

rajinisivaram commented Apr 29, 2016

Uh oh!

gwenshap commented Apr 29, 2016

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants