Add support for SSL-enabled Postgres servers #57

andrewtheis · 2018-05-10T02:02:00Z

This address issue #50, it is based on the previous PR by @FranzBusch.

Changes

Adds an optional tlsConfiguration property to PostgreSQLDatabaseConfig. An attempt to open an SSL connection will only be made if this is non-nil
Adds the handshake required by PostgreSQL to check for SSL support
Uses NIOOpenSSL to add a OpenSSLClientHandler to the PostgresSQLConnection channel pipeline using the aforementioned tlsConfiguration property
Updates contributing_bootstrap.sh to spin up a second container using a new postgres-ssl Docker image, which creates a self-signed certificate to enable ssl on the Postgres container
Updates PostgreSQLConnectionTests to add a SSL connection test

…yet)

…stgres with self-signed certificate

tanner0101

Great work. Very clean code! Just one comment regarding the TLS config.

tanner0101 · 2018-05-10T16:17:08Z

Sources/PostgreSQL/Connection/PostgreSQLConnection+TCP.swift


 extension PostgreSQLConnection {
    /// Connects to a Redis server using a TCP socket.
    public static func connect(
        hostname: String = "localhost",
        port: Int = 5432,
+        tlsConfiguration: TLSConfiguration? = nil,


The way I did this with HTTPClient was to wrap the different schemes in a pseudo-enum. I think this would be a solid solution here as well.

Something like:

public struct PostgreSQLScheme { internal enum Storage { case plainText case tls(TLSConfiguration?) } internal let storage: Storage // lots of conveniences here }

That way if we need to support more methods in the future, we don't bloat the connect method. We can also provide convenience extensions to that struct easier.

tanner0101 · 2018-05-10T16:19:35Z

Sources/PostgreSQL/Connection/PostgreSQLConnection+TCP.swift

+            if let tlsConfiguration = tlsConfiguration {
+                return connection.addSSLClientHandler(using: tlsConfiguration).transform(to: connection)
+            }
+            return Future.map(on: worker) { connection }


We should move to using the more performant worker.eventLoop.newSucceededFuture() method.

- Add PostgreSQLTransportConfig struct to wrap the possible transport methods - Use return worker.eventLoop.newSucceededFuture(result: connection) instead of Future.map

…enecheck/postgres-ssl docker image

tanner0101

Nice. Just some gardening comments.

tanner0101 · 2018-05-10T23:33:07Z

Sources/PostgreSQL/Connection/PostgreSQLConnection+TCP.swift


 extension PostgreSQLConnection {
    /// Connects to a Redis server using a TCP socket.
    public static func connect(
        hostname: String = "localhost",
        port: Int = 5432,
+        transportConfig: PostgreSQLTransportConfig = .cleartext,


I would use just transport for the param label, we have the strong type here anyway.

PostgreSQLConnection.connect(transport: .clearText)

That reads nicely.

tanner0101 · 2018-05-10T23:33:30Z

Sources/PostgreSQL/Connection/PostgreSQLConnection+TCP.swift

@@ -19,8 +21,12 @@ extension PostgreSQLConnection {
                }
        }

-        return bootstrap.connect(host: hostname, port: port).map(to: PostgreSQLConnection.self) { channel in
-            return .init(queue: handler, channel: channel)
+        return bootstrap.connect(host: hostname, port: port).flatMap(to: PostgreSQLConnection.self) { channel in


We can omit the to: arg in map/flatMap now.

tanner0101 · 2018-05-10T23:34:18Z

Sources/PostgreSQL/Connection/PostgreSQLConnection.swift

+                throw PostgreSQLError(identifier: "SSL support check", reason: "Unsupported message encountered during SSL support check: \(message).", source: .capture())
+            }
+            guard response == .supported else {
+                throw PostgreSQLError(identifier: "SSL support check", reason: "tlsConfiguration given in PostgresSQLConfiguration, but SSL connection not supported by PostgreSQL server", source: .capture())


Error messages should be grammatically correct (sentences should end with a period).

tanner0101 · 2018-05-10T23:35:37Z

Sources/PostgreSQL/Database/PostgreSQLTransportConfig.swift

+
+public struct PostgreSQLTransportConfig {
+    /// Does not attempt to enable TLS (this is the default)
+    public static var cleartext: PostgreSQLTransportConfig {


This should be clearText (with a capital T) so it matches Vapor's usage of plainText.

tanner0101 · 2018-05-10T23:36:33Z

Sources/PostgreSQL/Database/PostgreSQLTransportConfig.swift

+    }
+
+    /// Enables TLS and allows you to use a set `TLSConfiguration`
+    public static func customTLS(_ tlsConfiguration: TLSConfiguration)-> PostgreSQLTransportConfig {


Parameters should have doc blocks. I know the rest of this repo doesn't, but all officially tagged repos do.

tanner0101 · 2018-05-10T23:37:45Z

Sources/PostgreSQL/Message+Parse/PostgreSQLMessageDecoder.swift

+                let message = try PostgreSQLMessage.sslSupportResponse(decoder.decode())
+                ctx.fireChannelRead(wrapInboundOut(message))
+                VERBOSE("   [message=\(message)]")
+                return .continue


I tend to avoid early return in an if (since there's no fall-through error). The .needMoreData case should be in an else.

tanner0101 · 2018-05-10T23:38:13Z

Sources/PostgreSQL/Connection/PostgreSQLConnection+TCP.swift

+            if case .tls(let tlsConfiguration) = transportConfig.method {
+                return connection.addSSLClientHandler(using: tlsConfiguration).transform(to: connection)
+            }
+            return worker.eventLoop.newSucceededFuture(result: connection)


else here as well (re: comment about early return).

tanner0101 · 2018-05-10T23:38:45Z

Sources/PostgreSQL/Message/PostgreSQLSSLSupportResponse.swift

+/// For more info, see https://www.postgresql.org/docs/10/static/protocol-flow.html#id-1.10.5.7.11
+enum PostgreSQLSSLSupportResponse: UInt8, Decodable {
+    case supported = 0x53
+    case notSupported = 0x4E


Each case should have a doc block (required to reach 100% cov).

tanner0101 · 2018-05-10T23:39:23Z

Tests/PostgreSQLTests/PostgreSQLConnectionTests.swift

        #else
-        hostname = "localhost"
+        let hostname = "localhost"
+        let hostnameSSL = "localhost-ssl"


Really awesome solution with this 👍. I will try to emulate this with docker-compose in the future so that we don't need the #if Xcode here.

tanner0101 · 2018-05-10T23:40:03Z

circle.yml

@@ -18,6 +18,12 @@ jobs:
          POSTGRES_USER: vapor_username
          POSTGRES_DB: vapor_database
          POSTGRES_PASSWORD: vapor_password
+      - image: scenecheck/postgres-ssl:latest


Should we host this on Vapor's dockerhub? Unless you are happy to maintain this going forward.

This should hopefully require very little maintenance - it will always use the latest postgres version. The image is on Docker Hub so it shouldn't go away anytime soon.

tanner0101

Great work here. Thanks!

penny-coin · 2018-05-11T00:15:49Z

Hey @andrewtheis, you just merged a pull request, have a coin!

You now have 1 coins.

Andrew Theis added 3 commits May 8, 2018 22:43

Add support for SSL connections to PostgreSQL servers (no unit tests …

8bc1856

…yet)

Move SSL handshake to PostgreSQLConnection.connect static method

8ac3869

Add unit test for testing SSL and new docker image for spinning up po…

792778c

…stgres with self-signed certificate

andrewtheis changed the title ~~Add support for SSL (v2)~~ Add support for SSL-enabled Postgres servers May 10, 2018

Andrew Theis added 2 commits May 9, 2018 21:15

Minor cleanup and code doc comment improvement

fe583de

Remove commented line

a45a570

tanner0101 reviewed May 10, 2018

View reviewed changes

tanner0101 added the enhancement New feature or request label May 10, 2018

tanner0101 self-assigned this May 10, 2018

Andrew Theis added 10 commits May 10, 2018 16:05

Update based on PR feedback:

f784169

- Add PostgreSQLTransportConfig struct to wrap the possible transport methods - Use return worker.eventLoop.newSucceededFuture(result: connection) instead of Future.map

Update Circle.yml and contribute_bootstrap.sh to use new published sc…

f8ff921

…enecheck/postgres-ssl docker image

Trial and error

02f0115

Trial and error

f6bc8b6

Use different names

1dbc57d

Trial and error

afd5b54

Lets try hostnames 🤷🏼‍♂️

bc91f45

Typo

4cd934f

🤞

179c014

🤞

8daee2b

tanner0101 reviewed May 10, 2018

View reviewed changes

Andrew Theis added 3 commits May 10, 2018 18:44

Cleanup makeTest function

b36f057

Updates based on PR feedback

0d58f22

More updates based on PR feedback

2220abd

tanner0101 approved these changes May 11, 2018

View reviewed changes

tanner0101 added this to the 1.0.0-rc.2.2 milestone May 11, 2018

tanner0101 merged commit e4bbd47 into vapor:master May 11, 2018

tanner0101 mentioned this pull request May 11, 2018

Add support for SSL connections #51

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for SSL-enabled Postgres servers #57

Add support for SSL-enabled Postgres servers #57

andrewtheis commented May 10, 2018 •

edited

Loading

tanner0101 left a comment

tanner0101 May 10, 2018

tanner0101 May 10, 2018

tanner0101 left a comment

tanner0101 May 10, 2018 •

edited

Loading

tanner0101 May 10, 2018

tanner0101 May 10, 2018

tanner0101 May 10, 2018

tanner0101 May 10, 2018

tanner0101 May 10, 2018

tanner0101 May 10, 2018 •

edited

Loading

tanner0101 May 10, 2018

tanner0101 May 10, 2018

tanner0101 May 10, 2018

andrewtheis May 11, 2018

tanner0101 left a comment

penny-coin commented May 11, 2018

Add support for SSL-enabled Postgres servers #57

Add support for SSL-enabled Postgres servers #57

Conversation

andrewtheis commented May 10, 2018 • edited Loading

tanner0101 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanner0101 left a comment

Choose a reason for hiding this comment

tanner0101 May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanner0101 May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanner0101 left a comment

Choose a reason for hiding this comment

penny-coin commented May 11, 2018

andrewtheis commented May 10, 2018 •

edited

Loading

tanner0101 May 10, 2018 •

edited

Loading

tanner0101 May 10, 2018 •

edited

Loading