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

mysql-client: fix add authentication plugins in [email protected] #181805

Closed

Conversation

cswingler
Copy link

@cswingler cswingler commented Aug 20, 2024

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

This addresses #180498 .

The MySQL 9.0 client is not built with support for older authentication methods by default, which breaks the ability for the MySQL client, and packages that depend on it, to connect to servers still using authentication methods like mysql_native_password. This adjusts the build process to add those back in as plugins.

A bug has been opened upstream to address the discrepancy between the documentation and the actual configuration: https://bugs.mysql.com/bug.php?id=116616

@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Aug 20, 2024
@cswingler cswingler force-pushed the mysql-client-90-auth-plugin branch from 297f0cb to ba8b9c8 Compare August 20, 2024 17:27
This adds the `-DWITH_AUTHENTICATION_CLIENT_PLUGINS=yes` flag to add
support for older MySQL authentication methods. This support was in the
default build prior to MySQL 9.0, but were dropped with that release.
Closes Homebrew#180498
@cswingler cswingler force-pushed the mysql-client-90-auth-plugin branch from ba8b9c8 to 0dc3ffd Compare August 20, 2024 17:31
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Aug 20, 2024
@SMillerDev
Copy link
Member

Can you make an issue upstream requesting they enable this by default and add that as a comment? From the linked thread it seems like upstream thinks it's default already.

@cho-m cho-m changed the title Turn on authentication plugins in [email protected] mysql-client: fix add authentication plugins in [email protected] Sep 2, 2024
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Sep 23, 2024
@github-actions github-actions bot closed this Oct 1, 2024
@zhangyangyu
Copy link

zhangyangyu commented Oct 15, 2024

Can you make an issue upstream requesting they enable this by default and add that as a comment? From the linked thread it seems like upstream thinks it's default already.

@SMillerDev I downloaded the MySQL Community Server 9.0.1 Innovation on macOS from https://dev.mysql.com/downloads/mysql/5.5.html?os=31&version=5.1, it gets mysql_native_password plugin and in INFO_BIN WITH_AUTHENTICATION_CLIENT_PLUGINS is on. So I think Oracle do make it default already?
image
I can successfully connect to old version servers using native_password auth.

➜  bin ./mysql --comments -u '<user>' -h gateway01.us-east-1.dev.shared.aws.tidbcloud.com -P 4000 -D 'bikeshare' --ssl-mode=VERIFY_IDENTITY --ssl-ca=/etc/ssl/cert.pem -p'<password>' --plugin-dir=/Users/zhangyangyu/Downloads/mysql-9.0.1-macos14-arm64/lib/plugin

@thuvh
Copy link

thuvh commented Nov 11, 2024

should reopen this merge request?

@SMillerDev
Copy link
Member

If Oracle made it the default there is nothing for Homebrew to change. If they didn't, someone should make an issue about it on their side before we re-open anything.

@zent1n0
Copy link

zent1n0 commented Nov 11, 2024

If Oracle made it the default there is nothing for Homebrew to change. If they didn't, someone should make an issue about it on their side before we re-open anything.

image
Could tell that Oracle release is shipped with this plugin enabled.

@cswingler
Copy link
Author

Filed bug upstream: https://bugs.mysql.com/bug.php?id=116616&thanks=4

To note, the quoted documentation in #180498 is from the MySQL 9.0 server release notes, which are likely not treated as "official" documentation by upstream.

@fulghum
Copy link
Contributor

fulghum commented Dec 12, 2024

@SMillerDev – Now that Oracle's MySQL support team has verified and agreed with the bug @cswingler opened, can we please reconsider accepting this patch?

I understand Homebrew doesn't want to be in the business of modifying default build config, and you're absolutely correct that the root fix is upstream. However, this issue is affecting a lot of customers and it seems worth patching quickly in Homebrew, in addition to fixing in upstream. Could we please consider an exception since this is the second most upvoted issue in the Homebrew repository?

There is strong evidence that this was an accident by Oracle and will eventually be fixed upstream:

  • the MySQL-9.0 release notes call out that the mysql-9.0 client is intended to be backwards compatible and should support mysql_native_password by default.
  • the official MySQL-9.0 binaries from Oracle are built with this flag and include support for mysql_native_password by default in the client.
  • the MySQL support team has accepted, verified, and agreed with the upstream bug report.

We're seeing customer issue reports from Homebrew customers for our product and I know it would help a lot of other Homebrew customers to accept this patch, instead of waiting for Oracle's much slower release cycle.

Thank you for considering this! 🙏 We are very appreciative of all the work Homebrew maintainers do and I know accepting a build config patch should not be done lightly.

@SMillerDev
Copy link
Member

@fulghum I'm happy to review a PR like this one with a link to the upstream bug report.

@fulghum
Copy link
Contributor

fulghum commented Dec 12, 2024

Thank you @SMillerDev!!! 🙏

Are you able to reopen this PR if @cswingler updates the description to include the link to the accepted upstream bug report?

Or would you prefer we open a new/fresh PR with this change?

Thank you for considering an exception here!

@cswingler
Copy link
Author

I've modified the description to add the bug report as a link.

@SMillerDev can you reopen this PR, or should I just go ahead and submit a new one?

@SMillerDev
Copy link
Member

I can't reopen it. You'll have to make a new one.

@fulghum
Copy link
Contributor

fulghum commented Dec 18, 2024

Thank you @SMillerDev! I've opened up a PR (#201679) that applies the fix to the mysql formula, to fix the client included in that formula. I'm happy to apply @cswingler's fix to the mysql-client formula as well if that first PR is accepted. Just wanted to start there since that's what most of our customers have reported using.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants