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

Refactor authentication server plugin mechanism #8503

Merged
merged 3 commits into from
Jul 23, 2021

Conversation

dbussink
Copy link
Contributor

This change implements a large refactor of the plugin authentication mechanism. This refactor needed for a number of reasons:

  • The current AuthServer interface is not well designed. Many of the AuthServer implementations implement at least one if not multiple methods as a panic() as they are unused or they implement something never really called (like the salt generation). This indicates that the current interface doesn't abstract things well.
  • Adding caching_sha2_password as a server side mechanism would make this interface problem worse. It would add even more methods that would have emply implementations / panic().
  • It is impossible right now for a plugin to support multiple authentication mechanisms. The caching_sha2_password plugin allows for caching of passwords and could be used to improve for example the LDAP plugin. But we can't then still keep the mysql_clear_password as a fallback for such an improvement in the LDAP plugin.

Based on these issues, I'm proposing this refactor. It allows for a number of things that also make auth server implementations easier.

  • An auth server can implement multiple auth methods. This allows for the above mentioned optimization for the LDAP plugin and many other improvements. Note that these improvements are not implemented here, but it opens up the opportunity.
  • There are helpers for all the already supported authentication mechanisms plus basic support for caching_sha2_password. The last only supports TLS connections or Unix sockets right now, since the full authentication protocol is much complicated. The value of the latter is also debatable in the context of Vitess. It depends on a TLS certificate and private key also for the plain text full auth cycle and in that case, TLS would already be available as well anyway. For clients that want caching_sha2_password, switching those to enable TLS seems like a much simpler solution.
  • With the helpers, specific interfaces are provided that plugins can implement. Those interfaces provide a simpler way than the complex Negotiate from before which might need to read packets etc. For these helpers, all reading / writing of the protocol is already dealt with and not something auth server plugin writers need to be concerned with.

Given the above advantages, I think the current setup is a better design and better fits what we'd want to use for authentication plugins.

Lastly, this does break any existing plugin implementations that are written in Go and that people might have done themselves in their own Vitess forks etc. I don't know if that's an expected interface / something that needs a larger announcement.

The functionality of all the built in plugins is exactly the same. They have been switched to the new internal APIs, but the external functionality, config files, configuration flags etc. have all stayed exactly the same.

Related Issue(s)

#5399 is related, but that was closed after only client side support for caching_sha2_password was implemented.

With the refactors here, it also becomes possible to implement caching_sha2_password for the server side component in Vitess.

Checklist

  • Tests were added or are not required
  • Documentation was added or is not required

Not really sure about the documentation. I don't think we document the internal Go interfaces, only things like configuration flags etc. for plugins that all stay the same?

@dbussink dbussink added Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Jul 20, 2021
@dbussink dbussink requested a review from vmg July 20, 2021 14:34
@dbussink dbussink force-pushed the more-flexible-auth-server branch 2 times, most recently from 1e95cc3 to dabd8f5 Compare July 20, 2021 16:08
go/mysql/auth_server.go Show resolved Hide resolved
go/mysql/auth_server.go Outdated Show resolved Hide resolved
go/mysql/auth_server.go Show resolved Hide resolved
This change implements a large refactor of the plugin authentication
mechanism. This refactor needed for a number of reasons:

- The current AuthServer interface is not well designed. Many of the
  AuthServer implementations implement at least one if not multiple
  methods as a panic() as they are unused or they implement something
  never really called (like the salt generation). This indicates that
  the current interface doesn't abstract things well.
- Adding `caching_sha2_password` as a server side mechanism would make
  this interface problem worse. It would add even more methods that
  would have emply implementations / panic().
- It is impossible right now for a plugin to support multiple
  authentication mechanisms. The caching_sha2_password plugin allows for
  caching of passwords and could be used to improve for example the LDAP
  plugin. But we can't then still keep the `mysql_clear_password` as a
  fallback for such an improvement in the LDAP plugin.

Based on these issues, I'm proposing this refactor. It allows for a
number of things that also make auth server implementations easier.

- An auth server can implement multiple auth methods. This allows for
  the above mentioned optimization for the LDAP plugin and many other
  improvements. Note that these improvements are not implemented here, but
  it opens up the opportunity.
- There are helpers for all the already supported authentication
  mechanisms plus basic support for caching_sha2_password. The last only
  supports TLS connections or Unix sockets right now, since the full
  authentication protocol is much complicated. The value of the latter
  is also debatable in the context of Vitess. It depends on a TLS
  certificate and private key also for the plain text full auth cycle
  and in that case, TLS would already be available as well anyway. For
  clients that want caching_sha2_password, switching those to enable TLS
  seems like a much simpler solution.
- With the helpers, specific interfaces are provided that plugins can
  implement. Those interfaces provide a simpler way than the complex
  Negotiate from before which might need to read packets etc. For these
  helpers, all reading / writing of the protocol is already dealt with and
  not something auth server plugin writers need to be concerned with.

Given the above advantages, I think the current setup is a better
design and better fits what we'd want to use for authentication plugins.

Signed-off-by: Dirkjan Bussink <[email protected]>
@dbussink dbussink force-pushed the more-flexible-auth-server branch from dabd8f5 to d158fb7 Compare July 21, 2021 13:26
Copy link
Collaborator

@vmg vmg left a comment

Choose a reason for hiding this comment

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

This looks good to me. It's complex code but I don't see how to make it significantly simpler. It just has to do a lot!

go/mysql/auth_server.go Outdated Show resolved Hide resolved
go/mysql/auth_server.go Show resolved Hide resolved
go/mysql/auth_server_vault.go Show resolved Hide resolved
@harshit-gangal harshit-gangal merged commit 48da281 into vitessio:main Jul 23, 2021
@dbussink dbussink deleted the more-flexible-auth-server branch July 23, 2021 11:54
fulghum added a commit to dolthub/vitess that referenced this pull request Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants