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

Implement mysql_clear_password #2533

Merged
merged 6 commits into from Jul 31, 2023
Merged

Implement mysql_clear_password #2533

merged 6 commits into from Jul 31, 2023

Conversation

ldanilek
Copy link
Contributor

@ldanilek ldanilek commented Jun 7, 2023

Fixes #2443 by implementing AuthPlugin::MySqlClearPassword as described in mysql docs and sufficient to connect to a MySQL server hosted by AWS using IAM authentication with ephemeral passwords.

For consistency with other clients, like mysql, and to avoid accidental security issues, we also add a flag to ConnectionOpts to enable this plugin, as described here.

Copy link

@nipunn1313 nipunn1313 left a comment

Choose a reason for hiding this comment

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

nice!!

sqlx-mysql/src/protocol/connect/auth_switch.rs Outdated Show resolved Hide resolved
sqlx-mysql/src/protocol/connect/auth_switch.rs Outdated Show resolved Hide resolved
@nipunn1313
Copy link

nipunn1313 commented Jun 7, 2023

looks good!
If one of the maintainers could take a look and decide whether to merge, that would be great.

@abonander
Copy link
Collaborator

@ldanilek cargo fmt needs to be run.

@ldanilek
Copy link
Contributor Author

@abonander i ran cargo fmt. can you take another look 🙏

Copy link
Collaborator

@abonander abonander left a comment

Choose a reason for hiding this comment

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

I've been putting off merging this because honestly the idea is rather horrifying to me.

With this plugin enabled, and without additional defensive configuration like ssl-mode=VERIFY_IDENTITY (which isn't even suggested in the AWS tutorial), it would be quite straightforward for an attacker, e.g. by a compromised router, to trick the application into divulging its credentials.

The documentation on the configuration option should at the very least recommend a stronger .ssl_mode() setting than the default, either Required or VerifyCa in the case of connecting to AWS where a root certificate is provided.

sqlx-mysql/src/options/mod.rs Outdated Show resolved Hide resolved
@ldanilek
Copy link
Contributor Author

The documentation on the configuration option should at the very least recommend a stronger .ssl_mode() setting than the default, either Required or VerifyCa in the case of connecting to AWS where a root certificate is provided.

i could add an assertion on ssl-mode when enable_cleartext_plugin is true, if that would reduce the security risk. maybe in DoHandshake::new we could throw an error if enable_cleartext_plugin && ssl_mode < REQUIRED? does that sound like a good place to put such an assertion, and what kind of error would we throw?

@abonander
Copy link
Collaborator

We shouldn't have a panic that you can't work around without enabling TLS as that could get really annoying.

I would accept maybe logging a warning, but making the implications clearer in the documentation is the main point.

@ldanilek ldanilek requested a review from abonander July 17, 2023 18:40
@ldanilek
Copy link
Contributor Author

@abonander could you take another look please 🙏 . i've updated the comments to be scarier, and added a warning log when connecting in an insecure manner.

@abonander abonander merged commit febf9ed into launchbadge:main Jul 31, 2023
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for mysql_clear_password auth
3 participants