Re: libpq sslpassword parameter and callback function

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