Potentially misleading name of libpq pass phrase hook
Reviewing TLS changes for v13 I came across one change which I think might be
better off with a library qualified name. The libpq frontend sslpassword hook
added in 4dc63552109f65 is OpenSSL specific, but it has a very generic name:
PQsetSSLKeyPassHook(PQsslKeyPassHook_type hook);
This IMO has potential for confusion if we ever commit another TLS backend,
since the above hook wont work for any other library (except maybe OpenSSL
derivatives like LibreSSL et.al). The backends will always have differently
named hooks, as the signatures will be different, but having one with a generic
name and another with a library qualified name doesn't seem too friendly to
anyone implementing with libpq.
As a point of reference; in the backend we added a TLS init hook in commit
896fcdb230e72 which also is specific to OpenSSL, but the name is library
qualified making the purpose and usecase perfectly clear: openssl_tls_init_hook.
Since we haven't shipped this there is still time to rename, which IMO is the
right way forward. PQsslKeyPassHook_<library>_type would be one option, but
perhaps there are better alternatives?
Thoughts?
cheers ./daniel
On Thu, May 14, 2020 at 3:03 PM Daniel Gustafsson <daniel@yesql.se> wrote:
Reviewing TLS changes for v13 I came across one change which I think might
be
better off with a library qualified name. The libpq frontend sslpassword
hook
added in 4dc63552109f65 is OpenSSL specific, but it has a very generic
name:PQsetSSLKeyPassHook(PQsslKeyPassHook_type hook);
This IMO has potential for confusion if we ever commit another TLS backend,
since the above hook wont work for any other library (except maybe OpenSSL
derivatives like LibreSSL et.al). The backends will always have
differently
named hooks, as the signatures will be different, but having one with a
generic
name and another with a library qualified name doesn't seem too friendly to
anyone implementing with libpq.As a point of reference; in the backend we added a TLS init hook in commit
896fcdb230e72 which also is specific to OpenSSL, but the name is library
qualified making the purpose and usecase perfectly clear:
openssl_tls_init_hook.Since we haven't shipped this there is still time to rename, which IMO is
the
right way forward. PQsslKeyPassHook_<library>_type would be one option,
but
perhaps there are better alternatives?Thoughts?
ISTM this should be renamed yeah -- and it should probably go on the open
item lists, and with the schedule for the beta perhaps dealt with rather
urgently?
//Magnus
On 2020-May-15, Magnus Hagander wrote:
On Thu, May 14, 2020 at 3:03 PM Daniel Gustafsson <daniel@yesql.se> wrote:
Since we haven't shipped this there is still time to rename, which
IMO is the right way forward. PQsslKeyPassHook_<library>_type would
be one option, but perhaps there are better alternatives?ISTM this should be renamed yeah -- and it should probably go on the open
item lists, and with the schedule for the beta perhaps dealt with rather
urgently?
Seems easy enough to do! +1 on Daniel's suggested renaming.
CCing Andrew as committer.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Magnus Hagander <magnus@hagander.net> writes:
On Thu, May 14, 2020 at 3:03 PM Daniel Gustafsson <daniel@yesql.se> wrote:
Since we haven't shipped this there is still time to rename, which IMO
is the right way forward. PQsslKeyPassHook_<library>_type would be one
option, but perhaps there are better alternatives?
ISTM this should be renamed yeah -- and it should probably go on the open
item lists, and with the schedule for the beta perhaps dealt with rather
urgently?
+1. Once beta1 is out the cost to change the name goes up noticeably.
Not that we *couldn't* do it later, but it'd be better to have it be
right in beta1.
regards, tom lane
On 5/15/20 8:21 PM, Tom Lane wrote:
Magnus Hagander <magnus@hagander.net> writes:
On Thu, May 14, 2020 at 3:03 PM Daniel Gustafsson <daniel@yesql.se> wrote:
Since we haven't shipped this there is still time to rename, which IMO
is the right way forward. PQsslKeyPassHook_<library>_type would be one
option, but perhaps there are better alternatives?ISTM this should be renamed yeah -- and it should probably go on the open
item lists, and with the schedule for the beta perhaps dealt with rather
urgently?+1. Once beta1 is out the cost to change the name goes up noticeably.
Not that we *couldn't* do it later, but it'd be better to have it be
right in beta1.
+1 on all of the above.
I noticed this has been added to Open Items; I added a note that the
plan is to fix before the Beta 1 wrap.
Jonathan
On Fri, May 15, 2020 at 09:21:52PM -0400, Jonathan S. Katz wrote:
+1 on all of the above.
I noticed this has been added to Open Items; I added a note that the
plan is to fix before the Beta 1 wrap.
+1. Thanks.
Agreed. PQsslKeyPassHook_<library>_type sounds fine to me as
convention. Wouldn't we want to also rename PQsetSSLKeyPassHook and
PQgetSSLKeyPassHook, appending an "_OpenSSL" to both?
--
Michael
On 16 May 2020, at 03:56, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, May 15, 2020 at 09:21:52PM -0400, Jonathan S. Katz wrote:
+1 on all of the above.
I noticed this has been added to Open Items; I added a note that the
plan is to fix before the Beta 1 wrap.+1. Thanks.
Agreed. PQsslKeyPassHook_<library>_type sounds fine to me as
convention. Wouldn't we want to also rename PQsetSSLKeyPassHook and
PQgetSSLKeyPassHook, appending an "_OpenSSL" to both?
Yes, I think we should. The attached performs the rename of the hook functions
and the type, and also fixes an off-by-one-'=' in a header comment which my OCD
couldn't unsee.
cheers ./daniel
Attachments:
openssl_hook_rename.patchapplication/octet-stream; name=openssl_hook_rename.patch; x-unix-mode=0644Download+21-22
On 5/15/20 8:08 PM, Alvaro Herrera wrote:
On 2020-May-15, Magnus Hagander wrote:
On Thu, May 14, 2020 at 3:03 PM Daniel Gustafsson <daniel@yesql.se> wrote:
Since we haven't shipped this there is still time to rename, which
IMO is the right way forward. PQsslKeyPassHook_<library>_type would
be one option, but perhaps there are better alternatives?ISTM this should be renamed yeah -- and it should probably go on the open
item lists, and with the schedule for the beta perhaps dealt with rather
urgently?Seems easy enough to do! +1 on Daniel's suggested renaming.
CCing Andrew as committer.
I'll attend to this today.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 5/16/20 3:16 AM, Daniel Gustafsson wrote:
On 16 May 2020, at 03:56, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, May 15, 2020 at 09:21:52PM -0400, Jonathan S. Katz wrote:
+1 on all of the above.
I noticed this has been added to Open Items; I added a note that the
plan is to fix before the Beta 1 wrap.+1. Thanks.
Agreed. PQsslKeyPassHook_<library>_type sounds fine to me as
convention. Wouldn't we want to also rename PQsetSSLKeyPassHook and
PQgetSSLKeyPassHook, appending an "_OpenSSL" to both?Yes, I think we should. The attached performs the rename of the hook functions
and the type, and also fixes an off-by-one-'=' in a header comment which my OCD
couldn't unsee.
Reviewed, overall looks good to me. My question is around the name. It
appears the convention is to do "openssl" on hooks[1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/include/libpq/libpq-be.h;hb=HEAD#l293, with the
convention being a single hook I could find. But scanning the codebase,
it appears we either use "OPENSSL" for definers and "openssl" in
function names.
So, my 2¢ is to use all lowercase to stick with convention.
Thanks!
Jonathan
[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/include/libpq/libpq-be.h;hb=HEAD#l293
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/include/libpq/libpq-be.h;hb=HEAD#l293
Daniel Gustafsson <daniel@yesql.se> writes:
On 16 May 2020, at 03:56, Michael Paquier <michael@paquier.xyz> wrote:
Agreed. PQsslKeyPassHook_<library>_type sounds fine to me as
convention. Wouldn't we want to also rename PQsetSSLKeyPassHook and
PQgetSSLKeyPassHook, appending an "_OpenSSL" to both?
Yes, I think we should. The attached performs the rename of the hook functions
and the type, and also fixes an off-by-one-'=' in a header comment which my OCD
couldn't unsee.
The patch as committed missed renaming PQgetSSLKeyPassHook() itself,
but did rename its result type, which seemed to me to be clearly
wrong. I took it on myself to fix that up, and also to fix exports.txt
which some of the buildfarm insists be correct ;-)
regards, tom lane
On 5/16/20 7:47 PM, Tom Lane wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
On 16 May 2020, at 03:56, Michael Paquier <michael@paquier.xyz> wrote:
Agreed. PQsslKeyPassHook_<library>_type sounds fine to me as
convention. Wouldn't we want to also rename PQsetSSLKeyPassHook and
PQgetSSLKeyPassHook, appending an "_OpenSSL" to both?Yes, I think we should. The attached performs the rename of the hook functions
and the type, and also fixes an off-by-one-'=' in a header comment which my OCD
couldn't unsee.The patch as committed missed renaming PQgetSSLKeyPassHook() itself,
but did rename its result type, which seemed to me to be clearly
wrong. I took it on myself to fix that up, and also to fix exports.txt
which some of the buildfarm insists be correct ;-)
argh! thanks!
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services