Re: PQinitSSL broken in some use casesf - Mailing list pgsql-hackers

Merlin Moncure wrote:
> It is still a bug in the sense that it is impossible to properly
> initialize crypto features in some scenarios.  A doc patch (which I
> argued is the best way to go for 8.4) fails to properly raise the
> seriousness of the issue and also fails to suggest a workaround.
>
> I think a proper way to document this issue would be something like this:
>
> "
> If your application initializes libcrypto, but not libssl, you must
> not call PQinitSSL(1) because it will overwrite your libcrypto
> initialization.  In order to safely use libpq in your application, you
> must include ssl headers and call the following functions:
>
>     #include <openssl/ssl.h>
>     #include <openssl/conf.h>
>
>     OPENSSL_config(NULL);
>     SSL_library_init();
>     SSL_load_error_strings();
>     PQinitSSL(0);
>
> In order to initialize libpq properly for SSL connections.
> "
>
> > I think there is a good argument that PQinitSSL(X) where X > 1 would
> > work fine for more fine-grained control. ?The new libpq init function
> > idea was interesting, but having a documented solution for
> > WSAStartup()/WSACleanup() usage, we now don't have another libpq init
> > use-case so it is hard to suggest a new libpq function.
>
> This feature when discussed at the time was not enough _by itself_ to
> support a PQinit feature (I agree with this reasoning), but surely
> should be considered as valid supporting evidence that a library
> initialization feature is useful.  IOW, the whole of the argument is
> equal to the sum of its parts.   (yes, we have an agenda here: we were
> not happy that our events patch could not establish behavior at
> library initialization time).

Well, we are not the "Make Merlin Happy club".  ;-)

Making usable software requires adjustments on everyone's part.  I don't
think we have enough demand to hardcode those details in our
documentation.

Personally, I feel adding #defines for PQinitSSL is the right approach,
and I think that gives enough checks at compile time, but it doesn't
guard against cases where the application is built against one version
of libpq but is run against an older version, which I think can happen.

My personal opinion is that adding #defines to PQinitSSL() is the right
level of fix, and if we ever implement a libpq init mechanism we can
convert PGinitSSL to a macro. I am attaching a sample patch for
feedback.

However, as I mentioned above, this is not the "Make Bruce Happy club"
either so I need support from other developers that this is the right
fix.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +
Index: doc/src/sgml/libpq.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.280
diff -c -c -r1.280 libpq.sgml
*** doc/src/sgml/libpq.sgml    28 Mar 2009 01:36:11 -0000    1.280
--- doc/src/sgml/libpq.sgml    28 Mar 2009 19:06:45 -0000
***************
*** 6172,6181 ****
     If your application initializes <literal>libssl</> or
     <literal>libcrypto</> libraries and <application>libpq</application>
     is built with <acronym>SSL</> support, you should call
!    <function>PQinitSSL(0)</> to tell <application>libpq</application>
     that the <literal>libssl</> and <literal>libcrypto</> libraries
     have been initialized by your application so
     <application>libpq</application> will not initialize those libraries.
     <!-- If this URL changes replace it with a URL to www.archive.org. -->
     See <ulink
     url="http://h71000.www7.hp.com/doc/83final/BA554_90007/ch04.html"></ulink>
--- 6172,6185 ----
     If your application initializes <literal>libssl</> or
     <literal>libcrypto</> libraries and <application>libpq</application>
     is built with <acronym>SSL</> support, you should call
!    <function>PQinitSSL(PG_NO_INIT_SSL_CRYPTO)</> to tell <application>libpq</application>
     that the <literal>libssl</> and <literal>libcrypto</> libraries
     have been initialized by your application so
     <application>libpq</application> will not initialize those libraries.
+    To have <application>libpq</application> initialize only
+    <literal>libssl</>, use <function>PQinitSSL(PG_INIT_SSL_ONLY)</>, and
+    use <function>PQinitSSL(PG_INIT_CRYPTO_ONLY)</> to initialize only
+    <literal>libcrypto</>.
     <!-- If this URL changes replace it with a URL to www.archive.org. -->
     See <ulink
     url="http://h71000.www7.hp.com/doc/83final/BA554_90007/ch04.html"></ulink>
Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.121
diff -c -c -r1.121 fe-secure.c
*** src/interfaces/libpq/fe-secure.c    28 Mar 2009 18:48:55 -0000    1.121
--- src/interfaces/libpq/fe-secure.c    28 Mar 2009 19:06:45 -0000
***************
*** 99,104 ****
--- 99,105 ----
  static void SSLerrfree(char *buf);

  static bool pq_init_ssl_lib = true;
+ static bool pq_init_crypto_lib = true;
  static SSL_CTX *SSL_context = NULL;

  #ifdef ENABLE_THREAD_SAFETY
***************
*** 173,179 ****
  PQinitSSL(int do_init)
  {
  #ifdef USE_SSL
!     pq_init_ssl_lib = do_init;
  #endif
  }

--- 174,185 ----
  PQinitSSL(int do_init)
  {
  #ifdef USE_SSL
!     if (do_init == PG_NO_INIT_SSL_CRYPTO ||
!         do_init == PG_INIT_CRYPTO_ONLY)
!         pq_init_ssl_lib = false;
!     if (do_init == PG_NO_INIT_SSL_CRYPTO ||
!         do_init == PG_INIT_SSL_ONLY)
!         pq_init_crypto_lib = false;
  #endif
  }

***************
*** 840,846 ****
      if (pthread_mutex_lock(&ssl_config_mutex))
          return -1;

!     if (pq_init_ssl_lib)
      {
          /*
           * If necessary, set up an array to hold locks for OpenSSL. OpenSSL will
--- 846,852 ----
      if (pthread_mutex_lock(&ssl_config_mutex))
          return -1;

!     if (pq_init_crypto_lib)
      {
          /*
           * If necessary, set up an array to hold locks for OpenSSL. OpenSSL will
***************
*** 928,934 ****
      if (pthread_mutex_lock(&ssl_config_mutex))
          return;

!     if (pq_init_ssl_lib)
      {
          if (ssl_open_connections > 0)
              --ssl_open_connections;
--- 934,940 ----
      if (pthread_mutex_lock(&ssl_config_mutex))
          return;

!     if (pq_init_crypto_lib)
      {
          if (ssl_open_connections > 0)
              --ssl_open_connections;
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.145
diff -c -c -r1.145 libpq-fe.h
*** src/interfaces/libpq/libpq-fe.h    1 Jan 2009 17:24:03 -0000    1.145
--- src/interfaces/libpq/libpq-fe.h    28 Mar 2009 19:06:45 -0000
***************
*** 36,41 ****
--- 36,47 ----
  #define PG_COPYRES_EVENTS         0x04
  #define PG_COPYRES_NOTICEHOOKS    0x08

+ /* Option flags for PQinitSSL */
+ #define PG_NO_INIT_SSL_CRYPTO        0    /* only pre-8.4 value supported */
+ #define PG_INIT_SSL_CRYPTO            1    /* default behavior, not documented */
+ #define PG_INIT_SSL_ONLY            2
+ #define PG_INIT_CRYPTO_ONLY            3
+
  /* Application-visible enum types */

  typedef enum