Thread: Re: PQinitSSL broken in some use casesf
On Tue, Feb 10, 2009 at 5:02 PM, Bruce Momjian <bruce@momjian.us> wrote: > Merlin Moncure wrote: >> > PQinitSSL(0) was specifically designed to allow applications to set up >> > SSL on their own. How does this not work properly? >> >> this has nothing to do with who initializes ssl. this is all about >> *crypto*. remember, crypto and ssl are two separate libraries. The >> application or library in question may not even link with ssl or use >> ssl headers. >> >> The problem is PQinitSSL (re-) initializes crypto without asking if that's ok. > > PQinitSSL(false) initializes crypto? Please point me to exact function > calls that are the problem? Everything is very vague. nooo, you are not listening :-) PQinitSSL(0) initializes libpq for ssl but leaves crypto and ssl initialization to the app PQinitSSL(1) initializes libpq, crypto, and ssl libraries Now, consider an app that uses libcrypto for its own requirements *but not libssl*. It initializes libcrypto, passing its own lock vector, etc. It cannot however initialize ssl because it does not link with ssl, or include ssl headers. There are no ssl functions to call, and it wouldn't make sense to expect the app to do this even if there were. Now, if this app also has libpq dependency, it needs a way to tell libpq: 'i have already initialized the crypto library, but could you please set up libssl'. otherwise you end up re-initializing libcrypto with different lock vector which is very bad if there are any locks already in use, which is quite likely. There is no way to do that with libpq....so you see that no matter how you call PQinitSSL, the application is broken in some way. Passing 0 breaks because ssl never ends up getting set up, and passing 1 breaks because libcrypto's locks get messed up. The main problem is that libpq PQinitSSL makes broad (and extremely dangerous assumption) that it is the only one interested in libcrypto lock vector. In short, it's broken. merlin
> On Tue, Feb 10, 2009 at 5:02 PM, Bruce Momjian <bruce@momjian.us> wrote: >> PQinitSSL(false) initializes crypto? Please point me to exact function >> calls that are the problem? Everything is very vague. File: src/interfaces/libpq/fe-secure.c Func: init_ssl_system Line: 823 Starting at around line 853, this function prepares a lock array for CRYPTO_set_locking_callback. This function is not part of libssl, its part of libcrypto. It also calls CRYPTO_set_id_callback. The rest of that function appears to only make libssl calls. There should be an "if (pq_initcryptolib)" around those libcrypto calls, serving the same purpose as the pq_initssllib variable. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote:
>
> > On Tue, Feb 10, 2009 at 5:02 PM, Bruce Momjian <bruce@momjian.us> wrote:
> >> PQinitSSL(false) initializes crypto? Please point me to exact function
> >> calls that are the problem? Everything is very vague.
>
> File: src/interfaces/libpq/fe-secure.c
> Func: init_ssl_system
> Line: 823
>
> Starting at around line 853, this function prepares a lock array for
> CRYPTO_set_locking_callback. This function is not part of libssl, its
> part of libcrypto. It also calls CRYPTO_set_id_callback. The rest of
> that function appears to only make libssl calls.
>
> There should be an "if (pq_initcryptolib)" around those libcrypto calls,
> serving the same purpose as the pq_initssllib variable.
Why not just call PQinitSSL(true) and do everything in your
application?; from the libpq manual:
If you are using <acronym>SSL</> inside your application (in addition to inside <application>libpq</application>),
youcan use <function>PQinitSSL(int)</> to tell <application>libpq</application> that the <acronym>SSL</> library has
alreadybeen initialized by your application.
Actually, that wording doesn't say what the parameter means so I updated
the documentation:
If you are using <acronym>SSL</> inside your application (in addition
! to inside <application>libpq</application>), you can call
! <function>PQinitSSL(int)</> with <literal>0</> to tell
! <application>libpq</application> that the <acronym>SSL</> library
! has already been initialized by your application.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Andrew Chernow wrote: > > > > > On Tue, Feb 10, 2009 at 5:02 PM, Bruce Momjian <bruce@momjian.us> wrote: > > >> PQinitSSL(false) initializes crypto? Please point me to exact function > > >> calls that are the problem? Everything is very vague. > > > > File: src/interfaces/libpq/fe-secure.c > > Func: init_ssl_system > > Line: 823 > > > > Starting at around line 853, this function prepares a lock array for > > CRYPTO_set_locking_callback. This function is not part of libssl, its > > part of libcrypto. It also calls CRYPTO_set_id_callback. The rest of > > that function appears to only make libssl calls. > > > > There should be an "if (pq_initcryptolib)" around those libcrypto calls, > > serving the same purpose as the pq_initssllib variable. > > Why not just call PQinitSSL(true) and do everything in your Sorry, meant 'false'. Also, I read Merlin's comments and understand he doesn't want his application to have to set up SSL. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Tue, Feb 10, 2009 at 11:09 PM, Bruce Momjian <bruce@momjian.us> wrote: > Why not just call PQinitSSL(true) and do everything in your > application?; from the libpq manual: > > If you are using <acronym>SSL</> inside your application (in addition > to inside <application>libpq</application>), you can use > <function>PQinitSSL(int)</> to tell <application>libpq</application> > that the <acronym>SSL</> library has already been initialized by your > application. I think this question has been answered about four times on this thread already. I thought the OP's explanation was pretty clear: http://archives.postgresql.org/pgsql-hackers/2009-01/msg02488.php It's also re-explained here, here, and here: http://archives.postgresql.org/message-id/b42b73150902100713mdbfd64ah706ced5170897a59@mail.gmail.com http://archives.postgresql.org/message-id/603c8f070902100849n44034028p5423f18e6e1b9317@mail.gmail.com http://archives.postgresql.org/message-id/b42b73150902101420m6c263f7ayafc10090af841e15@mail.gmail.com The point is that there are three sane things you might want/need PQinitSSL() to do, and we allow two of them. I think the request for a behavior change is totally reasonable, but I think Andrew or Merlin should be the ones to write the patch (and then others can review). I am not in love with the idea of using PQinitSSL(SOME_MAGIC_VALUE) for this purpose, but haven't read the code enough to determine the least-ugly API. ...Robert
Robert Haas wrote: > I am not in love with the idea of using PQinitSSL(SOME_MAGIC_VALUE) > The issue I see is the inability to toggle crypto initialization. I think crypto init and ssl init are 2 different things. Thus, I proposed the below: http://archives.postgresql.org/pgsql-hackers/2009-02/msg00488.php Andrew Chernow eSilo, LLC -- every bit count
On Feb 11, 2009, at 12:12 AM, Andrew Chernow <ac@esilo.com> wrote: > Robert Haas wrote: >> I am not in love with the idea of using PQinitSSL(SOME_MAGIC_VALUE) > > The issue I see is the inability to toggle crypto initialization. > I think crypto init and ssl init are 2 different things. Thus, I > proposed the below: > > http://archives.postgresql.org/pgsql-hackers/2009-02/msg00488.php If that can be made transparent to existing apps, I'm all for it. ...Robert
Robert Haas wrote: > On Feb 11, 2009, at 12:12 AM, Andrew Chernow <ac@esilo.com> wrote: > >> Robert Haas wrote: >>> I am not in love with the idea of using PQinitSSL(SOME_MAGIC_VALUE) >> >> The issue I see is the inability to toggle crypto initialization. I >> think crypto init and ssl init are 2 different things. Thus, I >> proposed the below: >> >> http://archives.postgresql.org/pgsql-hackers/2009-02/msg00488.php > > If that can be made transparent to existing apps, I'm all for it. > Defaulting the crypto initialization to TRUE should make this transparent. The downside is that this breaks backwards compatibility, since an unknown symbol is being referenced. Although, that only occurs when PQinitCrypto is actually used. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Robert Haas wrote: > On Feb 11, 2009, at 12:12 AM, Andrew Chernow <ac@esilo.com> wrote: > >> Robert Haas wrote: >>> I am not in love with the idea of using PQinitSSL(SOME_MAGIC_VALUE) >> >> The issue I see is the inability to toggle crypto initialization. I >> think crypto init and ssl init are 2 different things. Thus, I >> proposed the below: >> >> http://archives.postgresql.org/pgsql-hackers/2009-02/msg00488.php > > If that can be made transparent to existing apps, I'm all for it. > > ...Robert > Should I create a patch implementing the PQinitCrypto idea? -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
On Fri, Feb 13, 2009 at 9:17 AM, Andrew Chernow <ac@esilo.com> wrote: > Should I create a patch implementing the PQinitCrypto idea? I think that would be helpful. Seeing the code will give everyone a better idea of exactly what the proposed change is and whether it's acceptable. ...Robert
Robert Haas wrote: > On Fri, Feb 13, 2009 at 9:17 AM, Andrew Chernow <ac@esilo.com> wrote: >> Should I create a patch implementing the PQinitCrypto idea? > > I think that would be helpful. Seeing the code will give everyone a > better idea of exactly what the proposed change is and whether it's > acceptable. > > ...Robert > > Patch attached. One thing I noticed is the ssl_open_connections variable is ref counting connections when pq_initssllib is true. But, it now only affects crypto library init and cleanup calls. Point is, ref counting is only needed if pq_initcryptolib is true and it should be renamed to crypto_open_connections. I didn't do this in the patch. Its the same old name and the counter is incremented if pq_initssllib or pq_initcryptolib is true. Please advise. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: src/interfaces/libpq/exports.txt =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v retrieving revision 1.22 diff -C6 -r1.22 exports.txt *** src/interfaces/libpq/exports.txt 22 Sep 2008 13:55:14 -0000 1.22 --- src/interfaces/libpq/exports.txt 13 Feb 2009 17:01:22 -0000 *************** *** 149,154 **** --- 149,155 ---- PQinstanceData 147 PQsetInstanceData 148 PQresultInstanceData 149 PQresultSetInstanceData 150 PQfireResultCreateEvents 151 PQconninfoParse 152 + PQinitCrypto 153 \ No newline at end of file Index: src/interfaces/libpq/fe-secure.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-secure.c,v retrieving revision 1.119 diff -C6 -r1.119 fe-secure.c *** src/interfaces/libpq/fe-secure.c 28 Jan 2009 15:06:47 -0000 1.119 --- src/interfaces/libpq/fe-secure.c 13 Feb 2009 17:01:22 -0000 *************** *** 96,107 **** --- 96,108 ---- static PostgresPollingStatusType open_client_SSL(PGconn *); static void close_SSL(PGconn *); static char *SSLerrmessage(void); static void SSLerrfree(char *buf); static bool pq_initssllib = true; + static bool pq_initcryptolib = true; static SSL_CTX *SSL_context = NULL; #ifdef ENABLE_THREAD_SAFETY static int ssl_open_connections = 0; #ifndef WIN32 *************** *** 175,186 **** --- 176,199 ---- #ifdef USE_SSL pq_initssllib = do_init; #endif } /* + * Exported function to allow application to tell us it's already + * initialized the OpenSSL Crypto library. + */ + void + PQinitCrypto(int do_init) + { + #ifdef USE_SSL + pq_initcryptolib = do_init; + #endif + } + + /* * Initialize global context */ int pqsecure_initialize(PGconn *conn) { int r = 0; *************** *** 820,831 **** --- 833,846 ---- * message - no connection local setup is made. */ static int init_ssl_system(PGconn *conn) { #ifdef ENABLE_THREAD_SAFETY + int num_ssl_conns = 0; + #ifdef WIN32 /* Also see similar code in fe-connect.c, default_threadlock() */ if (ssl_config_mutex == NULL) { while (InterlockedExchange(&win32_ssl_create_mutex, 1) == 1) /* loop, another thread own the lock */ ; *************** *** 837,849 **** InterlockedExchange(&win32_ssl_create_mutex, 0); } #endif if (pthread_mutex_lock(&ssl_config_mutex)) return -1; ! if (pq_initssllib) { /* * If necessary, set up an array to hold locks for OpenSSL. OpenSSL will * tell us how big to make this array. */ if (pq_lockarray == NULL) --- 852,873 ---- InterlockedExchange(&win32_ssl_create_mutex, 0); } #endif if (pthread_mutex_lock(&ssl_config_mutex)) return -1; ! /* ! * Increment connection count if we are responsible for ! * intiializing the SSL or Crypto library. Currently, only ! * crypto needs this during setup and cleanup. That may ! * change in the future so we always update the counter. ! */ ! if (pq_initssllib || pq_initcryptolib) ! num_ssl_conns = ssl_open_connections++; ! ! if (pq_initcryptolib) { /* * If necessary, set up an array to hold locks for OpenSSL. OpenSSL will * tell us how big to make this array. */ if (pq_lockarray == NULL) *************** *** 865,877 **** pthread_mutex_unlock(&ssl_config_mutex); return -1; } } } ! if (ssl_open_connections++ == 0) { /* These are only required for threaded SSL applications */ CRYPTO_set_id_callback(pq_threadidcallback); CRYPTO_set_locking_callback(pq_lockingcallback); } } --- 889,901 ---- pthread_mutex_unlock(&ssl_config_mutex); return -1; } } } ! if (num_ssl_conns == 0) { /* These are only required for threaded SSL applications */ CRYPTO_set_id_callback(pq_threadidcallback); CRYPTO_set_locking_callback(pq_lockingcallback); } } *************** *** 924,955 **** { #ifdef ENABLE_THREAD_SAFETY /* Mutex is created in initialize_ssl_system() */ if (pthread_mutex_lock(&ssl_config_mutex)) return; ! if (pq_initssllib) { ! if (ssl_open_connections > 0) ! --ssl_open_connections; ! if (ssl_open_connections == 0) ! { ! /* No connections left, unregister all callbacks */ ! CRYPTO_set_locking_callback(NULL); ! CRYPTO_set_id_callback(NULL); ! ! /* ! * We don't free the lock array. If we get another connection ! * from the same caller, we will just re-use it with the existing ! * mutexes. ! * ! * This means we leak a little memory on repeated load/unload ! * of the library. ! */ ! } } pthread_mutex_unlock(&ssl_config_mutex); #endif return; } --- 948,976 ---- { #ifdef ENABLE_THREAD_SAFETY /* Mutex is created in initialize_ssl_system() */ if (pthread_mutex_lock(&ssl_config_mutex)) return; ! if ((pq_initssllib || pq_initcryptolib) && ssl_open_connections > 0) ! --ssl_open_connections; ! ! if (pq_initcryptolib && ssl_open_connections == 0) { ! /* No connections left, unregister all callbacks */ ! CRYPTO_set_locking_callback(NULL); ! CRYPTO_set_id_callback(NULL); ! /* ! * We don't free the lock array. If we get another connection ! * from the same caller, we will just re-use it with the existing ! * mutexes. ! * ! * This means we leak a little memory on repeated load/unload ! * of the library. ! */ } pthread_mutex_unlock(&ssl_config_mutex); #endif return; } Index: src/interfaces/libpq/libpq-fe.h =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v retrieving revision 1.145 diff -C6 -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 13 Feb 2009 17:01:22 -0000 *************** *** 299,310 **** --- 299,313 ---- * unencrypted connections or if any other TLS library is in use. */ extern void *PQgetssl(PGconn *conn); /* Tell libpq whether it needs to initialize OpenSSL */ extern void PQinitSSL(int do_init); + /* Tell libpq whether it needs to initialize the OpenSSL Crypto library. */ + extern void PQinitCrypto(int do_init) + /* Set verbosity for PQerrorMessage and PQresultErrorMessage */ extern PGVerbosity PQsetErrorVerbosity(PGconn *conn, PGVerbosity verbosity); /* Enable/disable tracing */ extern void PQtrace(PGconn *conn, FILE *debug_port); extern void PQuntrace(PGconn *conn);
On Fri, Feb 13, 2009 at 12:06 PM, Andrew Chernow <ac@esilo.com> wrote: > Patch attached. > > One thing I noticed is the ssl_open_connections variable is ref counting > connections when pq_initssllib is true. But, it now only affects crypto > library init and cleanup calls. Point is, ref counting is only needed if > pq_initcryptolib is true and it should be renamed to > crypto_open_connections. I didn't do this in the patch. Its the same old > name and the counter is incremented if pq_initssllib or pq_initcryptolib is > true. Please advise. I'll review this in more detail when I have a chance, but it certainly won't be committable without doc changes, and it's probably best if you write those and include them in the patch. ...Robert
Robert Haas wrote: > On Fri, Feb 13, 2009 at 12:06 PM, Andrew Chernow <ac@esilo.com> wrote: >> Patch attached. >> >> One thing I noticed is the ssl_open_connections variable is ref counting >> connections when pq_initssllib is true. But, it now only affects crypto >> library init and cleanup calls. Point is, ref counting is only needed if >> pq_initcryptolib is true and it should be renamed to >> crypto_open_connections. I didn't do this in the patch. Its the same old >> name and the counter is incremented if pq_initssllib or pq_initcryptolib is >> true. Please advise. > > I'll review this in more detail when I have a chance, but it certainly > won't be committable without doc changes, and it's probably best if > you write those and include them in the patch. > > ...Robert > > Okay. Added docs in the same place PQinitSSL is. Apparently, initssl doesn't have formal func docs. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: doc/src/sgml/libpq.sgml =================================================================== RCS file: /projects/cvsroot/pgsql/doc/src/sgml/libpq.sgml,v retrieving revision 1.278 diff -C6 -r1.278 libpq.sgml *** doc/src/sgml/libpq.sgml 11 Feb 2009 04:08:47 -0000 1.278 --- doc/src/sgml/libpq.sgml 13 Feb 2009 18:11:40 -0000 *************** *** 60,72 **** <function>PQsetdbLogin</>. Note that these functions will always return a non-null object pointer, unless perhaps there is too little memory even to allocate the <structname>PGconn</> object. The <function>PQstatus</> function should be called to check whether a connection was successfully made before queries are sent via the connection object. ! <note> <para> On Windows, there is a way to improve performance if a single database connection is repeatedly started and shutdown. Internally, libpq calls WSAStartup() and WSACleanup() for connection startup and shutdown, respectively. WSAStartup() increments an internal --- 60,72 ---- <function>PQsetdbLogin</>. Note that these functions will always return a non-null object pointer, unless perhaps there is too little memory even to allocate the <structname>PGconn</> object. The <function>PQstatus</> function should be called to check whether a connection was successfully made before queries are sent via the connection object. ! <note> <para> On Windows, there is a way to improve performance if a single database connection is repeatedly started and shutdown. Internally, libpq calls WSAStartup() and WSACleanup() for connection startup and shutdown, respectively. WSAStartup() increments an internal *************** *** 6169,6181 **** <para> If you are using <acronym>SSL</> inside your application (in addition to inside <application>libpq</application>), you can call <function>PQinitSSL(int)</> with <literal>0</> to tell <application>libpq</application> that the <acronym>SSL</> library ! has already been initialized by your application. <!-- 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> for details on the SSL API. </para> --- 6169,6184 ---- <para> If you are using <acronym>SSL</> inside your application (in addition to inside <application>libpq</application>), you can call <function>PQinitSSL(int)</> with <literal>0</> to tell <application>libpq</application> that the <acronym>SSL</> library ! has already been initialized by your application. Additionally, you ! can call <function>PQinitCrypto(int)</> with <literal>0</> to tell ! <application>libpq</application> that the <acronym>Crypto</> library has ! already been initialized by your application. <!-- 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> for details on the SSL API. </para> Index: src/interfaces/libpq/exports.txt =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v retrieving revision 1.22 diff -C6 -r1.22 exports.txt *** src/interfaces/libpq/exports.txt 22 Sep 2008 13:55:14 -0000 1.22 --- src/interfaces/libpq/exports.txt 13 Feb 2009 18:11:40 -0000 *************** *** 149,154 **** --- 149,155 ---- PQinstanceData 147 PQsetInstanceData 148 PQresultInstanceData 149 PQresultSetInstanceData 150 PQfireResultCreateEvents 151 PQconninfoParse 152 + PQinitCrypto 153 \ No newline at end of file Index: src/interfaces/libpq/fe-secure.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-secure.c,v retrieving revision 1.119 diff -C6 -r1.119 fe-secure.c *** src/interfaces/libpq/fe-secure.c 28 Jan 2009 15:06:47 -0000 1.119 --- src/interfaces/libpq/fe-secure.c 13 Feb 2009 18:11:40 -0000 *************** *** 96,107 **** --- 96,108 ---- static PostgresPollingStatusType open_client_SSL(PGconn *); static void close_SSL(PGconn *); static char *SSLerrmessage(void); static void SSLerrfree(char *buf); static bool pq_initssllib = true; + static bool pq_initcryptolib = true; static SSL_CTX *SSL_context = NULL; #ifdef ENABLE_THREAD_SAFETY static int ssl_open_connections = 0; #ifndef WIN32 *************** *** 175,186 **** --- 176,199 ---- #ifdef USE_SSL pq_initssllib = do_init; #endif } /* + * Exported function to allow application to tell us it's already + * initialized the OpenSSL Crypto library. + */ + void + PQinitCrypto(int do_init) + { + #ifdef USE_SSL + pq_initcryptolib = do_init; + #endif + } + + /* * Initialize global context */ int pqsecure_initialize(PGconn *conn) { int r = 0; *************** *** 820,831 **** --- 833,846 ---- * message - no connection local setup is made. */ static int init_ssl_system(PGconn *conn) { #ifdef ENABLE_THREAD_SAFETY + int num_ssl_conns = 0; + #ifdef WIN32 /* Also see similar code in fe-connect.c, default_threadlock() */ if (ssl_config_mutex == NULL) { while (InterlockedExchange(&win32_ssl_create_mutex, 1) == 1) /* loop, another thread own the lock */ ; *************** *** 837,849 **** InterlockedExchange(&win32_ssl_create_mutex, 0); } #endif if (pthread_mutex_lock(&ssl_config_mutex)) return -1; ! if (pq_initssllib) { /* * If necessary, set up an array to hold locks for OpenSSL. OpenSSL will * tell us how big to make this array. */ if (pq_lockarray == NULL) --- 852,873 ---- InterlockedExchange(&win32_ssl_create_mutex, 0); } #endif if (pthread_mutex_lock(&ssl_config_mutex)) return -1; ! /* ! * Increment connection count if we are responsible for ! * intiializing the SSL or Crypto library. Currently, only ! * crypto needs this during setup and cleanup. That may ! * change in the future so we always update the counter. ! */ ! if (pq_initssllib || pq_initcryptolib) ! num_ssl_conns = ssl_open_connections++; ! ! if (pq_initcryptolib) { /* * If necessary, set up an array to hold locks for OpenSSL. OpenSSL will * tell us how big to make this array. */ if (pq_lockarray == NULL) *************** *** 865,877 **** pthread_mutex_unlock(&ssl_config_mutex); return -1; } } } ! if (ssl_open_connections++ == 0) { /* These are only required for threaded SSL applications */ CRYPTO_set_id_callback(pq_threadidcallback); CRYPTO_set_locking_callback(pq_lockingcallback); } } --- 889,901 ---- pthread_mutex_unlock(&ssl_config_mutex); return -1; } } } ! if (num_ssl_conns == 0) { /* These are only required for threaded SSL applications */ CRYPTO_set_id_callback(pq_threadidcallback); CRYPTO_set_locking_callback(pq_lockingcallback); } } *************** *** 924,955 **** { #ifdef ENABLE_THREAD_SAFETY /* Mutex is created in initialize_ssl_system() */ if (pthread_mutex_lock(&ssl_config_mutex)) return; ! if (pq_initssllib) { ! if (ssl_open_connections > 0) ! --ssl_open_connections; ! if (ssl_open_connections == 0) ! { ! /* No connections left, unregister all callbacks */ ! CRYPTO_set_locking_callback(NULL); ! CRYPTO_set_id_callback(NULL); ! ! /* ! * We don't free the lock array. If we get another connection ! * from the same caller, we will just re-use it with the existing ! * mutexes. ! * ! * This means we leak a little memory on repeated load/unload ! * of the library. ! */ ! } } pthread_mutex_unlock(&ssl_config_mutex); #endif return; } --- 948,976 ---- { #ifdef ENABLE_THREAD_SAFETY /* Mutex is created in initialize_ssl_system() */ if (pthread_mutex_lock(&ssl_config_mutex)) return; ! if ((pq_initssllib || pq_initcryptolib) && ssl_open_connections > 0) ! --ssl_open_connections; ! ! if (pq_initcryptolib && ssl_open_connections == 0) { ! /* No connections left, unregister all callbacks */ ! CRYPTO_set_locking_callback(NULL); ! CRYPTO_set_id_callback(NULL); ! /* ! * We don't free the lock array. If we get another connection ! * from the same caller, we will just re-use it with the existing ! * mutexes. ! * ! * This means we leak a little memory on repeated load/unload ! * of the library. ! */ } pthread_mutex_unlock(&ssl_config_mutex); #endif return; } Index: src/interfaces/libpq/libpq-fe.h =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v retrieving revision 1.145 diff -C6 -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 13 Feb 2009 18:11:40 -0000 *************** *** 299,310 **** --- 299,313 ---- * unencrypted connections or if any other TLS library is in use. */ extern void *PQgetssl(PGconn *conn); /* Tell libpq whether it needs to initialize OpenSSL */ extern void PQinitSSL(int do_init); + /* Tell libpq whether it needs to initialize the OpenSSL Crypto library. */ + extern void PQinitCrypto(int do_init) + /* Set verbosity for PQerrorMessage and PQresultErrorMessage */ extern PGVerbosity PQsetErrorVerbosity(PGconn *conn, PGVerbosity verbosity); /* Enable/disable tracing */ extern void PQtrace(PGconn *conn, FILE *debug_port); extern void PQuntrace(PGconn *conn);
Andrew Chernow wrote: > Robert Haas wrote: >> On Fri, Feb 13, 2009 at 12:06 PM, Andrew Chernow <ac@esilo.com> wrote: >>> Patch attached. >>> >>> One thing I noticed is the ssl_open_connections variable is ref counting >>> connections when pq_initssllib is true. But, it now only affects crypto >>> library init and cleanup calls. Point is, ref counting is only >>> needed if >>> pq_initcryptolib is true and it should be renamed to >>> crypto_open_connections. I didn't do this in the patch. Its the >>> same old >>> name and the counter is incremented if pq_initssllib or >>> pq_initcryptolib is >>> true. Please advise. >> >> I'll review this in more detail when I have a chance, but it certainly >> won't be committable without doc changes, and it's probably best if >> you write those and include them in the patch. >> One problem with this patch is that a libpq app using PQinitSSL(0) is under the assumption that this shuts off ssl init and crypto init. That app might be doing its own crypto init which would be overwritten by libpq because the app is unaware of PQinitCrypto (if and when it eventually links with 8.4 libpq). This feels like a very uncommon situation, but a possible gotcha. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote:
> Andrew Chernow wrote:
>> Robert Haas wrote:
>>> On Fri, Feb 13, 2009 at 12:06 PM, Andrew Chernow <ac@esilo.com> wrote:
>>>> Patch attached.
>>>>
>>>> One thing I noticed is the ssl_open_connections variable is ref
>>>> counting
>>>> connections when pq_initssllib is true. But, it now only affects
>>>> crypto
>>>> library init and cleanup calls. Point is, ref counting is only
>>>> needed if
>>>> pq_initcryptolib is true and it should be renamed to
>>>> crypto_open_connections. I didn't do this in the patch. Its the
>>>> same old
>>>> name and the counter is incremented if pq_initssllib or
>>>> pq_initcryptolib is
>>>> true. Please advise.
>>>
>>> I'll review this in more detail when I have a chance, but it certainly
>>> won't be committable without doc changes, and it's probably best if
>>> you write those and include them in the patch.
>>>
>
> One problem with this patch is that a libpq app using PQinitSSL(0) is
> under the assumption that this shuts off ssl init and crypto init. That
> app might be doing its own crypto init which would be overwritten by
> libpq because the app is unaware of PQinitCrypto (if and when it
> eventually links with 8.4 libpq). This feels like a very uncommon
> situation, but a possible gotcha.
>
(sorry I keep posting)
>This feels like a very uncommon situation
I take that back. Not so sure it is uncommon, any threaded libpq app
would probably get bit if they called PQinitSSL. On top of that, it
could take up to a year before complaints start rolling in, as 8.4 hits
the distros. Yuck.
I now think the the orignal suggestion of PQinitSSLExtended is better
than PQinitCrypto. With PQinitSSLExtended, PQinitSSL needs a minor
implementation adjustment but the behvior remains the same. The
extended version is probably:
/* IMHO appending "Ex" is a little nicer */
void PQinitSSLEx(int ssl_init, int crypto_init);
/* PQinitSSL wraps PQinitSSLEx */
void PQinitSSL(int do_init)
{ PQinitSSLEx(do_init, do_init);
}
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
Andrew Chernow <ac@esilo.com> writes:
>> One problem with this patch is that a libpq app using PQinitSSL(0) is
>> under the assumption that this shuts off ssl init and crypto init. That
>> app might be doing its own crypto init which would be overwritten by
>> libpq because the app is unaware of PQinitCrypto (if and when it
>> eventually links with 8.4 libpq). This feels like a very uncommon
>> situation, but a possible gotcha.
> I take that back. Not so sure it is uncommon,
I agree, we should *not* change the existing behavior for either case.
This probably means that an independent PQinitCrypto function is the
wrong thing, even though it would've been the cleanest solution if
we were starting from scratch.
At this point I like Merlin's proposal of a third parameter value to
PQinitSSL the best.
> /* IMHO appending "Ex" is a little nicer */
> void PQinitSSLEx(int ssl_init, int crypto_init);
Ugh, ugh, ugh. We do not do "Ex" around here --- uninterpretable
abbreviations are not helpful to the reader. Call it Extended
if you must have it.
Also, this definition feels a bit wrong --- it's not possible for
all four cases to be valid, is it?
regards, tom lane
> At this point I like Merlin's proposal of a third parameter value to > PQinitSSL the best. I'm not opposed to it, although I don't think it is as clean as a new function. > > Also, this definition feels a bit wrong --- it's not possible for > all four cases to be valid, is it? > Yes it is. PQinitSSLExtended(0, 0); // don't init anything, PQinitSSL(0) PQinitSSLExtended(1, 0); // init ssl, don't init crypto PQinitSSLExtended(0, 1); // don't init ssl, init crypto PQinitSSLExtended(1, 1); // init both, default behavior, PQinitSSL(1) -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: >> At this point I like Merlin's proposal of a third parameter value to >> PQinitSSL the best. > > I'm not opposed to it, although I don't think it is as clean as a new > function. > >> >> Also, this definition feels a bit wrong --- it's not possible for >> all four cases to be valid, is it? >> > > Yes it is. > > PQinitSSLExtended(0, 0); // don't init anything, PQinitSSL(0) > PQinitSSLExtended(1, 0); // init ssl, don't init crypto > PQinitSSLExtended(0, 1); // don't init ssl, init crypto > PQinitSSLExtended(1, 1); // init both, default behavior, PQinitSSL(1) > Maybe the argument to PQinitSSLExtended should be a bit mask, making this version more extendable ... PG_INITSSL, PG_INITCRYPTO? Also, how about calling this PQinitSecure(int flags), since SSL is only one thing it can init. This is just like merlin's suggestion but without hacking the existing PQinitSSL. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow <ac@esilo.com> writes:
>> Also, this definition feels a bit wrong --- it's not possible for
>> all four cases to be valid, is it?
> Yes it is.
> PQinitSSLExtended(0, 0); // don't init anything, PQinitSSL(0)
> PQinitSSLExtended(1, 0); // init ssl, don't init crypto
> PQinitSSLExtended(0, 1); // don't init ssl, init crypto
> PQinitSSLExtended(1, 1); // init both, default behavior, PQinitSSL(1)
I know what you're thinking the flags should mean, I'm saying that it's
not possible for the third case to be sane. It implies that the
application initialized ssl but not crypto, which isn't possible.
regards, tom lane
Tom Lane wrote: > Andrew Chernow <ac@esilo.com> writes: >>> Also, this definition feels a bit wrong --- it's not possible for >>> all four cases to be valid, is it? > >> Yes it is. > >> PQinitSSLExtended(0, 0); // don't init anything, PQinitSSL(0) >> PQinitSSLExtended(1, 0); // init ssl, don't init crypto >> PQinitSSLExtended(0, 1); // don't init ssl, init crypto >> PQinitSSLExtended(1, 1); // init both, default behavior, PQinitSSL(1) > > I know what you're thinking the flags should mean, I'm saying that it's > not possible for the third case to be sane. It implies that the > application initialized ssl but not crypto, which isn't possible. > Or that the application called PQinitSSLExtended(0, 1) and then initialized SSL itself, which is sane. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow <ac@esilo.com> writes:
> Maybe the argument to PQinitSSLExtended should be a bit mask, making
> this version more extendable ... PG_INITSSL, PG_INITCRYPTO?
+1 for thinking ahead to the next time, but is a bit mask the right thing?
regards, tom lane
On Fri, Feb 13, 2009 at 3:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andrew Chernow <ac@esilo.com> writes: >> Maybe the argument to PQinitSSLExtended should be a bit mask, making >> this version more extendable ... PG_INITSSL, PG_INITCRYPTO? > > +1 for thinking ahead to the next time, but is a bit mask the right thing? What would you suggest? struct pointer? varargs? If you are down with the struct pointer idea, maybe we could revive the idea of PQinit: http://archives.postgresql.org/pgsql-hackers/2009-01/msg01349.php We could fix up the windows issue at the same time, and leave room for other things. Fixing windows WSA issues or installing an event proc hook into the library :D. merlin
Merlin Moncure <mmoncure@gmail.com> writes:
> On Fri, Feb 13, 2009 at 3:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> +1 for thinking ahead to the next time, but is a bit mask the right thing?
> What would you suggest? struct pointer? varargs?
Not sure. By definition, we're trying to predict an unforeseen
requirement, and that's always going to be tough.
I'm not too thrilled about a struct pointer, because that will introduce
the problem "which version of the struct is the client passing?".
A varargs arrangement could cover almost anything, but it also seems
like overkill.
I don't actually have an idea that sounds better than a bitmask,
I just wanted to see if anyone else did.
BTW, the bitmask isn't perfect either --- doesn't it just reintroduce
the problem already complained of with your idea for PQinitSSL? That
is, how does the client know whether the function recognized all the
bits it passed?
regards, tom lane
Tom Lane wrote:
> is, how does the client know whether the function recognized all the
> bits it passed?
How about returning the bits it could set?
int mask = PG_INITSSL | PG_INITCRYPTO;
if (!(PQinitSecure(mask) & PG_INITCRYPTO)) ; // no support for crypto
...OR...
consider a generic PQinit call per system/object/etc..
int PQinit(int which, void *data);
int mask = PG_SECURE_SSL | PG_SECURE_CRYPTO;
PQinit(PG_INIT_SECURE, &mask); // or PG_INIT_OPENSSL
xxx_t xxx = {0, "blah", 12};
PQinit(PG_INIT_xxx, &xxx);
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
> BTW, the bitmask isn't perfect either --- doesn't it just reintroduce > the problem already complained of with your idea for PQinitSSL? That > is, how does the client know whether the function recognized all the > bits it passed? Well, if we add the PQgetLibraryVersion() function I suggested upthread, then it can check that first. I find it difficult to believe that isn't a good idea independently of how we solve this particular problem. ...Robert
On Fri, Feb 13, 2009 at 4:53 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> BTW, the bitmask isn't perfect either --- doesn't it just reintroduce >> the problem already complained of with your idea for PQinitSSL? That >> is, how does the client know whether the function recognized all the >> bits it passed? > > Well, if we add the PQgetLibraryVersion() function I suggested > upthread, then it can check that first. I find it difficult to > believe that isn't a good idea independently of how we solve this > particular problem. I'd prefer PQversion() as a name. Also, it doesn't necessarily handle the issue directly. For example, it doesn't tell you which bits are valid...you have to guess. Also, do we really need a function for this? Is the generic init worth discussion or a non starter? I guess we have a scheduling problem here...I think the ssl problem is serious enough to warrant a fast-track into 8.4, but maybe it's 'too much' to hash out a generic library initialization routine this late in the cycle. merlin
Robert Haas <robertmhaas@gmail.com> writes: >> BTW, the bitmask isn't perfect either --- doesn't it just reintroduce >> the problem already complained of with your idea for PQinitSSL? That >> is, how does the client know whether the function recognized all the >> bits it passed? > Well, if we add the PQgetLibraryVersion() function I suggested > upthread, then it can check that first. I find it difficult to > believe that isn't a good idea independently of how we solve this > particular problem. Personally, I still subscribe to the PNG design theory that conditionalizing code on overall version numbers isn't a very good idea. Version information should be tied as tightly as possible to the particular behavior for which it applies. http://www.w3.org/TR/PNG-Rationale.html#R.Chunk-naming-conventions It's even worse when the available version numbers aren't really "overall", which would be the case here. If we had such a function you can bet your bottom dollar that people would test it to distinguish behaviors that are really server-side, and then get burnt in the field when their app gets used with mismatched server and libpq versions. For the problem at hand, if we think it's important for an app to be able to tell whether libpq supports a particular initialization behavior, then adding a new function to provide that behavior seems like the best bet to me. So maybe that says that designing for extensibility in the function signature is misguided anyhow ... regards, tom lane
> Personally, I still subscribe to the PNG design theory that > conditionalizing code on overall version numbers isn't a very good idea. > Version information should be tied as tightly as possible to the > particular behavior for which it applies. > http://www.w3.org/TR/PNG-Rationale.html#R.Chunk-naming-conventions I don't disagree with you, but it's not always easy to separate things that cleanly. pg_dump doesn't try to distinguish server capabilities, just server versions (and I'm sure you'd scream bloody murder if someone submitted a patch to refactor it... because that patch would be hideous). > It's even worse when the available version numbers aren't really > "overall", which would be the case here. If we had such a function > you can bet your bottom dollar that people would test it to distinguish > behaviors that are really server-side, and then get burnt in the field > when their app gets used with mismatched server and libpq versions. I think that could be largely addressed through documentation. There is no helpful for terminal stupidity anyway. The reality is, right now we have a way to distinguish server versions, but not client versions. That doesn't seem particularly logical or consistent. Testing for client versions lets you do things like check for breakage known to exist in versions < X, where there may be no difference in the API, just the behavior. It also reduces the need to make every non-backward-compatible change an API change. Many people will only ever link against the most recent version and will not care about backward-compatibility. It doesn't hurt to provide a hook to those who do care. > For the problem at hand, if we think it's important for an app to be > able to tell whether libpq supports a particular initialization > behavior, then adding a new function to provide that behavior seems > like the best bet to me. > > So maybe that says that designing for extensibility in the function > signature is misguided anyhow ... It could certainly be overdone. ...Robert
So far, we have the below ideas: 1. void PQinitSSL(MAGIC_VALUES) The idea is to use some magic values to add more options, converting the do_init argument from a flag to an enumeration. The idea is choose magic values that would most likely never be used by existing libpq applications. The downside to this method is there is no way to detect if the request was honored: meaning a new app passes a magic value to an older libpq. 2. void PQinitSecure(int init_ssl, int int_crypto) Another proposed name was PQinitSSLExtended or PQinitOpenSSL. This leaves the existing PQinitSSL alone. The problem here is that it can't be extended like the current PQinitSSL, so we fixed today's problem but created it again for future (sounds like government). That leads us to #3. 3. void PQinitSecure(int init_mask) Works just like #2 but uses a bit mask, like PG_INITSSL or something, instead of a fixed set of flags. It trys to make the function more extendable. Although, we bang into the #1 problem again - new apps using new bits that older libpqs are not aware of. There is no way to know if the request was honored. This leads us to #4 (#5 was also a suggested fix). 4. int PQinitSecure(int init_mask) This works just like #3 but returns the bits it understood. It allows an app to determine if the loaded libpq supports a bit it has tried to use. I have not heard any comments on this, I am still looking for its faults. You could claim its akward, but I've seen much worse. An alternative is to add a second argument, int *support_mask, instead of returning it. Or, make the single argument an input and output value .. (int *mask). 5. ??? PQgetLibraryVersion(???) Another proposed name was PQversion. This was suggested as a way of fixing the faults of #1 and #3. The idea is to use the libpq version to determine if a particular init bit is supported. 6. PQinit(int init_who, void *data) Revived from a recent WSACleanup thread. Appears to always be the last kid picked for the kick ball team, so I won't dive to deep. The idea is to create a single init function for libpq, that can handle init'n any module/feature/runtime-settings/etc.. Another name could be PQcontrol. I think that's everything. Are there any favorites or combos? Personally, I'm not really stuck on any of them. I'd just like to see the feature added. If I had to choose, it would be #4. A close second would be #1 + #5 (using PQversion as the name). -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow <ac@esilo.com> writes:
> 4. int PQinitSecure(int init_mask)
> This works just like #3 but returns the bits it understood. It allows an app to
> determine if the loaded libpq supports a bit it has tried to use.
Maybe better, have it return a zero/nonzero error code; where one of the
possibilities for failure is "you passed a bit I didn't understand".
This would depend on whether we intend that the function would ever
actually *do* anything, as opposed to just save its argument to be
consulted later. Maybe that's not appropriate/necessary.
regards, tom lane
> Maybe better, have it return a zero/nonzero error code; where one of the> possibilities for failure is "you passed a bitI didn't understand". Why not just return those bit(s) instead of an arbitrary code? How about: -1 = error (if it ever does anything that can fail) 0 = success (all bits known)>0 = unknown bits (remaining known bits *have*been set) -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: > > > Maybe better, have it return a zero/nonzero error code; where one of the > > possibilities for failure is "you passed a bit I didn't understand". > > Why not just return those bit(s) instead of an arbitrary code? How about: > > -1 = error (if it ever does anything that can fail) > 0 = success (all bits known) > >0 = unknown bits (remaining known bits *have* been set) > I attached a patch that implements the above, using PQinitSecure as the function name. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: doc/src/sgml/libpq.sgml =================================================================== RCS file: /projects/cvsroot/pgsql/doc/src/sgml/libpq.sgml,v retrieving revision 1.278 diff -C6 -r1.278 libpq.sgml *** doc/src/sgml/libpq.sgml 11 Feb 2009 04:08:47 -0000 1.278 --- doc/src/sgml/libpq.sgml 18 Feb 2009 15:22:04 -0000 *************** *** 6174,6185 **** --- 6174,6215 ---- <application>libpq</application> that the <acronym>SSL</> library has already been initialized by your application. <!-- 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> for details on the SSL API. + + <variablelist> + <varlistentry> + <term> + <function>PQinitSecure</function> + <indexterm> + <primary>PQinitSecure</primary> + </indexterm> + </term> + + <listitem> + <para> + Allows applications to select which secure components to initialize. + <synopsis> + int PQinitSecure(int flags); + </synopsis> + </para> + + <para> + The flags argument can be any of the following: PG_SECURE_SSL, + PG_SECURE_CRYPTO. PG_SECURE_SSL will initialize the SSL portion of + the OpenSSL library. PG_SECURE_CRYPTO will initialize the crypto + portion of the OpenSSL library. The function returns the bits it + did not understand or zero indicating it understood all bits in flags. + If an error occurs, such as calling this function without SSL + support enabled, -1 is returned. + </para> + </listitem> + </varlistentry> + </variablelist> </para> <table id="libpq-ssl-file-usage"> <title>Libpq/Client SSL File Usage</title> <tgroup cols="3"> <thead> Index: src/interfaces/libpq/exports.txt =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v retrieving revision 1.22 diff -C6 -r1.22 exports.txt *** src/interfaces/libpq/exports.txt 22 Sep 2008 13:55:14 -0000 1.22 --- src/interfaces/libpq/exports.txt 18 Feb 2009 15:22:04 -0000 *************** *** 149,154 **** --- 149,155 ---- PQinstanceData 147 PQsetInstanceData 148 PQresultInstanceData 149 PQresultSetInstanceData 150 PQfireResultCreateEvents 151 PQconninfoParse 152 + PQinitSecure 153 \ No newline at end of file Index: src/interfaces/libpq/fe-secure.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-secure.c,v retrieving revision 1.119 diff -C6 -r1.119 fe-secure.c *** src/interfaces/libpq/fe-secure.c 28 Jan 2009 15:06:47 -0000 1.119 --- src/interfaces/libpq/fe-secure.c 18 Feb 2009 15:22:04 -0000 *************** *** 96,107 **** --- 96,108 ---- static PostgresPollingStatusType open_client_SSL(PGconn *); static void close_SSL(PGconn *); static char *SSLerrmessage(void); static void SSLerrfree(char *buf); static bool pq_initssllib = true; + static bool pq_initcryptolib = true; static SSL_CTX *SSL_context = NULL; #ifdef ENABLE_THREAD_SAFETY static int ssl_open_connections = 0; #ifndef WIN32 *************** *** 175,186 **** --- 176,205 ---- #ifdef USE_SSL pq_initssllib = do_init; #endif } /* + * Exported function to allow application to tell us which secure + * components to initialize. + */ + int + PQinitSecure(int flags) + { + int code = -1; + + #ifdef USE_SSL + pq_initssllib = flags & PG_SECURE_SSL ? true : false; + pq_initcryptolib = flags & PG_SECURE_CRYPTO ? true : false;; + code = flags & ~(PG_SECURE_SSL | PG_SECURE_CRYPTO); + #endif + + return code; + } + + /* * Initialize global context */ int pqsecure_initialize(PGconn *conn) { int r = 0; *************** *** 820,831 **** --- 839,852 ---- * message - no connection local setup is made. */ static int init_ssl_system(PGconn *conn) { #ifdef ENABLE_THREAD_SAFETY + int num_ssl_conns = 0; + #ifdef WIN32 /* Also see similar code in fe-connect.c, default_threadlock() */ if (ssl_config_mutex == NULL) { while (InterlockedExchange(&win32_ssl_create_mutex, 1) == 1) /* loop, another thread own the lock */ ; *************** *** 837,849 **** InterlockedExchange(&win32_ssl_create_mutex, 0); } #endif if (pthread_mutex_lock(&ssl_config_mutex)) return -1; ! if (pq_initssllib) { /* * If necessary, set up an array to hold locks for OpenSSL. OpenSSL will * tell us how big to make this array. */ if (pq_lockarray == NULL) --- 858,879 ---- InterlockedExchange(&win32_ssl_create_mutex, 0); } #endif if (pthread_mutex_lock(&ssl_config_mutex)) return -1; ! /* ! * Increment connection count if we are responsible for ! * intiializing the SSL or Crypto library. Currently, only ! * crypto needs this during setup and cleanup. That may ! * change in the future so we always update the counter. ! */ ! if (pq_initssllib || pq_initcryptolib) ! num_ssl_conns = ssl_open_connections++; ! ! if (pq_initcryptolib) { /* * If necessary, set up an array to hold locks for OpenSSL. OpenSSL will * tell us how big to make this array. */ if (pq_lockarray == NULL) *************** *** 865,877 **** pthread_mutex_unlock(&ssl_config_mutex); return -1; } } } ! if (ssl_open_connections++ == 0) { /* These are only required for threaded SSL applications */ CRYPTO_set_id_callback(pq_threadidcallback); CRYPTO_set_locking_callback(pq_lockingcallback); } } --- 895,907 ---- pthread_mutex_unlock(&ssl_config_mutex); return -1; } } } ! if (num_ssl_conns == 0) { /* These are only required for threaded SSL applications */ CRYPTO_set_id_callback(pq_threadidcallback); CRYPTO_set_locking_callback(pq_lockingcallback); } } *************** *** 924,955 **** { #ifdef ENABLE_THREAD_SAFETY /* Mutex is created in initialize_ssl_system() */ if (pthread_mutex_lock(&ssl_config_mutex)) return; ! if (pq_initssllib) { ! if (ssl_open_connections > 0) ! --ssl_open_connections; ! if (ssl_open_connections == 0) ! { ! /* No connections left, unregister all callbacks */ ! CRYPTO_set_locking_callback(NULL); ! CRYPTO_set_id_callback(NULL); ! ! /* ! * We don't free the lock array. If we get another connection ! * from the same caller, we will just re-use it with the existing ! * mutexes. ! * ! * This means we leak a little memory on repeated load/unload ! * of the library. ! */ ! } } pthread_mutex_unlock(&ssl_config_mutex); #endif return; } --- 954,982 ---- { #ifdef ENABLE_THREAD_SAFETY /* Mutex is created in initialize_ssl_system() */ if (pthread_mutex_lock(&ssl_config_mutex)) return; ! if ((pq_initssllib || pq_initcryptolib) && ssl_open_connections > 0) ! --ssl_open_connections; ! ! if (pq_initcryptolib && ssl_open_connections == 0) { ! /* No connections left, unregister all callbacks */ ! CRYPTO_set_locking_callback(NULL); ! CRYPTO_set_id_callback(NULL); ! /* ! * We don't free the lock array. If we get another connection ! * from the same caller, we will just re-use it with the existing ! * mutexes. ! * ! * This means we leak a little memory on repeated load/unload ! * of the library. ! */ } pthread_mutex_unlock(&ssl_config_mutex); #endif return; } Index: src/interfaces/libpq/libpq-fe.h =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v retrieving revision 1.145 diff -C6 -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 18 Feb 2009 15:22:04 -0000 *************** *** 33,44 **** --- 33,50 ---- */ #define PG_COPYRES_ATTRS 0x01 #define PG_COPYRES_TUPLES 0x02 /* Implies PG_COPYRES_ATTRS */ #define PG_COPYRES_EVENTS 0x04 #define PG_COPYRES_NOTICEHOOKS 0x08 + /* + * Flags for PQinitSecure + */ + #define PG_SECURE_SSL 0x01 + #define PG_SECURE_CRYPTO 0x02 + /* Application-visible enum types */ typedef enum { /* * Although it is okay to add to this list, values which become unused *************** *** 299,310 **** --- 305,319 ---- * unencrypted connections or if any other TLS library is in use. */ extern void *PQgetssl(PGconn *conn); /* Tell libpq whether it needs to initialize OpenSSL */ extern void PQinitSSL(int do_init); + /* Tell libpq which secure components to initialize. */ + extern int PQinitSecure(int flags); + /* Set verbosity for PQerrorMessage and PQresultErrorMessage */ extern PGVerbosity PQsetErrorVerbosity(PGconn *conn, PGVerbosity verbosity); /* Enable/disable tracing */ extern void PQtrace(PGconn *conn, FILE *debug_port); extern void PQuntrace(PGconn *conn);
Andrew Chernow wrote: > Andrew Chernow wrote: >> >> > Maybe better, have it return a zero/nonzero error code; where one >> of the >> > possibilities for failure is "you passed a bit I didn't understand". >> >> Why not just return those bit(s) instead of an arbitrary code? How >> about: >> >> -1 = error (if it ever does anything that can fail) >> 0 = success (all bits known) >> >0 = unknown bits (remaining known bits *have* been set) >> > > I attached a patch that implements the above, using PQinitSecure as the > function name. > > Fixed a bug in the patch. I forgot to make PQinitSSL update the new pg_initcryptolib flag. My fix was to make PQinitSSL proxy to PQinitSecure. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: doc/src/sgml/libpq.sgml =================================================================== RCS file: /projects/cvsroot/pgsql/doc/src/sgml/libpq.sgml,v retrieving revision 1.278 diff -C6 -r1.278 libpq.sgml *** doc/src/sgml/libpq.sgml 11 Feb 2009 04:08:47 -0000 1.278 --- doc/src/sgml/libpq.sgml 19 Feb 2009 16:32:11 -0000 *************** *** 6174,6185 **** --- 6174,6215 ---- <application>libpq</application> that the <acronym>SSL</> library has already been initialized by your application. <!-- 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> for details on the SSL API. + + <variablelist> + <varlistentry> + <term> + <function>PQinitSecure</function> + <indexterm> + <primary>PQinitSecure</primary> + </indexterm> + </term> + + <listitem> + <para> + Allows applications to select which secure components to initialize. + <synopsis> + int PQinitSecure(int flags); + </synopsis> + </para> + + <para> + The flags argument can be any of the following: PG_SECURE_SSL, + PG_SECURE_CRYPTO. PG_SECURE_SSL will initialize the SSL portion of + the OpenSSL library. PG_SECURE_CRYPTO will initialize the crypto + portion of the OpenSSL library. The function returns the bits it + did not understand or zero indicating it understood all bits in flags. + If an error occurs, such as calling this function without SSL + support enabled, -1 is returned. + </para> + </listitem> + </varlistentry> + </variablelist> </para> <table id="libpq-ssl-file-usage"> <title>Libpq/Client SSL File Usage</title> <tgroup cols="3"> <thead> Index: src/interfaces/libpq/exports.txt =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v retrieving revision 1.22 diff -C6 -r1.22 exports.txt *** src/interfaces/libpq/exports.txt 22 Sep 2008 13:55:14 -0000 1.22 --- src/interfaces/libpq/exports.txt 19 Feb 2009 16:32:11 -0000 *************** *** 149,154 **** --- 149,155 ---- PQinstanceData 147 PQsetInstanceData 148 PQresultInstanceData 149 PQresultSetInstanceData 150 PQfireResultCreateEvents 151 PQconninfoParse 152 + PQinitSecure 153 \ No newline at end of file Index: src/interfaces/libpq/fe-secure.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-secure.c,v retrieving revision 1.119 diff -C6 -r1.119 fe-secure.c *** src/interfaces/libpq/fe-secure.c 28 Jan 2009 15:06:47 -0000 1.119 --- src/interfaces/libpq/fe-secure.c 19 Feb 2009 16:32:11 -0000 *************** *** 96,107 **** --- 96,108 ---- static PostgresPollingStatusType open_client_SSL(PGconn *); static void close_SSL(PGconn *); static char *SSLerrmessage(void); static void SSLerrfree(char *buf); static bool pq_initssllib = true; + static bool pq_initcryptolib = true; static SSL_CTX *SSL_context = NULL; #ifdef ENABLE_THREAD_SAFETY static int ssl_open_connections = 0; #ifndef WIN32 *************** *** 170,186 **** * initialized OpenSSL. */ void PQinitSSL(int do_init) { #ifdef USE_SSL ! pq_initssllib = do_init; #endif } /* * Initialize global context */ int pqsecure_initialize(PGconn *conn) { int r = 0; --- 171,205 ---- * initialized OpenSSL. */ void PQinitSSL(int do_init) { #ifdef USE_SSL ! (void) PQinitSecure(do_init ? (PG_SECURE_SSL | PG_SECURE_CRYPTO) : 0); #endif } /* + * Exported function to allow application to tell us which secure + * components to initialize. + */ + int + PQinitSecure(int flags) + { + int code = -1; + + #ifdef USE_SSL + pq_initssllib = flags & PG_SECURE_SSL ? true : false; + pq_initcryptolib = flags & PG_SECURE_CRYPTO ? true : false; + code = flags & ~(PG_SECURE_SSL | PG_SECURE_CRYPTO); + #endif + + return code; + } + + /* * Initialize global context */ int pqsecure_initialize(PGconn *conn) { int r = 0; *************** *** 820,831 **** --- 839,852 ---- * message - no connection local setup is made. */ static int init_ssl_system(PGconn *conn) { #ifdef ENABLE_THREAD_SAFETY + int num_ssl_conns = 0; + #ifdef WIN32 /* Also see similar code in fe-connect.c, default_threadlock() */ if (ssl_config_mutex == NULL) { while (InterlockedExchange(&win32_ssl_create_mutex, 1) == 1) /* loop, another thread own the lock */ ; *************** *** 837,849 **** InterlockedExchange(&win32_ssl_create_mutex, 0); } #endif if (pthread_mutex_lock(&ssl_config_mutex)) return -1; ! if (pq_initssllib) { /* * If necessary, set up an array to hold locks for OpenSSL. OpenSSL will * tell us how big to make this array. */ if (pq_lockarray == NULL) --- 858,879 ---- InterlockedExchange(&win32_ssl_create_mutex, 0); } #endif if (pthread_mutex_lock(&ssl_config_mutex)) return -1; ! /* ! * Increment connection count if we are responsible for ! * intiializing the SSL or Crypto library. Currently, only ! * crypto needs this during setup and cleanup. That may ! * change in the future so we always update the counter. ! */ ! if (pq_initssllib || pq_initcryptolib) ! num_ssl_conns = ssl_open_connections++; ! ! if (pq_initcryptolib) { /* * If necessary, set up an array to hold locks for OpenSSL. OpenSSL will * tell us how big to make this array. */ if (pq_lockarray == NULL) *************** *** 865,877 **** pthread_mutex_unlock(&ssl_config_mutex); return -1; } } } ! if (ssl_open_connections++ == 0) { /* These are only required for threaded SSL applications */ CRYPTO_set_id_callback(pq_threadidcallback); CRYPTO_set_locking_callback(pq_lockingcallback); } } --- 895,907 ---- pthread_mutex_unlock(&ssl_config_mutex); return -1; } } } ! if (num_ssl_conns == 0) { /* These are only required for threaded SSL applications */ CRYPTO_set_id_callback(pq_threadidcallback); CRYPTO_set_locking_callback(pq_lockingcallback); } } *************** *** 924,955 **** { #ifdef ENABLE_THREAD_SAFETY /* Mutex is created in initialize_ssl_system() */ if (pthread_mutex_lock(&ssl_config_mutex)) return; ! if (pq_initssllib) { ! if (ssl_open_connections > 0) ! --ssl_open_connections; ! if (ssl_open_connections == 0) ! { ! /* No connections left, unregister all callbacks */ ! CRYPTO_set_locking_callback(NULL); ! CRYPTO_set_id_callback(NULL); ! ! /* ! * We don't free the lock array. If we get another connection ! * from the same caller, we will just re-use it with the existing ! * mutexes. ! * ! * This means we leak a little memory on repeated load/unload ! * of the library. ! */ ! } } pthread_mutex_unlock(&ssl_config_mutex); #endif return; } --- 954,982 ---- { #ifdef ENABLE_THREAD_SAFETY /* Mutex is created in initialize_ssl_system() */ if (pthread_mutex_lock(&ssl_config_mutex)) return; ! if ((pq_initssllib || pq_initcryptolib) && ssl_open_connections > 0) ! --ssl_open_connections; ! ! if (pq_initcryptolib && ssl_open_connections == 0) { ! /* No connections left, unregister all callbacks */ ! CRYPTO_set_locking_callback(NULL); ! CRYPTO_set_id_callback(NULL); ! /* ! * We don't free the lock array. If we get another connection ! * from the same caller, we will just re-use it with the existing ! * mutexes. ! * ! * This means we leak a little memory on repeated load/unload ! * of the library. ! */ } pthread_mutex_unlock(&ssl_config_mutex); #endif return; } Index: src/interfaces/libpq/libpq-fe.h =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v retrieving revision 1.145 diff -C6 -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 19 Feb 2009 16:32:11 -0000 *************** *** 33,44 **** --- 33,50 ---- */ #define PG_COPYRES_ATTRS 0x01 #define PG_COPYRES_TUPLES 0x02 /* Implies PG_COPYRES_ATTRS */ #define PG_COPYRES_EVENTS 0x04 #define PG_COPYRES_NOTICEHOOKS 0x08 + /* + * Flags for PQinitSecure + */ + #define PG_SECURE_SSL 0x01 + #define PG_SECURE_CRYPTO 0x02 + /* Application-visible enum types */ typedef enum { /* * Although it is okay to add to this list, values which become unused *************** *** 299,310 **** --- 305,319 ---- * unencrypted connections or if any other TLS library is in use. */ extern void *PQgetssl(PGconn *conn); /* Tell libpq whether it needs to initialize OpenSSL */ extern void PQinitSSL(int do_init); + /* Tell libpq which secure components to initialize. */ + extern int PQinitSecure(int flags); + /* Set verbosity for PQerrorMessage and PQresultErrorMessage */ extern PGVerbosity PQsetErrorVerbosity(PGconn *conn, PGVerbosity verbosity); /* Enable/disable tracing */ extern void PQtrace(PGconn *conn, FILE *debug_port); extern void PQuntrace(PGconn *conn);
Merlin Moncure wrote:
> On Tue, Feb 10, 2009 at 5:02 PM, Bruce Momjian <bruce@momjian.us> wrote:
> > Merlin Moncure wrote:
> >> > PQinitSSL(0) was specifically designed to allow applications to set up
> >> > SSL on their own. How does this not work properly?
> >>
> >> this has nothing to do with who initializes ssl. this is all about
> >> *crypto*. remember, crypto and ssl are two separate libraries. The
> >> application or library in question may not even link with ssl or use
> >> ssl headers.
> >>
> >> The problem is PQinitSSL (re-) initializes crypto without asking if that's ok.
> >
> > PQinitSSL(false) initializes crypto? Please point me to exact function
> > calls that are the problem? Everything is very vague.
>
> nooo, you are not listening :-)
>
> PQinitSSL(0) initializes libpq for ssl but leaves crypto and ssl
> initialization to the app
> PQinitSSL(1) initializes libpq, crypto, and ssl libraries
>
> Now, consider an app that uses libcrypto for its own requirements *but
> not libssl*. It initializes libcrypto, passing its own lock vector,
> etc. It cannot however initialize ssl because it does not link with
> ssl, or include ssl headers. There are no ssl functions to call, and
> it wouldn't make sense to expect the app to do this even if there
> were.
>
> Now, if this app also has libpq dependency, it needs a way to tell
> libpq: 'i have already initialized the crypto library, but could you
> please set up libssl'. otherwise you end up re-initializing libcrypto
> with different lock vector which is very bad if there are any locks
> already in use, which is quite likely.
>
> There is no way to do that with libpq....so you see that no matter how
> you call PQinitSSL, the application is broken in some way. Passing 0
> breaks because ssl never ends up getting set up, and passing 1 breaks
> because libcrypto's locks get messed up.
>
> The main problem is that libpq PQinitSSL makes broad (and extremely
> dangerous assumption) that it is the only one interested in libcrypto
> lock vector. In short, it's broken.
I am back to looking at this. I dropped off this discussion back in
February because I felt people didn't want to answer questions I had,
but now it seems we have to close this out somehow.
I have applied the attached patch which does several things:
o documents that libssl _and_ libcrypto initialization is
turned off by PQinitSSL(0)
o clarified cases where this behavior is important
o added comments that the CRYPTO_set_* calls reference
libcrypto, not libssl
I think we can now say that the current behavior is not a bug because it
is documented, even though the PQinitSSL() function name is inaccurate.
In fact, 8.4 is the first time we are documenting the valid parameter
value to PQinitSSL(), in 8.3 we have:
to inside <application>libpq</application>), you can use
<function>PQinitSSL(int)</> to tell <application>libpq</application>
that the <acronym>SSL</> library has already been initialized by your
application.
So we have some flexibility in defining how it behaves, and this also
illustrates how little the function is used because no one ever
complained about it.
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.
I am figuring we have to keep the current behavior and see what happens
after 8.4; the new documentation should make the behavior clear and
perhaps trigger other users to report suggestions.
I assume this item is closed for 8.4 unless I hear otherwise.
--
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.279
diff -c -c -r1.279 libpq.sgml
*** doc/src/sgml/libpq.sgml 23 Mar 2009 01:45:29 -0000 1.279
--- doc/src/sgml/libpq.sgml 28 Mar 2009 01:17:21 -0000
***************
*** 6169,6179 ****
</para>
<para>
! If you are using <acronym>SSL</> inside your application (in addition
! to inside <application>libpq</application>), you can call
! <function>PQinitSSL(int)</> with <literal>0</> to tell
! <application>libpq</application> that the <acronym>SSL</> library
! has already been initialized by your application.
<!-- 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>
--- 6169,6181 ----
</para>
<para>
! 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>
Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.119
diff -c -c -r1.119 fe-secure.c
*** src/interfaces/libpq/fe-secure.c 28 Jan 2009 15:06:47 -0000 1.119
--- src/interfaces/libpq/fe-secure.c 28 Mar 2009 01:17:22 -0000
***************
*** 870,875 ****
--- 870,876 ----
if (ssl_open_connections++ == 0)
{
+ /* This is actually libcrypto, not libssl. */
/* These are only required for threaded SSL applications */
CRYPTO_set_id_callback(pq_threadidcallback);
CRYPTO_set_locking_callback(pq_lockingcallback);
***************
*** 934,939 ****
--- 935,941 ----
if (ssl_open_connections == 0)
{
+ /* This is actually libcrypto, not libssl. */
/* No connections left, unregister all callbacks */
CRYPTO_set_locking_callback(NULL);
CRYPTO_set_id_callback(NULL);
Bruce Momjian wrote: > 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. If you look back through the list, the PQinit idea was offered up several times while discussing WSA* stuff. There were few buyers. I don't see how having or not having a documented solution for WSA* usage would make a bit of difference. > > I am figuring we have to keep the current behavior and see what happens > after 8.4; the new documentation should make the behavior clear and > perhaps trigger other users to report suggestions. > > This is not a battle I find worth fighting. But I am having trouble staying completely quiet; I typically have this issue when I disagree :) This patch merely documents the problem, when anotherfully documented working patch "fixed" it; following the discussions on the list. http://archives.postgresql.org//pgsql-hackers/2009-02/msg01018.php Was this reviewed and/or rejected? Andrew Chernow
Andrew Chernow wrote: > Bruce Momjian wrote: > > > 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. > > If you look back through the list, the PQinit idea was offered up > several times while discussing WSA* stuff. There were few buyers. I > don't see how having or not having a documented solution for WSA* usage > would make a bit of difference. It only means we don't have _another_ use for a more general libpq init function. > > I am figuring we have to keep the current behavior and see what happens > > after 8.4; the new documentation should make the behavior clear and > > perhaps trigger other users to report suggestions. > > > > > > This is not a battle I find worth fighting. But I am having trouble > staying completely quiet; I typically have this issue when I disagree :) > This patch merely documents the problem, when another fully documented > working patch "fixed" it; following the discussions on the list. > > http://archives.postgresql.org//pgsql-hackers/2009-02/msg01018.php > > Was this reviewed and/or rejected? Comments Tom made were that there was no consensus on the proper fix/direction, and I agree. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Fri, Mar 27, 2009 at 9:38 PM, Bruce Momjian <bruce@momjian.us> wrote: > I have applied the attached patch which does several things: > > o documents that libssl _and_ libcrypto initialization is > turned off by PQinitSSL(0) > o clarified cases where this behavior is important > o added comments that the CRYPTO_set_* calls reference > libcrypto, not libssl > > I think we can now say that the current behavior is not a bug because it > is documented, even though the PQinitSSL() function name is inaccurate. 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). merlin
On Sat, Mar 28, 2009 at 9:23 AM, Merlin Moncure <mmoncure@gmail.com> 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 Meant to say: 'your doc patch" merlin
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
Bruce Momjian <bruce@momjian.us> writes:
> Well, we are not the "Make Merlin Happy club". ;-)
Merlin and Andrew were the ones complaining initially. If they feel
that a proposed patch doesn't fix the problem, then I'd say that it
isn't fixing the problem.
> 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.
This is just a rehash of one of the patches that was discussed earlier.
There wasn't consensus for it then, and there's not now.
regards, tom lane
Tom Lane wrote: > This is just a rehash of one of the patches that was discussed earlier. > There wasn't consensus for it then, and there's not now. > I am personally out of ideas. It feels like this issue has been beaten to death. There are only a few ways to do it and I believe they have all been put on the table. Any suggestions? -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: > Tom Lane wrote: > > This is just a rehash of one of the patches that was discussed earlier. > > There wasn't consensus for it then, and there's not now. > > > > I am personally out of ideas. It feels like this issue has been beaten > to death. There are only a few ways to do it and I believe they have > all been put on the table. Any suggestions? Well, at least the current behavior is documented now. I personally thought the #define was the best minimal fix because it only added a few lines to the docs and the C code. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Sat, Mar 28, 2009 at 4:26 PM, Bruce Momjian <bruce@momjian.us> wrote: > Andrew Chernow wrote: >> Tom Lane wrote: >> > This is just a rehash of one of the patches that was discussed earlier. >> > There wasn't consensus for it then, and there's not now. >> > >> >> I am personally out of ideas. It feels like this issue has been beaten >> to death. There are only a few ways to do it and I believe they have >> all been put on the table. Any suggestions? > > Well, at least the current behavior is documented now. You mean, with your proposed documentation patch? (if you meant my proposed advice, then ignore the following paragraph) Not only does it not suggest the problem, it actually misinforms you...it suggests you call PQinitSSL(0) when "your application initializes <literal>libssl</> or <literal>libcrypto</> libraries". The operative word being 'or'. If you follow the advice after only having initialized the libcrypto library, you would get a failure trying to use libpq/SSL. merlin
On Sat, Mar 28, 2009 at 3:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Bruce Momjian <bruce@momjian.us> writes: >> Well, we are not the "Make Merlin Happy club". ;-) > > Merlin and Andrew were the ones complaining initially. If they feel > that a proposed patch doesn't fix the problem, then I'd say that it > isn't fixing the problem. +1. >> 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. > > This is just a rehash of one of the patches that was discussed earlier. > There wasn't consensus for it then, and there's not now. OK, I read this patch. Wazzamatterwithit? AIUI, we need to define an API that allows libssl and libcrypto to be initialized or not initialized in any combination. We can do this by modifying the meaning of the argument to PQinitSSL(), by adding a new API function, by creating some more general initialization function, or by some other method. It doesn't REALLY matter. I think I'm personally of the opinion that PQinitSSL(int) should be left alone and we should define PQinitSSLCrypto(int, int), just to avoid the possibility of difficult-to-debug backward-compatibility problems, but the number of people calling PQinitSSL(2) in existing applications is doubtless vanishingly small, because there is probably no reason to call it with a non-constant argument, so who is going to pick 2 rather than 1? So if someone has some sort of principled objection to adding a new API call, then let's just modify the behavior of the existing call instead and move on. Is there more substance here than meets the eye? ...Robert
Robert Haas wrote: > > Is there more substance here than meets the eye? > No, you about summed it up. We need a way to init libssl and libcrypto in any combo. Along the way, PQinit() was discussed which may have muddied the waters. I prefer leaving the PQinitSSL function alone, thus my patch that implements PQinitSecure(flags). Adding PQinitSSL(new_value) seem reasonable to me. My only complaint has been that the API user has no way of knowing if the function understood their request. An older libpq would treat any non-zero argument as one, which would silently fail/mis-behave from a new apps perspective. Not sure this can be solved. In the end, anyway you do it will have an issue or two. I agree that it really doesn't matter, all methods would probably do the trick. Andrew Chernow eSilo, LLC
Andrew Chernow wrote: > Robert Haas wrote: > > > > Is there more substance here than meets the eye? > > > > No, you about summed it up. We need a way to init libssl and libcrypto > in any combo. Along the way, PQinit() was discussed which may have > muddied the waters. > > I prefer leaving the PQinitSSL function alone, thus my patch that > implements PQinitSecure(flags). > > Adding PQinitSSL(new_value) seem reasonable to me. My only complaint > has been that the API user has no way of knowing if the function > understood their request. An older libpq would treat any non-zero > argument as one, which would silently fail/mis-behave from a new apps > perspective. Not sure this can be solved. > > In the end, anyway you do it will have an issue or two. I agree that it > really doesn't matter, all methods would probably do the trick. I think doing PQinitSSL(new_value) is probably the least invasive change to solve this, which is why I suggested it. It does have a compile-time check by referencing the #define. We never documented the valid values to PQinitSSL(), and no one ever reported this as a bug, so the existing use of PQinitSSL() is probably very small. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> Andrew Chernow wrote:
>> Adding PQinitSSL(new_value) seem reasonable to me. My only complaint
>> has been that the API user has no way of knowing if the function
>> understood their request.
> I think doing PQinitSSL(new_value) is probably the least invasive change
> to solve this, which is why I suggested it. It does have a compile-time
> check by referencing the #define.
You're missing the point, which is that it isn't certain whether the
right thing happens at runtime. It would be very hard to debug the
failure if an app compiled against new headers was run with an old
shlib. The separate two-argument function would avoid that: the failure
would manifest as "called function doesn't exist", which would at least
make it obvious what was wrong.
I personally would be happy with the two-argument function solution.
Now, that only addresses the specific problem of libcrypto vs libssl.
IIUC, Merlin's current thought is that we should be looking for a more
general solution. But it seems a bit dangerous to try to design a
general solution when we have only one example to work from.
regards, tom lane
Tom Lane wrote: > > I personally would be happy with the two-argument function solution. > The patch I submitted pretty much does this, except it uses a flags argument instead of 2 fixed arguments. It can be easily changed to support the 2 argument idea: 1. Change prototype to: void PQinitSecure(int ssl, int crypto); - previous return value can go away, new version can bevoid. 2. Remove PG_SECURE_SSL, PG_SECURE_CRYPTO from libpq-fe.h 3. Modify wrapping code in PQinitSSL to use new prototype. - either pass 0,0 or 1,1, to PQinitSecure. Outside of that, the patch goes unchanged. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Andrew Chernow wrote: > >> Adding PQinitSSL(new_value) seem reasonable to me. My only complaint > >> has been that the API user has no way of knowing if the function > >> understood their request. > > > I think doing PQinitSSL(new_value) is probably the least invasive change > > to solve this, which is why I suggested it. It does have a compile-time > > check by referencing the #define. > > You're missing the point, which is that it isn't certain whether the > right thing happens at runtime. It would be very hard to debug the > failure if an app compiled against new headers was run with an old > shlib. The separate two-argument function would avoid that: the failure > would manifest as "called function doesn't exist", which would at least > make it obvious what was wrong. > > I personally would be happy with the two-argument function solution. > Now, that only addresses the specific problem of libcrypto vs libssl. > IIUC, Merlin's current thought is that we should be looking for a more > general solution. But it seems a bit dangerous to try to design a > general solution when we have only one example to work from. I think this is where we got stuck because extending libpq with a new function is a larger API change, and not having a clear plan of what initialization stuff we might need in the future, it seems unwise, and also perhaps overkill. FYI, libcrypto is initialized only in threaded libpq builds, and non-zero calls to PQinitSSL were always no-ops because they just enabled the default behavior. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Sun, Mar 29, 2009 at 1:56 PM, Bruce Momjian <bruce@momjian.us> wrote: > I think this is where we got stuck because extending libpq with a new > function is a larger API change, and not having a clear plan of what > initialization stuff we might need in the future, it seems unwise, and > also perhaps overkill. right, maybe this is a case of 'the perfect being the enemy of the good'...I have no qualms with fixing it now with the two argument version. The stuff I'd like to see is a separate issue, I guess. merlin
Tom Lane wrote: > > I personally would be happy with the two-argument function solution. > I modified my previous patch to use a two-argument function solution. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: doc/src/sgml/libpq.sgml =================================================================== RCS file: /projects/cvsroot/pgsql/doc/src/sgml/libpq.sgml,v retrieving revision 1.280 diff -C6 -r1.280 libpq.sgml *** doc/src/sgml/libpq.sgml 28 Mar 2009 01:36:11 -0000 1.280 --- doc/src/sgml/libpq.sgml 30 Mar 2009 13:29:48 -0000 *************** *** 6179,6190 **** --- 6179,6219 ---- <!-- 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> for details on the SSL API. </para> + <para> + <variablelist> + <varlistentry> + <term> + <function>PQinitSecure</function> + <indexterm> + <primary>PQinitSecure</primary> + </indexterm> + </term> + + <listitem> + <para> + Allows applications to select which secure components to initialize. + <synopsis> + void PQinitSecure(int do_ssl, init do_crypto); + </synopsis> + </para> + + <para> + When do_ssl is non-zero, OpenSSL's SSL library will be initialized. + When do_crypto is non-zero, OpenSSL's Crypto library will be initialized. + By default, both libraries are initialized. When SSL support is not + compiled in, this function does nothing. + </para> + </listitem> + </varlistentry> + </variablelist> + </para> + <table id="libpq-ssl-file-usage"> <title>Libpq/Client SSL File Usage</title> <tgroup cols="3"> <thead> <row> <entry>File</entry> Index: src/interfaces/libpq/exports.txt =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v retrieving revision 1.22 diff -C6 -r1.22 exports.txt *** src/interfaces/libpq/exports.txt 22 Sep 2008 13:55:14 -0000 1.22 --- src/interfaces/libpq/exports.txt 30 Mar 2009 13:29:48 -0000 *************** *** 149,154 **** --- 149,155 ---- PQinstanceData 147 PQsetInstanceData 148 PQresultInstanceData 149 PQresultSetInstanceData 150 PQfireResultCreateEvents 151 PQconninfoParse 152 + PQinitSecure 153 Index: src/interfaces/libpq/fe-secure.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-secure.c,v retrieving revision 1.121 diff -C6 -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 30 Mar 2009 13:29:49 -0000 *************** *** 96,107 **** --- 96,108 ---- static PostgresPollingStatusType open_client_SSL(PGconn *); static void close_SSL(PGconn *); static char *SSLerrmessage(void); 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 static int ssl_open_connections = 0; #ifndef WIN32 *************** *** 170,182 **** * initialized OpenSSL. */ void PQinitSSL(int do_init) { #ifdef USE_SSL ! pq_init_ssl_lib = do_init; #endif } /* * Initialize global context */ --- 171,196 ---- * initialized OpenSSL. */ void PQinitSSL(int do_init) { #ifdef USE_SSL ! PQinitSecure(do_init, do_init); ! #endif ! } ! ! /* ! * Exported function to allow application to tell us which secure ! * components to initialize. ! */ ! void ! PQinitSecure(int do_ssl, int do_crypto) ! { ! #ifdef USE_SSL ! pq_init_ssl_lib = do_ssl; ! pq_init_crypto_lib = do_crypto; #endif } /* * Initialize global context */ *************** *** 820,831 **** --- 834,847 ---- * message - no connection local setup is made. */ static int init_ssl_system(PGconn *conn) { #ifdef ENABLE_THREAD_SAFETY + int num_ssl_conns = 0; + #ifdef WIN32 /* Also see similar code in fe-connect.c, default_threadlock() */ if (ssl_config_mutex == NULL) { while (InterlockedExchange(&win32_ssl_create_mutex, 1) == 1) /* loop, another thread own the lock */ ; *************** *** 837,849 **** InterlockedExchange(&win32_ssl_create_mutex, 0); } #endif 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 * tell us how big to make this array. */ if (pq_lockarray == NULL) --- 853,874 ---- InterlockedExchange(&win32_ssl_create_mutex, 0); } #endif if (pthread_mutex_lock(&ssl_config_mutex)) return -1; ! /* ! * Increment connection count if we are responsible for ! * intiializing the SSL or Crypto library. Currently, only ! * crypto needs this during setup and cleanup. That may ! * change in the future so we always update the counter. ! */ ! if (pq_init_ssl_lib || pq_init_crypto_lib) ! num_ssl_conns = ssl_open_connections++; ! ! if (pq_init_crypto_lib) { /* * If necessary, set up an array to hold locks for OpenSSL. OpenSSL will * tell us how big to make this array. */ if (pq_lockarray == NULL) *************** *** 865,877 **** pthread_mutex_unlock(&ssl_config_mutex); return -1; } } } ! if (ssl_open_connections++ == 0) { /* This is actually libcrypto, not libssl. */ /* These are only required for threaded SSL applications */ CRYPTO_set_id_callback(pq_threadidcallback); CRYPTO_set_locking_callback(pq_lockingcallback); } --- 890,902 ---- pthread_mutex_unlock(&ssl_config_mutex); return -1; } } } ! if (num_ssl_conns == 0) { /* This is actually libcrypto, not libssl. */ /* These are only required for threaded SSL applications */ CRYPTO_set_id_callback(pq_threadidcallback); CRYPTO_set_locking_callback(pq_lockingcallback); } *************** *** 925,957 **** { #ifdef ENABLE_THREAD_SAFETY /* Mutex is created in initialize_ssl_system() */ if (pthread_mutex_lock(&ssl_config_mutex)) return; ! if (pq_init_ssl_lib) { ! if (ssl_open_connections > 0) ! --ssl_open_connections; ! if (ssl_open_connections == 0) ! { ! /* This is actually libcrypto, not libssl. */ ! /* No connections left, unregister all callbacks */ ! CRYPTO_set_locking_callback(NULL); ! CRYPTO_set_id_callback(NULL); ! ! /* ! * We don't free the lock array. If we get another connection ! * from the same caller, we will just re-use it with the existing ! * mutexes. ! * ! * This means we leak a little memory on repeated load/unload ! * of the library. ! */ ! } } pthread_mutex_unlock(&ssl_config_mutex); #endif return; } --- 950,979 ---- { #ifdef ENABLE_THREAD_SAFETY /* Mutex is created in initialize_ssl_system() */ if (pthread_mutex_lock(&ssl_config_mutex)) return; ! if ((pq_init_ssl_lib || pq_init_crypto_lib) && ssl_open_connections > 0) ! --ssl_open_connections; ! ! if (pq_init_crypto_lib && ssl_open_connections == 0) { ! /* This is actually libcrypto, not libssl. */ ! /* No connections left, unregister all callbacks */ ! CRYPTO_set_locking_callback(NULL); ! CRYPTO_set_id_callback(NULL); ! /* ! * We don't free the lock array. If we get another connection ! * from the same caller, we will just re-use it with the existing ! * mutexes. ! * ! * This means we leak a little memory on repeated load/unload ! * of the library. ! */ } pthread_mutex_unlock(&ssl_config_mutex); #endif return; } Index: src/interfaces/libpq/libpq-fe.h =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v retrieving revision 1.145 diff -C6 -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 30 Mar 2009 13:29:49 -0000 *************** *** 299,310 **** --- 299,313 ---- * unencrypted connections or if any other TLS library is in use. */ extern void *PQgetssl(PGconn *conn); /* Tell libpq whether it needs to initialize OpenSSL */ extern void PQinitSSL(int do_init); + /* Tell libpq which secure components to initialize. */ + extern void PQinitSecure(int do_ssl, int do_crypto); + /* Set verbosity for PQerrorMessage and PQresultErrorMessage */ extern PGVerbosity PQsetErrorVerbosity(PGconn *conn, PGVerbosity verbosity); /* Enable/disable tracing */ extern void PQtrace(PGconn *conn, FILE *debug_port); extern void PQuntrace(PGconn *conn);
Andrew Chernow <ac@esilo.com> writes:
> I modified my previous patch to use a two-argument function solution.
It sounds like everyone has converged on agreeing that this way is okay
for 8.4? Object now or hold your peace ...
regards, tom lane
Robert Haas <robertmhaas@gmail.com> writes:
> This looks OK to me, except I think we should modify the documentation
> to PQinitSSL() to say that it you must not use both that function and
> PQinitSecure(), and explain that if you need to control initialization
> of libcrypto and libssl, you should use that function instead.
Agreed, the docs need a bit more effort. I'll have a go at that.
regards, tom lane
On Mon, Mar 30, 2009 at 9:36 AM, Andrew Chernow <ac@esilo.com> wrote: > Tom Lane wrote: >> >> I personally would be happy with the two-argument function solution. >> > > I modified my previous patch to use a two-argument function solution. This looks OK to me, except I think we should modify the documentation to PQinitSSL() to say that it you must not use both that function and PQinitSecure(), and explain that if you need to control initialization of libcrypto and libssl, you should use that function instead. ...Robert
Tom Lane wrote: > Andrew Chernow <ac@esilo.com> writes: > > I modified my previous patch to use a two-argument function solution. > > It sounds like everyone has converged on agreeing that this way is okay > for 8.4? Object now or hold your peace ... What are we doing with PQinitSSL()? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > This looks OK to me, except I think we should modify the documentation > > to PQinitSSL() to say that it you must not use both that function and > > PQinitSecure(), and explain that if you need to control initialization > > of libcrypto and libssl, you should use that function instead. > > Agreed, the docs need a bit more effort. I'll have a go at that. Right; I saw no change to PSinitSSL documentation in the patch, which was my concern. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Mon, Mar 30, 2009 at 10:22 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Mar 30, 2009 at 9:36 AM, Andrew Chernow <ac@esilo.com> wrote: >> Tom Lane wrote: >>> >>> I personally would be happy with the two-argument function solution. >>> >> >> I modified my previous patch to use a two-argument function solution. > > This looks OK to me, except I think we should modify the documentation > to PQinitSSL() to say that it you must not use both that function and > PQinitSecure(), and explain that if you need to control initialization > of libcrypto and libssl, you should use that function instead. do you think PQinitSSL should be deprecated? merlin
Merlin Moncure wrote: > On Mon, Mar 30, 2009 at 10:22 AM, Robert Haas <robertmhaas@gmail.com> wrote: > > On Mon, Mar 30, 2009 at 9:36 AM, Andrew Chernow <ac@esilo.com> wrote: > >> Tom Lane wrote: > >>> > >>> I personally would be happy with the two-argument function solution. > >>> > >> > >> I modified my previous patch to use a two-argument function solution. > > > > This looks OK to me, except I think we should modify the documentation > > to PQinitSSL() to say that it you must not use both that function and > > PQinitSecure(), and explain that if you need to control initialization > > of libcrypto and libssl, you should use that function instead. > > do you think PQinitSSL should be deprecated? Well, I think having duplicate capability in an API is confusing, so yea. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Merlin Moncure <mmoncure@gmail.com> writes:
> On Mon, Mar 30, 2009 at 10:22 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> This looks OK to me, except I think we should modify the documentation
>> to PQinitSSL() to say that it you must not use both that function and
>> PQinitSecure(), and explain that if you need to control initialization
>> of libcrypto and libssl, you should use that function instead.
> do you think PQinitSSL should be deprecated?
I see no reason to deprecate it per se. I was planning to just define
it as equivalent to PQinitSecure(do_init, do_init).
BTW, unless someone objects I'd like to make the name of that function
PQinitSecurity. The other looks like it's implying that it is replacing
an insecure PQinit() function ...
regards, tom lane
Merlin Moncure wrote: > On Mon, Mar 30, 2009 at 10:22 AM, Robert Haas <robertmhaas@gmail.com> wrote: > > On Mon, Mar 30, 2009 at 9:36 AM, Andrew Chernow <ac@esilo.com> wrote: > >> Tom Lane wrote: > >>> > >>> I personally would be happy with the two-argument function solution. > >>> > >> > >> I modified my previous patch to use a two-argument function solution. > > > > This looks OK to me, except I think we should modify the documentation > > to PQinitSSL() to say that it you must not use both that function and > > PQinitSecure(), and explain that if you need to control initialization > > of libcrypto and libssl, you should use that function instead. > > do you think PQinitSSL should be deprecated? FYI, libcrypto has been initializing with SSL in threaded builds since Postgres 8.0. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Merlin Moncure wrote: >> On Mon, Mar 30, 2009 at 10:22 AM, Robert Haas <robertmhaas@gmail.com> wrote: >>> On Mon, Mar 30, 2009 at 9:36 AM, Andrew Chernow <ac@esilo.com> wrote: >>>> Tom Lane wrote: >>>>> I personally would be happy with the two-argument function solution. >>>>> >>>> I modified my previous patch to use a two-argument function solution. >>> This looks OK to me, except I think we should modify the documentation >>> to PQinitSSL() to say that it you must not use both that function and >>> PQinitSecure(), and explain that if you need to control initialization >>> of libcrypto and libssl, you should use that function instead. >> do you think PQinitSSL should be deprecated? > > Well, I think having duplicate capability in an API is confusing, so > yea. > Maybe document that PQinitSSL(do_init) is now the same as PQinitSecure(do_init, do_init). -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Tom Lane wrote: > BTW, unless someone objects I'd like to make the name of that function > PQinitSecurity. > > Agreed. Although, both PQinitSecure and PQinitSecurity are very general names. "Security" can mean a lot of things. Maybe the name should be more particular, like PQinitOpenSSL. I think its okay to reference the openssl project name being how this function is designed to work with openssl's library split. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
On 30 mar 2009, at 17.24, Andrew Chernow <ac@esilo.com> wrote: > Tom Lane wrote: >> BTW, unless someone objects I'd like to make the name of that >> function >> PQinitSecurity. > > Agreed. Although, both PQinitSecure and PQinitSecurity are very > general names. "Security" can mean a lot of things. Maybe the name > should be more particular, like PQinitOpenSSL. I think its okay to > reference the openssl project name being how this function is > designed to work with openssl's library split. > +1 It's quite likely that well want to support another ssl library in the future. At least likely enough that any api we define should take it into consideration. /Magnus
On Mon, Mar 30, 2009 at 11:56 AM, Magnus Hagander <magnus@hagander.net> wrote: > On 30 mar 2009, at 17.24, Andrew Chernow <ac@esilo.com> wrote: > >> Tom Lane wrote: >>> >>> BTW, unless someone objects I'd like to make the name of that function >>> PQinitSecurity. >> >> Agreed. Although, both PQinitSecure and PQinitSecurity are very general >> names. "Security" can mean a lot of things. Maybe the name should be more >> particular, like PQinitOpenSSL. I think its okay to reference the openssl >> project name being how this function is designed to work with openssl's >> library split. >> > +1 > > It's quite likely that well want to support another ssl library in the > future. At least likely enough that any api we define should take it into > consideration. +1. ...Robert
Magnus Hagander wrote: > On 30 mar 2009, at 17.24, Andrew Chernow <ac@esilo.com> wrote: > > > Tom Lane wrote: > >> BTW, unless someone objects I'd like to make the name of that > >> function > >> PQinitSecurity. > > > > Agreed. Although, both PQinitSecure and PQinitSecurity are very > > general names. "Security" can mean a lot of things. Maybe the name > > should be more particular, like PQinitOpenSSL. I think its okay to > > reference the openssl project name being how this function is > > designed to work with openssl's library split. > > > +1 > > It's quite likely that well want to support another ssl library in the > future. At least likely enough that any api we define should take it > into consideration. Are we sure we don't want to add a more general libpq initialization control at this time? PQinitOptions(PG_NO_SSL_INIT | PG_NO_CRYPTO_INIT); -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Magnus Hagander wrote: >> On 30 mar 2009, at 17.24, Andrew Chernow <ac@esilo.com> wrote: >> >>> Tom Lane wrote: >>>> BTW, unless someone objects I'd like to make the name of that >>>> function >>>> PQinitSecurity. >>> Agreed. Although, both PQinitSecure and PQinitSecurity are very >>> general names. "Security" can mean a lot of things. Maybe the name >>> should be more particular, like PQinitOpenSSL. I think its okay to >>> reference the openssl project name being how this function is >>> designed to work with openssl's library split. >>> >> +1 >> >> It's quite likely that well want to support another ssl library in the >> future. At least likely enough that any api we define should take it >> into consideration. > > Are we sure we don't want to add a more general libpq initialization > control at this time? > > PQinitOptions(PG_NO_SSL_INIT | PG_NO_CRYPTO_INIT); Could be an option but if we go down that path, it needs to be PG_NO_OPENSSL_SSL_INIT and PG_NO_OPENSSL_CRYPTO_INIT.... //Magnus
Magnus Hagander <magnus@hagander.net> writes:
> Bruce Momjian wrote:
>> Are we sure we don't want to add a more general libpq initialization
>> control at this time?
>>
>> PQinitOptions(PG_NO_SSL_INIT | PG_NO_CRYPTO_INIT);
> Could be an option but if we go down that path, it needs to be
> PG_NO_OPENSSL_SSL_INIT and PG_NO_OPENSSL_CRYPTO_INIT....
And we get into the whole question of error handling, which is what
shot down that proposal last time.
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: > > Bruce Momjian wrote: > >> Are we sure we don't want to add a more general libpq initialization > >> control at this time? > >> > >> PQinitOptions(PG_NO_SSL_INIT | PG_NO_CRYPTO_INIT); > > > Could be an option but if we go down that path, it needs to be > > PG_NO_OPENSSL_SSL_INIT and PG_NO_OPENSSL_CRYPTO_INIT.... > > And we get into the whole question of error handling, which is what > shot down that proposal last time. Can you remind me of the details? I don't remember that issue. Currently PQinitSSL() returns void, so I don't see an issue there. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> And we get into the whole question of error handling, which is what
>> shot down that proposal last time.
> Can you remind me of the details? I don't remember that issue.
> Currently PQinitSSL() returns void, so I don't see an issue there.
The point is exactly the same as the complaint about turning PQinitSSL's
argument into a bitmask: if you are trying to define an extensible API
then you need a way for the app to determine whether all the bits it
passed were recognizable by the library.
I think we should stick with the simple two-argument function and not
try to design a solution for unknown problems. Otherwise we are right
back at the point where the previous thread petered out for lack of
consensus.
regards, tom lane
Tom Lane wrote: > > I think we should stick with the simple two-argument function and not +1000000 Generic PQinit concept was already punted several times. Using a bit mask for initsecure or something was also tried and rejected. The well is dry on this. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Tom Lane wrote: > >> And we get into the whole question of error handling, which is what > >> shot down that proposal last time. > > > Can you remind me of the details? I don't remember that issue. > > Currently PQinitSSL() returns void, so I don't see an issue there. > > The point is exactly the same as the complaint about turning PQinitSSL's > argument into a bitmask: if you are trying to define an extensible API > then you need a way for the app to determine whether all the bits it > passed were recognizable by the library. It seems having the init function return the bits it recognized would be the logical return value. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Mon, Mar 30, 2009 at 2:31 PM, Bruce Momjian <bruce@momjian.us> wrote: > Tom Lane wrote: >> Bruce Momjian <bruce@momjian.us> writes: >> > Tom Lane wrote: >> >> And we get into the whole question of error handling, which is what >> >> shot down that proposal last time. >> >> > Can you remind me of the details? I don't remember that issue. >> > Currently PQinitSSL() returns void, so I don't see an issue there. >> >> The point is exactly the same as the complaint about turning PQinitSSL's >> argument into a bitmask: if you are trying to define an extensible API >> then you need a way for the app to determine whether all the bits it >> passed were recognizable by the library. > > It seems having the init function return the bits it recognized would be > the logical return value. I'm disinclined to go this route because, as Tom pointed out upthread, there's no evidence at all that this problem is an instance of some more general class. If it turns out that we need to initialize other things, they're likely to be totally unrelated to SSL, and whether they are or not, they may not be things that can be indicated using a bitmask. For example, what happens if we someday add support for a separate SSL library called FooSSL, and we happen to need a "char *" to describe how it should be initialized? Or we might need a whole "char *" or "int" argument for some purpose unrelated to SSL. Then we'll have a PQinit() function that pretends to be a general-purpose initialization mechanism but really isn't. With enough wrangling, we might be able to convince ourselves that we've designed something which is general enough to cover all contingencies, but I'm not sure there's any good reason to put in that much work. Adding a new API call is not really that big a deal, especially given that we're retaining the old one for backward compatibility. If we discover we need something else in the future, we'll just add an API call for that, too. ...Robert
Bruce Momjian wrote: > It seems having the init function return the bits it recognized would be > the logical return value. > That's what my first PQionitSecure patch did, actually it returned the bits it didn't understand. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Robert Haas wrote: > Or we might need a whole "char *" or "int" argument for some purpose > unrelated to SSL. Then we'll have a PQinit() function that pretends > to be a general-purpose initialization mechanism but really isn't. > I proposed a PQinit() that included a void data pointer argument. The idea was PQinit(which, data, data_len) would be called multiple times to init different things. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
On Mon, Mar 30, 2009 at 2:59 PM, Andrew Chernow <ac@esilo.com> wrote: > Robert Haas wrote: >> >> Or we might need a whole "char *" or "int" argument for some purpose >> unrelated to SSL. Then we'll have a PQinit() function that pretends >> to be a general-purpose initialization mechanism but really isn't. >> > > I proposed a PQinit() that included a void data pointer argument. The idea > was PQinit(which, data, data_len) would be called multiple times to init > different things. I guess that'd work but it might be overkill. ...Robert
Robert Haas <robertmhaas@gmail.com> writes:
> I guess that'd work but it might be overkill.
The real bottom line is that *all* the proposals for generic init
functions are overkill. We have no evidence that we need one and
no certainty about what the requirements for it would be if we did.
I think we should just do PQinitOpenSSL(2 args) and be done with it.
regards, tom lane
On Mon, Mar 30, 2009 at 3:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I guess that'd work but it might be overkill. > > The real bottom line is that *all* the proposals for generic init > functions are overkill. We have no evidence that we need one and > no certainty about what the requirements for it would be if we did. > > I think we should just do PQinitOpenSSL(2 args) and be done with it. I was pushing for generic PQinit, but have come around and agree with this point of view. merlin
Merlin Moncure wrote: > On Mon, Mar 30, 2009 at 3:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > >> I guess that'd work but it might be overkill. > > > > The real bottom line is that *all* the proposals for generic init > > functions are overkill. ?We have no evidence that we need one and > > no certainty about what the requirements for it would be if we did. > > > > I think we should just do PQinitOpenSSL(2 args) and be done with it. > > I was pushing for generic PQinit, but have come around and agree with > this point of view. OK, we are _go_ then. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Andrew Chernow <ac@esilo.com> writes:
> I modified my previous patch to use a two-argument function solution.
Applied with minor changes (rename to PQinitOpenSSL per discussion,
some comment and docs improvements).
regards, tom lane
Bruce Momjian <bruce@momjian.us> writes:
> Merlin Moncure wrote:
>>> I think we should just do PQinitOpenSSL(2 args) and be done with it.
>>
>> I was pushing for generic PQinit, but have come around and agree with
>> this point of view.
> OK, we are _go_ then.
Done. Let's hope this thread stays dead.
regards, tom lane