-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
server: handle clients without authplugin support #27931
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
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
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 for the rest of the code, but I'm not an expert of authfication, just want to check if this is expected:
mysql/5.1.30/bin/mysql -u u1 -h 127.0.0.1 -P 4000 -pabc -e QUIT
# OK
mysql/5.1.30/bin/mysql -u u2 -h 127.0.0.1 -P 4000 -pabc -e QUIT
ERROR 1045 (28000): Access denied for user 'u2'@'127.0.0.1' (using password: YES)
mysql/5.1.30/bin/mysql -u u3 -h 127.0.0.1 -P 4000 -pabc -e QUIT
ERROR 1045 (28000): Access denied for user 'u2'@'127.0.0.1' (using password: YES)
(the case from your comment: #27855 (comment))
3ec9d93
to
013102d
Compare
User exists, password is correct and authentication method is supported by client and server => OK
Authentication method (
Account doesn't exist, failing as expected. Testing the same against MySQL 8.0.26:
So there is a difference between the MySQL behavior and this PR: The error for the second case is different. |
I've met the same issue by client |
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. Old client should keep the compatibility because many old codes like availability monitors runs online.
@morgo plz help to merge~ 🤣 Then we can skip the v5.2.1 to a new version before putting the cluster into production. |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 013102d
|
cherry pick to release-5.2 in PR #28734 |
Signed-off-by: ti-srebot <[email protected]>
What problem does this PR solve?
Issue Number: close #27855
Problem Summary:
Old (MySQL 5.1 and before) clients that don't send authentication plugin info in the login packet were having issues authenticating and/or got an incorrect error message
What is changed and how it works?
mysql.ClientPluginAuth
inresp.Capability
.Check List
Tests
Documentation
Release note