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

Add support for SSL-enabled Postgres servers #57

Merged
merged 18 commits into from May 11, 2018

Conversation

3 participants
@andrewtheis
Copy link
Contributor

andrewtheis commented May 10, 2018

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

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

andrewtheis added some commits May 10, 2018

@tanner0101
Copy link
Member

tanner0101 left a comment

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


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

This comment has been minimized.

@tanner0101

tanner0101 May 10, 2018

Member

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.

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

This comment has been minimized.

@tanner0101

tanner0101 May 10, 2018

Member

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

@tanner0101 tanner0101 self-assigned this May 10, 2018

andrewtheis added some commits May 10, 2018

Update based on PR feedback:
- Add PostgreSQLTransportConfig struct to wrap the possible transport methods
- Use return worker.eventLoop.newSucceededFuture(result: connection) instead of Future.map
@tanner0101
Copy link
Member

tanner0101 left a comment

Nice. Just some gardening comments.


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

This comment has been minimized.

@tanner0101

tanner0101 May 10, 2018

Member

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

PostgreSQLConnection.connect(transport: .clearText) 

That reads nicely.

@@ -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

This comment has been minimized.

@tanner0101

tanner0101 May 10, 2018

Member

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

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())

This comment has been minimized.

@tanner0101

tanner0101 May 10, 2018

Member

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


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

This comment has been minimized.

@tanner0101

tanner0101 May 10, 2018

Member

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

}

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

This comment has been minimized.

@tanner0101

tanner0101 May 10, 2018

Member

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

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

This comment has been minimized.

@tanner0101

tanner0101 May 10, 2018

Member

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

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

This comment has been minimized.

@tanner0101

tanner0101 May 10, 2018

Member

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

/// 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

This comment has been minimized.

@tanner0101

tanner0101 May 10, 2018

Member

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

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

This comment has been minimized.

@tanner0101

tanner0101 May 10, 2018

Member

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.

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

This comment has been minimized.

@tanner0101

tanner0101 May 10, 2018

Member

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

This comment has been minimized.

@andrewtheis

andrewtheis May 11, 2018

Author Contributor

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.

andrewtheis added some commits May 10, 2018

@tanner0101
Copy link
Member

tanner0101 left a comment

Great work here. Thanks!

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

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

3 checks passed

ci/circleci: linux Your tests passed on CircleCI!
Details
ci/circleci: linux-fluent Your tests passed on CircleCI!
Details
ci/circleci: linux-release Your tests passed on CircleCI!
Details
@penny-coin

This comment has been minimized.

Copy link

penny-coin commented May 11, 2018

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

You now have 1 coins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment