Making openssl_tls_init_hook OpenSSL specific
Commit 896fcdb230e72 (sorry for chiming in too late, I missed that thread)
added a TLS init hook which is OpenSSL specific: openssl_tls_init_hook. Since
the rest of the TLS support in the backend is library agnostic, we should IMO
make this hook follow that pattern, else this will make a non-OpenSSL backend
not compile.
If we make the hook generic, extension authors must have a way to tell which
backend invoked it, so maybe the best option is to simply wrap this hook in
USE_OPENSSL ifdefs and keep the name/signature? Looking at the Secure
Transport patch I wrote, there is really no equivalent callsite; the same goes
for a libnss patch which I haven't yet submitted.
The attached adds USE_OPENSSL guards.
cheers ./daniel
Attachments:
openssl_hook_guards.patchapplication/octet-stream; name=openssl_hook_guards.patch; x-unix-mode=0644Download
From 7e66928afe35e9d272237ea5c2a9bc97083bf513 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Thu, 16 Apr 2020 14:13:34 +0200
Subject: [PATCH] Wrap OpenSSL specific hook in USE_OPENSSL guards
The openssl_tls_init_hook hook is specific to the OpenSSL backend,
and will cause compilation failures for other backends should they
be implemented. Wrap the hook in USE_OPENSSL guards to keep the
TLS backend library agnostic.
---
src/include/libpq/libpq-be.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 67697836ba..0a65cc17f7 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -287,9 +287,11 @@ extern void be_tls_get_peer_serial(Port *port, char *ptr, size_t len);
extern char *be_tls_get_certificate_hash(Port *port, size_t *len);
#endif
+#if defined(USE_OPENSSL)
/* init hook for SSL, the default sets the password callback if appropriate */
typedef void(* openssl_tls_init_hook_typ)(SSL_CTX *context, bool isServerStart);
extern PGDLLIMPORT openssl_tls_init_hook_typ openssl_tls_init_hook;
+#endif
#endif /* USE_SSL */
--
2.21.1 (Apple Git-122.3)
On Thu, Apr 16, 2020 at 02:17:33PM +0200, Daniel Gustafsson wrote:
Commit 896fcdb230e72 (sorry for chiming in too late, I missed that thread)
added a TLS init hook which is OpenSSL specific: openssl_tls_init_hook. Since
the rest of the TLS support in the backend is library agnostic, we should IMO
make this hook follow that pattern, else this will make a non-OpenSSL backend
not compile.
Better sooner than later, thanks for the report.
If we make the hook generic, extension authors must have a way to tell which
backend invoked it, so maybe the best option is to simply wrap this hook in
USE_OPENSSL ifdefs and keep the name/signature? Looking at the Secure
Transport patch I wrote, there is really no equivalent callsite; the same goes
for a libnss patch which I haven't yet submitted.The attached adds USE_OPENSSL guards.
I agree that this looks like an oversight of the original commit
introducing the hook as it gets called in the OpenSSL code path of
be_tls_init(), so I think that your patch is right (though I would
have just used #ifdef USE_OPENSSL here). And if the future proves
that this hook has more uses for other SSL implementations, we could
always rework it at this point, if necessary. Andrew, would you
prefer fixing that yourself?
--
Michael
On 4/16/20 9:57 PM, Michael Paquier wrote:
On Thu, Apr 16, 2020 at 02:17:33PM +0200, Daniel Gustafsson wrote:
Commit 896fcdb230e72 (sorry for chiming in too late, I missed that thread)
added a TLS init hook which is OpenSSL specific: openssl_tls_init_hook. Since
the rest of the TLS support in the backend is library agnostic, we should IMO
make this hook follow that pattern, else this will make a non-OpenSSL backend
not compile.Better sooner than later, thanks for the report.
If we make the hook generic, extension authors must have a way to tell which
backend invoked it, so maybe the best option is to simply wrap this hook in
USE_OPENSSL ifdefs and keep the name/signature? Looking at the Secure
Transport patch I wrote, there is really no equivalent callsite; the same goes
for a libnss patch which I haven't yet submitted.The attached adds USE_OPENSSL guards.
I agree that this looks like an oversight of the original commit
introducing the hook as it gets called in the OpenSSL code path of
be_tls_init(), so I think that your patch is right (though I would
have just used #ifdef USE_OPENSSL here). And if the future proves
that this hook has more uses for other SSL implementations, we could
always rework it at this point, if necessary. Andrew, would you
prefer fixing that yourself?
Sure, I'll do it.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services