Adding ssl config params to Postgres URI

[cbaker6]

Use pg-promise native pg-connection-string to parse uri instead of PostgresConfigParser.js. The change allows for a more felxible uri for ssl along with the ability to supply additional parameters for establishing a connection with pg-promise.

Update: Adding parameter parsing to PostgresConfigParser.js instead...

For example, the current PostgresConfigParser can only take a boolean ssl value and PostgresConfigParser limits the variables that can passed to pg-promise through an URI. This is important because #6555 upgraded pg-promise and added changes in node-postgres 8 below:
Change default behavior when not specifying rejectUnauthorized with the SSL connection parameters. Previously we defaulted to rejectUnauthorized: false when it was not specifically included. We now default to rejectUnauthorized: true. Manually specify { ssl: { rejectUnauthorized: false } } for old behavior.

In previous versions of pg-promise, this gave a warning. Currently you can't pass rejectUnauthorized through a URI string as an object to ssl. This means that postgres users who use ssl not known to node-posgres are automatically rejected (which is basically any cert because you can't pass in ca with the current PostgresConfigParser.js). Switching to the pg-connection-string (which is part of the node-postures driver) allows for:

The resulting config contains a subset of the following properties:

- host - Postgres server hostname or, for UNIX domain sockets, the socket filename
- port - port on which to connect
- user - User with which to authenticate to the server
- password - Corresponding password
- database - Database name within the server
- client_encoding - string encoding the client will use
- ssl, either a boolean or an object with properties
-- cert
-- key
-- ca
any other query parameters (for example, application_name) are preserved intact.

Allowing you to pass parameters like:

Query parameters follow a ? character, including the following special query parameters:

- host=<host> - sets host property, overriding the URL's host
- encoding=<encoding> - sets the client_encoding property
- ssl=1, ssl=true, ssl=0, ssl=false - sets ssl to true or false, accordingly
- sslcert=<filename> - reads data from the given file and includes the result as ssl.cert
- sslkey=<filename> - reads data from the given file and includes the result as ssl.key
- sslrootcert=<filename> - reads data from the given file and includes the result as ssl.ca

PostgresConfigParser is left intact for legacy purposes as well is it can possibly be used to parse other custom items out of the uri string in the future.

[cbaker6]

Note that the parse-spec along with with any testing pg-promise and node-postures perform handles the test cases needed for this

[cbaker6]

@dplewis this passed in travis, but for some reason CI still shows yellow here. Can you rerun when you get a chance?

If you see no issues with this PR, it's ready to be merged

[davimacedo]

I believe it is ready to merge. @dplewis agree?

[dplewis]

I believe the Custom Parser is there because of the limited options pg-connections has. I see the appeal to remove it but you could just add the additional options you want to it like sslcert etc to fix your use case.

We do parse out a few custom items like poolSize which would no longer be there.

@vitaly-t Thoughts?

[cbaker6]

@dplewis If I'm understanding the readme of pg-connection-string correctly, "any other query parameters (for example, application_name) are preserved intact.". The custom parser actually provides the opposite of what you mentioned when compared to "pg-connection-string" because only parameters that the custom parser knows can be passed. Since all params are "preserved intact" and passed down to node-postgres, things like poolSize and any other parameter can be used assuming they are in a format node-postgres understands.

If this is not the case, then using @vitaly-t connection-string might be a better option. I wrote about this a little bit here

[cbaker6]

Another option could be to roll back to the previous version of pg-promise for now, if node-postgrres can't use the additional parameters passed to pg-connection-string. When a uri solution is provided, move back to the latest version

[cbaker6]

I found a discussion about this on pg-connection-string, gajus/slonik#60.

It doesn't look they decided on which one to use, connection-string or pg-connection-string. If pg-connection-string doesn't work the way I assumed it works above, using connection-string or editing the custom parser will be the next step.

[vitaly-t]

I wasted too much time in the past, trying to get the base driver to start using the generic connection-string. I even did a complete PR for to patch the limited string parser.

But after all that, nothing happened.

The internal to pg parser is limited, so is pg-connection-string, whereas the generic connection-string has no limitation. You can define parameters as you like, and then create a connection object from it, as shown here.

[cbaker6]

@dplewis @davimacedo based on the discussion I've reverted this back to using PostgresConfigParser and added in the ability to parse ssl params as well as the rest of the ones that show up on pg-promise. I've also added some test cases...

A note, to create the ssl object as described on pg-promise, a uri is can be created using
postgres://address/db?ca=/path&pfx=/path&cert=/path&key=/path. Basically parameters that belong to the ssl object like "ca, pfx, key, etc." are passed as regular parameters. This is reasonable because each sub-parameter is still unique when added to the set of all possible parameters parsed. This is slightly different from the approach that pg-connection-string took as they require "sslca, sslpfx, etc." and then discard "ssl" after. I didn't think this was needed because of the uniqueness mentioned above. Also, since the custom parser is part of Parse, you will know when you add a parameter that isn't unique.

Lastly, I removed the default test-case of when ssl is not supplied and setting it to "ssl=false". This is already done by default in node-postgres and not needed. If I left it, it would create the scenario of switching the ssl variable from a boolean to an object, which I guess is allowed in js, but not something I like doing since I'm use to strongly-typed languages and I can see it creating some bugs in the future. The way I set it up, ssl is either a "boolean" or an "object".

Let me know what you think...

[codecov]

Codecov Report

Merging #6580 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6580      +/-   ##
==========================================
+ Coverage   93.90%   93.95%   +0.05%     
==========================================
  Files         169      169              
  Lines       11968    12301     +333     
==========================================
+ Hits        11239    11558     +319     
- Misses        729      743      +14     

