Issue 786 mysql ssl support #2491

p-alik · 2020-08-28T19:33:18Z

This PR aims to add ssl connection support for MySQL backend, which is requested in the issue #786 .
At this stage it allow setting of mysql option MYSQL_OPT_SSL_MODE only.
The connection could be established with MYSQL_OPT_SSL_MODE set to SSL_MODE_DISABLED, SSL_MODE_PREFERRED and SSL_MODE_REQUIRED.

Tested with
DATABASE_URL='mysql://***?ssl_mode=(disabled|required|preferred)' cargo test --manifest-path diesel/Cargo.toml --features mysql --no-default-features mysql

https://dev.mysql.com/doc/c-api/8.0/en/mysql-options.html
https://dev.mysql.com/doc/c-api/8.0/en/c-api-encrypted-connections.html

ELD

This looks good to me, though the Windows and Rust 1.40.0 tests are failing. It’s looking like the mysqlclient_sys library doesn’t export the SSL mode variants on Windows, which is perplexing, but I’m not positive that’s the root cause. I haven’t had a chance to dig more into it.

I’m not comfortable giving approval on this PR until we can figure out why Windows isn’t building, but it does look good as-is right now!

p-alik · 2020-08-29T20:22:50Z

I've to improve this PR for sake of Windows and I suppose to do that next week.

p-alik · 2020-08-30T18:42:02Z

Due to sgrif/mysqlclient-sys#27 there is no way to set ssl-mode for MySQL connection on Windows in same way as on Linux/Unix yet.

Would it be appropriate to restrict setting of MySQL option MYSQL_OPT_SSL_MODE for #[cfg(not(windows))]? Any other suggestions?

ELD · 2020-08-30T19:49:42Z

That would probably be a suitable alternative for now, since the corresponding Windows bindings don’t work right now.

weiznich

I've left some comments, otherwise this is looking good 👍

(Sorry for taking some time to do a review here, it took some time to catch up with everything after holiday.)

diesel/Cargo.toml

diesel/src/mysql/connection/raw.rs

biot023 · 2021-05-25T08:48:07Z

Hi -- I guess nothing happened with this?
Are there any workarounds for SSL-secured access?
I'm going to talk to our DBA service about maybe SSH tunnelling but I'm not confident that will work out...

weiznich · 2021-05-25T14:21:21Z

@biot023 Commenting on old issues/PR's just to ask for updates is something that is highly discouraged. Please stop doing that if you have nothing meaningful (like how to resolve the issues outlined above) to contribute.

biot023 · 2021-05-26T09:18:22Z

@biot023 Commenting on old issues/PR's just to ask for updates is something that is highly discouraged. Please stop doing that if you have nothing meaningful (like how to resolve the issues outlined above) to contribute.

It was a genuine question. Never mind, consider me suitably discouraged.

biot023 · 2021-05-26T14:49:17Z

Maybe not entirely discouraged.
I've been able to set up ProxySQL (https://proxysql.com) in a docker instance to handle the SSL bit of the connection.
I can just point any diesel services at that image.
Hope that's of some help to somebody else struggling with this.

Neo-Zhixing · 2021-12-30T01:22:50Z

Maybe as a stopgap measure we should fork libmysqlclient-sys into the diesel org for now and merge all pending PRs there? There are no new commits in that repo since 2019 and @sgrif has been inactive since July 2021. MySQL SSL support is a real use case, and doing that would unblock us on multiple issues.

weiznich · 2021-12-30T08:05:44Z

@Neo-Zhixing There is no need to fork mysqlclient-sys. I have all merge and publish rights for that crate. Unfortunately I do not have the time to do a lot active work there while maintaining diesel at the same time, so this is basically waiting for someone laying out a basic plan about the future of mysqlclient-sys and someone who implements it.

thomasmost · 2022-01-21T16:44:10Z

I've been able to set up ProxySQL (https://proxysql.com) in a docker instance to handle the SSL bit of the connection.
I can just point any diesel services at that image.
Hope that's of some help to somebody else struggling with this.

@biot023 this is I think exactly what I'll need to do—you don't happen to have an example Dockerfile/diesel config that you'd feel comfortable sharing do you (scrubbed of anything sensitive, obviously)?

