auth: fix CheckScrambledPassword() panic for invalid input

pingcap / parser

A MySQL Compatible SQL Parser

Apache License 2.0

1.41k stars 489 forks source link

auth: fix CheckScrambledPassword() panic for invalid input #1197

Closed tiancaiamao closed 3 years ago

tiancaiamao commented 3 years ago

What problem does this PR solve?

What is changed and how it works?

The input data may be invalid, but calling CheckScrambledPassword() on malformed data should not make TiDB server panic.

Check List

Tests

Unit test

Related changes

Need to cherry-pick to the release branch
Need to update the documentation
Need to be included in the release note

tiancaiamao commented 3 years ago

The malformed data maybe related to https://github.com/pingcap/tidb/pull/19603, maybe we are not resolving the protocol correctly, but I have no clue.

PTAL @djshow832 @morgo

kennytm commented 3 years ago

/lgtm

tiancaiamao commented 3 years ago

Add some test cases?

https://github.com/pingcap/parser/pull/1197/files#diff-b06746870e62f95017714121be6134cffdc4bd1d46dd21f8cae8c484e3e2829dR47-R49 That's all for it.

djshow832 commented 3 years ago

/lgtm

djshow832 commented 3 years ago

/merge

ti-srebot commented 3 years ago

/run-all-tests

ti-srebot commented 3 years ago

@tiancaiamao merge failed.

morgo commented 3 years ago

The malformed data maybe related to pingcap/tidb#19603, maybe we are not resolving the protocol correctly, but I have no clue.

PTAL @djshow832 @morgo

I took a look. The authSwitchRequest is relatively self contained unless the resp.AuthPlugin is caching_sha2_password (it doesn't attempt to authSwitch for any other plugins, even though it could). Do you know if it is a client attempting to use MySQL 8.0 auth?

There are other authentication plugins besides caching_sha2_password OR mysql_native_password that clients could attempt to use too, and it's also entirely possible it is a connector that doesn't speak the protocol correctly.

morgo commented 3 years ago

/run-all-tests

kennytm commented 3 years ago

I think we should just get rid of the integration test. The TiDB is always lagging behind making it almost always failing and impossible to use "/merge".

ti-srebot commented 3 years ago

cherry pick to release-5.0 failed

tiancaiamao commented 3 years ago

I took a look. The authSwitchRequest is relatively self contained unless the resp.AuthPlugin is caching_sha2_password (it doesn't attempt to authSwitch for any other plugins, even though it could). Do you know if it is a client attempting to use MySQL 8.0 auth?

There are other authentication plugins besides caching_sha2_password OR mysql_native_password that clients could attempt to use too, and it's also entirely possible it is a connector that doesn't speak the protocol correctly.

Yup, I've said I have no clue. I'm not sure how that happen and what trigger the malformed input. We need more feedback from the user.