Skip to content

server: Fix authentication with MySQL 5.1 and older clients#29732

Closed
dveeden wants to merge 2 commits intopingcap:masterfrom
dveeden:old_client_authplugin
Closed

server: Fix authentication with MySQL 5.1 and older clients#29732
dveeden wants to merge 2 commits intopingcap:masterfrom
dveeden:old_client_authplugin

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

ti-chi-bot commented Nov 12, 2021

[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.

Details

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 force-pushed the old_client_authplugin branch from 9279a76 to c4b33c6 Compare November 12, 2021 10:01
@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 :)

@dveeden dveeden mentioned this pull request Nov 12, 2021
Merged
12 tasks
djshow832
djshow832 reviewed Nov 13, 2021
View reviewed changes
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

djshow832 commented Nov 13, 2021

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

djshow832 commented Nov 13, 2021

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

@bb7133 bb7133 mentioned this pull request Nov 15, 2021
Merged
12 tasks
@dveeden
Copy link
Contributor Author

dveeden commented Nov 16, 2021

Closing this as this is now part of #29738

@dveeden dveeden closed this Nov 16, 2021
ti-chi-bot pushed a commit that referenced this pull request Nov 16, 2021
@bb7133
server: fix authentication with MySQL 5.1 and older clients (#29732) (#…
c83f032 
…29808)
@ti-srebot ti-srebot mentioned this pull request Nov 29, 2021
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@djshow832 djshow832 djshow832 left review comments

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

4 participants

@dveeden @ti-chi-bot @bb7133 @djshow832