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
KAFKA-6004: Allow authentication providers to override error message #4015
KAFKA-6004: Allow authentication providers to override error message #4015
Conversation
@ijuma Can you review please, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, a couple of comments.
@@ -381,9 +381,11 @@ private void handleSaslToken(byte[] clientToken) throws IOException { | |||
// For versions with SASL_AUTHENTICATE header, send a response to SASL_AUTHENTICATE request even if token is empty. | |||
ByteBuffer responseBuf = responseToken == null ? EMPTY_BUFFER : ByteBuffer.wrap(responseToken); | |||
sendKafkaResponse(requestContext, new SaslAuthenticateResponse(Errors.NONE, null, responseBuf)); | |||
} catch (SaslException e) { | |||
} catch (Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional that we catch all exceptions, not just SaslException and AuthenticationException?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, fixed.
@@ -93,11 +94,11 @@ public PlainSaslServer(JaasContext jaasContext) { | |||
String expectedPassword = jaasContext.configEntryOption(JAAS_USER_PREFIX + username, | |||
PlainLoginModule.class.getName()); | |||
if (!password.equals(expectedPassword)) { | |||
throw new SaslException("Authentication failed: Invalid username or password"); | |||
throw new AuthenticationException("Authentication failed: Invalid username or password"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have to update ScramSaslServer
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left the default message for SCRAM since clients dont exactly provide username/password in the SCRAM message and we dont want to say which check failed. I have updated the message when the authorization id requested is not the username.
8cbbbae
to
853129d
Compare
@ijuma Thank you for the review. Have updated the code and added upgrade note. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
@ijuma Thank you for the review, will merge when builds pass. |
No description provided.