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
From 0c97e3ad8c0bf3fa0398e9d6318063b0dd4244eb Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Sat, 16 May 2020 09:09:53 +0200
Subject: [PATCH] Rename OpenSSL specific password hook
---
doc/src/sgml/libpq.sgml | 14 +++++++-------
src/interfaces/libpq/fe-secure-openssl.c | 10 +++++-----
src/interfaces/libpq/fe-secure.c | 8 ++++----
src/interfaces/libpq/libpq-fe.h | 10 +++++-----
4 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 5bc54b2044..ca9aa623a2 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -777,16 +777,16 @@ PGPing PQping(const char *conninfo);
</varlistentry>
<varlistentry id="libpq-pqsetsslkeypasshook">
- <term><function>PQsetSSLKeyPassHook</function><indexterm><primary>PQsetSSLKeyPassHook</primary></indexterm></term>
+ <term><function>PQsetSSLKeyPassHook_OpenSSL</function><indexterm><primary>PQsetSSLKeyPassHook_OpenSSL</primary></indexterm></term>
<listitem>
<para>
- <function>PQsetSSLKeyPassHook</function> lets an application override
+ <function>PQsetSSLKeyPassHook_OpenSSL</function> lets an application override
<literal>libpq</literal>'s <link linkend="libpq-ssl-clientcert">default
handling of encrypted client certificate key files</link> using
<xref linkend="libpq-connect-sslpassword"/> or interactive prompting.
<synopsis>
-void PQsetSSLKeyPassHook(PQsslKeyPassHook_type hook);
+void PQsetSSLKeyPassHook_OpenSSL(PQsslKeyPassHook_OpenSSL_type hook);
</synopsis>
The application passes a pointer to a callback function with signature:
@@ -794,13 +794,13 @@ void PQsetSSLKeyPassHook(PQsslKeyPassHook_type hook);
int callback_fn(char *buf, int size, PGconn *conn);
</programlisting>
which <literal>libpq</literal> will then call <emphasis>instead of</emphasis>
- its default <function>PQdefaultSSLKeyPassHook</function> handler. The callback
+ its default <function>PQdefaultSSLKeyPassHook_OpenSSL</function> handler. The callback
should determine the password for the key and copy it to result-buffer
<literal>buf</literal> of size <literal>size</literal>. The string in <literal>
buf</literal> must be null-terminated. The callback must return the length of
the password stored in <literal>buf</literal> excluding the null terminator.
On failure, the callback should set <literal>buf[0] = '\0'</literal> and return 0.
- See <function>PQdefaultSSLKeyPassHook</function> in <literal>libpq</literal>'s
+ See <function>PQdefaultSSLKeyPassHook_OpenSSL</function> in <literal>libpq</literal>'s
source code for an example.
</para>
@@ -814,7 +814,7 @@ int callback_fn(char *buf, int size, PGconn *conn);
<para>
The app callback may choose to delegate unhandled cases to
- <function>PQdefaultSSLKeyPassHook</function>,
+ <function>PQdefaultSSLKeyPassHook_OpenSSL</function>,
or call it first and try something else if it returns 0, or completely override it.
</para>
@@ -835,7 +835,7 @@ int callback_fn(char *buf, int size, PGconn *conn);
if none has been set.
<synopsis>
-PQsslKeyPassHook_type PQgetSSLKeyPassHook(void);
+PQsslKeyPassHook_OpenSSL_type PQgetSSLKeyPassHook(void);
</synopsis>
</para>
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index df1ac209f9..fb66546b8b 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -95,7 +95,7 @@ static long win32_ssl_create_mutex = 0;
#endif
#endif /* ENABLE_THREAD_SAFETY */
-static PQsslKeyPassHook_type PQsslKeyPassHook = NULL;
+static PQsslKeyPassHook_OpenSSL_type PQsslKeyPassHook = NULL;
static int ssl_protocol_version_to_openssl(const char *protocol);
/* ------------------------------------------------------------ */
@@ -1669,7 +1669,7 @@ err:
* prevent openssl from ever prompting on stdin.
*/
int
-PQdefaultSSLKeyPassHook(char *buf, int size, PGconn *conn)
+PQdefaultSSLKeyPassHook_OpenSSL(char *buf, int size, PGconn *conn)
{
if (conn->sslpassword)
{
@@ -1686,14 +1686,14 @@ PQdefaultSSLKeyPassHook(char *buf, int size, PGconn *conn)
}
}
-PQsslKeyPassHook_type
+PQsslKeyPassHook_OpenSSL_type
PQgetSSLKeyPassHook(void)
{
return PQsslKeyPassHook;
}
void
-PQsetSSLKeyPassHook(PQsslKeyPassHook_type hook)
+PQsetSSLKeyPassHook_OpenSSL(PQsslKeyPassHook_OpenSSL_type hook)
{
PQsslKeyPassHook = hook;
}
@@ -1711,7 +1711,7 @@ PQssl_passwd_cb(char *buf, int size, int rwflag, void *userdata)
if (PQsslKeyPassHook)
return PQsslKeyPassHook(buf, size, conn);
else
- return PQdefaultSSLKeyPassHook(buf, size, conn);
+ return PQdefaultSSLKeyPassHook_OpenSSL(buf, size, conn);
}
/*
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index b455b45e96..3311fd7a5b 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -431,20 +431,20 @@ PQsslAttributeNames(PGconn *conn)
return result;
}
-PQsslKeyPassHook_type
-PQgetSSLKeyPassHook(void)
+PQsslKeyPassHook_OpenSSL_type
+PQgetSSLKeyPassHook_OpenSSL(void)
{
return NULL;
}
void
-PQsetSSLKeyPassHook(PQsslKeyPassHook_type hook)
+PQsetSSLKeyPassHook_OpenSSL(PQsslKeyPassHook_OpenSSL_type hook)
{
return;
}
int
-PQdefaultSSLKeyPassHook(char *buf, int size, PGconn *conn)
+PQdefaultSSLKeyPassHook_OpenSSL(char *buf, int size, PGconn *conn)
{
return 0;
}
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index ea13f5afb8..f104bbfa4a 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -617,13 +617,13 @@ extern int pg_char_to_encoding(const char *name);
extern const char *pg_encoding_to_char(int encoding);
extern int pg_valid_server_encoding_id(int encoding);
-/* == in fe-secure-openssl.c === */
+/* === in fe-secure-openssl.c === */
/* Support for overriding sslpassword handling with a callback. */
-typedef int (*PQsslKeyPassHook_type) (char *buf, int size, PGconn *conn);
-extern PQsslKeyPassHook_type PQgetSSLKeyPassHook(void);
-extern void PQsetSSLKeyPassHook(PQsslKeyPassHook_type hook);
-extern int PQdefaultSSLKeyPassHook(char *buf, int size, PGconn *conn);
+typedef int (*PQsslKeyPassHook_OpenSSL_type) (char *buf, int size, PGconn *conn);
+extern PQsslKeyPassHook_OpenSSL_type PQgetSSLKeyPassHook(void);
+extern void PQsetSSLKeyPassHook_OpenSSL(PQsslKeyPassHook_OpenSSL_type hook);
+extern int PQdefaultSSLKeyPassHook_OpenSSL(char *buf, int size, PGconn *conn);
#ifdef __cplusplus
}
--
2.21.1 (Apple Git-122.3)
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