Impacted Files	Coverage Δ
.../Adapters/Storage/Postgres/PostgresConfigParser.js	100.00% <100.00%> (ø)
src/GraphQL/loaders/parseClassQueries.js	96.07% <0.00%> (-1.84%)	⬇️
...rc/Adapters/Storage/Mongo/MongoSchemaCollection.js	96.42% <0.00%> (-1.38%)	⬇️
src/GraphQL/loaders/parseClassTypes.js	93.93% <0.00%> (-0.22%)	⬇️
src/RestWrite.js	93.64% <0.00%> (-0.17%)	⬇️
src/Controllers/DatabaseController.js	95.19% <0.00%> (ø)
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	93.52% <0.00%> (+0.02%)	⬆️
src/GraphQL/helpers/objectsQueries.js	91.66% <0.00%> (+0.29%)	⬆️
src/GraphQL/ParseGraphQLSchema.js	97.71% <0.00%> (+0.30%)	⬆️
src/middlewares.js	98.17% <0.00%> (+0.89%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2fa25d...90914fa. Read the comment docs.

[cbaker6]

I've tested params like "rejectUnauthorized=false/true" and "ca=/path/to/file" on my servers that use ssl certs and this works.

Some anecdotal notes about using pg-promise and it's underlying drivers with ssl:

• Certificates from certified CA's (postgres://username:password@address/db?ca=/path/to/file)
  -- When using the ca parameter I had to use the whole certificate chain, meaning it had to contain the root cert or else I got errors like, UNABLE_TO_GET_ISSUER_CERT. The cert being verified didn't need to be in the chain.
  -- In this case rejectUnauthorized=true should be used or not supplied at all (the default value is rejectUnauthorized=true) or else you will get errors when trying to connect.
• Self-signed certificates (postgres://username:password@address/db?rejectUnauthorized=false)
  -- In this case use rejectUnauthorized=false. If you leave this value as default (rejectUnauthorized=true), you will get errors like, Failed: self signed certificate in certificate chain
  -- More details here

A discussion about the ssl changes in pg-promise/node-postgres is here, but only some of the notes above are in the direct discussion

[cbaker6]

This may need to be documented somewhere:

Parse-server versions that use Postgres have the following compatibility with ssl:

• 4.2.1 or above
  -- MITM protection if configured properly, meaning ca=/path/to/file and rejectUnauthorized=true (this is true by default in 4.2.0 and up) in URI
  -- See this comment to see how to use self-signed vs a CA signed cert
  -- URI supports postgres://address/db?ssl=true&ca=/path/to/file&pfx=/path/to/file&cert=/path/to/file&key=/path/to/file&passphrase=string&secureOptions=number

ssl {
        ca?: string (file path)
        pfx?: string (file path)
        cert?: string (file path) 
        key?: string (file path) 
        passphrase?: string
        rejectUnauthorized?: boolean
        secureOptions?: number
    }

• 4.2.0
  -- No ssl capability (this is because older versions of PostgresConfigParser.js and the changes made to node-postgres 8). Since rejectUnauthorized is defaulted to true, you need to pass in a CA for verification or all SSL connections will be rejected
  -- No MITM protection
  -- if you attempt to use ssl=true you receive the following error when you try to connect: "code":"UNABLE_TO_VERIFY_LEAF_SIGNATURE","stack":"Error: unable to verify the first certificate\n. Note that I get this when testing a cert from InCommon.
  -- There may be a way to enable ssl by using a node-postgres environment variable, PGSSLMODE='require', but as pointed out on pg-promise, Please note that overriding defaults via pg.defaults does not work for all parameters (see #699), and should be avoided. If this does work, I'm sure you can't use ssl=true/false as it may offset the what's set by the environment variable
  -- For details see this discussion to see why ssl is broken because of the changes made in node-postgres. Basically PostgresConfigParser doesn’t have the ability to allow changes to rejectUnauthorized or provide a ca cert, so all ssl connections are blocked
• <= 4.1.0
  -- Only supports ssl=true/false in URI
  -- No MITM protection (this is because older versions of PostgresConfigParser.js didn't support)

[cbaker6]

Note that this is ready to be reviewed, it passed in Travis, but for some reason didn't update here.

[cbaker6]

@dplewis have you had a chance to look at the changes here. What do you think about my comments about SSL and MITM in the previous versions of parse-server?

[dplewis]

@cbaker6 Thanks for taking the time to research and document your findings. Its really helpful. We just let users know to update to the latest version of Parse Server.

I don't know why travis keeps getting stuck.

[cbaker6]

@dplewis I couldn't determine a good place to document how to use the updated postgres uri. All available are:

-- URI supports postgres://address/db?ssl=true&rejectUnauthorized=false&ca=/path/to/file&pfx=/path/to/file&cert=/path/to/file&key=/path/to/file&passphrase=string&secureOptions=number&max=number&query_timeout=idleTimeoutMillis=number&poolSize=number&binary=false&keepAlive=true

ssl {
        ca?: string (file path)
        pfx?: string (file path)
        cert?: string (file path) 
        key?: string (file path) 
        passphrase?: string
        rejectUnauthorized?: boolean
        secureOptions?: number
    }

Some useful combinations are below:

• SSL w/ no verification - postgres://address/db?ssl=true&rejectUnauthorized=false
• SSL w/ verification - postgres://address/db?ca=/path/to/file

People should be able to figure out the rest from there...

[dplewis]

Hmm the Parse Server Guide would be a good place.
https://docs.parseplatform.org/parse-server/guide/#postgres

[cbaker6]

Just adding some more details about ssl (w & w/o verification). node-postgres has made similar adjustments pg-connection-string, https://github.com/brianc/node-postgres/blob/master/CHANGELOG.md#pg810

No changes needed for parse-server since the changes in this PR on PostgresConfgParser.js worked around these issues.