-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add support for SHA256 auth plugin #583
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
Conversation
I don't want to add dependency |
@methane I use
to make sure this module is not required. |
Can you use |
Sure, let me check this lib. |
@methane , I'm afraid of that I can not use
which do not have any algorithm which is related with RSA. |
I don't dislike cryptography. I just don't want any dependency. |
As far as I read the manual, RSA is not required when tls is used. |
And I don't want to add any new code to |
@methane You are right, RSA works when the connection is not using SSL. And only built with OpenSSL but not YaSSL(which is default), this mechanism will works. I'll make sure that when using SSL, the exception will not thrown, and when the MySQL server communicate with RSA, it will throw the module needed, or you have any other suggestion? For the connection.py code is too heavy, sure, let me move it to pymysql/auth/sha256password.py. |
pymysql/connections.py
Outdated
|
||
def _sha256_rsa_crypt(password, salt, public_key): | ||
if not HAVE_CRYPTOGRAPHY: | ||
raise NotImplementedError("cryptography module not found") |
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.
Please use OperationalError instead of NotImplementedError.
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.
Sure.
pymysql/connections.py
Outdated
def _xor_password(password, salt): | ||
password_bytes = bytearray(password, 'ascii') | ||
for i in range(len(password_bytes)): | ||
password_bytes[i] ^= ord(salt[i % len(salt)]) |
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.
Write salt_len = len(salt)
before loop and use it.
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.
Sure.
pymysql/connections.py
Outdated
password_bytes = bytearray(password, 'ascii') | ||
for i in range(len(password_bytes)): | ||
password_bytes[i] ^= ord(salt[i % len(salt)]) | ||
return password_bytes.decode('ascii') |
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.
This may cause UnicodeDecodeError. Keep bytearray type as-is.
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.
Sure.
pymysql/connections.py
Outdated
@@ -374,6 +396,9 @@ def is_auth_switch_request(self): | |||
# http://dev.mysql.com/doc/internals/en/connection-phase-packets.html#packet-Protocol::AuthSwitchRequest | |||
return self._data[0:1] == b'\xfe' | |||
|
|||
def is_rsa_public_key(self): | |||
return self._data[0:1] == b'\x01' |
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.
Is \x01
really RSA_PUBLIC_KEY packet?
Isn't it a more general purpose packet which has other semantics in other context?
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.
I think it is more general purpose packet, but I can not find a proper name, I'll fix it by the next commit for a proper packet name.
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.
I found the real meaning of \x01
, it means Extra Authentication Data.
Since Oracle removes server-client protocol document, it's hard to review protocol... |
I use this web archived document https://web.archive.org/web/20170404163410/https://dev.mysql.com/doc/internals/en/sha256.html . |
I know webarchive, but all images (including important sequence diagram) gone. |
@methane Could you help to review it again? |
Hi @methane ,I squashed this PR and fixed up the tests. Could you help to review it? |
You can use extras_require in setup(
name="PyMySQL",
# ...
extras_require={
'auth_sha256': ['cryptography>=2.1'],
},
classifiers=[
# ...
],
) Users can then |
@elemount Why did you close this? May I continue your work? |
I closed it because I abandoned using SHA256 in my MySQL server. |
@elemount, thanks for your work. I've started to look at this. Overall looks pretty good and I've only added a couple of Py3 compatibility fixes so far. Hope the auth plugin interface wasn't too bad/buggy. WIP: grooverdan/PyMySQL@travis-docker-mysql-8.0...grooverdan:sha256_auth_plugin Next steps: |
FYI, I'll release a few bugfix (0.8.x) in May. |
For issue #532
SHA256 authentication plugin is the recommended auth way for the MySQL now, for security reason.
This covers all the SHA256 auth code pass.
I test code in both 4 scenario. But can not add a proper unit test for it now. May need some help.