add support for authentication plugins. #552
Conversation
auth.go
Outdated
|
|
||
| import "bytes" | ||
|
|
||
| const mysqlClearPassword = "mysql_clear_password" |
There was a problem hiding this comment.
Use
const (
mysqlClearPassword = "mysql_clear_password"
mysqlNativePassword = "mysql_native_password"
...
)for a list of constants like this. I think go vet or go fmt should also catch this.
| type AuthPlugin interface { | ||
| // Next takes a server's challenge and returns | ||
| // the bytes to send back or an error. | ||
| Next(challenge []byte) ([]byte, error) |
There was a problem hiding this comment.
Why is this called Next and not e.g. Auth (which I believe is what this does)
There was a problem hiding this comment.
This is called next because it might be called more than once. All the current plugins, clear, native, and old all are single step. Next is called once and they are complete. However, not all plugins will be this way, so calling it Auth and having it called multiple times is weird (to me). Precedent was pulled from Java's driver, which calls it Next as well. However, I'm certainly not tied to that name.
There was a problem hiding this comment.
That makes sense. Maybe also add a short comment to the code that it might be called more than once.
auth.go
Outdated
| return nil, ErrCleartextPassword | ||
| } | ||
|
|
||
| // \0-terminated |
There was a problem hiding this comment.
In other parts of the driver we refer to \0 as NUL, please also use it here
craiggwilson
force-pushed
the
auth_plugins
branch
from
030d761
to
ad91cf5
Compare
|
I very much would like to see support for authentication plugins. However, one problem with the current implementation is, that it seems to change existing behavior. We had many problems because of slightly broken server implementations in the past. Since these were problems with specific server implementations, I think we also do not have regression tests for most of these, since we don't have a mock implementation of a MySQL server in our test suite. I think older clients also don't send an authentication plugin name by default, so it might not work with older servers. PS: I can find neither your name nor MongoDB in the AUTHORS file |
craiggwilson
force-pushed
the
auth_plugins
branch
from
ad91cf5
to
e273589
Compare
|
Oops, had it in the AUTHORS file but accidentally reverted. Yeah, that was my concern also. I have tested this against mysql 5.7 and against our MySQL proxy. I don't think behavior is changed in that I tried very hard to make sure that the code flow remained identical, albeit through different functions. But I can't be completely sure and certainly have not exhaustively tested all versions. |
|
Oh, as for older servers not sending a plugin name, I believe you are correct. This should be accounted for by defaulting to use the default plugin (i.e. attempting to use native) and then falling back to old, the same way it was handled previously. |
…eed to be cleaned up.
| @@ -92,6 +93,10 @@ func (mc *mysqlConn) Close() (err error) { | |||
| // closed the network connection. | |||
| func (mc *mysqlConn) cleanup() { | |||
| // Makes cleanup idempotent | |||
| if mc.authPlugin != nil { | |||
| mc.authPlugin.Close() | |||
There was a problem hiding this comment.
The auth plugin is not used again after the connection is established and can therefore be closed in the init phase already, I believe. We also don't have to add it as a field to mc then.
|
We're probably also not the first mysql driver to implement this. We should also take a look how it is implemented in drivers for other languages. Maybe @methane knows more |
|
@methane please take a look when time permits it |
| var authPluginName string | ||
| if len(data) > pos { | ||
| // auth-plugin name (string[NUL]) | ||
| authPluginName = string(data[pos : len(data)-1]) |
There was a problem hiding this comment.
Search NUL byte. When NUL not found, plugin name is data[pos:].
When NUL found, plugin name is data[pos:nul_index].
| return err | ||
| } | ||
|
|
||
| return handleAuthResult(mc, authData) |
There was a problem hiding this comment.
I don't like recursion. Please loop outside of this function.
|
@craiggwilson Any update on this? |
|
@craiggwilson are you interested in updating your pull request? We changed much of the auth handling in the meantime, the updated PR should actually be a lot simpler now. |
|
No, we can close it. Feel free to use any code that might still be pertinent, but I don't have time at this moment. Thanks for the great work! |
Description
Currently, the driver does not support authentication plugins. I have refactored the code to support plugins and the existing authentication verification to be plugin-based. That is to say that cleartext, native, and old are now plugins. All tests pass on windows, although I have not tested on linux, so before merging, it would be great if that were vetted as well. I don't believe this warrants README file changes.
Motivation for this is that my company (MongoDB) needs to use a different authentication mechanism. We have developed the auth plugin separately and tested it against this implementation.
Thanks for your consideration.
Checklist