Implement support for cleartext authentication plugin #171

wildmaples · 2024-04-03T20:36:56Z

Resolves: #157
Redo of: #168

The server doesn't request for cleartext plugin in the first handshake as it is not a default plugin. The client should only provide cleartext password when the server requests an auth switch to it, and the client has enable_cleartext_plugin option enabled. If the client does not have cleartext auth plugin enabled, we will raise a Trilogy::AuthPluginError.

This is the authentication flow for a server that uses the cleartext client side plugin (#168 (comment)):

Server sends handshake, with auth plugin caching_sha2_password
Client sends handshake response, still with auth plugin caching_sha2_password
Server sends auth switch request, now with auth plugin mysql_clear_password
Client sends auth switch response with just the cleartext password and nothing else
Server sends OK and we are connected

This PR introduces these changes:

Supports cleartext auth plugin!
Raises when the server auth switches to a plugin that Trilogy does not support. Note that at the moment Trilogy supports mysql_native_password, caching_sha2_password and now mysql_clear_password. This is the recommended behaviour according to mysql docs "Authentication Method Mismatch".
Adds a test dependency on this mysql test server plugin. It's unclear if the file will stick around in the long term but seeing that it is only a test dependency and makes our lives easier than the alternative testing options, we are inclined to just use it for the time being.

cc: @ngan @composerinteralia

In the auth switch, the server can request the client to send the password in cleartext. The client will only send the password in cleartext if `enable_cleartext_plugin` flag is true. Otherwise, raise `Trilogy::AuthPluginError`.

These tests rely on a mysql test server plugin (https://github.com/mysql/mysql-server/blob/824e2b4064053f7daf17d7f3f84b7a3ed92e5fb4/plugin/auth/test_plugin.cc#L176-L217) It is unclear if this test plugin will continue to exist in the future, but it will do for now.

src/protocol.c

Co-authored-by: Daniel Colson <composerinteralia@github.com>

ngan · 2024-04-04T16:29:06Z

I created an issue on the MySQL docker repo to see if they'd be open to building and tagging a mysql-community-test variant that we can use. If they do, we can switch to that and get the test auth plugin for free. If they don't, we can keep what we have (checked-in plugins for various mysql version), or build our own MySQL image.

docker-library/mysql#1040

composerinteralia

This looks great to me, but I'll leave it for a bit in case anybody else has feedback.

contrib/ruby/ext/trilogy-ruby/cext.c

casperisfine · 2024-04-05T06:43:35Z

test/auth_plugins/5.7/auth_test_plugin.so

This probably shouldn't be committed?

Should we be building our own MySQL image instead of committing the plugins?

I think we can and should, but that would be quite a big detour from the original intent of this PR. I suggest we land as-is then I can spend some time refactoring the test setup.

Follow up sounds good to me 👍 Thanks!

This has been addressed here: #174

Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com>

adrianna-chang-shopify

Thanks for working on this! Would you mind squashing your commits?

My preference would be for us to build our own MySQL image extending mysql-community-test over checking in those plugins, but don't feel strongly if folks would prefer to ship as is.

adrianna-chang-shopify · 2024-04-05T13:42:42Z

contrib/ruby/test/auth_test.rb

@@ -0,0 +1,16 @@
+require "test_helper"
+
+class AuthTest < TrilogyTest


Does it make sense for the mysql_native_password and caching_sha2_password connection tests from client_test.rb to live in this test suite too?

Actually, I could only add it for native password. We don't use cache_sha2 for mysql 5 so the ruby test fails there. I'm going to revisit this after this PR.

I think we already have that test elsewhere, but we can sort it out later. We might want to move all connection-related stuff into something like ConnectionTest.

adrianna-chang-shopify · 2024-04-05T14:26:53Z

test/auth_plugins/5.7/auth_test_plugin.so

Should we be building our own MySQL image instead of committing the plugins?

composerinteralia · 2024-04-05T14:41:39Z

Would you mind squashing your commits?

I do like a clean git history, but we haven't historically been as opinionated about this as (for example) Rails.

adrianna-chang-shopify

I do like a clean git history, but we haven't historically been as opinionated about this as (for example) Rails.

I'll defer to you folks, I wasn't sure if we were following Rails' stance of squash-everything-before-merge! 🙇‍♀️

composerinteralia · 2024-04-05T15:46:25Z

Thank you!

Followup to #171 (comment) This puts the caching_sha2 and native plugin tests together with the cleartext ones.

wildmaples added 2 commits April 3, 2024 16:08

Support cleartext password auth plugin

2e85275

In the auth switch, the server can request the client to send the password in cleartext. The client will only send the password in cleartext if `enable_cleartext_plugin` flag is true. Otherwise, raise `Trilogy::AuthPluginError`.

wildmaples mentioned this pull request Apr 3, 2024

Implement support for cleartext authentication plugin #168

Closed

composerinteralia reviewed Apr 3, 2024

View reviewed changes

src/protocol.c Outdated Show resolved Hide resolved

composerinteralia and others added 6 commits April 3, 2024 16:59

Merge branch 'main' into cleartext-plugin-auth-switch

a76d9c7

Fix wrong plugin type oops

57f9c10

Co-authored-by: Daniel Colson <composerinteralia@github.com>

Package and mount the auth_test_plugin ourselves

6dde8d9

Merge branch 'main' into cleartext-plugin-auth-switch

01877bd

Add mysql 5.7 auth test plugin

03e9c4f

Remove curlies

9d6b1cf

composerinteralia approved these changes Apr 4, 2024

View reviewed changes

casperisfine reviewed Apr 5, 2024

View reviewed changes

contrib/ruby/ext/trilogy-ruby/cext.c Outdated Show resolved Hide resolved

casperisfine reviewed Apr 5, 2024

View reviewed changes

Register global before assigning

e9a4c65

Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com>

adrianna-chang-shopify reviewed Apr 5, 2024

View reviewed changes

adrianna-chang-shopify approved these changes Apr 5, 2024

View reviewed changes

ngan force-pushed the cleartext-plugin-auth-switch branch 2 times, most recently from 79e5e56 to e9a4c65 Compare April 5, 2024 15:35

composerinteralia merged commit 770961e into trilogy-libraries:main Apr 5, 2024
22 checks passed

ngan mentioned this pull request Apr 7, 2024

Build mysql test image for CI #174

Merged

composerinteralia added a commit that referenced this pull request Apr 8, 2024

Put auth tests together

445f94f

Followup to #171 (comment) This puts the caching_sha2 and native plugin tests together with the cleartext ones.

composerinteralia mentioned this pull request Apr 8, 2024

Move auth tests together #177

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for cleartext authentication plugin #171

Implement support for cleartext authentication plugin #171

wildmaples commented Apr 3, 2024 •

edited

Loading

ngan commented Apr 4, 2024

composerinteralia left a comment

casperisfine Apr 5, 2024

adrianna-chang-shopify Apr 5, 2024

ngan Apr 5, 2024

adrianna-chang-shopify Apr 5, 2024

ngan Apr 8, 2024

adrianna-chang-shopify left a comment

adrianna-chang-shopify Apr 5, 2024

ngan Apr 5, 2024

ngan Apr 5, 2024

composerinteralia Apr 5, 2024

adrianna-chang-shopify Apr 5, 2024

composerinteralia commented Apr 5, 2024

adrianna-chang-shopify left a comment

composerinteralia commented Apr 5, 2024

		@@ -0,0 +1,16 @@
		require "test_helper"

		class AuthTest < TrilogyTest

Implement support for cleartext authentication plugin #171

Implement support for cleartext authentication plugin #171

Conversation

wildmaples commented Apr 3, 2024 • edited Loading

ngan commented Apr 4, 2024

composerinteralia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianna-chang-shopify left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

composerinteralia commented Apr 5, 2024

adrianna-chang-shopify left a comment

Choose a reason for hiding this comment

composerinteralia commented Apr 5, 2024

wildmaples commented Apr 3, 2024 •

edited

Loading