biot023 · 2022-01-24T08:55:33Z

@biot023 this is I think exactly what I'll need to do—you don't happen to have an example Dockerfile/diesel config that you'd feel comfortable sharing do you (scrubbed of anything sensitive, obviously)?

Hi -- I really struggled with this. I got it working, as I recall, but then had an issue with tons of connections being established to the main server and never dropped -- took the whole site down in about 4 minutes, when I tested it!
It's probable that you could get it working but in our case I went past the point of sunken cost fallacy and we abandoned diesel for that particular project.
That's not throwing shade on diesel, btw -- I still use it whenever I can, it just didn't work out in this scenario for us.
Sorry I'm not more help, man.
Cheers,
Doug.

weiznich · 2022-01-24T14:50:21Z

@biot023 and @thomasmost Just to reiterate what I've written above: I would be more than happy to merge support for mysql ssl connections to diesel, but that requires work from someone. I'm happy to provide some pointers + discuss design, but implementation needs to be done by someone else.

thomasmost · 2022-01-24T17:21:22Z

@weiznich Let's do it! What is your schedule like/what forum would you prefer for such a discussion?

weiznich · 2022-01-25T11:37:47Z

@thomasmost I had a quick look at open questions here: The interface proposed in this PR looks good to me. There are the following open points from my point of view:

Windows support
- This is the largest open point
- Requires changes to mysqlclient-sys, which may require restructuring the code there. That's the point that is quite fuzzy for me now.
Rebasing the PR to the current master branch
Tests?

thomasmost · 2022-01-25T16:00:44Z

Got it! I will see if I can get in there and work this out.

p-alik · 2022-01-26T13:14:17Z

Will rebase the feature branch.

thomasmost · 2022-01-26T21:02:47Z

@p-alik how can I help get these tests passing?

thomasmost · 2022-01-28T16:15:09Z

@weiznich now this is lookin pretty good

weiznich · 2022-01-28T16:20:24Z

@thomasmost Yes the diesel side of this change looks good, beside the fact that this only supports linux/macos. I would like to solve the windows issue first before finally merging this. As already mentioned that would require updating the bindings in mysqlclient-sys, which requires access to bindgen on windows. That's unfortunately something I cannot help with other than merging PR's and cutting releases.
I image something like following here:

Update the generated bindings in mysqlclient-sys via a PR there (that should likely update bindings for both variants)
Update this PR to use the mysqlclient-sys version based on the previous PR to check everything works fine
I will merge the mysqlclient-sys PR + cut a release there
Update this PR again to use the now newly released mysqlclient-sys version
Merge this PR

ssl_mode is defined as Option<mysqlclient_sys::mysql_ssl_mode>

Windows binding of mysqlclient-sys doesn't provide values for the option MYSQL_OPT_SSL_MODE See: sgrif/mysqlclient-sys#27

0.2.0 do not provide enum mysql_ssl_mode. The enum was introduces in sgrif/mysqlclient-sys@6575414

* update diesel/Cargo.toml * remove obsolete windows checks Co-authored-by: Thomas Constantine Moore <tomismore@gmail.com>

weiznich

Looks good for me if the following things are added:

A changelog entry here + adjusting the minimal supported mysqlclient-sys version under "REMOVED"
A note in the documentation of MysqlConnection::establish on the available options.

* add `ssl_mode` support to `MysqlConnection::establish` * remove support for `mysqlclient-sys` < `0.2.5`

p-alik · 2022-01-30T12:00:44Z

Added requested notes to the changelog.

thomasmost

LGTM!

thomasmost · 2022-01-31T23:53:25Z

@weiznich one last follow-up; just so I understand, are we talking days, weeks, or months before this gets included in a release of the crate?

Until that time, any reason I can't get my app working with an (admittedly, unstable) build using the master branch as its dependency, i.e.

[dependencies]
diesel = { git = "https://github.com/diesel-rs/diesel", features = ["mysql", "extras", "chrono"] }

?

weiznich · 2022-02-01T10:45:41Z

one last follow-up; just so I understand, are we talking days, weeks, or months before this gets included in a release of the crate?

We generally do not give any estimates when a certain release happens other than: When it's done. Additionally as this PR is not merged yet (due to missing documentation) it's not certain if the next release will contain this feature or not.

