libpq sslpassword parameter and callback function

This time with attachment.

On 10/31/19 6:33 PM, Andrew Dunstan wrote:

This patch provides for an sslpassword parameter for libpq, and a hook
that a client can fill in for a callback function to set the password.

This provides similar facilities to those already available in the JDBC
driver.

There is also a function to fetch the sslpassword from the connection
parameters, in the same way that other settings can be fetched.

This is mostly the excellent work of my colleague Craig Ringer, with a
few embellishments from me.

Here are his notes:

    Allow libpq to non-interactively decrypt client certificates that
are stored
    encrypted by adding a new "sslpassword" connection option.
   
    The sslpassword option offers a middle ground between a cleartext
key and
    setting up advanced key mangement via openssl engines, PKCS#11, USB
crypto
    offload and key escrow, etc.
   
    Previously use of encrypted client certificate keys only worked if
the user
    could enter the key's password interactively on stdin, in response
to openssl's
    default prompt callback:
   
        Enter PEM passhprase:
   
    That's infesible in many situations, especially things like use from
    postgres_fdw.
   
    This change also allows admins to prevent libpq from ever prompting
for a
    password by calling:
   
        PQsetSSLKeyPassHook(PQdefaultSSLKeyPassHook);
   
    which is useful since OpenSSL likes to open /dev/tty to prompt for a
password,
    so even closing stdin won't stop it blocking if there's no user
input available.
    Applications may also override or extend SSL password fetching with
their own
    callback.
   
    There is deliberately no environment variable equivalent for the
sslpassword
    option.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 10/31/19 6:34 PM, Andrew Dunstan wrote:

This time with attachment.

On 10/31/19 6:33 PM, Andrew Dunstan wrote:

This patch provides for an sslpassword parameter for libpq, and a hook
that a client can fill in for a callback function to set the password.

This provides similar facilities to those already available in the JDBC
driver.

There is also a function to fetch the sslpassword from the connection
parameters, in the same way that other settings can be fetched.

This is mostly the excellent work of my colleague Craig Ringer, with a
few embellishments from me.

Here are his notes:

    Allow libpq to non-interactively decrypt client certificates that
are stored
    encrypted by adding a new "sslpassword" connection option.
   
    The sslpassword option offers a middle ground between a cleartext
key and
    setting up advanced key mangement via openssl engines, PKCS#11, USB
crypto
    offload and key escrow, etc.
   
    Previously use of encrypted client certificate keys only worked if
the user
    could enter the key's password interactively on stdin, in response
to openssl's
    default prompt callback:
   
        Enter PEM passhprase:
   
    That's infesible in many situations, especially things like use from
    postgres_fdw.
   
    This change also allows admins to prevent libpq from ever prompting
for a
    password by calling:
   
        PQsetSSLKeyPassHook(PQdefaultSSLKeyPassHook);
   
    which is useful since OpenSSL likes to open /dev/tty to prompt for a
password,
    so even closing stdin won't stop it blocking if there's no user
input available.
    Applications may also override or extend SSL password fetching with
their own
    callback.
   
    There is deliberately no environment variable equivalent for the
sslpassword
    option.

I should also mention that this patch provides for support for DER
format certificates and keys.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, 1 Nov 2019 at 07:27, Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
wrote:

On 10/31/19 6:34 PM, Andrew Dunstan wrote:

This time with attachment.

On 10/31/19 6:33 PM, Andrew Dunstan wrote:

This patch provides for an sslpassword parameter for libpq, and a hook
that a client can fill in for a callback function to set the password.

This provides similar facilities to those already available in the JDBC
driver.

There is also a function to fetch the sslpassword from the connection
parameters, in the same way that other settings can be fetched.

This is mostly the excellent work of my colleague Craig Ringer, with a
few embellishments from me.

Here are his notes:

Allow libpq to non-interactively decrypt client certificates that
are stored
encrypted by adding a new "sslpassword" connection option.

The sslpassword option offers a middle ground between a cleartext
key and
setting up advanced key mangement via openssl engines, PKCS#11, USB
crypto
offload and key escrow, etc.

Previously use of encrypted client certificate keys only worked if
the user
could enter the key's password interactively on stdin, in response
to openssl's
default prompt callback:

Enter PEM passhprase:

That's infesible in many situations, especially things like use from
postgres_fdw.

This change also allows admins to prevent libpq from ever prompting
for a
password by calling:

PQsetSSLKeyPassHook(PQdefaultSSLKeyPassHook);

which is useful since OpenSSL likes to open /dev/tty to prompt for a
password,
so even closing stdin won't stop it blocking if there's no user
input available.
Applications may also override or extend SSL password fetching with
their own
callback.

There is deliberately no environment variable equivalent for the
sslpassword
option.

I should also mention that this patch provides for support for DER
format certificates and keys.

