-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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: Remove old handshake support #27768
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. |
I'll check that. Let's set this to draft status until that's done |
https://cbonte.github.io/haproxy-dconv/2.4/configuration.html#4.2-option%20mysql-check says this supports both pre-v4.1 and post-v4.1 auth and the default is post-v4.1. See also: haproxy/haproxy@62f79fe I've tested with haproxy (compiled from master at bc1223be7) and it does work with the default and with the |
/cc @morgo |
@dveeden: GitHub didn't allow me to request PR reviews from the following users: lysu. Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@lysu: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 08aa2d0cdabcd94e7c643901ae3a664bedcf1492
|
@lysu I sent an invitation to respect you as a TiDB committer as the consensus in pingcap/community#522 . You can accept by https://github.com/orgs/pingcap/invitation and approve this PR. |
@lysu oops a race happened :) |
/hold @lysu @wjhuang2016 @morgo please be aware of the compatibility breaker and unhold merging if no further concern |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 1915ea191087e4eb3f75cfd6854a5bd74dc14d70
|
/assign @bb7133 |
Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/14f4daf800ba5531a175bddd70f5823406594a72 |
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; removing will help improve security, since an old auth mechanism like this will surely become a target.
/unhold |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 009ac3e
|
TiDB MergeCI notify✅ Well Done! New fixed [1] after this pr merged.
|
What problem does this PR solve?
Issue Number: close #36922
Problem Summary:
Remove support for:
Protocol::HandshakeV9
Protocol::HandshakeResponse320
This removes support for pre-4.1 MySQL clients. As TiDB doesn't support the old pre-4.1 hashing method I don't think these were ever fully supported.
See also https://dev.mysql.com/doc/refman/5.6/en/password-hashing.html
Check List
Tests
Side effects
Documentation
Release note
Questions to reviewers