KAFKA-7182: SASL/OAUTHBEARER client response missing %x01 seps

[rondagostino]

The format of the SASL/OAUTHBEARER client response is defined in
RFC 7628 Section 3.1 as follows:

 kvsep          = %x01
 key            = 1*(ALPHA)
 value          = *(VCHAR / SP / HTAB / CR / LF )
 kvpair         = key "=" value kvsep
 client-resp    = (gs2-header kvsep *kvpair kvsep) / kvsep

;;gs2-header = See RFC 5801 (Section 4)

The SASL/OAUTHBEARER client response as currently implemented in
OAuthBearerSaslClient sends the valid gs2-header "n,," but then
sends the "auth" key and value immediately after it, like this:

String.format("n,,auth=Bearer %s", callback.token().value())

This does not conform to the specification because there is no
%x01 after the gs2-header, no %x01 after the auth value, and no
terminating %x01. The code should instead be as follows:

String.format("n,,\u0001auth=Bearer %s\u0001\u0001",
callback.token().value())

Similarly, the parsing of the client response in
OAuthBearerSaslServer, which currently allows the malformed text,
must also change.

This should be fixed prior to the initial release of the
SASL/OAUTHBEARER code in 2.0.0 to prevent compatibility problems.

Signed-off-by: Ron Dagostino [email protected]

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[rondagostino]

@rajinisivaram and @stanislavkozlovski Please review. I think this should be fixed prior to the 2.0.0 release. Also, looking forward, it probably has some impact on KIP-342.

[stanislavkozlovski]

Could we try to reuse

kafka/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/OAuthBearerSaslClient.java

Line 52 in 03e788b

static final byte BYTE_CONTROL_A = (byte) 0x01;

in this part of the code? I'm not sure how simple it is since I don't have extensive experience in Java, but reusing code and having explicit separators is always a good thing. If we could do it in the server as well, that'd be great

[stanislavkozlovski]

Very good catch! I left a very minor comment but you should let Rajini review, as she is more experienced with Kafka.
In regards to it having an impact with KIP-342 - should only be a few minor changes. Even then, KIP-342 is not meant for 2.0.0, so it doesn't really matter

[rajinisivaram]

@rondagostino Thanks for the PR. Can we move the creation of the client response and the parsing of the response in the server to methods which can be unit tested? For Scram, we have a test ScramFormatterTest.rfc7677Example, which runs our implementation against the example in the RFC. I found that useful to verify that we were implementing the protocol correctly. Perhaps a test like that with the example from the RFC would be useful here too, even though this is just a single message?

[rondagostino]

@rajinisivaram Yes I will make this change and add a new commit ASAP

[rajinisivaram]

@rondagostino We need to support extensions on the server-side for parsing to be compliant with the RFC. Section 3.1 says:

The key/value pairs take the place of the corresponding HTTP headers and values
to convey the information necessary to complete an OAuth-style HTTP authorization.
Unknown key/value pairs MUST be ignored by the server.

So I think we shouldn't fail authentication if there are additional keys. Can you take a look at this commit (rajinisivaram@8ce6b20) and pull in code into your PR which you think make sense? Or I could do a PR for you to review. Ideally, I would like to merge this change today and create another 2.0.0 RC today - we need to wait for builds of the PR as well as the main build on Apache Jenkins, so not sure whether there will be time to do it, but I will try and get it done if you are able to take a look at this when you get in this morning.

[rondagostino]

Hi @rajinisivaram. I'll pull in your code. I'm running the tests locally now just to be sure everything is fine and should be able to add the commit soon. Thanks for putting that together; I understand the desire to get this done quickly, and I wouldn't have had anything for a couple of hours. Push to follow soon...

[rajinisivaram]

@rondagostino Thank you! Could you also look through the patterns and make sure they match the RFC, in case I missed something? Thanks.

[rondagostino]

@rajinisivaram I double-checked and the patterns are correct. The JDK 8 build just failed -- looks like some kind of storage issue, unrelated to the code?

[rajinisivaram]

@rondagostino Thank you! Yes, the PR build failure looks unrelated.

[rajinisivaram]

retest this please

[ijuma]

The test failures are not related to these changes. @rajinisivaram, there are some SslTransportLayerTest failures, are they related to the recent fix you contributed?

[rajinisivaram]

@ijuma One of the failures is in the new test I added and could be a test issue, but not sure why we had two test failures which failed to close the channel. I will merge this one to get the builds going and look into those failures.

[rajinisivaram]

@rondagostino Thanks for the JIRA and PR, merging to trunk and 2.0.

[ijuma]

Thanks @rajinisivaram and @rondagostino!