Re: Patch: Don't set LoadedSSL unless secure_initialize succeeds - Mailing list pgsql-hackers

Gurjeet Singh <gurjeet@singh.im> writes:
> On Thu, May 26, 2022 at 12:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> so maybe those comments in libpq-be.h
>> should be moved to their respective functions?  In any case, I'm not
>> excited about having three separate comments covering the same point.

> By 3 locations, I suppose you're referring to the definition of
> secure_initialize(), extern declaration of be_tls_init(), and the
> definition of be_tls_init().

No, I was counting the third comment as the one you proposed to add to
secure_initialize's caller.  I think it's not a great idea to add such
comments to call sites, as they're very likely to not get maintained
when somebody adjusts the API of the function.  (We have a hard enough
time getting people to update the comments directly next to the
function :-(.)

I think what we ought to do here is just move the oddly-placed comments
in libpq-be.h to be adjacent to the function definitions, as attached.
(I just deleted the .h comments for the GSSAPI functions, as they seem
to have adequate comments in their .c file already.)

            regards, tom lane

diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 3d0168a369..85ce9a4ee0 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -85,6 +85,13 @@ static const char *ssl_protocol_version_to_string(int v);
 /*                         Public interface                        */
 /* ------------------------------------------------------------ */

+/*
+ * Initialize global SSL context.
+ *
+ * If isServerStart is true, report any errors as FATAL (so we don't return).
+ * Otherwise, log errors at LOG level and return -1 to indicate trouble,
+ * preserving the old SSL state if any.  Returns 0 if OK.
+ */
 int
 be_tls_init(bool isServerStart)
 {
@@ -397,6 +404,9 @@ error:
     return -1;
 }

+/*
+ * Destroy global SSL context, if any.
+ */
 void
 be_tls_destroy(void)
 {
@@ -406,6 +416,9 @@ be_tls_destroy(void)
     ssl_loaded_verify_locations = false;
 }

+/*
+ * Attempt to negotiate SSL connection.
+ */
 int
 be_tls_open_server(Port *port)
 {
@@ -659,6 +672,9 @@ aloop:
     return 0;
 }

+/*
+ * Close SSL connection.
+ */
 void
 be_tls_close(Port *port)
 {
@@ -689,6 +705,9 @@ be_tls_close(Port *port)
     }
 }

+/*
+ * Read data from a secure connection.
+ */
 ssize_t
 be_tls_read(Port *port, void *ptr, size_t len, int *waitfor)
 {
@@ -748,6 +767,9 @@ be_tls_read(Port *port, void *ptr, size_t len, int *waitfor)
     return n;
 }

+/*
+ * Write data to a secure connection.
+ */
 ssize_t
 be_tls_write(Port *port, void *ptr, size_t len, int *waitfor)
 {
@@ -1249,6 +1271,10 @@ SSLerrmessage(unsigned long ecode)
     return errbuf;
 }

+/*
+ * The following functions return various information about the SSL connection.
+ */
+
 int
 be_tls_get_cipher_bits(Port *port)
 {
@@ -1320,6 +1346,16 @@ be_tls_get_peer_serial(Port *port, char *ptr, size_t len)
         ptr[0] = '\0';
 }

+/*
+ * Get the server certificate hash for SCRAM channel binding type
+ * tls-server-end-point.
+ *
+ * The result is a palloc'd hash of the server certificate with its
+ * size, and NULL if there is no certificate available.
+ *
+ * This is not supported with old versions of OpenSSL that don't have
+ * the X509_get_signature_nid() function.
+ */
 #ifdef HAVE_X509_GET_SIGNATURE_NID
 char *
 be_tls_get_certificate_hash(Port *port, size_t *len)
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 90c20da22b..fccdb6a586 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -246,43 +246,12 @@ fDKQXkYuNs474553LBgOhgObJ4Oi7Aeij7XFXfBvTFLJ3ivL9pVYFxg5lUl86pVq\n\
  * SSL implementation (e.g. be-secure-openssl.c)
  */

-/*
- * Initialize global SSL context.
- *
- * If isServerStart is true, report any errors as FATAL (so we don't return).
- * Otherwise, log errors at LOG level and return -1 to indicate trouble,
- * preserving the old SSL state if any.  Returns 0 if OK.
- */
 extern int    be_tls_init(bool isServerStart);
-
-/*
- * Destroy global SSL context, if any.
- */
 extern void be_tls_destroy(void);
-
-/*
- * Attempt to negotiate SSL connection.
- */
 extern int    be_tls_open_server(Port *port);
-
-/*
- * Close SSL connection.
- */
 extern void be_tls_close(Port *port);
-
-/*
- * Read data from a secure connection.
- */
 extern ssize_t be_tls_read(Port *port, void *ptr, size_t len, int *waitfor);
-
-/*
- * Write data to a secure connection.
- */
 extern ssize_t be_tls_write(Port *port, void *ptr, size_t len, int *waitfor);
-
-/*
- * Return information about the SSL connection.
- */
 extern int    be_tls_get_cipher_bits(Port *port);
 extern const char *be_tls_get_version(Port *port);
 extern const char *be_tls_get_cipher(Port *port);
@@ -290,16 +259,6 @@ extern void be_tls_get_peer_subject_name(Port *port, char *ptr, size_t len);
 extern void be_tls_get_peer_issuer_name(Port *port, char *ptr, size_t len);
 extern void be_tls_get_peer_serial(Port *port, char *ptr, size_t len);

-/*
- * Get the server certificate hash for SCRAM channel binding type
- * tls-server-end-point.
- *
- * The result is a palloc'd hash of the server certificate with its
- * size, and NULL if there is no certificate available.
- *
- * This is not supported with old versions of OpenSSL that don't have
- * the X509_get_signature_nid() function.
- */
 #if defined(USE_OPENSSL) && defined(HAVE_X509_GET_SIGNATURE_NID)
 #define HAVE_BE_TLS_GET_CERTIFICATE_HASH
 extern char *be_tls_get_certificate_hash(Port *port, size_t *len);
@@ -314,16 +273,13 @@ extern PGDLLIMPORT openssl_tls_init_hook_typ openssl_tls_init_hook;
 #endif                            /* USE_SSL */

 #ifdef ENABLE_GSS
-/*
- * Return information about the GSSAPI authenticated connection
- */
+
 extern bool be_gssapi_get_auth(Port *port);
 extern bool be_gssapi_get_enc(Port *port);
 extern const char *be_gssapi_get_princ(Port *port);
-
-/* Read and write to a GSSAPI-encrypted connection. */
 extern ssize_t be_gssapi_read(Port *port, void *ptr, size_t len);
 extern ssize_t be_gssapi_write(Port *port, void *ptr, size_t len);
+
 #endif                            /* ENABLE_GSS */

 extern PGDLLIMPORT ProtocolVersion FrontendProtocol;