Skip to content
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: Fix authentication with MySQL 5.1 and older clients #29732

Closed
wants to merge 2 commits into from

Conversation

dveeden
Copy link
Contributor

@dveeden dveeden commented Nov 12, 2021

What problem does this PR solve?

Issue Number: close #29725

Problem Summary:

handleAuthPlugin checks for the ClientPluginAuth capability but this didn't set the AuthPlugin causing readOptionalSSLRequestAndHandshakeResponse to return an error.

Note that this allows authentication for users that use mysql_native_password to work, but users using caching_sha2_passwords won't work.

This does now return a more specific error in case authentication with caching_sha2_password is used.

[dvaneeden@dve-carbon ~]$ ~/opt/mysql/5.1.73/bin/mysql -h 127.0.0.1 -u sha -P 4000 -psha
ERROR 1105 (HY000): ERROR 1251 (08004): Client does not support authentication protocol requested by server; consider upgrading MySQL client

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 12, 2021
@dveeden dveeden marked this pull request as ready for review November 12, 2021 10:02
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 12, 2021
@dveeden
Copy link
Contributor Author

dveeden commented Nov 12, 2021

This will need careful merging with #29577

@bb7133
Copy link
Member

bb7133 commented Nov 12, 2021

Note that this allows authentication for users that use mysql_native_password to work, but users using caching_sha2_passwords won't work.

This does now return a more specific error in case authentication with caching_sha2_password is used.

So your point is, if a user with caching_sha2_password authentication is defined, login with old MySQL client(v5.1) will not be allowed, right?

@bb7133
Copy link
Member

bb7133 commented Nov 12, 2021

Please add a test case for your change, if it can be a functional test for handleAuthPlugin() only, that would be great :)

Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no such test cases in TiDB, which is the reason why there are many fix-on-a-fix problems.
Can you try to add a test case so that future modifications won't break the compatibility?

@djshow832
Copy link
Contributor

Also, please add a release note so that users can figure out from the user doc that the bugfix is contained in which versions.

@djshow832
Copy link
Contributor

There are no such test cases in TiDB, which is the reason why there are many fix-on-a-fix problems. Can you try to add a test case so that future modifications won't break the compatibility?

Will any authentication test cases be ported from the MySQL test? @morgo

@dveeden
Copy link
Contributor Author

dveeden commented Nov 16, 2021

Closing this as this is now part of #29738

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tidb returns 'Unknown auth plugin' when some old clients to connect
4 participants