Until that time, any reason I can't get my app working with an (admittedly, unstable) build using the master branch as its dependency, i.e.

Please open an discussion thread about that with more details about what you understand under "can't get working". A PR is definitively not the right place to discuss such issues.

p-alik · 2022-02-01T13:32:31Z

as this PR is not merged yet (due to missing documentation) it's not certain if the next release will contain this feature or not.

9dc434e aims to add missed documentation.

database URL may contain GET parameters * `unix_socket` * `ssl_mode`

thomasmost · 2022-02-01T17:15:55Z

Until that time, any reason I can't get my app working with an (admittedly, unstable) build using the master branch as its dependency, i.e.

Please open an discussion thread about that with more details about what you understand under "can't get working". A PR is definitively not the right place to discuss such issues.

I think you misunderstood me @weiznich — I wasn't asking for support — nvm

Thanks again for your work on this crate

yerke · 2022-02-01T20:37:02Z

@p-alik @weiznich Thank you very much for working on this!

libsilverwolf · 2024-06-14T09:26:06Z

Is this feature also usable with diesel-cli ?

I tried

DATABASE_URL="mysql://***:3306/port?ssl_mode=required" diesel print-schema

but got

Could not connect to database via `mysql://***:3306/port?ssl_mode=required`: Connections using insecure transport are prohibited while --require_secure_transport=ON.

I'm using MariaDB.

$ diesel --version
diesel 
 Version: 2.2.1
 Supported Backends: mysql

p-alik force-pushed the issue-786-mysql-ssl-support branch from b3b63c4 to 58882b3 Compare August 29, 2020 06:46

weiznich requested a review from a team August 29, 2020 07:17

ELD reviewed Aug 29, 2020

View reviewed changes

p-alik force-pushed the issue-786-mysql-ssl-support branch from b967bbc to f7bec43 Compare August 31, 2020 19:50

p-alik mentioned this pull request Sep 3, 2020

diesel_cli Mysql tests would fail on database url with query parameters #2494

Closed

weiznich reviewed Sep 15, 2020

View reviewed changes

diesel/Cargo.toml Outdated Show resolved Hide resolved

diesel/src/mysql/connection/raw.rs Outdated Show resolved Hide resolved

diesel/src/mysql/connection/raw.rs Outdated Show resolved Hide resolved

weiznich modified the milestone: 2.0 Jan 26, 2021

p-alik force-pushed the issue-786-mysql-ssl-support branch from 1ccf832 to 04c4dc4 Compare January 26, 2022 13:43

p-alik force-pushed the issue-786-mysql-ssl-support branch 2 times, most recently from 1fce5b8 to 04c4dc4 Compare January 27, 2022 12:35

p-alik force-pushed the issue-786-mysql-ssl-support branch 3 times, most recently from d88faf1 to 5a4671f Compare January 29, 2022 07:18

p-alik and others added 5 commits January 29, 2022 08:23

add ssl_mode field to mysql ConnectionOptions struct

744be0f

ssl_mode is defined as Option<mysqlclient_sys::mysql_ssl_mode>

mysql::RawConnection sets ssl_mode

41b6435

restrict MySQL SSL connection mode for non Windows only

7778780

Windows binding of mysqlclient-sys doesn't provide values for the option MYSQL_OPT_SSL_MODE See: sgrif/mysqlclient-sys#27

bump mysqlclient-sys version to 0.2.2

18bc18a

0.2.0 do not provide enum mysql_ssl_mode. The enum was introduces in sgrif/mysqlclient-sys@6575414

Upgrade mysqlclient-sys dependency to 0.2.5

1a66269

* update diesel/Cargo.toml * remove obsolete windows checks Co-authored-by: Thomas Constantine Moore <tomismore@gmail.com>

p-alik force-pushed the issue-786-mysql-ssl-support branch from 5a4671f to 1a66269 Compare January 29, 2022 07:23

weiznich approved these changes Jan 30, 2022

View reviewed changes

changelog: note support MySQL SSL connection support

a06e025

* add `ssl_mode` support to `MysqlConnection::establish` * remove support for `mysqlclient-sys` < `0.2.5`

thomasmost approved these changes Jan 31, 2022

