trilogy-libraries / trilogy

Trilogy is a client library for MySQL-compatible database servers, designed for performance, flexibility, and ease of embedding.
MIT License
698 stars 69 forks source link

Feature: caching_sha2_password over insecure connections #199

Open casperisfine opened 2 weeks ago

casperisfine commented 2 weeks ago

I ran into something I don't quite understand the other day when making a gem compatible with trilogy: https://github.com/cainlevy/scenic-mysql_adapter/pull/2 / https://github.com/cainlevy/scenic-mysql_adapter/actions/runs/10516090228/

That gem CI uses the default mysql baked into GitHub Actions ubuntu-latest images:

    steps:
      - uses: actions/checkout@v3
      - name: Setup Ruby
        uses: ruby/setup-ruby@v1
        with:
          ruby-version: ${{ matrix.ruby }}
          bundler-cache: true
      - name: Start and create DB
        run: |
          sudo service mysql start
          sleep 10
          mysql -u root -proot -e "CREATE DATABASE scenic_mysql_adapter_test;"
      - name: Run tests
        env:
          DATABASE_URL: ${{ matrix.client }}://root:root@127.0.0.1/scenic_mysql_adapter_test
        run: |
          bundle exec rake test

Nothing else changed in the test suite, just the same code using Active Record, one with mysql2 the other with trilogy.

The mysql2 jobs went fine, but the trilogy ones failed to connect with:

ActiveRecord::ConnectionNotEstablished: trilogy_auth_recv: caching_sha2_password requires either TCP with TLS or a unix socket: TRILOGY_UNSUPPORTED
    vendor/bundle/ruby/3.3.0/gems/activerecord-7.2.1/lib/active_record/connection_adapters/trilogy_adapter.rb:34:in `rescue in new_client'
    vendor/bundle/ruby/3.3.0/gems/activerecord-7.2.1/lib/active_record/connection_adapters/trilogy_adapter.rb:30:in `new_client'
    vendor/bundle/ruby/3.3.0/gems/activerecord-7.2.1/lib/active_record/connection_adapters/trilogy_adapter.rb:[17](https://github.com/cainlevy/scenic-mysql_adapter/actions/runs/10516090228/job/29137551437#step:5:18)4:in `connect'
    vendor/bundle/ruby/3.3.0/gems/activerecord-7.2.1/lib/active_record/connection_adapters/trilogy_adapter.rb:182:in `reconnect'
    vendor/bundle/ruby/3.3.0/gems/activerecord-7.2.1/lib/active_record/connection_adapters/abstract_adapter.rb:662:in `block in reconnect!'

So I don't know what mysql2 is doing, if it somehow accept to do caching_sha2 without TLS, or if somehow it fallbacks to another method, but I think we should do the same thing if we want to ease the transition from mysql2 to trilogy.

cc @adrianna-chang-shopify @eileencodes @jhawthorn @matthewd

composerinteralia commented 2 weeks ago

Maybe this? https://github.com/mysql/mysql-server/blob/596f0d238489a9cf9f43ce1ff905984f58d227b6/sql-common/client_authentication.cc#L695-L791

casperisfine commented 2 weeks ago

Good catch, looks like it.

composerinteralia commented 2 weeks ago

Actually I wonder if the incompatibility is mainly a difference in mysql2 defaulting to ssl enabled and trilogy defaulting to disabled. If I disable ssl for mysql2 I get a similar error (this error message one of the possible paths from the code I linked):

Mysql2::Client.new(host: '127.0.0.1', username: 'caching_sha2', password: 'abc', ssl_mode: :disabled)
#=> Authentication plugin 'caching_sha2_password' reported error: Authentication requires secure connection. (Mysql2::Error)

(I set up a caching_sha2 user called caching_sha2)

tisba commented 1 week ago

AFIK this is by design by the trilogy authors, see https://github.com/trilogy-libraries/trilogy/issues/26#issuecomment-1995521936 and https://github.com/trilogy-libraries/trilogy/pull/165

Note that we have chosen on purpose to only implement the path where TLS or a unix socket is used.

casperisfine commented 1 week ago

Yeah, I discussed this with GitHub folks. It wasn't implemented because it's a lot of work and tricky, and not the most used part. So it was deemed better to ship the caching_sha2+TLS part and leave the raw TCP part of the protocol unimplemented for now.

I still think we should implement it at some point because if mysql-server ship with a caching_sha2 user but no SSL that may be an issue.

tisba commented 1 week ago

It would be nice to also be closer to a drop-in placement for the mysql2 gem in many cases. I for example stopped evaluating it some months ago because of this. Would probably have worked fine in prod, but would have added some complications to our dev and test setup. Still possible, but wasn't worth it.

jhawthorn commented 3 days ago

We'd gladly accept a PR, but per-the previous PR, it is quite complicated.