Yep, that was a trivial change I rolled into it.

FWIW, this is related to two other patches: the patch to allow passwordless
fdw connections with explicit superuser approval, and the patch to allow
sslkey/sslpassword to be set as user mapping options in postgres_fdw .
Together all three patches make it possible to use SSL client certificates
to manage authentication in postgres_fdw user mappings.

--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise

On Sun, Nov 10, 2019 at 05:47:24PM +0800, Craig Ringer wrote:

Yep, that was a trivial change I rolled into it.

FWIW, this is related to two other patches: the patch to allow passwordless fdw
connections with explicit superuser approval, and the patch to allow sslkey/
sslpassword to be set as user mapping options in postgres_fdw . Together all
three patches make it possible to use SSL client certificates to manage
authentication in postgres_fdw user mappings.

Oh, nice, greatly needed!

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On 10/31/19 7:27 PM, Andrew Dunstan wrote:

On 10/31/19 6:34 PM, Andrew Dunstan wrote:

This time with attachment.

On 10/31/19 6:33 PM, Andrew Dunstan wrote:

This patch provides for an sslpassword parameter for libpq, and a hook
that a client can fill in for a callback function to set the password.

This provides similar facilities to those already available in the JDBC
driver.

There is also a function to fetch the sslpassword from the connection
parameters, in the same way that other settings can be fetched.

This is mostly the excellent work of my colleague Craig Ringer, with a
few embellishments from me.

Here are his notes:

    Allow libpq to non-interactively decrypt client certificates that
are stored
    encrypted by adding a new "sslpassword" connection option.
   
    The sslpassword option offers a middle ground between a cleartext
key and
    setting up advanced key mangement via openssl engines, PKCS#11, USB
crypto
    offload and key escrow, etc.
   
    Previously use of encrypted client certificate keys only worked if
the user
    could enter the key's password interactively on stdin, in response
to openssl's
    default prompt callback:
   
        Enter PEM passhprase:
   
    That's infesible in many situations, especially things like use from
    postgres_fdw.
   
    This change also allows admins to prevent libpq from ever prompting
for a
    password by calling:
   
        PQsetSSLKeyPassHook(PQdefaultSSLKeyPassHook);
   
    which is useful since OpenSSL likes to open /dev/tty to prompt for a
password,
    so even closing stdin won't stop it blocking if there's no user
input available.
    Applications may also override or extend SSL password fetching with
their own
    callback.
   
    There is deliberately no environment variable equivalent for the
sslpassword
    option.

I should also mention that this patch provides for support for DER
format certificates and keys.

Here's an updated version of the patch, adjusted to the now committed
changes to TestLib.pm.

cheers

andrew

--

Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 11/25/19 4:09 PM, Andrew Dunstan wrote:

On 10/31/19 7:27 PM, Andrew Dunstan wrote:

On 10/31/19 6:34 PM, Andrew Dunstan wrote:

This time with attachment.

On 10/31/19 6:33 PM, Andrew Dunstan wrote:

This patch provides for an sslpassword parameter for libpq, and a hook
that a client can fill in for a callback function to set the password.

This provides similar facilities to those already available in the JDBC
driver.

There is also a function to fetch the sslpassword from the connection
parameters, in the same way that other settings can be fetched.

This is mostly the excellent work of my colleague Craig Ringer, with a
few embellishments from me.

Here are his notes:

    Allow libpq to non-interactively decrypt client certificates that
are stored
    encrypted by adding a new "sslpassword" connection option.
   
    The sslpassword option offers a middle ground between a cleartext
key and
    setting up advanced key mangement via openssl engines, PKCS#11, USB
crypto
    offload and key escrow, etc.
   
    Previously use of encrypted client certificate keys only worked if
the user
    could enter the key's password interactively on stdin, in response
to openssl's
    default prompt callback:
   
        Enter PEM passhprase:
   
    That's infesible in many situations, especially things like use from
    postgres_fdw.
   
    This change also allows admins to prevent libpq from ever prompting
for a
    password by calling:
   
        PQsetSSLKeyPassHook(PQdefaultSSLKeyPassHook);
   
    which is useful since OpenSSL likes to open /dev/tty to prompt for a
password,
    so even closing stdin won't stop it blocking if there's no user
input available.
    Applications may also override or extend SSL password fetching with
their own
    callback.
   
    There is deliberately no environment variable equivalent for the
sslpassword
    option.

I should also mention that this patch provides for support for DER
format certificates and keys.

Here's an updated version of the patch, adjusted to the now committed
changes to TestLib.pm.

Here's an update now we have backed out the TestLib changes. The tests
that need a pty are skipped.

cheers

andrew

--

Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, failed

Hi Andrew,

I've reviewed your "libpq sslpassword parameter and callback function" patch (0001-libpq-sslpassword-der-support.patch), and only found a few minor things (otherwise it looked good to me):