View reviewed changes

p-alik force-pushed the issue-786-mysql-ssl-support branch from 24b612d to 3ce9b48 Compare February 1, 2022 13:36

Document features of MysqlConnection::establish

9dc434e

database URL may contain GET parameters * `unix_socket` * `ssl_mode`

p-alik force-pushed the issue-786-mysql-ssl-support branch from 3ce9b48 to 9dc434e Compare February 1, 2022 13:37

weiznich merged commit 41404e6 into diesel-rs:master Feb 1, 2022

weiznich mentioned this pull request Feb 1, 2022

Add SSL support for mysql #786

Closed

p-alik deleted the issue-786-mysql-ssl-support branch February 1, 2022 19:05

thomasmost mentioned this pull request Feb 18, 2022

add ssl_ca to accepted connection url query params #3049

Merged

This comment was marked as off-topic.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 786 mysql ssl support #2491

Issue 786 mysql ssl support #2491

p-alik commented Aug 28, 2020

ELD left a comment

p-alik commented Aug 29, 2020

p-alik commented Aug 30, 2020

ELD commented Aug 30, 2020

weiznich left a comment

biot023 commented May 25, 2021

weiznich commented May 25, 2021

biot023 commented May 26, 2021

biot023 commented May 26, 2021

Neo-Zhixing commented Dec 30, 2021

weiznich commented Dec 30, 2021

thomasmost commented Jan 21, 2022

biot023 commented Jan 24, 2022

weiznich commented Jan 24, 2022

thomasmost commented Jan 24, 2022

weiznich commented Jan 25, 2022

thomasmost commented Jan 25, 2022

p-alik commented Jan 26, 2022

thomasmost commented Jan 26, 2022 •

edited

Loading

thomasmost commented Jan 28, 2022

weiznich commented Jan 28, 2022 •

edited

Loading

weiznich left a comment

p-alik commented Jan 30, 2022

thomasmost left a comment

thomasmost commented Jan 31, 2022 •

edited

Loading

weiznich commented Feb 1, 2022

p-alik commented Feb 1, 2022 •

edited

Loading

thomasmost commented Feb 1, 2022 •

edited

Loading

yerke commented Feb 1, 2022

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

libsilverwolf commented Jun 14, 2024 •

edited

Loading

Issue 786 mysql ssl support #2491

Issue 786 mysql ssl support #2491

Conversation

p-alik commented Aug 28, 2020

ELD left a comment

Choose a reason for hiding this comment

p-alik commented Aug 29, 2020

p-alik commented Aug 30, 2020

ELD commented Aug 30, 2020

weiznich left a comment

Choose a reason for hiding this comment

biot023 commented May 25, 2021

weiznich commented May 25, 2021

biot023 commented May 26, 2021

biot023 commented May 26, 2021

Neo-Zhixing commented Dec 30, 2021

weiznich commented Dec 30, 2021

thomasmost commented Jan 21, 2022

biot023 commented Jan 24, 2022

weiznich commented Jan 24, 2022

thomasmost commented Jan 24, 2022

weiznich commented Jan 25, 2022

thomasmost commented Jan 25, 2022

p-alik commented Jan 26, 2022

thomasmost commented Jan 26, 2022 • edited Loading

thomasmost commented Jan 28, 2022

weiznich commented Jan 28, 2022 • edited Loading

weiznich left a comment

Choose a reason for hiding this comment

p-alik commented Jan 30, 2022

thomasmost left a comment

Choose a reason for hiding this comment

thomasmost commented Jan 31, 2022 • edited Loading

weiznich commented Feb 1, 2022

p-alik commented Feb 1, 2022 • edited Loading

thomasmost commented Feb 1, 2022 • edited Loading

yerke commented Feb 1, 2022

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

libsilverwolf commented Jun 14, 2024 • edited Loading

thomasmost commented Jan 26, 2022 •

edited

Loading

weiznich commented Jan 28, 2022 •

edited

Loading

thomasmost commented Jan 31, 2022 •

edited

Loading

p-alik commented Feb 1, 2022 •

edited

Loading

thomasmost commented Feb 1, 2022 •

edited

Loading

libsilverwolf commented Jun 14, 2024 •

edited

Loading