feat: mysql and pg server support tls #641

SSebo · 2022-11-26T12:54:49Z

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

add tls support for mysql and pg

Please explain IN DETAIL what the changes are in this PR and why they are needed:

add a common TlsOption to save cert and key path, can also generate rustls::ServerConfig
pass tls option to mysql setup logic, then feed ServerConfig to opensrv_mysql
pass tls option to gp setup logic, then feed TlsAcceptor to pgwire

Checklist

I have written the necessary rustdoc comments.
I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

#515

SSebo · 2022-11-26T13:03:10Z

@sunng87 @MichaelScofield Lack of testing for now. Can I add integration test which use mysql-client to execute echo "SELECT * FROM foo" | mysql -h 127.0.0.1 --table --ssl-mode=REQUIRED for mysql, and something similar for pg?

codecov · 2022-11-26T13:04:31Z

Codecov Report

Merging #641 (7ee73b0) into develop (53ab19e) will increase coverage by 0.05%.
The diff coverage is 95.76%.

@@             Coverage Diff             @@
##           develop     #641      +/-   ##
===========================================
+ Coverage    86.32%   86.38%   +0.05%     
===========================================
  Files          406      407       +1     
  Lines        51351    51524     +173     
===========================================
+ Hits         44330    44508     +178     
+ Misses        7021     7016       -5

