Thread: 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
Attachment
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
Attachment
> 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
Attachment
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], 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
Attachment
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