1) There's a few trailing white-space warnings on patch application (from where you modified to skip 2 of the tests):
git apply 0001-libpq-sslpassword-der-support.patch
0001-libpq-sslpassword-der-support.patch:649: trailing whitespace.
# so they don't hang. For now they are not performed.
0001-libpq-sslpassword-der-support.patch:659: trailing whitespace.

warning: 2 lines add whitespace errors.

2) src/interfaces/libpq/libpq-fe.h
The following portion of the comment should be removed.

+ * 2ndQPostgres extension. If you need to be compatible with unpatched libpq
+ * you must dlsym() these.

3) Documentation for the "PQsslpassword" function should be added to the libpq "33.2 Connection Status Functions" section.

I made the following notes about how/what I reviewed and tested:

- Applied patch and built Postgres (--with-openssl --enable-tap-tests), checked build output
- Checked patch code modifications (format, logic, memory usage, efficiency, corner cases etc.)
- Built documentation and checked updated portions (format, grammar, details, completeness etc.)
- Checked test updates
- Ran updated contrib/dblink tests - confirmed all passed
- Ran updated SSL (TAP) tests - confirmed all passed
- Ran "make installcheck-world", as per review requirements
- Wrote small libpq-based app to test:
- new APIs (PQsslpassword, PQsetSSLKeyPassHook, PQgetSSLKeyPassHook, PQdefaultSSLKeyPassHook)
- passphrase-protected key with/without patch
- patch with/without new key password callack
- patch and certificate with/without pass phrase protection on key
- default callback, callback delegation
- PEM/DER keys

Regards,
Greg

On 11/28/19 10:25 PM, Greg Nancarrow wrote:

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, failed

Hi Andrew,

I've reviewed your "libpq sslpassword parameter and callback function" patch (0001-libpq-sslpassword-der-support.patch), and only found a few minor things (otherwise it looked good to me):

1) There's a few trailing white-space warnings on patch application (from where you modified to skip 2 of the tests):
git apply 0001-libpq-sslpassword-der-support.patch
0001-libpq-sslpassword-der-support.patch:649: trailing whitespace.
# so they don't hang. For now they are not performed.
0001-libpq-sslpassword-der-support.patch:659: trailing whitespace.

warning: 2 lines add whitespace errors.

2) src/interfaces/libpq/libpq-fe.h
The following portion of the comment should be removed.

+ * 2ndQPostgres extension. If you need to be compatible with unpatched libpq
+ * you must dlsym() these.

3) Documentation for the "PQsslpassword" function should be added to the libpq "33.2 Connection Status Functions" section.

I made the following notes about how/what I reviewed and tested:

- Applied patch and built Postgres (--with-openssl --enable-tap-tests), checked build output
- Checked patch code modifications (format, logic, memory usage, efficiency, corner cases etc.)
- Built documentation and checked updated portions (format, grammar, details, completeness etc.)
- Checked test updates
- Ran updated contrib/dblink tests - confirmed all passed
- Ran updated SSL (TAP) tests - confirmed all passed
- Ran "make installcheck-world", as per review requirements
- Wrote small libpq-based app to test:
- new APIs (PQsslpassword, PQsetSSLKeyPassHook, PQgetSSLKeyPassHook, PQdefaultSSLKeyPassHook)
- passphrase-protected key with/without patch
- patch with/without new key password callack
- patch and certificate with/without pass phrase protection on key
- default callback, callback delegation
- PEM/DER keys

Thanks, nice thorough review.

Here's an updated patch that I think fixes all the things you mentioned.
I plan to commit this tomorrow.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Nov 29, 2019 at 09:27:02AM -0500, Andrew Dunstan wrote:

On 11/28/19 10:25 PM, Greg Nancarrow wrote:

3) Documentation for the "PQsslpassword" function should be added to the libpq "33.2 Connection Status Functions" section.

I plan to commit this tomorrow.

The PQsslpassword() function retrieves a connection parameter value, which one
can retrieve with PQconninfo(). Since introducing PQconninfo(), we have not
added any per-parameter accessor functions. Would you remove PQsslpassword()?

On 12/6/19 2:57 AM, Noah Misch wrote:

On Fri, Nov 29, 2019 at 09:27:02AM -0500, Andrew Dunstan wrote:

On 11/28/19 10:25 PM, Greg Nancarrow wrote:

3) Documentation for the "PQsslpassword" function should be added to the libpq "33.2 Connection Status Functions" section.

I plan to commit this tomorrow.

The PQsslpassword() function retrieves a connection parameter value, which one
can retrieve with PQconninfo(). Since introducing PQconninfo(), we have not
added any per-parameter accessor functions. Would you remove PQsslpassword()?

*sigh* ok

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services