Flag	Coverage Δ
rust	`86.38% <95.76%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/datanode/src/server.rs	`0.00% <0.00%> (ø)`
src/frontend/src/server.rs	`0.00% <0.00%> (ø)`
src/servers/src/error.rs	`55.26% <0.00%> (+28.23%)`	⬆️
src/servers/src/lib.rs	`100.00% <ø> (ø)`
src/servers/src/postgres/auth_handler.rs	`82.71% <93.75%> (+2.41%)`	⬆️
src/servers/src/mysql/server.rs	`95.34% <95.23%> (+0.80%)`	⬆️
src/frontend/src/mysql.rs	`100.00% <100.00%> (ø)`
src/frontend/src/postgres.rs	`100.00% <100.00%> (ø)`
src/servers/src/postgres/server.rs	`98.38% <100.00%> (+0.34%)`	⬆️
src/servers/src/tls.rs	`100.00% <100.00%> (ø)`
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

src/servers/Cargo.toml

src/datanode/src/datanode.rs

src/common/base/src/tls.rs

src/frontend/src/mysql.rs

sunng87 · 2022-11-27T01:59:22Z

More on mysql and postgres ssl configuration:
https://dev.mysql.com/doc/refman/5.7/en/using-encrypted-connections.html
https://www.percona.com/blog/enabling-and-enforcing-ssl-tls-for-postgresql-connections/

sunng87 · 2022-11-27T02:01:33Z

@SSebo Is ssl supported in those client libraries we are using for testing? Using mysql-client for testing will add more dependencies in our build and CI requirements, which is not preferred.

SSebo · 2022-11-27T11:01:09Z

@SSebo Is ssl supported in those client libraries we are using for testing? Using mysql-client for testing will add more dependencies in our build and CI requirements, which is not preferred.

for opensrv-mysql integration test, mysql-client is installed in github action ubuntun by default.
I'll try to find out if rust based client support ssl.

SSebo · 2022-11-27T11:03:24Z

@sunng87 I found there is no way to force pgwire to reject connect if TlsAcceptor is supplied but client want to connect normally. This is used for TlsMode::Require

sunng87/pgwire#22

sunng87 · 2022-11-27T13:14:48Z

@SSebo perhaps we can reject the client by checking its ssl state in startup handler. When the client sends its Startup message to server, we can reply with error. I will check postgresql's behaviour to see if we really need to cut it during SslRequest.

sunng87 · 2022-11-28T08:47:44Z

@SSebo I confirmed postgres enforces this check when handling Startup message. So we can do it in on_startup callback of pgwire's auth handler

This is how postgresql receives startup message and responds plain text connection when ssl is enforced.

Frame 24: 145 bytes on wire (1160 bits), 145 bytes captured (1160 bits) on interface lo, id 0
Ethernet II, Src: 00:00:00_00:00:00 (00:00:00:00:00:00), Dst: 00:00:00_00:00:00 (00:00:00:00:00:00)
Internet Protocol Version 4, Src: 127.0.0.1, Dst: 127.0.0.1
Transmission Control Protocol, Src Port: 59040, Dst Port: 5432, Seq: 1, Ack: 1, Len: 79
PostgreSQL
    Type: Startup message
    Length: 79
    Protocol major version: 3
    Protocol minor version: 0
    Parameter name: user
    Parameter value: sunng
    Parameter name: database
    Parameter value: testdb
    Parameter name: application_name
    Parameter value: psql
    Parameter name: client_encoding
    Parameter value: UTF8


Frame 26: 219 bytes on wire (1752 bits), 219 bytes captured (1752 bits) on interface lo, id 0
Ethernet II, Src: 00:00:00_00:00:00 (00:00:00:00:00:00), Dst: 00:00:00_00:00:00 (00:00:00:00:00:00)
Internet Protocol Version 4, Src: 127.0.0.1, Dst: 127.0.0.1
Transmission Control Protocol, Src Port: 5432, Dst Port: 59040, Seq: 1, Ack: 80, Len: 153
PostgreSQL
    Type: Error
    Length: 152
    Severity: FATAL
    Text: FATAL
    Code: 28000
    Message: no pg_hba.conf entry for host "127.0.0.1", user "sunng", database "testdb", no encryption
    File: auth.c
    Line: 545
    Routine: ClientAuthentication

SSebo · 2022-11-28T10:26:20Z

@sunng87 Since we should handle the plain client connect to secure required server as the real server, how about we put the logic in opensrv-mysql and pgwire. Every user who use opensrv-mysql and pgwire enable tls feature may need this. Currently I just drop the plain connection in secure required mysql which is not proper I think.

sunng87 · 2022-11-28T10:49:31Z

@SSebo I realized that opensrv-mysql and pgwire may provide different level of abstraction. pgwire expose on_startup to developer to customize this behaviour, while opensrv-mysql does not expose this startup apis.

In real world, this ssl policy is a little complex,it involves tuple of host, user and database to decide if ssl is to be enforced. It's better to be implemented where these three entities are defined and provided.

So the problem is we need to customize mysql connection handshake (or startup) process. We have another issue #600 can be related to this as well.

But for now I think it's ok to simply drop the plain connection to move on. We can resolve that in future PR

SSebo · 2022-11-28T13:24:06Z

@sunng87 mysql and pg both covered by unit test.

src/servers/src/mysql/server.rs

src/servers/src/postgres/auth_handler.rs

sunng87 · 2022-11-29T02:16:04Z

@SSebo Look almost good to me! Please check these two minor comments.

@MichaelScofield Please take a look as well. Thank you!

killme2008

What a good job! Thanks a lot. I have a few comments, PTAL.

src/servers/Cargo.toml

src/servers/src/tls.rs

src/servers/tests/mysql/mysql_server_test.rs

src/servers/tests/postgres/mod.rs

src/servers/src/tls.rs

.cargo/config.toml

killme2008

LGTM. Almost done, Good job.

MichaelScofield

LGTM, ping me if you resolve the remaining comments, and I'll merge this PR, thx!

killme2008 · 2022-11-30T03:42:47Z

@SSebo There is also a code format issue, https://github.com/GreptimeTeam/greptimedb/actions/runs/3580016212/jobs/6021767404

SSebo · 2022-11-30T04:09:15Z

@SSebo There is also a code format issue, https://github.com/GreptimeTeam/greptimedb/actions/runs/3580016212/jobs/6021767404

got it.

killme2008 · 2022-11-30T04:46:47Z

@SSebo Congrats on your first PR! Thanks a lot.

sunng87 · 2022-11-30T07:33:39Z

Congratulations! Thanks for your effort! This is a big step for enabling greptimedb in shared environment.

* feat: mysql and pg server support tls * chore: replace opensrv-mysql to original * chore: TlsOption is required but supply default value * feat: mysql server support force tls * chore: move TlsOption to servers * test: mysql server disable / prefer / required tls mode * test: pg server disable / prefer / required tls mode * chore: add doc and remove no used code * chore: add TODO and restore cargo linker config

SSebo force-pushed the develop branch from ac45aaf to d06eec9 Compare November 26, 2022 13:00

SSebo commented Nov 26, 2022

View reviewed changes

src/servers/Cargo.toml Outdated Show resolved Hide resolved

PsiACE mentioned this pull request Nov 26, 2022

Feature: tls support for mysql handler databendlabs/databend#8983

Closed

sunng87 reviewed Nov 27, 2022

View reviewed changes

src/datanode/src/datanode.rs Outdated Show resolved Hide resolved

sunng87 reviewed Nov 27, 2022

View reviewed changes

src/common/base/src/tls.rs Outdated Show resolved Hide resolved

sunng87 reviewed Nov 27, 2022

View reviewed changes

src/common/base/src/tls.rs Outdated Show resolved Hide resolved

sunng87 reviewed Nov 27, 2022

View reviewed changes

src/frontend/src/mysql.rs Outdated Show resolved Hide resolved

SSebo force-pushed the develop branch from 2cb433b to 6bed566 Compare November 27, 2022 10:18

SSebo requested a review from sunng87 November 28, 2022 08:27

SSebo force-pushed the develop branch from c689940 to ff34397 Compare November 28, 2022 08:32

sunng87 reviewed Nov 29, 2022

View reviewed changes

src/servers/src/mysql/server.rs Outdated Show resolved Hide resolved

sunng87 reviewed Nov 29, 2022

View reviewed changes

src/servers/src/postgres/auth_handler.rs Show resolved Hide resolved

sunng87 requested review from MichaelScofield and shuiyisong November 29, 2022 02:15

sunng87 linked an issue Nov 29, 2022 that may be closed by this pull request

TLS support for database protocols #515

Closed

killme2008 reviewed Nov 29, 2022

View reviewed changes

sunng87 added the docs-required This change requires docs update. label Nov 29, 2022

MichaelScofield reviewed Nov 29, 2022

View reviewed changes

src/servers/src/tls.rs Outdated Show resolved Hide resolved

src/servers/src/tls.rs Outdated Show resolved Hide resolved

src/servers/src/tls.rs Outdated Show resolved Hide resolved

SSebo requested review from killme2008, sunng87 and MichaelScofield and removed request for killme2008 and sunng87 November 29, 2022 18:41

killme2008 reviewed Nov 30, 2022

View reviewed changes

.cargo/config.toml Outdated Show resolved Hide resolved

killme2008 approved these changes Nov 30, 2022

View reviewed changes

MichaelScofield approved these changes Nov 30, 2022

View reviewed changes

sunng87 added docs-required This change requires docs update. and removed docs-required This change requires docs update. labels Nov 30, 2022

sunng87 mentioned this pull request Nov 30, 2022

Update docs for feat: mysql and pg server support tls GreptimeTeam/docs#85

Closed

SSebo added 9 commits November 30, 2022 12:06

feat: mysql and pg server support tls

c7e8d81

chore: replace opensrv-mysql to original

7be763e

chore: TlsOption is required but supply default value

6c68f91

feat: mysql server support force tls

5444a3f

chore: move TlsOption to servers

1277606

test: mysql server disable / prefer / required tls mode

5c23c81

test: pg server disable / prefer / required tls mode

a7e36fe

chore: add doc and remove no used code

2dc1139

chore: add TODO and restore cargo linker config

7ee73b0

SSebo force-pushed the develop branch from 9123097 to 7ee73b0 Compare November 30, 2022 04:07

SSebo requested a review from MichaelScofield November 30, 2022 04:20

killme2008 merged commit 68c2de8 into GreptimeTeam:develop Nov 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: mysql and pg server support tls #641

feat: mysql and pg server support tls #641

SSebo commented Nov 26, 2022 •

edited

Loading

SSebo commented Nov 26, 2022 •

edited

Loading

codecov bot commented Nov 26, 2022 •

edited

Loading

sunng87 commented Nov 27, 2022

sunng87 commented Nov 27, 2022

SSebo commented Nov 27, 2022

SSebo commented Nov 27, 2022 •

edited

Loading

sunng87 commented Nov 27, 2022

sunng87 commented Nov 28, 2022

SSebo commented Nov 28, 2022

sunng87 commented Nov 28, 2022

SSebo commented Nov 28, 2022

sunng87 commented Nov 29, 2022

killme2008 left a comment

killme2008 left a comment

MichaelScofield left a comment

killme2008 commented Nov 30, 2022

SSebo commented Nov 30, 2022

killme2008 commented Nov 30, 2022

sunng87 commented Nov 30, 2022

feat: mysql and pg server support tls #641

feat: mysql and pg server support tls #641

Conversation

SSebo commented Nov 26, 2022 • edited Loading

What's changed and what's your intention?

Checklist

Refer to a related PR or issue link (optional)

SSebo commented Nov 26, 2022 • edited Loading

codecov bot commented Nov 26, 2022 • edited Loading

Codecov Report

sunng87 commented Nov 27, 2022

sunng87 commented Nov 27, 2022

SSebo commented Nov 27, 2022

SSebo commented Nov 27, 2022 • edited Loading

sunng87 commented Nov 27, 2022

sunng87 commented Nov 28, 2022

SSebo commented Nov 28, 2022

sunng87 commented Nov 28, 2022

SSebo commented Nov 28, 2022

sunng87 commented Nov 29, 2022

killme2008 left a comment

Choose a reason for hiding this comment

killme2008 left a comment

Choose a reason for hiding this comment

MichaelScofield left a comment

Choose a reason for hiding this comment

killme2008 commented Nov 30, 2022

SSebo commented Nov 30, 2022

killme2008 commented Nov 30, 2022

sunng87 commented Nov 30, 2022

SSebo commented Nov 26, 2022 •

edited

Loading

SSebo commented Nov 26, 2022 •

edited

Loading

codecov bot commented Nov 26, 2022 •

edited

Loading

SSebo commented Nov 27, 2022 •

edited

Loading