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

*: Support for caching_sha2_password authentication #24991

Merged
merged 4 commits into from
Jul 5, 2021

Conversation

dveeden
Copy link
Contributor

@dveeden dveeden commented May 31, 2021

What problem does this PR solve?

Issue Number: close #9411

Problem Summary:

  • mysql_native_password uses the SHA1 hash algorithm which is not future proof
  • Migrating users that use caching_sha2_password from MySQL to TiDB doesn't work

What is changed and how it works?

What this does:

  • Check the plugin column of the mysql.user table.
  • Based on the plugin from the user record and the plugin the client
    sent we may need to switch the authentication plugin to match the
    one from the user record
  • For accounts with caching_sha2_password send the "fast
    authentication failed" response to trigger full authentication.
  • call auth.CheckShaPassword to validate the user.

Implemented functionality:

  • Full authentication with caching_sha2_password over TLS
  • The default_authentication_plugin variable
  • CREATE USER... IDENTIFIED WITH 'caching_sha2_password'...

Missing functionality:

  • Support for the RSA public key request packet & response
  • Support for RSA key based secret exchange
  • Fast authentication (validate against cached entry)

Related changes

Tests

  • Manual test (add detailed scripts or steps below)
  • Unit tests

Create users with:

  • mysql_native_password
  • caching_sha2_password
  • An empty password

Try to authenticate with all of them.

Side effects

  • Unknown

Release note

  • Support for caching_sha2_password authentication

@ti-chi-bot ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 31, 2021
@sre-bot
Copy link
Contributor

sre-bot commented May 31, 2021

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@ti-srebot
Copy link
Contributor

@dveeden dveeden changed the title Support for caching_sha2_password authentication server,privilege,session: Support for caching_sha2_password authentication May 31, 2021
@dveeden
Copy link
Contributor Author

dveeden commented May 31, 2021

cc @morgo @bb7133

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label May 31, 2021
Comment on lines +667 to +698
default:
return errors.New("Unknown auth plugin")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You could potentially use an authSwitchRequest (from deleted code above) if the plugin is unknown. The downside is that this is hard to test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the client presents an authentication plugin that doesn't match the plugin used by the server checkAuthPlugin will do the authSwitchRequest so I don't think this code will be reached during normal operation. It may be reached if someone bypasses the GRANT/REVOKE etc and changes mysql.user directly.

privilege/privileges/privileges.go Outdated Show resolved Hide resolved
server/conn.go Outdated Show resolved Hide resolved
server/conn.go Outdated Show resolved Hide resolved
@dveeden dveeden marked this pull request as ready for review June 1, 2021 10:49
@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 Jun 1, 2021
@dveeden dveeden requested a review from a team as a code owner June 1, 2021 22:46
@dveeden dveeden requested review from wshwsh12 and removed request for a team June 1, 2021 22:46
@github-actions github-actions bot added the sig/execution SIG execution label Jun 1, 2021
@dveeden dveeden changed the title server,privilege,session: Support for caching_sha2_password authentication executor,server,privilege,session: Support for caching_sha2_password authentication Jun 1, 2021
@wshwsh12 wshwsh12 requested review from tiancaiamao and removed request for wshwsh12 June 3, 2021 04:49
@tiancaiamao tiancaiamao changed the title executor,server,privilege,session: Support for caching_sha2_password authentication *: Support for caching_sha2_password authentication Jun 3, 2021
server/conn.go Outdated Show resolved Hide resolved
session/session.go Show resolved Hide resolved
return nil
}

func (cc *clientConn) openSessionAndDoAuth(authData []byte) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

If open session success, but do auth fails, will there be a potential resource leak?

@@ -715,6 +775,32 @@ func (cc *clientConn) openSessionAndDoAuth(authData []byte) error {
return nil
}

func (cc *clientConn) checkAuthPlugin(ctx context.Context, authPlugin *string) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, there are:

  1. the default auth plugin (we set in the initial handshake packet)
  2. the user's auth plugin, which stores in mysql.user authentication related column
  3. the auth plugin from the client response packet

Which one is which? I'm a bit confused.

Pass *string as both input and output of a function is an idiom in C.
Go support multiple return, you don't need to do that.

@dveeden dveeden marked this pull request as draft June 3, 2021 09:55
@ti-chi-bot ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 3, 2021
@dveeden
Copy link
Contributor Author

dveeden commented Jun 30, 2021

/assign @tiancaiamao @morgo

if err != nil {
return err
}
cc.ctx = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a hint: open a new session may need to load the global variables (maybe not true when they're cached), and do it once again could slow down the connection speed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with your comment.

The cache is improved with sysvar cache, but it is still expensive. The current code will re-run validation on all of the sysvars. This can be improved though: in future we can cache the validated values in the sysvar cache, and when a session is initialized just copy the sysvar cache map to it.

The complicated case will be eliminating the need to call the 'init functions' (i.e. calling sv.SetSession() so that s.VarXXX is set.) I think we can remove a lot of SetSession() funcs by adding something like sv.ToBool(), sv.ToUint64().

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, creating a new session for handshake is relatively heavy, indirectly leading to the occurrence of #44502.

@tiancaiamao
Copy link
Contributor

@morgo would you like to take another look? or I'll merge this one.

@morgo
Copy link
Contributor

morgo commented Jul 1, 2021

@morgo would you like to take another look? or I'll merge this one.

It LGTM. There are some known issues with improving fast connection/disconnection rate, but these are existing.

We should suggest the QA team have scenario testing for it because it will affect connectors like php and perl. Issues like #24326 could also be discovered quicker.

@tiancaiamao
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: e2e9861

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 5, 2021
@ti-chi-bot
Copy link
Member

@dveeden: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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.

@ti-chi-bot ti-chi-bot merged commit f23e100 into pingcap:master Jul 5, 2021
dveeden added a commit to dveeden/docs that referenced this pull request Jul 5, 2021
dveeden added a commit to dveeden/docs that referenced this pull request Jul 6, 2021
dveeden added a commit to dveeden/docs that referenced this pull request Jul 12, 2021
dveeden added a commit to dveeden/docs that referenced this pull request Jul 12, 2021
dveeden added a commit to dveeden/docs that referenced this pull request Jul 12, 2021
dveeden added a commit to dveeden/docs that referenced this pull request Jul 20, 2021
Related to pingcap/tidb#24991

Co-authored-by: TomShawn <41534398+TomShawn@users.noreply.github.com>
dveeden added a commit to dveeden/docs that referenced this pull request Jul 20, 2021
Related to pingcap/tidb#24991

Co-authored-by: TomShawn <41534398+TomShawn@users.noreply.github.com>
dveeden added a commit to dveeden/docs that referenced this pull request Jul 20, 2021
Related to pingcap/tidb#24991

Co-authored-by: TomShawn <41534398+TomShawn@users.noreply.github.com>
dveeden added a commit to dveeden/docs that referenced this pull request Jul 20, 2021
Related to pingcap/tidb#24991

Co-authored-by: TomShawn <41534398+TomShawn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution sig/sql-infra SIG: SQL Infra size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for caching_sha2_password
7 participants