Thread: BUG #4869: No proper initialization of OpenSSL-Engine in libpq
The following bug has been logged online: Bug reference: 4869 Logged by: Lars Kanis Email address: kanis@comcard.de PostgreSQL version: 8.4rc1 Operating system: Linux c1170lx 2.6.24-23-generic #1 SMP Wed Apr 1 21:47:28 UTC 2009 i686 GNU/Linux Description: No proper initialization of OpenSSL-Engine in libpq Details: When using OpenSSL-engine pkcs11 with PGSSLKEY=pkcs11:id_45 the authentication to the PG-server fails with "engine not initialized". According to the OpenSSL-docs (http://www.openssl.org/docs/crypto/engine.html) the structural reference returned by ENGINE_by_id needs to be initialized first before use. The buildin engine doesn't need this, but most of external engines don't work otherwise. Moreover the structural and functional references should be freed in any case. The following patch solves the problem: diff -ur postgresql-8.4rc1.orig/src/interfaces/libpq/fe-secure.c postgresql-8.4rc1/src/interfaces/libpq/fe-secure.c --- postgresql-8.4rc1.orig/src/interfaces/libpq/fe-secure.c 2009-06-11 16:49:14.000000000 +0200 +++ postgresql-8.4rc1/src/interfaces/libpq/fe-secure.c 2009-06-22 10:56:38.000000000 +0200 @@ -689,6 +689,20 @@ ERR_pop_to_mark(); return 0; } + + if (ENGINE_init(engine_ptr) == 0) + { + char *err = SSLerrmessage(); + + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("could not initialize SSL engine \"%s\": %s\n"), + engine_str, err); + SSLerrfree(err); + ENGINE_free(engine_ptr); + free(engine_str); + ERR_pop_to_mark(); + return 0; + } *pkey = ENGINE_load_private_key(engine_ptr, engine_colon, NULL, NULL); @@ -700,6 +714,8 @@ libpq_gettext("could not read private SSL key \"%s\" from engine \"%s\": %s\n"), engine_colon, engine_str, err); SSLerrfree(err); + ENGINE_finish(engine_ptr); + ENGINE_free(engine_ptr); free(engine_str); ERR_pop_to_mark(); return 0;
Lars Kanis wrote: > The following bug has been logged online: > > Bug reference: 4869 > Logged by: Lars Kanis > Email address: kanis@comcard.de > PostgreSQL version: 8.4rc1 > Operating system: Linux c1170lx 2.6.24-23-generic #1 SMP Wed Apr 1 > 21:47:28 UTC 2009 i686 GNU/Linux > Description: No proper initialization of OpenSSL-Engine in libpq > Details: > > When using OpenSSL-engine pkcs11 with PGSSLKEY=pkcs11:id_45 the > authentication to the PG-server fails with "engine not initialized". > > According to the OpenSSL-docs > (http://www.openssl.org/docs/crypto/engine.html) the structural reference > returned by ENGINE_by_id needs to be initialized first before use. The > buildin engine doesn't need this, but most of external engines don't work > otherwise. > > Moreover the structural and functional references should be freed in any > case. > > > The following patch solves the problem: This looks good in generael to me. I remember looking at the engine code wondering why we didn't do that, but since I don't have a good environment to test that part in, I forgot about it :( Shouldn't there be an ENGINE_free() in the error path of ENGINE_init()? Should we not also call ENGINE_finish() and ENGINE_free() in the success path of this code? Your patch adds it to the case where we didn't get the private key, but what if we did? I assume they should also go outside the error path, per the attached patch - or will that break their usage? Can you test that and verify that it doesn't break for you? -- Magnus Hagander Self: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c index bfc0965..53ac526 100644 --- a/src/interfaces/libpq/fe-secure.c +++ b/src/interfaces/libpq/fe-secure.c @@ -690,6 +690,20 @@ client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) return 0; } + if (ENGINE_init(engine_ptr) == 0) + { + char *err = SSLerrmessage(); + + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("could not initialize SSL engine \"%s\": %s\n"), + engine_str, err); + SSLerrfree(err); + ENGINE_free(engine_ptr); + free(engine_str); + ERR_pop_to_mark(); + return 0; + } + *pkey = ENGINE_load_private_key(engine_ptr, engine_colon, NULL, NULL); if (*pkey == NULL) @@ -700,10 +714,14 @@ client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) libpq_gettext("could not read private SSL key \"%s\" from engine \"%s\": %s\n"), engine_colon, engine_str, err); SSLerrfree(err); + ENGINE_finish(engine_ptr); + ENGINE_free(engine_ptr); free(engine_str); ERR_pop_to_mark(); return 0; } + ENGINE_finish(engine_ptr); + ENGINE_free(engine_ptr); free(engine_str); fnbuf[0] = '\0'; /* indicate we're not going to load from a
Lars Kanis wrote: >>> The following patch solves the problem: >> This looks good in generael to me. I remember looking at the engine code >> wondering why we didn't do that, but since I don't have a good >> environment to test that part in, I forgot about it :( >> >> Shouldn't there be an ENGINE_free() in the error path of ENGINE_init()? > In the patch it is already there, isn't it? Eh. So it is. Don't know where I got the idea that it didn't :-) >> Should we not also call ENGINE_finish() and ENGINE_free() in the success >> path of this code? Your patch adds it to the case where we didn't get >> the private key, but what if we did? I assume they should also go >> outside the error path, per the attached patch - or will that break >> their usage? > That's right. I thought about it, but I don't know where the right place is. > >> Can you test that and verify that it doesn't break for you? > It breaks with Segmentation fault in my smartcard-based setup. The pkey-handle > is all we have from the engine, when client_cert_cb() is finished. Obviously > the ref-counting of openssl does not take the pkey-handle into account, so we > need to keep the engine_ptr for later freeing. So we need to keep the engine initialized during this time? Ugh. We don't currently carry around the engine pointer. I guess we have to. > close_SSL() should be the right place for ENGINE_finish() and ENGINE_free() ? Yup. How about the attached patch? Does it work for you? A question from that then, for others, is it Ok to add a field to the PGconn structure during RC? :-) It's only in libpq-int.h, but? Comments? Tom, perhaps? -- Magnus Hagander Self: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ *** a/src/interfaces/libpq/fe-secure.c --- b/src/interfaces/libpq/fe-secure.c *************** *** 669,683 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) ) { /* Colon, but not in second character, treat as engine:key */ - ENGINE *engine_ptr; char *engine_str = strdup(conn->sslkey); char *engine_colon = strchr(engine_str, ':'); *engine_colon = '\0'; /* engine_str now has engine name */ engine_colon++; /* engine_colon now has key name */ ! engine_ptr = ENGINE_by_id(engine_str); ! if (engine_ptr == NULL) { char *err = SSLerrmessage(); --- 669,682 ---- ) { /* Colon, but not in second character, treat as engine:key */ char *engine_str = strdup(conn->sslkey); char *engine_colon = strchr(engine_str, ':'); *engine_colon = '\0'; /* engine_str now has engine name */ engine_colon++; /* engine_colon now has key name */ ! conn->engine = ENGINE_by_id(engine_str); ! if (conn->engine == NULL) { char *err = SSLerrmessage(); *************** *** 690,696 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) return 0; } ! *pkey = ENGINE_load_private_key(engine_ptr, engine_colon, NULL, NULL); if (*pkey == NULL) { --- 689,710 ---- return 0; } ! if (ENGINE_init(conn->engine) == 0) ! { ! char *err = SSLerrmessage(); ! ! printfPQExpBuffer(&conn->errorMessage, ! libpq_gettext("could not initialize SSL engine \"%s\": %s\n"), ! engine_str, err); ! SSLerrfree(err); ! ENGINE_free(conn->engine); ! conn->engine = NULL; ! free(engine_str); ! ERR_pop_to_mark(); ! return 0; ! } ! ! *pkey = ENGINE_load_private_key(conn->engine, engine_colon, NULL, NULL); if (*pkey == NULL) { *************** *** 700,705 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) --- 714,722 ---- libpq_gettext("could not read private SSL key \"%s\" from engine \"%s\": %s\n"), engine_colon, engine_str, err); SSLerrfree(err); + ENGINE_finish(conn->engine); + ENGINE_free(conn->engine); + conn->engine = NULL; free(engine_str); ERR_pop_to_mark(); return 0; *************** *** 1217,1222 **** close_SSL(PGconn *conn) --- 1234,1246 ---- X509_free(conn->peer); conn->peer = NULL; } + + if (conn->engine) + { + ENGINE_finish(conn->engine); + ENGINE_free(conn->engine); + conn->engine = NULL; + } } /* *** a/src/interfaces/libpq/libpq-int.h --- b/src/interfaces/libpq/libpq-int.h *************** *** 383,388 **** struct pg_conn --- 383,389 ---- X509 *peer; /* X509 cert of server */ char peer_dn[256 + 1]; /* peer distinguished name */ char peer_cn[SM_USER + 1]; /* peer common name */ + ENGINE *engine; /* SSL engine, if any */ #endif #ifdef ENABLE_GSS
Magnus Hagander <magnus@hagander.net> writes:
> A question from that then, for others, is it Ok to add a field to the
> PGconn structure during RC? :-) It's only in libpq-int.h, but? Comments?
Changing PGconn internals doesn't bother me, but ...
IIUC this is a pre-existing bug/limitation in an extremely seldom-used
feature that we don't have any very good way to test. So I'm not really
excited about trying to fix it in RC at all. The chances of breaking
something seem much higher than the usefulness of the fix would warrant.
I'd suggest holding the matter until 8.5 development opens.
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> A question from that then, for others, is it Ok to add a field to the >> PGconn structure during RC? :-) It's only in libpq-int.h, but? Comments? > > Changing PGconn internals doesn't bother me, but ... > > IIUC this is a pre-existing bug/limitation in an extremely seldom-used > feature that we don't have any very good way to test. So I'm not really > excited about trying to fix it in RC at all. The chances of breaking > something seem much higher than the usefulness of the fix would warrant. > > I'd suggest holding the matter until 8.5 development opens. I think we'll see this feature used a lot more now, since we support client certificate authentication. I bet that's the reason why Lars is using it now, but wasn't using it before. Correct, Lars? That would be the argument for doing it now. We previously supported engines for client certificates, but using client certificates at all wasn't very useful in pre-8.4, and that's why it wasn't used almost at all. While I don't expect a huge number of users of it in 8.4, I think it is a *much* more useful feature now, and thus will be used a lot more. -- Magnus Hagander Self: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Am Montag, 22. Juni 2009 16:38:32 schrieben Sie: > Tom Lane wrote: > > Magnus Hagander <magnus@hagander.net> writes: > >> A question from that then, for others, is it Ok to add a field to the > >> PGconn structure during RC? :-) It's only in libpq-int.h, but? Comment= s? > > > > Changing PGconn internals doesn't bother me, but ... > > > > IIUC this is a pre-existing bug/limitation in an extremely seldom-used > > feature that we don't have any very good way to test. So I'm not really > > excited about trying to fix it in RC at all. The chances of breaking > > something seem much higher than the usefulness of the fix would warrant. > > > > I'd suggest holding the matter until 8.5 development opens. > > I think we'll see this feature used a lot more now, since we support > client certificate authentication. I bet that's the reason why Lars is > using it now, but wasn't using it before. Correct, Lars? That's right. Because clientside crypto engines and proper certificate=20 authentication is supported now, I would like to use a strong smartcard-bas= ed=20 login in our high security environment. > That would be the argument for doing it now. We previously supported > engines for client certificates, but using client certificates at all > wasn't very useful in pre-8.4, and that's why it wasn't used almost at > all. While I don't expect a huge number of users of it in 8.4, I think > it is a *much* more useful feature now, and thus will be used a lot more. I could live with the patch during 8.4 cycle. But if we support crypto engi= nes=20 now, we may do it the way that it really works. regards Lars
Am Montag, 22. Juni 2009 15:55:58 schrieben Sie: > Lars Kanis wrote: > >> Should we not also call ENGINE_finish() and ENGINE_free() in the succe= ss > >> path of this code? Your patch adds it to the case where we didn't get > >> the private key, but what if we did? I assume they should also go > >> outside the error path, per the attached patch - or will that break > >> their usage? > > > > That's right. I thought about it, but I don't know where the right place > > is. > > > >> Can you test that and verify that it doesn't break for you? > > > > It breaks with Segmentation fault in my smartcard-based setup. The > > pkey-handle is all we have from the engine, when client_cert_cb() is > > finished. Obviously the ref-counting of openssl does not take the > > pkey-handle into account, so we need to keep the engine_ptr for later > > freeing. > > So we need to keep the engine initialized during this time? Ugh. We > don't currently carry around the engine pointer. I guess we have to. > > > close_SSL() should be the right place for ENGINE_finish() and > > ENGINE_free() ? > > Yup. > > How about the attached patch? Does it work for you? Yes, it works perfect. Now, if I close the connection, the engine is closed= =20 too, so I have to type the PIN another time for a next connection. That=20 should be the correct behaviour.=20 > > A question from that then, for others, is it Ok to add a field to the > PGconn structure during RC? :-) It's only in libpq-int.h, but? Comments? > Tom, perhaps?
Hi Magnus, thanks for reply. > > The following patch solves the problem: > > This looks good in generael to me. I remember looking at the engine code > wondering why we didn't do that, but since I don't have a good > environment to test that part in, I forgot about it :( > > Shouldn't there be an ENGINE_free() in the error path of ENGINE_init()? In the patch it is already there, isn't it? > Should we not also call ENGINE_finish() and ENGINE_free() in the success > path of this code? Your patch adds it to the case where we didn't get > the private key, but what if we did? I assume they should also go > outside the error path, per the attached patch - or will that break > their usage? That's right. I thought about it, but I don't know where the right place is. > Can you test that and verify that it doesn't break for you? It breaks with Segmentation fault in my smartcard-based setup. The pkey-han= dle=20 is all we have from the engine, when client_cert_cb() is finished. Obviousl= y=20 the ref-counting of openssl does not take the pkey-handle into account, so = we=20 need to keep the engine_ptr for later freeing. close_SSL() should be the right place for ENGINE_finish() and ENGINE_free()= ? --=20 Mit freundlichen Gr=C3=BC=C3=9Fen, Lars Kanis Bereichsleiter IT Tel +49 3745 769 -422=20 Fax +49 3745 769 -334=20 eMail: kanis@comcard.de=20 Sie k=C3=B6nnen sich unter http://www.comcard.de unseren Newsletter abonnie= ren! ComCard GmbH Hammerbr=C3=BCcker Stra=C3=9Fe 3 08223 Falkenstein Gesch=C3=A4ftsf=C3=BChrer: Dipl.-Ing. Ralph Siegel Amtgericht Chemnitz HRB 3255 Ust.ID DE811118514
Lars Kanis <kanis@comcard.de> writes:
> Am Montag, 22. Juni 2009 16:38:32 schrieben Sie:
>> Tom Lane wrote:
>>> IIUC this is a pre-existing bug/limitation in an extremely seldom-used
>>> feature that we don't have any very good way to test. So I'm not really
>>> excited about trying to fix it in RC at all. The chances of breaking
>>> something seem much higher than the usefulness of the fix would warrant.
>> I think we'll see this feature used a lot more now, since we support
>> client certificate authentication. I bet that's the reason why Lars is
>> using it now, but wasn't using it before. Correct, Lars?
> That's right. Because clientside crypto engines and proper certificate
> authentication is supported now, I would like to use a strong smartcard-based
> login in our high security environment.
OK, but I'm still worried about making a change of this sort (ie,
modifying our interface to code that we don't control) so late in the
release cycle. It seems like there is large potential for failure in
contexts other than the one or two you are going to be able to test
right now. Is there anything that can be done to reduce the risk?
regards, tom lane
Magnus Hagander <magnus@hagander.net> writes:
> How about the attached patch? Does it work for you?
The existing references to typedef ENGINE and associated functions are
wrapped in
#if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
I rather imagine that this patch will cause complete failure in builds
where that's not true. I'm also a bit concerned about wrapping a struct
field inside such an #if, as that's likely to cause hard-to-debug
problems if two compilations read libpq-int.h with different #define
environments.
regards, tom lane
On 22 jun 2009, at 18.05, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> How about the attached patch? Does it work for you? > > The existing references to typedef ENGINE and associated functions are > wrapped in > > #if (SSLEAY_VERSION_NUMBER >=3D 0x00907000L) && !defined=20 > (OPENSSL_NO_ENGINE) > > I rather imagine that this patch will cause complete failure in builds > where that's not true. Yeah, that will have to be fixed. > I'm also a bit concerned about wrapping a struct > field inside such an #if, as that's likely to cause hard-to-debug > problems if two compilations read libpq-int.h with different #define > environments. > Since it's a pointer we could just declare it as void, no? Or even #if=20= =20 between void and "real" pointer. Wouldn't that be safe? (we already have a similar issue with the whole #ifdef use_ssl, don't=20=20 we? But having one isn't an excuse to create another of course=E2=80=A6) /Magnus
On 22 jun 2009, at 17.46, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Lars Kanis <kanis@comcard.de> writes: >> Am Montag, 22. Juni 2009 16:38:32 schrieben Sie: >>> Tom Lane wrote: >>>> IIUC this is a pre-existing bug/limitation in an extremely seldom- >>>> used >>>> feature that we don't have any very good way to test. So I'm not >>>> really >>>> excited about trying to fix it in RC at all. The chances of >>>> breaking >>>> something seem much higher than the usefulness of the fix would >>>> warrant. > >>> I think we'll see this feature used a lot more now, since we support >>> client certificate authentication. I bet that's the reason why >>> Lars is >>> using it now, but wasn't using it before. Correct, Lars? > >> That's right. Because clientside crypto engines and proper >> certificate >> authentication is supported now, I would like to use a strong >> smartcard-based >> login in our high security environment. > > OK, but I'm still worried about making a change of this sort (ie, > modifying our interface to code that we don't control) so late in the > release cycle. It seems like there is large potential for failure in > contexts other than the one or two you are going to be able to test > right now. Is there anything that can be done to reduce the risk? I share your concerns in general. But I think we nee to take into account that this simply does not work without the patch. So nobody should rely on the previous behaviour - how would their application work there... (I guess there is always a risk I get to eat those words later if someone did, but I don't see the scenario...) in fact, this is a but in an advertised feature in previous versions, so should maybe even consider backpatching it base on that.... /Magnus
Magnus Hagander <magnus@hagander.net> writes:
> On 22 jun 2009, at 18.05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm also a bit concerned about wrapping a struct
>> field inside such an #if, as that's likely to cause hard-to-debug
>> problems if two compilations read libpq-int.h with different #define
>> environments.
> Since it's a pointer we could just declare it as void, no? Or even #if
> between void and "real" pointer. Wouldn't that be safe?
Yeah, on reflection I think it'd be safe to do
#if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
ENGINE *ptr;
#else
/* dummy field to keep struct the same if OpenSSL version changes */
void *ptr;
#endif
(probably that #if should get wrapped up in an additional #define
instead of being duplicated in several places...)
> (we already have a similar issue with the whole #ifdef use_ssl, don't
> we? But having one isn't an excuse to create another of courseâ¦)
That's controlled by an entry in pg_config.h, though, and won't change
if someone happens to update their openssl install and then recompile
only parts of libpq.
regards, tom lane
Magnus Hagander <magnus@hagander.net> writes:
> On 22 jun 2009, at 17.46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It seems like there is large potential for failure in
>> contexts other than the one or two you are going to be able to test
>> right now. Is there anything that can be done to reduce the risk?
> I share your concerns in general. But I think we nee to take into
> account that this simply does not work without the patch.
Frankly, I don't much care whether openssl-with-an-external-engine
works; we never supported that before, and if it doesn't work now,
that's not a regression. What I'm worried about at this stage of the
release cycle is regressions, eg, does the code still compile at all
with openssl version 0.9.random-version-that-used-to-work.
regards, tom lane
Am Montag, 22. Juni 2009 17:46:22 schrieben Sie: > Lars Kanis <kanis@comcard.de> writes: > > Am Montag, 22. Juni 2009 16:38:32 schrieben Sie: > >> Tom Lane wrote: > >>> IIUC this is a pre-existing bug/limitation in an extremely seldom-used > >>> feature that we don't have any very good way to test. So I'm not > >>> really excited about trying to fix it in RC at all. The chances of > >>> breaking something seem much higher than the usefulness of the fix > >>> would warrant. > >> > >> I think we'll see this feature used a lot more now, since we support > >> client certificate authentication. I bet that's the reason why Lars is > >> using it now, but wasn't using it before. Correct, Lars? > > > > That's right. Because clientside crypto engines and proper certificate > > authentication is supported now, I would like to use a strong > > smartcard-based login in our high security environment. > > OK, but I'm still worried about making a change of this sort (ie, > modifying our interface to code that we don't control) so late in the > release cycle. It seems like there is large potential for failure in > contexts other than the one or two you are going to be able to test > right now. Is there anything that can be done to reduce the risk? The 3 different versions of the engine-code seems to me like this: 1. unpatched libpq engines aren't initialized (so some don't work), engines aren't freed at th= e=20 end of connection (memory leak), no changes to PGconn 2. the initial patch I posted engines are initialized (so can work), engines aren't freed (memory leak), = no=20 changes to PGconn only in engine code path 3. the current patch engines are initialized, engines are freed, but problematic changes to PGco= nn Maybe version 2 (my initial patch) could be an alternative ? regards Lars Kanis
Lars Kanis wrote: > Am Montag, 22. Juni 2009 17:46:22 schrieben Sie: >> Lars Kanis <kanis@comcard.de> writes: >>> Am Montag, 22. Juni 2009 16:38:32 schrieben Sie: >>>> Tom Lane wrote: >>>>> IIUC this is a pre-existing bug/limitation in an extremely seldom-used >>>>> feature that we don't have any very good way to test. So I'm not >>>>> really excited about trying to fix it in RC at all. The chances of >>>>> breaking something seem much higher than the usefulness of the fix >>>>> would warrant. >>>> I think we'll see this feature used a lot more now, since we support >>>> client certificate authentication. I bet that's the reason why Lars is >>>> using it now, but wasn't using it before. Correct, Lars? >>> That's right. Because clientside crypto engines and proper certificate >>> authentication is supported now, I would like to use a strong >>> smartcard-based login in our high security environment. >> OK, but I'm still worried about making a change of this sort (ie, >> modifying our interface to code that we don't control) so late in the >> release cycle. It seems like there is large potential for failure in >> contexts other than the one or two you are going to be able to test >> right now. Is there anything that can be done to reduce the risk? > > The 3 different versions of the engine-code seems to me like this: > > 1. unpatched libpq > engines aren't initialized (so some don't work), engines aren't freed at the > end of connection (memory leak), no changes to PGconn > > 2. the initial patch I posted > engines are initialized (so can work), engines aren't freed (memory leak), no > changes to PGconn only in engine code path > > 3. the current patch > engines are initialized, engines are freed, but problematic changes to PGconn > > > Maybe version 2 (my initial patch) could be an alternative ? Well, based on the "we don't know which different versions of openssl it'll break with", version 2 is no better than version 3 :( -- Magnus Hagander Self: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> On 22 jun 2009, at 17.46, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> It seems like there is large potential for failure in >>> contexts other than the one or two you are going to be able to test >>> right now. Is there anything that can be done to reduce the risk? > >> I share your concerns in general. But I think we nee to take into >> account that this simply does not work without the patch. > > Frankly, I don't much care whether openssl-with-an-external-engine > works; we never supported that before, and if it doesn't work now, Sure we did. It's in 8.3. It may not have ever worked properly, but that's a different thing. According do our code and documentation, the feature is there and supported. > that's not a regression. What I'm worried about at this stage of the > release cycle is regressions, eg, does the code still compile at all > with openssl version 0.9.random-version-that-used-to-work. Right, and we don't cover *that* many different versions with the buildfarm... So how about this: 1) We add a note to the documentation that states explicitly that this only works for engines that don't require intialization. 2) We add a section on the wiki for patches that fix known issues in 8.4, but that may cause behavioral changes and thus don't get applied to the main code until 8.5. I'm sure there will be other patches on this list eventually (after release time). Perhaps even a reference from #1 to #2? -- Magnus Hagander Self: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> Lars Kanis wrote:
>> Maybe version 2 (my initial patch) could be an alternative ?
> Well, based on the "we don't know which different versions of openssl
> it'll break with", version 2 is no better than version 3 :(
Yeah, if we do anything I think it should be more like #3.
I think all or most back releases of openssl are available from
openssl.org. If someone had time to do a test compile of the proposed
patch against all of 'em (or at least all the ones we still claim to
support), it would salve my worries at least a bit. I'm not sure if
that's a big task or not, though.
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Lars Kanis wrote: >>> Maybe version 2 (my initial patch) could be an alternative ? > >> Well, based on the "we don't know which different versions of openssl >> it'll break with", version 2 is no better than version 3 :( > > Yeah, if we do anything I think it should be more like #3. > > I think all or most back releases of openssl are available from > openssl.org. If someone had time to do a test compile of the proposed > patch against all of 'em (or at least all the ones we still claim to > support), it would salve my worries at least a bit. I'm not sure if > that's a big task or not, though. Well, if it's just to compile it, I guess it should even be scriptable :-) I'm not going to volunteer for that myself since I'm not sure I'll have the time. I can try to, but it's better if someone else has the time. -- Magnus Hagander Self: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Lars Kanis wrote: >>> Maybe version 2 (my initial patch) could be an alternative ? > >> Well, based on the "we don't know which different versions of openssl >> it'll break with", version 2 is no better than version 3 :( > > Yeah, if we do anything I think it should be more like #3. > > I think all or most back releases of openssl are available from > openssl.org. If someone had time to do a test compile of the proposed > patch against all of 'em (or at least all the ones we still claim to > support), it would salve my worries at least a bit. I'm not sure if > that's a big task or not, though. I ran a build with OpenSSL 0.9.8 (default on my machine) and 0.9.7. AFAICS, only 0.9.8 is supported at all these days. There is no point in running against <0.9.7, since there was no engine support in that. I tried 0.9.7m, 0.9.7c, 0.9.8g (Ubuntu) and 0.9.8b just to be sure. But they do seem to have a policy similar to our own - no features in backbranches. I only tested building (make clean, re-configure, make in pg) and it worked fine. Default configuration on OpenSSL. This is certainly not an exhaustive test, but I think it does a fair job of bracketing the possible versions with issues. //Magnus
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> On 22 jun 2009, at 18.05, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I'm also a bit concerned about wrapping a struct >>> field inside such an #if, as that's likely to cause hard-to-debug >>> problems if two compilations read libpq-int.h with different #define >>> environments. > >> Since it's a pointer we could just declare it as void, no? Or even #if >> between void and "real" pointer. Wouldn't that be safe? > > Yeah, on reflection I think it'd be safe to do > > #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE) > ENGINE *ptr; > #else > /* dummy field to keep struct the same if OpenSSL version changes */ > void *ptr; > #endif > > (probably that #if should get wrapped up in an additional #define > instead of being duplicated in several places...) Attached is an updated patch that does both of these. >> (we already have a similar issue with the whole #ifdef use_ssl, don't >> we? But having one isn't an excuse to create another of course…) > > That's controlled by an entry in pg_config.h, though, and won't change > if someone happens to update their openssl install and then recompile > only parts of libpq. True, agreed. -- Magnus Hagander Self: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ *** a/src/interfaces/libpq/fe-secure.c --- b/src/interfaces/libpq/fe-secure.c *************** *** 31,36 **** --- 31,37 ---- #include "libpq-fe.h" #include "fe-auth.h" #include "pqsignal.h" + #include "libpq-int.h" #ifdef WIN32 #include "win32.h" *************** *** 62,68 **** #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) #include <openssl/conf.h> #endif ! #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE) #include <openssl/engine.h> #endif --- 63,69 ---- #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) #include <openssl/conf.h> #endif ! #ifdef USE_SSL_ENGINE #include <openssl/engine.h> #endif *************** *** 661,667 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) */ if (conn->sslkey && strlen(conn->sslkey) > 0) { ! #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE) if (strchr(conn->sslkey, ':') #ifdef WIN32 && conn->sslkey[1] != ':' --- 662,668 ---- */ if (conn->sslkey && strlen(conn->sslkey) > 0) { ! #ifdef USE_SSL_ENGINE if (strchr(conn->sslkey, ':') #ifdef WIN32 && conn->sslkey[1] != ':' *************** *** 669,683 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) ) { /* Colon, but not in second character, treat as engine:key */ - ENGINE *engine_ptr; char *engine_str = strdup(conn->sslkey); char *engine_colon = strchr(engine_str, ':'); *engine_colon = '\0'; /* engine_str now has engine name */ engine_colon++; /* engine_colon now has key name */ ! engine_ptr = ENGINE_by_id(engine_str); ! if (engine_ptr == NULL) { char *err = SSLerrmessage(); --- 670,683 ---- ) { /* Colon, but not in second character, treat as engine:key */ char *engine_str = strdup(conn->sslkey); char *engine_colon = strchr(engine_str, ':'); *engine_colon = '\0'; /* engine_str now has engine name */ engine_colon++; /* engine_colon now has key name */ ! conn->engine = ENGINE_by_id(engine_str); ! if (conn->engine == NULL) { char *err = SSLerrmessage(); *************** *** 690,696 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) return 0; } ! *pkey = ENGINE_load_private_key(engine_ptr, engine_colon, NULL, NULL); if (*pkey == NULL) { --- 690,711 ---- return 0; } ! if (ENGINE_init(conn->engine) == 0) ! { ! char *err = SSLerrmessage(); ! ! printfPQExpBuffer(&conn->errorMessage, ! libpq_gettext("could not initialize SSL engine \"%s\": %s\n"), ! engine_str, err); ! SSLerrfree(err); ! ENGINE_free(conn->engine); ! conn->engine = NULL; ! free(engine_str); ! ERR_pop_to_mark(); ! return 0; ! } ! ! *pkey = ENGINE_load_private_key(conn->engine, engine_colon, NULL, NULL); if (*pkey == NULL) { *************** *** 700,705 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) --- 715,723 ---- libpq_gettext("could not read private SSL key \"%s\" from engine \"%s\": %s\n"), engine_colon, engine_str, err); SSLerrfree(err); + ENGINE_finish(conn->engine); + ENGINE_free(conn->engine); + conn->engine = NULL; free(engine_str); ERR_pop_to_mark(); return 0; *************** *** 1217,1222 **** close_SSL(PGconn *conn) --- 1235,1249 ---- X509_free(conn->peer); conn->peer = NULL; } + + #ifdef USE_SSL_ENGINE + if (conn->engine) + { + ENGINE_finish(conn->engine); + ENGINE_free(conn->engine); + conn->engine = NULL; + } + #endif } /* *** a/src/interfaces/libpq/libpq-int.h --- b/src/interfaces/libpq/libpq-int.h *************** *** 76,83 **** typedef struct --- 76,88 ---- #ifdef USE_SSL #include <openssl/ssl.h> #include <openssl/err.h> + + #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE) + #define USE_SSL_ENGINE #endif + #endif /* USE_SSL */ + /* * POSTGRES backend dependent Constants. */ *************** *** 383,389 **** struct pg_conn --- 388,400 ---- X509 *peer; /* X509 cert of server */ char peer_dn[256 + 1]; /* peer distinguished name */ char peer_cn[SM_USER + 1]; /* peer common name */ + #ifdef USE_SSL_ENGINE + ENGINE *engine; /* SSL engine, if any */ + #else + void *engine; /* dummy field to keep struct the same + if OpenSSL version changes */ #endif + #endif /* USE_SSL */ #ifdef ENABLE_GSS gss_ctx_id_t gctx; /* GSS context */
Am Dienstag, 23. Juni 2009 14:17:15 schrieben Sie: > Attached is an updated patch that does both of these. It works for me, but I could test it only with OpenSSL 0.9.8g. Moreover I tested the SSL-renegotiation, and it works quite fine with an=20 engine too - as long as I don't pull the smartcard with the auth-key out of= =20 the reader :) regards Lars Kanis
Magnus Hagander <magnus@hagander.net> writes:
> Attached is an updated patch that does both of these.
This looks reasonably sane to me, and I'm satisfied with the testing
that's been done. No objection from here.
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Attached is an updated patch that does both of these. > > This looks reasonably sane to me, and I'm satisfied with the testing > that's been done. No objection from here. Applied. //Magnus