Thread: OpenSSL Applink
On Windows the OpenSSL guys have included code with 0.9.8 and above to
allow OpenSSL to work correctly regardless of the MSVC runtime libraries
that have been used with the host application. This has become noticable
with the MSVC++ build in which any client apps that connect using libpq
with a client certificate will bail out with an error such as:
C:\pgsql-8.3>bin\psql -p 5433 postgres
OPENSSL_Uplink(00314010,05): no OPENSSL_Applink
The server doesn't seem to be affected, but the attached patch fixes the
problem for the client apps. Unfortunately it must be included in the
app itself, and not libpq.
I'm not sure what the OpenSSL guys were thinking here - apps like
pgAdmin which previously didn't use OpenSSL directly now need the source
code to build. I've also seen reports on the -odbc list that psqlODBC is
similarly affected, though how on earth we're meant to get the AppLink
code into apps such as MS Access or Crystal Reports is beyond me.
Regards, Dave
Index: bin/pg_dump/pg_dump.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_dump.c,v
retrieving revision 1.472
diff -u -r1.472 pg_dump.c
--- bin/pg_dump/pg_dump.c 3 Sep 2007 00:39:19 -0000 1.472
+++ bin/pg_dump/pg_dump.c 28 Sep 2007 14:09:59 -0000
@@ -197,6 +197,18 @@
static void check_sql_result(PGresult *res, PGconn *conn, const char *query,
ExecStatusType expected);
+/*
+ * We need to include OpenSSL's applink code with OpenSSL 0.9.8+ on Windows
+ * It needs to be included in every .exe so we include it here.
+ */
+#ifdef WIN32
+#ifdef USE_SSL
+#include <openssl/opensslv.h>
+#if (OPENSSL_VERSION_NUMBER >= 0x00908000L)
+#include <openssl/applink.c>
+#endif
+#endif
+#endif
int
main(int argc, char **argv)
Index: bin/pg_dump/pg_dumpall.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_dumpall.c,v
retrieving revision 1.92
diff -u -r1.92 pg_dumpall.c
--- bin/pg_dump/pg_dumpall.c 8 Jul 2007 19:07:38 -0000 1.92
+++ bin/pg_dump/pg_dumpall.c 28 Sep 2007 14:09:59 -0000
@@ -71,6 +71,19 @@
static FILE *OPF;
static char *filename = NULL;
+/*
+ * We need to include OpenSSL's applink code with OpenSSL 0.9.8+ on Windows
+ * It needs to be included in every .exe so we include it here.
+ */
+#ifdef WIN32
+#ifdef USE_SSL
+#include <openssl/opensslv.h>
+#if (OPENSSL_VERSION_NUMBER >= 0x00908000L)
+#include <openssl/applink.c>
+#endif
+#endif
+#endif
+
int
main(int argc, char *argv[])
{
Index: bin/pg_dump/pg_restore.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_restore.c,v
retrieving revision 1.84
diff -u -r1.84 pg_restore.c
--- bin/pg_dump/pg_restore.c 14 Oct 2006 23:07:22 -0000 1.84
+++ bin/pg_dump/pg_restore.c 28 Sep 2007 14:09:59 -0000
@@ -64,6 +64,19 @@
typedef struct option optType;
+/*
+ * We need to include OpenSSL's applink code with OpenSSL 0.9.8+ on Windows
+ * It needs to be included in every .exe so we include it here.
+ */
+#ifdef WIN32
+#ifdef USE_SSL
+#include <openssl/opensslv.h>
+#if (OPENSSL_VERSION_NUMBER >= 0x00908000L)
+#include <openssl/applink.c>
+#endif
+#endif
+#endif
+
int
main(int argc, char **argv)
{
Index: bin/psql/startup.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/psql/startup.c,v
retrieving revision 1.141
diff -u -r1.141 startup.c
--- bin/psql/startup.c 8 Jul 2007 19:07:38 -0000 1.141
+++ bin/psql/startup.c 28 Sep 2007 14:07:02 -0000
@@ -95,6 +95,18 @@
#endif
/*
+ * We need to include OpenSSL's applink code with OpenSSL 0.9.8+ on Windows
+ */
+#ifdef WIN32
+#ifdef USE_SSL
+#include <openssl/opensslv.h>
+#if (OPENSSL_VERSION_NUMBER >= 0x00908000L)
+#include <openssl/applink.c>
+#endif
+#endif
+#endif
+
+/*
*
* main
*
Index: bin/scripts/clusterdb.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/scripts/clusterdb.c,v
retrieving revision 1.18
diff -u -r1.18 clusterdb.c
--- bin/scripts/clusterdb.c 4 Jun 2007 10:02:40 -0000 1.18
+++ bin/scripts/clusterdb.c 28 Sep 2007 14:11:03 -0000
@@ -24,6 +24,18 @@
static void help(const char *progname);
+/*
+ * We need to include OpenSSL's applink code with OpenSSL 0.9.8+ on Windows
+ * It needs to be included in every .exe so we include it here.
+ */
+#ifdef WIN32
+#ifdef USE_SSL
+#include <openssl/opensslv.h>
+#if (OPENSSL_VERSION_NUMBER >= 0x00908000L)
+#include <openssl/applink.c>
+#endif
+#endif
+#endif
int
main(int argc, char *argv[])
Index: bin/scripts/createdb.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/scripts/createdb.c,v
retrieving revision 1.23
diff -u -r1.23 createdb.c
--- bin/scripts/createdb.c 4 Jun 2007 10:02:40 -0000 1.23
+++ bin/scripts/createdb.c 28 Sep 2007 14:11:03 -0000
@@ -19,6 +19,18 @@
static void help(const char *progname);
+/*
+ * We need to include OpenSSL's applink code with OpenSSL 0.9.8+ on Windows
+ * It needs to be included in every .exe so we include it here.
+ */
+#ifdef WIN32
+#ifdef USE_SSL
+#include <openssl/opensslv.h>
+#if (OPENSSL_VERSION_NUMBER >= 0x00908000L)
+#include <openssl/applink.c>
+#endif
+#endif
+#endif
int
main(int argc, char *argv[])
Index: bin/scripts/createlang.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/scripts/createlang.c,v
retrieving revision 1.26
diff -u -r1.26 createlang.c
--- bin/scripts/createlang.c 10 Aug 2007 00:39:31 -0000 1.26
+++ bin/scripts/createlang.c 28 Sep 2007 14:11:03 -0000
@@ -16,6 +16,18 @@
static void help(const char *progname);
+/*
+ * We need to include OpenSSL's applink code with OpenSSL 0.9.8+ on Windows
+ * It needs to be included in every .exe so we include it here.
+ */
+#ifdef WIN32
+#ifdef USE_SSL
+#include <openssl/opensslv.h>
+#if (OPENSSL_VERSION_NUMBER >= 0x00908000L)
+#include <openssl/applink.c>
+#endif
+#endif
+#endif
int
main(int argc, char *argv[])
Index: bin/scripts/createuser.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/scripts/createuser.c,v
retrieving revision 1.36
diff -u -r1.36 createuser.c
--- bin/scripts/createuser.c 4 Jun 2007 10:02:40 -0000 1.36
+++ bin/scripts/createuser.c 28 Sep 2007 14:11:03 -0000
@@ -24,6 +24,19 @@
TRI_YES
};
+/*
+ * We need to include OpenSSL's applink code with OpenSSL 0.9.8+ on Windows
+ * It needs to be included in every .exe so we include it here.
+ */
+#ifdef WIN32
+#ifdef USE_SSL
+#include <openssl/opensslv.h>
+#if (OPENSSL_VERSION_NUMBER >= 0x00908000L)
+#include <openssl/applink.c>
+#endif
+#endif
+#endif
+
int
main(int argc, char *argv[])
{
Index: bin/scripts/dropdb.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/scripts/dropdb.c,v
retrieving revision 1.20
diff -u -r1.20 dropdb.c
--- bin/scripts/dropdb.c 4 Jun 2007 10:02:40 -0000 1.20
+++ bin/scripts/dropdb.c 28 Sep 2007 14:11:03 -0000
@@ -17,6 +17,18 @@
static void help(const char *progname);
+/*
+ * We need to include OpenSSL's applink code with OpenSSL 0.9.8+ on Windows
+ * It needs to be included in every .exe so we include it here.
+ */
+#ifdef WIN32
+#ifdef USE_SSL
+#include <openssl/opensslv.h>
+#if (OPENSSL_VERSION_NUMBER >= 0x00908000L)
+#include <openssl/applink.c>
+#endif
+#endif
+#endif
int
main(int argc, char *argv[])
Index: bin/scripts/droplang.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/scripts/droplang.c,v
retrieving revision 1.23
diff -u -r1.23 droplang.c
--- bin/scripts/droplang.c 10 Aug 2007 00:39:31 -0000 1.23
+++ bin/scripts/droplang.c 28 Sep 2007 14:11:03 -0000
@@ -19,6 +19,18 @@
static void help(const char *progname);
+/*
+ * We need to include OpenSSL's applink code with OpenSSL 0.9.8+ on Windows
+ * It needs to be included in every .exe so we include it here.
+ */
+#ifdef WIN32
+#ifdef USE_SSL
+#include <openssl/opensslv.h>
+#if (OPENSSL_VERSION_NUMBER >= 0x00908000L)
+#include <openssl/applink.c>
+#endif
+#endif
+#endif
int
main(int argc, char *argv[])
Index: bin/scripts/dropuser.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/scripts/dropuser.c,v
retrieving revision 1.21
diff -u -r1.21 dropuser.c
--- bin/scripts/dropuser.c 4 Jun 2007 10:02:40 -0000 1.21
+++ bin/scripts/dropuser.c 28 Sep 2007 14:11:03 -0000
@@ -17,6 +17,18 @@
static void help(const char *progname);
+/*
+ * We need to include OpenSSL's applink code with OpenSSL 0.9.8+ on Windows
+ * It needs to be included in every .exe so we include it here.
+ */
+#ifdef WIN32
+#ifdef USE_SSL
+#include <openssl/opensslv.h>
+#if (OPENSSL_VERSION_NUMBER >= 0x00908000L)
+#include <openssl/applink.c>
+#endif
+#endif
+#endif
int
main(int argc, char *argv[])
Index: bin/scripts/reindexdb.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/scripts/reindexdb.c,v
retrieving revision 1.11
diff -u -r1.11 reindexdb.c
--- bin/scripts/reindexdb.c 4 Jun 2007 10:02:40 -0000 1.11
+++ bin/scripts/reindexdb.c 28 Sep 2007 14:11:03 -0000
@@ -29,6 +29,19 @@
const char *progname, bool echo);
static void help(const char *progname);
+/*
+ * We need to include OpenSSL's applink code with OpenSSL 0.9.8+ on Windows
+ * It needs to be included in every .exe so we include it here.
+ */
+#ifdef WIN32
+#ifdef USE_SSL
+#include <openssl/opensslv.h>
+#if (OPENSSL_VERSION_NUMBER >= 0x00908000L)
+#include <openssl/applink.c>
+#endif
+#endif
+#endif
+
int
main(int argc, char *argv[])
{
Index: bin/scripts/vacuumdb.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/scripts/vacuumdb.c,v
retrieving revision 1.18
diff -u -r1.18 vacuumdb.c
--- bin/scripts/vacuumdb.c 4 Jun 2007 10:02:40 -0000 1.18
+++ bin/scripts/vacuumdb.c 28 Sep 2007 14:11:03 -0000
@@ -26,6 +26,18 @@
static void help(const char *progname);
+/*
+ * We need to include OpenSSL's applink code with OpenSSL 0.9.8+ on Windows
+ * It needs to be included in every .exe so we include it here.
+ */
+#ifdef WIN32
+#ifdef USE_SSL
+#include <openssl/opensslv.h>
+#if (OPENSSL_VERSION_NUMBER >= 0x00908000L)
+#include <openssl/applink.c>
+#endif
+#endif
+#endif
int
main(int argc, char *argv[])
Dave Page <dpage@postgresql.org> writes:
> The server doesn't seem to be affected, but the attached patch fixes the
> problem for the client apps. Unfortunately it must be included in the
> app itself, and not libpq.
This is pretty much in the category of "they've got to be kidding".
I recommend sitting on the prior version until upstream fixes their
mistake.
regards, tom lane
Tom Lane wrote: > Dave Page <dpage@postgresql.org> writes: > >> The server doesn't seem to be affected, but the attached patch fixes the >> problem for the client apps. Unfortunately it must be included in the >> app itself, and not libpq. >> > > This is pretty much in the category of "they've got to be kidding". > I recommend sitting on the prior version until upstream fixes their > mistake. > > > That was my reaction. cheers andrew
Tom Lane wrote: > Dave Page <dpage@postgresql.org> writes: >> The server doesn't seem to be affected, but the attached patch fixes the >> problem for the client apps. Unfortunately it must be included in the >> app itself, and not libpq. > > This is pretty much in the category of "they've got to be kidding". Agreed. Unless I've completely missed the point this does seem *really* dumb. > I recommend sitting on the prior version until upstream fixes their > mistake. Unfortunately from what I've seen it doesn't look like thats on their agenda... and this has been around in release versions now since July 2005. I believe we just didn't notice it until now because the older Mingw builds use the MSVC 6.0 runtimes which just happened to be compatible with the OpenSSL binary builds (we're now using 8.0), in addition to which there are relatively few people using client-side certs I'd wager. /D
Dave Page wrote: > > I believe we just didn't notice it until now because the older Mingw > builds use the MSVC 6.0 runtimes which just happened to be compatible > with the OpenSSL binary builds (we're now using 8.0), in addition to > which there are relatively few people using client-side certs I'd wager. > > So SSL works without this wart if you don't have a client cert? cheers andrew
Andrew Dunstan wrote: > > > Dave Page wrote: >> >> I believe we just didn't notice it until now because the older Mingw >> builds use the MSVC 6.0 runtimes which just happened to be compatible >> with the OpenSSL binary builds (we're now using 8.0), in addition to >> which there are relatively few people using client-side certs I'd wager. >> >> > > So SSL works without this wart if you don't have a client cert? Yep. /D
Dave Page wrote: > Andrew Dunstan wrote: >> >> >> Dave Page wrote: >>> >>> I believe we just didn't notice it until now because the older Mingw >>> builds use the MSVC 6.0 runtimes which just happened to be >>> compatible with the OpenSSL binary builds (we're now using 8.0), in >>> addition to which there are relatively few people using client-side >>> certs I'd wager. >>> >>> >> >> So SSL works without this wart if you don't have a client cert? > > Yep. > > Then I think I'd rather disable use of client certs for the offending openssl versions in libpq, or let the apps die and refer the customers to the openssl people to lobby them for a sane solution. cheers andrew
Andrew Dunstan wrote: > Then I think I'd rather disable use of client certs for the offending > openssl versions in libpq, or let the apps die and refer the customers > to the openssl people to lobby them for a sane solution. If this were 8.0 I'd agree, but thats not a nice solution for those already using client certs (such as the pgAdmin user who brought this to my attention). :-( /D
Dave Page wrote: > Andrew Dunstan wrote: >> Dave Page wrote: >>> I believe we just didn't notice it until now because the older Mingw >>> builds use the MSVC 6.0 runtimes which just happened to be compatible >>> with the OpenSSL binary builds (we're now using 8.0), in addition to >>> which there are relatively few people using client-side certs I'd wager. >> >> So SSL works without this wart if you don't have a client cert? > > Yep. According to the OpenSSL FAQ, the purpose of the applink is to allow mixing release and debug versions or multi-threaded and non-multithreaded versions of libraries: http://www.openssl.org/support/faq.html#PROG2 How come we only bump into the crash with client certs? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas wrote: > Dave Page wrote: >> Andrew Dunstan wrote: >>> Dave Page wrote: >>>> I believe we just didn't notice it until now because the older Mingw >>>> builds use the MSVC 6.0 runtimes which just happened to be compatible >>>> with the OpenSSL binary builds (we're now using 8.0), in addition to >>>> which there are relatively few people using client-side certs I'd wager. >>> So SSL works without this wart if you don't have a client cert? >> Yep. > > According to the OpenSSL FAQ, the purpose of the applink is to allow > mixing release and debug versions or multi-threaded and > non-multithreaded versions of libraries: > > http://www.openssl.org/support/faq.html#PROG2 Yeah - which in itself is a pita because with their 'workaround' we now need to ensure we use a debug libpq with a debug pgadmin whereas previously mixing 'n' matching wasn't an issue. > How come we only bump into the crash with client certs? > I assume it uses fopen() or one of the other functions it does a GetProcAddress() on in that situation. /D
Dave Page <dpage@postgresql.org> writes:
> Andrew Dunstan wrote:
>> Then I think I'd rather disable use of client certs for the offending
>> openssl versions in libpq, or let the apps die and refer the customers
>> to the openssl people to lobby them for a sane solution.
> If this were 8.0 I'd agree, but thats not a nice solution for those
> already using client certs (such as the pgAdmin user who brought this to
> my attention).
Doesn't really matter. Even if we were willing to hack our own client
apps like that (which I'm not), we can *not* transfer such a requirement
onto every libpq-using application. It's just not acceptable.
regards, tom lane
Tom Lane wrote: > Dave Page <dpage@postgresql.org> writes: >> Andrew Dunstan wrote: >>> Then I think I'd rather disable use of client certs for the offending >>> openssl versions in libpq, or let the apps die and refer the customers >>> to the openssl people to lobby them for a sane solution. > >> If this were 8.0 I'd agree, but thats not a nice solution for those >> already using client certs (such as the pgAdmin user who brought this to >> my attention). > > Doesn't really matter. Even if we were willing to hack our own client > apps like that (which I'm not), we can *not* transfer such a requirement > onto every libpq-using application. It's just not acceptable. *We're* not transfering any requirement. I've fixed pgAdmin for example without any need to touch any Postgres code. If you don't want to include the fix (which I can quite understand) it'll just mean that the PG utilities won't work with client certs. /D
On 9/28/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Dave Page <dpage@postgresql.org> writes: > > Andrew Dunstan wrote: > >> Then I think I'd rather disable use of client certs for the offending > >> openssl versions in libpq, or let the apps die and refer the customers > >> to the openssl people to lobby them for a sane solution. > > > If this were 8.0 I'd agree, but thats not a nice solution for those > > already using client certs (such as the pgAdmin user who brought this to > > my attention). > > Doesn't really matter. Even if we were willing to hack our own client > apps like that (which I'm not), we can *not* transfer such a requirement > onto every libpq-using application. It's just not acceptable. Is it possible to use GNUTLS on Windows? -- marko
Marko Kreen wrote: > On 9/28/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Dave Page <dpage@postgresql.org> writes: >>> Andrew Dunstan wrote: >>>> Then I think I'd rather disable use of client certs for the offending >>>> openssl versions in libpq, or let the apps die and refer the customers >>>> to the openssl people to lobby them for a sane solution. >>> If this were 8.0 I'd agree, but thats not a nice solution for those >>> already using client certs (such as the pgAdmin user who brought this to >>> my attention). >> Doesn't really matter. Even if we were willing to hack our own client >> apps like that (which I'm not), we can *not* transfer such a requirement >> onto every libpq-using application. It's just not acceptable. > > Is it possible to use GNUTLS on Windows? > No, I don't think so. I didn't think we accepted Martijn's(?) patch for it anyway? Regards, Dave
Dave Page wrote: >> >> Is it possible to use GNUTLS on Windows? >> > > No, I don't think so. I didn't think we accepted Martijn's(?) patch > for it anyway? > > This mess might make that worth revisiting, if it can be used on Windows. See http://josefsson.org/gnutls4win/ cheers andrew
Dave Page <dpage@postgresql.org> writes:
> Tom Lane wrote:
>> Doesn't really matter. Even if we were willing to hack our own client
>> apps like that (which I'm not), we can *not* transfer such a requirement
>> onto every libpq-using application. It's just not acceptable.
> *We're* not transfering any requirement. I've fixed pgAdmin for example
> without any need to touch any Postgres code.
Well, yeah, we are, as evidenced by the fact that you had to do
something to pgAdmin. Every other application that uses libpq would
also be facing source code changes to make it work.
Is there really no way to solve this within libpq?
regards, tom lane
Tom Lane wrote: > Dave Page <dpage@postgresql.org> writes: >> Tom Lane wrote: >>> Doesn't really matter. Even if we were willing to hack our own client >>> apps like that (which I'm not), we can *not* transfer such a requirement >>> onto every libpq-using application. It's just not acceptable. > >> *We're* not transfering any requirement. I've fixed pgAdmin for example >> without any need to touch any Postgres code. > > Well, yeah, we are, as evidenced by the fact that you had to do > something to pgAdmin. Every other application that uses libpq would > also be facing source code changes to make it work. Yes, but it's not us requiring it (except in as much as we provide some level of SSL support) but the OpenSSL crew. > Is there really no way to solve this within libpq? Including the applink code in libpq was the first think I tried but it doesn't work (apparently because there is no way to ensure that any random module containing it isn't unloaded before it's needed which sorta makes sense). I did stumble across this text on a mailing list in response to someone with a similar problem in some JNI code. I know little of the OpenSSL API, but perhaps it rings bells with you before I spend my evening trying to figure it out? ----- But for new and evolving code [I suppose JNI would be rather new and evolving] applink should not be preferable option, as there is a way around it, namely avoid passing FILE* to BIO, but instead let BIO open files for you, i.e. avoid BIO_new_fp and stick to BIO_new_filename. Note that applink is engaged purely on demand and it doesn't have to be present in application if application doesn't pass FILE* to BIO. ----- /D
Dave Page wrote: > I did stumble across this text on a mailing list in response to someone > with a similar problem in some JNI code. I know little of the OpenSSL > API, but perhaps it rings bells with you before I spend my evening > trying to figure it out? OK, I think I've figured out a fix. Working up a patch now... /D
Dave Page wrote:
> Dave Page wrote:
>> I did stumble across this text on a mailing list in response to someone
>> with a similar problem in some JNI code. I know little of the OpenSSL
>> API, but perhaps it rings bells with you before I spend my evening
>> trying to figure it out?
>
> OK, I think I've figured out a fix. Working up a patch now...
Patch attached.
It appears to work fine except that if the client certificate is
missing, instead of:
could not open certificate file "C:\Documents and
Settings\Dave\Application Data/postgresql/postgresql.crt": No such file
or directory
I get:
Error connecting to the server: SSL SYSCALL error: Operation would block
(0x00002733/10035)
for reasons that are not clear to me. Any ideas?
Regards, Dave
Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.94
diff -c -r1.94 fe-secure.c
*** src/interfaces/libpq/fe-secure.c 16 Feb 2007 17:07:00 -0000 1.94
--- src/interfaces/libpq/fe-secure.c 28 Sep 2007 20:15:17 -0000
***************
*** 111,116 ****
--- 111,119 ----
#ifdef USE_SSL
#include <openssl/ssl.h>
+ #ifdef WIN32
+ #include <openssl/bio.h>
+ #endif
#if (SSLEAY_VERSION_NUMBER >= 0x00907000L)
#include <openssl/conf.h>
#endif
***************
*** 567,572 ****
--- 570,581 ----
* This callback is only called when the server wants a
* client cert.
*
+ * Note: On Windows we use OpenSSL's BIO functions to
+ * handle I/O to avoid issues passing FILE pointers to
+ * incompatible runtimes. We don't use them on other
+ * platforms because we couldn't then stat the keys to
+ * check for changes during execution.
+ *
* Must return 1 on success, 0 on no data or error.
*/
static int
***************
*** 579,585 ****
--- 588,598 ----
struct stat buf2;
#endif
char fnbuf[MAXPGPATH];
+ #ifndef WIN32
FILE *fp;
+ #else
+ BIO *bio;
+ #endif
PGconn *conn = (PGconn *) SSL_get_app_data(ssl);
char sebuf[256];
***************
*** 592,605 ****
--- 605,626 ----
/* read the user certificate */
snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE);
+ #ifndef WIN32
if ((fp = fopen(fnbuf, "r")) == NULL)
+ #else
+ if ((bio = BIO_new_file(fnbuf, "r")) == NULL)
+ #endif
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not open certificate file \"%s\": %s\n"),
fnbuf, pqStrerror(errno, sebuf, sizeof(sebuf)));
return 0;
}
+ #ifndef WIN32
if (PEM_read_X509(fp, x509, NULL, NULL) == NULL)
+ #else
+ if (PEM_read_bio_X509(bio, x509, NULL, NULL) == NULL)
+ #endif
{
char *err = SSLerrmessage();
***************
*** 607,616 ****
--- 628,645 ----
libpq_gettext("could not read certificate file \"%s\": %s\n"),
fnbuf, err);
SSLerrfree(err);
+ #ifndef WIN32
fclose(fp);
+ #else
+ BIO_free(bio);
+ #endif
return 0;
}
+ #ifndef WIN32
fclose(fp);
+ #else
+ BIO_free(bio);
+ #endif
#if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
if (getenv("PGSSLKEY"))
***************
*** 641,647 ****
SSLerrfree(err);
free(engine_str);
return 0;
! }
*pkey = ENGINE_load_private_key(engine_ptr, engine_colon + 1,
NULL, NULL);
--- 670,676 ----
SSLerrfree(err);
free(engine_str);
return 0;
! }
*pkey = ENGINE_load_private_key(engine_ptr, engine_colon + 1,
NULL, NULL);
***************
*** 655,661 ****
SSLerrfree(err);
free(engine_str);
return 0;
! }
free(engine_str);
}
else
--- 684,690 ----
SSLerrfree(err);
free(engine_str);
return 0;
! }
free(engine_str);
}
else
***************
*** 679,686 ****
fnbuf);
return 0;
}
! #endif
if ((fp = fopen(fnbuf, "r")) == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not open private key file \"%s\": %s\n"),
--- 708,718 ----
fnbuf);
return 0;
}
!
if ((fp = fopen(fnbuf, "r")) == NULL)
+ #else
+ if ((bio = BIO_new_file(fnbuf, "r")) == NULL)
+ #endif
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not open private key file \"%s\": %s\n"),
***************
*** 695,702 ****
libpq_gettext("private key file \"%s\" changed during execution\n"), fnbuf);
return 0;
}
! #endif
if (PEM_read_PrivateKey(fp, pkey, NULL, NULL) == NULL)
{
char *err = SSLerrmessage();
--- 727,737 ----
libpq_gettext("private key file \"%s\" changed during execution\n"), fnbuf);
return 0;
}
!
if (PEM_read_PrivateKey(fp, pkey, NULL, NULL) == NULL)
+ #else
+ if (PEM_read_bio_PrivateKey(bio, pkey, NULL, NULL) == NULL)
+ #endif
{
char *err = SSLerrmessage();
***************
*** 704,713 ****
--- 739,756 ----
libpq_gettext("could not read private key file \"%s\": %s\n"),
fnbuf, err);
SSLerrfree(err);
+ #ifndef WIN32
fclose(fp);
+ #else
+ BIO_free(bio);
+ #endif
return 0;
}
+ #ifndef WIN32
fclose(fp);
+ #else
+ BIO_free(bio);
+ #endif
}
/* verify that the cert and key go together */
Dave Page wrote: > Dave Page wrote: >> Dave Page wrote: >>> I did stumble across this text on a mailing list in response to someone >>> with a similar problem in some JNI code. I know little of the OpenSSL >>> API, but perhaps it rings bells with you before I spend my evening >>> trying to figure it out? >> OK, I think I've figured out a fix. Working up a patch now... > > Patch attached. (sorry, been offline for the day) Is there any reason not to just do this on *all* platforms, and get rid of all the #ifdefs? The new code actually seems cleaner to me than what we did before, really... Since it lets OpenSSL do all the work for it. > It appears to work fine except that if the client certificate is > missing, instead of: > > could not open certificate file "C:\Documents and > Settings\Dave\Application Data/postgresql/postgresql.crt": No such file > or directory > > I get: > > Error connecting to the server: SSL SYSCALL error: Operation would block > (0x00002733/10035) > > for reasons that are not clear to me. Any ideas? I wonder if it might be related to our socket/signal emulation stuff. I'd be interested to see what happens with the same code on Unix, but sorry, don't have time to test myself - will be offline again tomorrow :-( //Magnus
Magnus Hagander wrote: > Dave Page wrote: >> Dave Page wrote: >>> Dave Page wrote: >>>> I did stumble across this text on a mailing list in response to someone >>>> with a similar problem in some JNI code. I know little of the OpenSSL >>>> API, but perhaps it rings bells with you before I spend my evening >>>> trying to figure it out? >>> OK, I think I've figured out a fix. Working up a patch now... >> Patch attached. > > (sorry, been offline for the day) > > Is there any reason not to just do this on *all* platforms, and get rid > of all the #ifdefs? Yes, (see the comment in the code). We stat the private key on *nix to ensure it hasn't changed underneath us which can't be done using the BIO functions... though I wonder if we can get the FILE pointer from BIO and do it that way. Should be as safe on *nix as what we currently do. > I wonder if it might be related to our socket/signal emulation stuff. > I'd be interested to see what happens with the same code on Unix, but > sorry, don't have time to test myself - will be offline again tomorrow :-( NP. /D
Dave Page wrote: > Magnus Hagander wrote: >> Dave Page wrote: >>> Dave Page wrote: >>>> Dave Page wrote: >>>>> I did stumble across this text on a mailing list in response to someone >>>>> with a similar problem in some JNI code. I know little of the OpenSSL >>>>> API, but perhaps it rings bells with you before I spend my evening >>>>> trying to figure it out? >>>> OK, I think I've figured out a fix. Working up a patch now... >>> Patch attached. >> (sorry, been offline for the day) >> >> Is there any reason not to just do this on *all* platforms, and get rid >> of all the #ifdefs? > > Yes, (see the comment in the code). We stat the private key on *nix to > ensure it hasn't changed underneath us which can't be done using the BIO > functions... though I wonder if we can get the FILE pointer from BIO and > do it that way. Should be as safe on *nix as what we currently do. Hrrm. Obviously, I need to go sleep now. Sorry about that. But it'd be nice to get rid of all those #ifdef blocks.. //Magnus
Magnus Hagander wrote:
> Hrrm. Obviously, I need to go sleep now. Sorry about that.
>
> But it'd be nice to get rid of all those #ifdef blocks..
See the attached revision. This is untested as I don't have a linux box
to hand, but I believe it's right.
/D
Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.94
diff -c -r1.94 fe-secure.c
*** src/interfaces/libpq/fe-secure.c 16 Feb 2007 17:07:00 -0000 1.94
--- src/interfaces/libpq/fe-secure.c 28 Sep 2007 21:13:18 -0000
***************
*** 111,116 ****
--- 111,117 ----
#ifdef USE_SSL
#include <openssl/ssl.h>
+ #include <openssl/bio.h>
#if (SSLEAY_VERSION_NUMBER >= 0x00907000L)
#include <openssl/conf.h>
#endif
***************
*** 579,586 ****
struct stat buf2;
#endif
char fnbuf[MAXPGPATH];
! FILE *fp;
! PGconn *conn = (PGconn *) SSL_get_app_data(ssl);
char sebuf[256];
if (!pqGetHomeDirectory(homedir, sizeof(homedir)))
--- 580,588 ----
struct stat buf2;
#endif
char fnbuf[MAXPGPATH];
! FILE *fp;
! BIO *bio;
! PGconn *conn = (PGconn *) SSL_get_app_data(ssl);
char sebuf[256];
if (!pqGetHomeDirectory(homedir, sizeof(homedir)))
***************
*** 592,605 ****
/* read the user certificate */
snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE);
! if ((fp = fopen(fnbuf, "r")) == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not open certificate file \"%s\": %s\n"),
fnbuf, pqStrerror(errno, sebuf, sizeof(sebuf)));
return 0;
}
! if (PEM_read_X509(fp, x509, NULL, NULL) == NULL)
{
char *err = SSLerrmessage();
--- 594,608 ----
/* read the user certificate */
snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE);
! if ((bio = BIO_new_file(fnbuf, "r")) == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not open certificate file \"%s\": %s\n"),
fnbuf, pqStrerror(errno, sebuf, sizeof(sebuf)));
return 0;
}
!
! if (PEM_read_bio_X509(bio, x509, NULL, NULL) == NULL)
{
char *err = SSLerrmessage();
***************
*** 607,616 ****
libpq_gettext("could not read certificate file \"%s\": %s\n"),
fnbuf, err);
SSLerrfree(err);
! fclose(fp);
return 0;
}
! fclose(fp);
#if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
if (getenv("PGSSLKEY"))
--- 610,620 ----
libpq_gettext("could not read certificate file \"%s\": %s\n"),
fnbuf, err);
SSLerrfree(err);
! BIO_free(bio);
return 0;
}
!
! BIO_free(bio);
#if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
if (getenv("PGSSLKEY"))
***************
*** 641,647 ****
SSLerrfree(err);
free(engine_str);
return 0;
! }
*pkey = ENGINE_load_private_key(engine_ptr, engine_colon + 1,
NULL, NULL);
--- 645,651 ----
SSLerrfree(err);
free(engine_str);
return 0;
! }
*pkey = ENGINE_load_private_key(engine_ptr, engine_colon + 1,
NULL, NULL);
***************
*** 655,661 ****
SSLerrfree(err);
free(engine_str);
return 0;
! }
free(engine_str);
}
else
--- 659,665 ----
SSLerrfree(err);
free(engine_str);
return 0;
! }
free(engine_str);
}
else
***************
*** 679,686 ****
fnbuf);
return 0;
}
! #endif
! if ((fp = fopen(fnbuf, "r")) == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not open private key file \"%s\": %s\n"),
--- 683,690 ----
fnbuf);
return 0;
}
!
! if ((bio = BIO_new_file(fnbuf, "r")) == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not open private key file \"%s\": %s\n"),
***************
*** 688,693 ****
--- 692,698 ----
return 0;
}
#ifndef WIN32
+ BIO_get_fp(bio, &fp);
if (fstat(fileno(fp), &buf2) == -1 ||
buf.st_dev != buf2.st_dev || buf.st_ino != buf2.st_ino)
{
***************
*** 696,702 ****
return 0;
}
#endif
! if (PEM_read_PrivateKey(fp, pkey, NULL, NULL) == NULL)
{
char *err = SSLerrmessage();
--- 701,708 ----
return 0;
}
#endif
!
! if (PEM_read_bio_PrivateKey(bio, pkey, NULL, NULL) == NULL)
{
char *err = SSLerrmessage();
***************
*** 704,713 ****
libpq_gettext("could not read private key file \"%s\": %s\n"),
fnbuf, err);
SSLerrfree(err);
! fclose(fp);
return 0;
}
! fclose(fp);
}
/* verify that the cert and key go together */
--- 710,721 ----
libpq_gettext("could not read private key file \"%s\": %s\n"),
fnbuf, err);
SSLerrfree(err);
!
! BIO_free(bio);
return 0;
}
!
! BIO_free(bio);
}
/* verify that the cert and key go together */
Dave Page wrote:
> Magnus Hagander wrote:
>> Hrrm. Obviously, I need to go sleep now. Sorry about that.
>>
>> But it'd be nice to get rid of all those #ifdef blocks..
>
> See the attached revision. This is untested as I don't have a linux box
> to hand, but I believe it's right.
Ignore that - I managed to break it :-(. Here's a corrected version.
/D
Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.94
diff -c -r1.94 fe-secure.c
*** src/interfaces/libpq/fe-secure.c 16 Feb 2007 17:07:00 -0000 1.94
--- src/interfaces/libpq/fe-secure.c 28 Sep 2007 21:33:46 -0000
***************
*** 111,116 ****
--- 111,117 ----
#ifdef USE_SSL
#include <openssl/ssl.h>
+ #include <openssl/bio.h>
#if (SSLEAY_VERSION_NUMBER >= 0x00907000L)
#include <openssl/conf.h>
#endif
***************
*** 579,586 ****
struct stat buf2;
#endif
char fnbuf[MAXPGPATH];
! FILE *fp;
! PGconn *conn = (PGconn *) SSL_get_app_data(ssl);
char sebuf[256];
if (!pqGetHomeDirectory(homedir, sizeof(homedir)))
--- 580,588 ----
struct stat buf2;
#endif
char fnbuf[MAXPGPATH];
! FILE *fp;
! BIO *bio;
! PGconn *conn = (PGconn *) SSL_get_app_data(ssl);
char sebuf[256];
if (!pqGetHomeDirectory(homedir, sizeof(homedir)))
***************
*** 592,605 ****
/* read the user certificate */
snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE);
! if ((fp = fopen(fnbuf, "r")) == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not open certificate file \"%s\": %s\n"),
fnbuf, pqStrerror(errno, sebuf, sizeof(sebuf)));
return 0;
}
! if (PEM_read_X509(fp, x509, NULL, NULL) == NULL)
{
char *err = SSLerrmessage();
--- 594,608 ----
/* read the user certificate */
snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE);
! if ((bio = BIO_new_file(fnbuf, "r")) == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not open certificate file \"%s\": %s\n"),
fnbuf, pqStrerror(errno, sebuf, sizeof(sebuf)));
return 0;
}
!
! if (PEM_read_bio_X509(bio, x509, NULL, NULL) == NULL)
{
char *err = SSLerrmessage();
***************
*** 607,616 ****
libpq_gettext("could not read certificate file \"%s\": %s\n"),
fnbuf, err);
SSLerrfree(err);
! fclose(fp);
return 0;
}
! fclose(fp);
#if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
if (getenv("PGSSLKEY"))
--- 610,620 ----
libpq_gettext("could not read certificate file \"%s\": %s\n"),
fnbuf, err);
SSLerrfree(err);
! BIO_free(bio);
return 0;
}
!
! BIO_free(bio);
#if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
if (getenv("PGSSLKEY"))
***************
*** 641,647 ****
SSLerrfree(err);
free(engine_str);
return 0;
! }
*pkey = ENGINE_load_private_key(engine_ptr, engine_colon + 1,
NULL, NULL);
--- 645,651 ----
SSLerrfree(err);
free(engine_str);
return 0;
! }
*pkey = ENGINE_load_private_key(engine_ptr, engine_colon + 1,
NULL, NULL);
***************
*** 655,661 ****
SSLerrfree(err);
free(engine_str);
return 0;
! }
free(engine_str);
}
else
--- 659,665 ----
SSLerrfree(err);
free(engine_str);
return 0;
! }
free(engine_str);
}
else
***************
*** 680,686 ****
return 0;
}
#endif
! if ((fp = fopen(fnbuf, "r")) == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not open private key file \"%s\": %s\n"),
--- 684,691 ----
return 0;
}
#endif
!
! if ((bio = BIO_new_file(fnbuf, "r")) == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not open private key file \"%s\": %s\n"),
***************
*** 688,693 ****
--- 693,699 ----
return 0;
}
#ifndef WIN32
+ BIO_get_fp(bio, &fp);
if (fstat(fileno(fp), &buf2) == -1 ||
buf.st_dev != buf2.st_dev || buf.st_ino != buf2.st_ino)
{
***************
*** 696,702 ****
return 0;
}
#endif
! if (PEM_read_PrivateKey(fp, pkey, NULL, NULL) == NULL)
{
char *err = SSLerrmessage();
--- 702,709 ----
return 0;
}
#endif
!
! if (PEM_read_bio_PrivateKey(bio, pkey, NULL, NULL) == NULL)
{
char *err = SSLerrmessage();
***************
*** 704,713 ****
libpq_gettext("could not read private key file \"%s\": %s\n"),
fnbuf, err);
SSLerrfree(err);
! fclose(fp);
return 0;
}
! fclose(fp);
}
/* verify that the cert and key go together */
--- 711,722 ----
libpq_gettext("could not read private key file \"%s\": %s\n"),
fnbuf, err);
SSLerrfree(err);
!
! BIO_free(bio);
return 0;
}
!
! BIO_free(bio);
}
/* verify that the cert and key go together */
Dave Page <dpage@postgresql.org> writes: > Magnus Hagander wrote: >> Is there any reason not to just do this on *all* platforms, and get rid >> of all the #ifdefs? > Yes, (see the comment in the code). We stat the private key on *nix to > ensure it hasn't changed underneath us which can't be done using the BIO > functions... though I wonder if we can get the FILE pointer from BIO and > do it that way. Should be as safe on *nix as what we currently do. Perhaps you could still open the file yourself, and use BIO_new_fp() instead of BIO_new_file()? I'm not getting responses from openssl.org at the moment, but here's another copy of the relevant man page: http://developer.apple.com/documentation/Darwin/Reference/Manpages/man3/BIO_s_file.3ssl.html I concur with Magnus that it'll be better if there's not two code paths here. It's not entirely clear whether BIO_new_fp() would avoid the problematic calls, but it doesn't look like it'd be hard to try. regards, tom lane
> ------- Original Message ------- > From: Tom Lane <tgl@sss.pgh.pa.us> > To: Dave Page <dpage@postgresql.org> > Sent: 29/09/07, 01:28:09 > Subject: Re: [PATCHES] OpenSSL Applink > > I concur with Magnus that it'll be better if there's not two code paths > here. It's not entirely clear whether BIO_new_fp() would avoid the > problematic calls, but it doesn't look like it'd be hard to try. The last version of the patch I posted uses BIO_new_file() in all cases, and (from memory) BIO_get_fp() in the non-win32case to get a FILE* to pass to fstat. I believe the problem is sharing FILE*'s between differing runtime versions used by openssl and the app. Coding it with BIO_new_filemakes it less tempting for future changes to start mucking about with FILE*'s on Windows. Note that I've been away from my *nix boxes so I haven't tested the patch except on Windows yet. /D
"Dave Page" <dpage@postgresql.org> writes:
>> From: Tom Lane <tgl@sss.pgh.pa.us>
>> ... It's not entirely clear whether BIO_new_fp() would avoid the
>> problematic calls, but it doesn't look like it'd be hard to try.
> The last version of the patch I posted uses BIO_new_file() in all cases, and (from memory) BIO_get_fp() in the
non-win32case to get a FILE* to pass to fstat.
Did you manage to get rid of the bogus-error-message problem that
afflicted the first version of the patch? If so, this way is fine.
If not, keeping control of file-opening in our own hands might be
a way to dodge that.
regards, tom lane
Tom Lane wrote: > "Dave Page" <dpage@postgresql.org> writes: >>> From: Tom Lane <tgl@sss.pgh.pa.us> >>> ... It's not entirely clear whether BIO_new_fp() would avoid the >>> problematic calls, but it doesn't look like it'd be hard to try. > >> The last version of the patch I posted uses BIO_new_file() in all cases, and (from memory) BIO_get_fp() in the non-win32case to get a FILE* to pass to fstat. > > Did you manage to get rid of the bogus-error-message problem that > afflicted the first version of the patch? If so, this way is fine. No, thats still an issue. > If not, keeping control of file-opening in our own hands might be > a way to dodge that. That doesn't work because we still end up passing FILE pointers between potentially differing runtime builds/versions) which OpenSSL then detects and bleats about including OPENSSL_Applink(). /D
On Sat, Sep 29, 2007 at 09:01:16PM +0100, Dave Page wrote: > Tom Lane wrote: > > "Dave Page" <dpage@postgresql.org> writes: > >>> From: Tom Lane <tgl@sss.pgh.pa.us> > >>> ... It's not entirely clear whether BIO_new_fp() would avoid the > >>> problematic calls, but it doesn't look like it'd be hard to try. > > > >> The last version of the patch I posted uses BIO_new_file() in all cases, and (from memory) BIO_get_fp() in the non-win32case to get a FILE* to pass to fstat. > > > > Did you manage to get rid of the bogus-error-message problem that > > afflicted the first version of the patch? If so, this way is fine. > > No, thats still an issue. A guess on this - probably the BIO stuff overwrites some internal OpenSSL "errno" value, causing the wrong error to be passed up. Most likely, it's not save to call BIO functions from inside the callback. My bet is that it'll actually break without this patch, if you stick something that's invalid in there. It's just taht we picked up the "does not exist" error without calling BIO functions. A quick peek at the OpenSSL sources seems to confirm this. I think we want to either attempt to load the client certificate before we connect (and before it's requested) and just queue up the error to show it in only if it's requested, or we want to try some magic around ERR_set_mark()/ERR_pop_to_mark() to clear out any BIO errors before we hand control back. I'll see if I can put together a poc patch - need to reproduce the problem first :-) //Magnus
On Mon, Oct 01, 2007 at 02:37:44PM +0200, Magnus Hagander wrote: > On Sat, Sep 29, 2007 at 09:01:16PM +0100, Dave Page wrote: > > Tom Lane wrote: > > > "Dave Page" <dpage@postgresql.org> writes: > > >>> From: Tom Lane <tgl@sss.pgh.pa.us> > > >>> ... It's not entirely clear whether BIO_new_fp() would avoid the > > >>> problematic calls, but it doesn't look like it'd be hard to try. > > > > > >> The last version of the patch I posted uses BIO_new_file() in all cases, and (from memory) BIO_get_fp() in the non-win32case to get a FILE* to pass to fstat. > > > > > > Did you manage to get rid of the bogus-error-message problem that > > > afflicted the first version of the patch? If so, this way is fine. > > > > No, thats still an issue. > > A guess on this - probably the BIO stuff overwrites some internal OpenSSL > "errno" value, causing the wrong error to be passed up. Most likely, it's > not save to call BIO functions from inside the callback. My bet is that > it'll actually break without this patch, if you stick something that's > invalid in there. It's just taht we picked up the "does not exist" error > without calling BIO functions. > > A quick peek at the OpenSSL sources seems to confirm this. > > I think we want to either attempt to load the client certificate before we > connect (and before it's requested) and just queue up the error to show it > in only if it's requested, or we want to try some magic around > ERR_set_mark()/ERR_pop_to_mark() to clear out any BIO errors before we hand > control back. > > I'll see if I can put together a poc patch - need to reproduce the problem > first :-) Just a quick followup - this is also reproducible on Unix: mha@builder:~/inst-pg/head/bin$ PGSSLMODE=require ./psql -h localhost postgres psql: SSL SYSCALL error: Resource temporarily unavailable //Magnus
On Mon, Oct 01, 2007 at 03:16:13PM +0200, Magnus Hagander wrote: > On Mon, Oct 01, 2007 at 02:37:44PM +0200, Magnus Hagander wrote: > > On Sat, Sep 29, 2007 at 09:01:16PM +0100, Dave Page wrote: > > > Tom Lane wrote: > > > > "Dave Page" <dpage@postgresql.org> writes: > > > >>> From: Tom Lane <tgl@sss.pgh.pa.us> > > > >>> ... It's not entirely clear whether BIO_new_fp() would avoid the > > > >>> problematic calls, but it doesn't look like it'd be hard to try. > > > > > > > >> The last version of the patch I posted uses BIO_new_file() in all cases, and (from memory) BIO_get_fp() in the non-win32case to get a FILE* to pass to fstat. > > > > > > > > Did you manage to get rid of the bogus-error-message problem that > > > > afflicted the first version of the patch? If so, this way is fine. > > > > > > No, thats still an issue. > > > > A guess on this - probably the BIO stuff overwrites some internal OpenSSL > > "errno" value, causing the wrong error to be passed up. Most likely, it's > > not save to call BIO functions from inside the callback. My bet is that > > it'll actually break without this patch, if you stick something that's > > invalid in there. It's just taht we picked up the "does not exist" error > > without calling BIO functions. > > > > A quick peek at the OpenSSL sources seems to confirm this. > > > > I think we want to either attempt to load the client certificate before we > > connect (and before it's requested) and just queue up the error to show it > > in only if it's requested, or we want to try some magic around > > ERR_set_mark()/ERR_pop_to_mark() to clear out any BIO errors before we hand > > control back. > > > > I'll see if I can put together a poc patch - need to reproduce the problem > > first :-) Yes, that was the problem. Attached patch fixes the problem for me on Windows and Linux using the error mark functionality. It seems a lot cleaner than the other option. Dave - can you test this one? Assuming that works, I'll go ahead and apply it. I also think we have the same problem in the current code, just not for fopen(). We trap fopen() without anything happening inside OpenSSL, but if we get an error in the OpenSSL functions we call, something similar would likely happen. Should we backpatch all the stuff, or just the error push/pop thing? //Magnus
Attachment
Magnus Hagander wrote: > Yes, that was the problem. Attached patch fixes the problem for me on > Windows and Linux using the error mark functionality. It seems a lot > cleaner than the other option. > > Dave - can you test this one? Assuming that works, I'll go ahead and apply > it. Yep, looks good here. Thanks. > I also think we have the same problem in the current code, just not for > fopen(). We trap fopen() without anything happening inside OpenSSL, but if > we get an error in the OpenSSL functions we call, something similar would > likely happen. > > Should we backpatch all the stuff, or just the error push/pop thing? All of it. Anyone building just libpq/psql using the old MSVC makefiles will probably see the applink problem if they try to use client certs. /D
Dave Page <dpage@postgresql.org> writes:
> Magnus Hagander wrote:
>> Should we backpatch all the stuff, or just the error push/pop thing?
> All of it. Anyone building just libpq/psql using the old MSVC makefiles
> will probably see the applink problem if they try to use client certs.
I'd vote for backpatching, but only as far as 8.2, seeing that we're
abandoning the older branches on Windows.
regards, tom lane
On Mon, Oct 01, 2007 at 10:51:01AM -0400, Tom Lane wrote: > Dave Page <dpage@postgresql.org> writes: > > Magnus Hagander wrote: > >> Should we backpatch all the stuff, or just the error push/pop thing? > > > All of it. Anyone building just libpq/psql using the old MSVC makefiles > > will probably see the applink problem if they try to use client certs. > > I'd vote for backpatching, but only as far as 8.2, seeing that we're > abandoning the older branches on Windows. Should we backpatch a version of it to previous versions that does just the error stack manipulation, or just ignore on the grounds that nobody has reported the problem there (it's there, but it's a lot more narrow - I know it's there, but I havent' been able to provoket it due to not knowing enough about setting up certificate chains and such) //Magnus
Magnus Hagander <magnus@hagander.net> writes:
> On Mon, Oct 01, 2007 at 10:51:01AM -0400, Tom Lane wrote:
>> I'd vote for backpatching, but only as far as 8.2, seeing that we're
>> abandoning the older branches on Windows.
> Should we backpatch a version of it to previous versions that does just the
> error stack manipulation, or just ignore on the grounds that nobody has
> reported the problem there (it's there, but it's a lot more narrow - I know
> it's there, but I havent' been able to provoket it due to not knowing
> enough about setting up certificate chains and such)
That's only a cosmetic problem (wrong error message), right? I'd vote
for no backpatch, at least for now --- I'd rather see this change get
through beta testing first. Anytime you're fooling with interactions
with some other package, the risk of portability issues is high.
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> On Mon, Oct 01, 2007 at 10:51:01AM -0400, Tom Lane wrote: >>> I'd vote for backpatching, but only as far as 8.2, seeing that we're >>> abandoning the older branches on Windows. > >> Should we backpatch a version of it to previous versions that does just the >> error stack manipulation, or just ignore on the grounds that nobody has >> reported the problem there (it's there, but it's a lot more narrow - I know >> it's there, but I havent' been able to provoket it due to not knowing >> enough about setting up certificate chains and such) > > That's only a cosmetic problem (wrong error message), right? I'd vote > for no backpatch, at least for now --- I'd rather see this change get > through beta testing first. Anytime you're fooling with interactions > with some other package, the risk of portability issues is high. Right. Applied to HEAD. Will wait for a buildfarm cycle to make sure it doesn't kill anything else then try to backport to 8.2 (patch didn't apply cleanly to it) //Magnus
Magnus Hagander wrote: > Tom Lane wrote: >> Magnus Hagander <magnus@hagander.net> writes: >>> On Mon, Oct 01, 2007 at 10:51:01AM -0400, Tom Lane wrote: >>>> I'd vote for backpatching, but only as far as 8.2, seeing that we're >>>> abandoning the older branches on Windows. >>> Should we backpatch a version of it to previous versions that does just the >>> error stack manipulation, or just ignore on the grounds that nobody has >>> reported the problem there (it's there, but it's a lot more narrow - I know >>> it's there, but I havent' been able to provoket it due to not knowing >>> enough about setting up certificate chains and such) >> That's only a cosmetic problem (wrong error message), right? I'd vote >> for no backpatch, at least for now --- I'd rather see this change get >> through beta testing first. Anytime you're fooling with interactions >> with some other package, the risk of portability issues is high. > > Right. > Applied to HEAD. Will wait for a buildfarm cycle to make sure it doesn't > kill anything else then try to backport to 8.2 (patch didn't apply > cleanly to it) I guess you guys already found a solution that works, but there's yet another function, "BIO *BIO_new_mem_buf(void *data, int len)", that we could use. We could open and read the file all by ourselves into memory, then call BIO_new_mem_buf and pass that to PEM_read_X509. No need to pass around file pointers, and we could handle any file I/O errors ourselves. Presumably certificates are never very big, so reading it all in memory shouldn't be a problem. BIO_new_mem_buf was introduced in OpenSSL 0.9.7. What versions do we support? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki@enterprisedb.com> writes: > I guess you guys already found a solution that works, but there's yet > another function, "BIO *BIO_new_mem_buf(void *data, int len)", that we > could use. We could open and read the file all by ourselves into memory, > then call BIO_new_mem_buf and pass that to PEM_read_X509. No need to > pass around file pointers, and we could handle any file I/O errors > ourselves. Presumably certificates are never very big, so reading it all > in memory shouldn't be a problem. > BIO_new_mem_buf was introduced in OpenSSL 0.9.7. What versions do we > support? This seems like a good idea. To judge from the release history at http://www.openssl.org/news/ the OpenSSL boys stopped supporting 0.9.6 in 2004, so I figure we don't have to support it either. But 0.9.7 is still a live release branch, so it'd be good if we could play nice with it. http://www.openssl.org/docs/crypto/BIO_s_mem.html regards, tom lane
Tom Lane wrote: > Heikki Linnakangas <heikki@enterprisedb.com> writes: >> I guess you guys already found a solution that works, but there's yet >> another function, "BIO *BIO_new_mem_buf(void *data, int len)", that we >> could use. We could open and read the file all by ourselves into memory, >> then call BIO_new_mem_buf and pass that to PEM_read_X509. No need to >> pass around file pointers, and we could handle any file I/O errors >> ourselves. Presumably certificates are never very big, so reading it all >> in memory shouldn't be a problem. > >> BIO_new_mem_buf was introduced in OpenSSL 0.9.7. What versions do we >> support? > > This seems like a good idea. To judge from the release history at > http://www.openssl.org/news/ > the OpenSSL boys stopped supporting 0.9.6 in 2004, so I figure we > don't have to support it either. But 0.9.7 is still a live release > branch, so it'd be good if we could play nice with it. > > http://www.openssl.org/docs/crypto/BIO_s_mem.html Not sure it buys us much more than we already have. Sure, we can handle file I/O. But we still can't handle errors in the parsing of the file. I think that if we can open the file, then the chance that we fail to read it is extremely small, compared to the chance that we get a parsing error. //Magnus