Basic AuthPlugin support - #28
Conversation
Signed-off-by: Chojan Shang <psiace@outlook.com>
Signed-off-by: Chojan Shang <psiace@outlook.com>
Signed-off-by: Chojan Shang <psiace@outlook.com>
Signed-off-by: Chojan Shang <psiace@outlook.com>
Signed-off-by: Chojan Shang <psiace@outlook.com>
Signed-off-by: Chojan Shang <psiace@outlook.com>
[skip ci]
Signed-off-by: Chojan Shang <psiace@outlook.com>
Bump deps
[bump] nom into 7.0.0-alpha2
Merge upstream
| //! ``` | ||
| #![deny(missing_docs)] | ||
| #![deny(rust_2018_idioms)] | ||
| #![allow(clippy::from_over_into)] |
|
Thanks for the detailed review. Seems there are lots of styles/documents to improve. I'll try to apply the review suggestions. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
==========================================
- Coverage 81.19% 80.63% -0.56%
==========================================
Files 10 11 +1
Lines 2159 2391 +232
==========================================
+ Hits 1753 1928 +175
- Misses 406 463 +57 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jonhoo
left a comment
jonhoo
left a comment
There was a problem hiding this comment.
Thanks! I left some more comments inline.
| if capabilities.contains(CapabilityFlags::CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA) { | ||
| let (i, size) = read_length_encoded_number(i)?; | ||
| let mut i = i; | ||
| let size = i.read_lenenc_int().unwrap_or(0); |
There was a problem hiding this comment.
If read_lenenc_int fails, should we return an error here instead? Assuming it is 0 doesn't seem right.
There was a problem hiding this comment.
Yes, but I don't know how to initialize which kind of nom Error can be raised.
BTW I will continue to finish the pr #22 , after that is merged, then you can review this pr.
|
|
||
| /// Connection id | ||
| fn connect_id(&self) -> u32 { | ||
| /// Default connection id |
There was a problem hiding this comment.
I'd still like to see some more documentation here about this value is. Ideally with a link to some MySQL/MariaDB documentation about what the connection ID is used for. Otherwise, as an implementor, I don't really know what I'm supposed to return here.
There was a problem hiding this comment.
We should also document what the default value is.
| /// default plugin for authentication | ||
| /// Plugin methods: https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_connection_phase_authentication_methods.html |
There was a problem hiding this comment.
Here too I'd like to actually spell out what this is, what it's used for, when you might want to override it, and what the default value is (and why). I would copy-paste some of the documentation from that linked page, and include links to relevant subsections.
| /// Called when reading the handshake response by client's auth_plugin | ||
| /// See: https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_connection_phase.html#sect_protocol_connection_phase_auth_method_mismatch |
There was a problem hiding this comment.
Same thing here — let's give consumers of this API a bit more to go on. At the very least this should say something like
Allows you to determine the authentication plugin to use for a particular user, independently of what the server default is (see [
MysqlShim::default_auth_plugin]).
|
|
||
| /// Default salt(scramble) for auth plugin | ||
| fn salt(&self) -> [u8; SCRAMBLE_SIZE] { | ||
| /// Called when initializing the handshake |
There was a problem hiding this comment.
This doesn't actually tell someone looking to implement this trait what this is used for, or what kind of value they should return. Again, copying from the MySQL documentation may be the way to go here.
| ) | ||
| })?; | ||
| self.writer.set_seq(seq + 1); | ||
| auth_response = auth_response_data.to_vec(); |
There was a problem hiding this comment.
I believe you can avoid this .to_vec() and the .to_vec() on auth_response before drop(handshake) if you restructure the code above a little bit to:
let auth_ok = if user_custom_auth {
drop(handshake);
// the rest of the contents of the `if` until this line, then:
self.shim.authenticate(user_auth_plugin, &username, &salt, &auth_response)
} else {
self.shim.authenticate(&handshake.auth_plugin, &handshake.username, &salt, &handshake.auth_response)
};| drop(handshake); | ||
|
|
||
| let user_auth_plugin = self.shim.auth_plugin_for_user(&username); | ||
| // Start SwitchAuthRequest |
There was a problem hiding this comment.
Let's move this comment into the if, since that's what it applies to.
| .authenticate(user_auth_plugin, &username, &salt, &auth_response) | ||
| { | ||
| let err_msg = format!( | ||
| "Access denied for user '{}'@'{}' (using password: YES)", |
There was a problem hiding this comment.
Is "using password: YES" here conditional on something?
| self.socket_addr.ip() | ||
| ); | ||
| writers::write_err( | ||
| ErrorKind::ER_ACCESS_DENIED_ERROR, |
There was a problem hiding this comment.
Should we perhaps allow MysqlShim::authenticate to return Result<(), ErrorKind> so that the implementor can indicate the error type? Or perhaps even Result<(), (String, ErrorKind)> so they can also dictate the message?
| | CapabilityFlags::CLIENT_SECURE_CONNECTION // this is required for `Secure Password Authentication` | ||
| | CapabilityFlags::CLIENT_PLUGIN_AUTH | ||
| | CapabilityFlags::CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA | ||
| | CapabilityFlags::CLIENT_CONNECT_WITH_DB) |
There was a problem hiding this comment.
Do we actually support CLIENT_CONNECT_WITH_DB?
3 participants
Changes:
mysql_native_password