Re: libpq sslpassword parameter and callback function

From: Greg Nancarrow <gregn4422(at)gmail(dot)com>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Cc: Andrew Dunstan <adunstan(at)postgresql(dot)org>
Subject: Re: libpq sslpassword parameter and callback function
Date: 2019-11-29 03:25:45
Message-ID: 157499794571.13092.9894985502685806190.pgcf@coridan.postgresql.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Etsuro Fujita 2019-11-29 03:35:33 Re: A problem about partitionwise join
Previous Message Richard Guo 2019-11-29 03:07:53 Re: A problem about partitionwise join