KAFKA-7915: Don't return sensitive authentication errors to clients#6252
KAFKA-7915: Don't return sensitive authentication errors to clients#6252rajinisivaram merged 3 commits intoapache:trunkfrom
Conversation
| @@ -1819,8 +1882,8 @@ private void createAndCheckClientAuthenticationFailure(SecurityProtocol security | |||
| assertEquals(expectedErrorMessage, exception.getMessage()); | |||
| else { | |||
| String expectedErrorMessagePrefix = "Authentication failed during authentication due to invalid credentials with SASL mechanism " | |||
There was a problem hiding this comment.
nit: expectedErrorMessage rather than expectedErrorMessagePrefix
ijuma
left a comment
ijuma
left a comment
There was a problem hiding this comment.
Thanks for the PR. Good catch before the release. Just a few minor comments.
|
|
||
| SecurityProtocol securityProtocol = SecurityProtocol.SASL_PLAINTEXT; | ||
| TestJaasConfig jaasConfig = configureMechanisms("SCRAM-SHA-256", Collections.singletonList("SCRAM-SHA-256")); | ||
| jaasConfig.createOrUpdateEntry(TestJaasConfig.LOGIN_CONTEXT_SERVER, PlainLoginModule.class.getName(), new HashMap<String, Object>()); |
There was a problem hiding this comment.
Nit: can we use the diamond operator?
| + mechanism + ": "; | ||
| if (exception.getMessage().startsWith(expectedErrorMessagePrefix)) | ||
| + mechanism; | ||
| if (exception.getMessage().equals(expectedErrorMessagePrefix)) |
|
|
||
| public static class InvalidScramServerCallbackHandler implements AuthenticateCallbackHandler { | ||
| static volatile IOException sensitiveException; | ||
| static volatile SaslAuthenticationException clientFriendlyException; |
There was a problem hiding this comment.
Do we actually need two fields? From looking at the code, it wasn't clear to me.
There was a problem hiding this comment.
The only checked exception we can throw from the callback handler is IOException and the second exception is a RuntimeException, hence the two exceptions.
There was a problem hiding this comment.
Can we please add a comment to make it clear?
|
@rondagostino @ijuma Thanks for the reviews, I have addressed the comments. |
ijuma
left a comment
ijuma
left a comment
There was a problem hiding this comment.
Left a comment, but LGTM. We can merge once that's addressed, no re-review needed.
|
retest this please |
|
@mjsax we need this in 2.2 btw. We should be able to merge it today or tomorrow. |
|
Test failures are unrelated, merging to trunk and 2.2. |
…6252) Don't return error messages from `SaslException` to clients. Error messages to be returned to clients to aid debugging must be thrown as AuthenticationExceptions. This is a fix for a regression from KAFKA-7352. Reviewers: Ron Dagostino <rndgstn@gmail.com>, Ismael Juma <ismael@juma.me.uk
…pache#6252) Don't return error messages from `SaslException` to clients. Error messages to be returned to clients to aid debugging must be thrown as AuthenticationExceptions. This is a fix for a regression from KAFKA-7352. Reviewers: Ron Dagostino <rndgstn@gmail.com>, Ismael Juma <ismael@juma.me.uk
3 participants
Don't return error messages from
SaslExceptionto clients. Error messages to be returned to clients to aid debugging must be thrown as AuthenticationExceptions. This is a fix for a regression from KAFKA-7352 which is not yet in any release (will be in 2.2.0).Committer Checklist (excluded from commit message)