Обсуждение: new libpq SSL connection option
Who anyone be opposed to "ssldir = path" as a connection option? Currently, there is no way to change the homedir method ~/.postgresql ... or am I missing something? I am willing to supply a patch. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
On Fri, Dec 5, 2008 at 13:58, Andrew Chernow <ac@esilo.com> wrote: > Who anyone be opposed to "ssldir = path" as a connection option? Currently, > there is no way to change the homedir method ~/.postgresql ... or am I > missing something? I am willing to supply a patch. You mean something like the http://archives.postgresql.org/message-id/34d269d40811202107q489e2be0h771762398dd9fcdb@mail.gmail.com. ?
Alex Hunsaker wrote: > On Fri, Dec 5, 2008 at 13:58, Andrew Chernow <ac@esilo.com> wrote: >> Who anyone be opposed to "ssldir = path" as a connection option? Currently, >> there is no way to change the homedir method ~/.postgresql ... or am I >> missing something? I am willing to supply a patch. > > You mean something like the > http://archives.postgresql.org/message-id/34d269d40811202107q489e2be0h771762398dd9fcdb@mail.gmail.com. > > ? > yes, excately like that; apparently missed it. What is the status of that patch? I see it was left in pending review .. is the fest is over? -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
On Fri, Dec 5, 2008 at 14:22, Andrew Chernow <ac@esilo.com> wrote: > Alex Hunsaker wrote: >> >> On Fri, Dec 5, 2008 at 13:58, Andrew Chernow <ac@esilo.com> wrote: >>> >>> Who anyone be opposed to "ssldir = path" as a connection option? >>> Currently, >>> there is no way to change the homedir method ~/.postgresql ... or am I >>> missing something? I am willing to supply a patch. >> >> You mean something like the >> >> http://archives.postgresql.org/message-id/34d269d40811202107q489e2be0h771762398dd9fcdb@mail.gmail.com. >> >> ? >> > > yes, excately like that; apparently missed it. What is the status of that > patch? I see it was left in pending review .. is the fest is over? I think all that is left is changing PGROOTCERT to PGSSLROOTCERT, agreeing to IFDEF the params out or not ohand this little bit: > Magnus Hagander escribió: > > On Fri, Aug 1, 2008 at 13:31, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote: > >> Something that's bothering me is that PGSSLKEY is inconsistent with the > >> sslkey conninfo parameter. PGSSLKEY specifies an engine (basically a > >> driver for specialized hardware AFAICT) from which the key is to be > >> loaded, but sslkey is a simple filename. This means that there's no way > >> to load a key from hardware if you want to specify it per connection. > >> Not that I have any such hardware, but it looks bogus. >I think the above consideration needs some discussion too. Committing >it as-is doesn't seem OK because you can't change it later -- it's >user-visible.
Alex Hunsaker wrote: > On Fri, Dec 5, 2008 at 14:22, Andrew Chernow <ac@esilo.com> wrote: >> Alex Hunsaker wrote: >>> On Fri, Dec 5, 2008 at 13:58, Andrew Chernow <ac@esilo.com> wrote: >>>> Who anyone be opposed to "ssldir = path" as a connection option? >>>> Currently, >>>> there is no way to change the homedir method ~/.postgresql ... or am I >>>> missing something? I am willing to supply a patch. >>> You mean something like the >>> >>> http://archives.postgresql.org/message-id/34d269d40811202107q489e2be0h771762398dd9fcdb@mail.gmail.com. >>> >>> ? >>> >> yes, excately like that; apparently missed it. What is the status of that >> patch? I see it was left in pending review .. is the fest is over? > > I think all that is left is changing PGROOTCERT to PGSSLROOTCERT, > agreeing to IFDEF the params out or not oh > and this little bit: > >> Magnus Hagander escribió: >>> On Fri, Aug 1, 2008 at 13:31, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote: >>>> Something that's bothering me is that PGSSLKEY is inconsistent with the >>>> sslkey conninfo parameter. PGSSLKEY specifies an engine (basically a >>>> driver for specialized hardware AFAICT) from which the key is to be >>>> loaded, but sslkey is a simple filename. This means that there's no way >>>> to load a key from hardware if you want to specify it per connection. >>>> Not that I have any such hardware, but it looks bogus. > >> I think the above consideration needs some discussion too. Committing >> it as-is doesn't seem OK because you can't change it later -- it's >> user-visible. Here's a suggested update, which does *not* yet have documentation updates. Changes from previous patch: * Made all parameters available always and ignored for non-SSL connections * Renamed PGROOTCERT to PGSSLROOTCERT * Changes the way PGSSLKEY/sslkey is handled to this: When the string does not contain a colon, it's treated as a filename. If it does contain a colon (and on windows, if this colon is not in the second position indicating a drive letter), it's treated as engine:key as before. This should keep backwards compatibility. I would also like to look this over completely - we only support loading the KEY from the smartcard, but you still have to manually copy the certificate to your machine. I don't know exactly how you're supposed to do this in OpenSSL - some googling shows almost nobody else uses the functions quite the way we do. So I'd like to look over if we need to do more around this later, but this patch should make it possible to use keys from different files without breaking backwards compatibility with what we had before. So I'm considering that a separate step, that may not be done in time for 8.4. So. Comments? //Magnus *** a/doc/src/sgml/libpq.sgml --- b/doc/src/sgml/libpq.sgml *************** *** 318,323 **** --- 318,367 ---- </varlistentry> <varlistentry> + <term><literal>sslcert</literal></term> + <listitem> + <para> + This parameter specifies the file name of the client SSL + certificate. This option is only available if + <productname>PostgreSQL</> is compiled with SSL support. + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><literal>sslkey</literal></term> + <listitem> + <para> + This parameter specifies the file name of the client SSL key. + This option is only available if <productname>PostgreSQL</> is + compiled with SSL support. + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><literal>sslrootcert</literal></term> + <listitem> + <para> + This parameter specifies the file name of the root SSL certificate. + This option is only available if <productname>PostgreSQL</> is + compiled with SSL support. + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><literal>sslcrl</literal></term> + <listitem> + <para> + This parameter specifies the file name of the SSL certificate + revocation list (CRL). This option is only available if + <productname>PostgreSQL</> is compiled with SSL support. + </para> + </listitem> + </varlistentry> + + <varlistentry> <term><literal>krbsrvname</literal></term> <listitem> <para> *************** *** 5778,5783 **** myEventProc(PGEventId evtId, void *evtInfo, void *passThrough) --- 5822,5849 ---- <listitem> <para> <indexterm> + <primary><envar>PGROOTCERT</envar></primary> + </indexterm> + <envar>PGROOTCERT</envar> specifies the file name where the SSL + root certificate is stored. This can be overridden by the + <literal>sslrootcert</literal> connection parameter. + </para> + </listitem> + + <listitem> + <para> + <indexterm> + <primary><envar>PGSSLCRL</envar></primary> + </indexterm> + <envar>PGSSLCRL</envar> specifies the file name where the SSL certificate + revocation list is stored. This can be overridden by the + <literal>sslcrl</literal> connection parameter. + </para> + </listitem> + + <listitem> + <para> + <indexterm> <primary><envar>PGKRBSRVNAME</envar></primary> </indexterm> <envar>PGKRBSRVNAME</envar> sets the Kerberos service name to use *** a/src/interfaces/libpq/fe-connect.c --- b/src/interfaces/libpq/fe-connect.c *************** *** 177,184 **** static const PQconninfoOption PQconninfoOptions[] = { #endif /* ! * "sslmode" option is allowed even without client SSL support because the ! * client can still handle SSL modes "disable" and "allow". */ {"sslmode", "PGSSLMODE", DefaultSSLMode, NULL, "SSL-Mode", "", 8}, /* sizeof("disable") == 8 */ --- 177,186 ---- #endif /* ! * ssl options are allowed even without client SSL support because the ! * client can still handle SSL modes "disable" and "allow". Other parameters ! * have no effect on non-SSL connections, so there is no reason to exclude them ! * since none of them are mandatory. */ {"sslmode", "PGSSLMODE", DefaultSSLMode, NULL, "SSL-Mode", "", 8}, /* sizeof("disable") == 8 */ *************** *** 186,191 **** static const PQconninfoOption PQconninfoOptions[] = { --- 188,205 ---- {"sslverify", "PGSSLVERIFY", DefaultSSLVerify, NULL, "SSL-Verify", "", 5}, /* sizeof("chain") == 5 */ + {"sslcert", "PGSSLCERT", NULL, NULL, + "SSL-Client-Cert", "", 64}, + + {"sslkey", "PGSSLKEY", NULL, NULL, + "SSL-Client-Key", "", 64}, + + {"sslrootcert", "PGSSLROOTCERT", NULL, NULL, + "SSL-Root-Certificate", "", 64}, + + {"sslcrl", "PGSSLCRL", NULL, NULL, + "SSL-Revocation-List", "", 64}, + #if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI) /* Kerberos and GSSAPI authentication support specifying the service name */ {"krbsrvname", "PGKRBSRVNAME", PG_KRB_SRVNAM, NULL, *************** *** 419,424 **** connectOptions1(PGconn *conn, const char *conninfo) --- 433,446 ---- conn->sslmode = tmp ? strdup(tmp) : NULL; tmp = conninfo_getval(connOptions, "sslverify"); conn->sslverify = tmp ? strdup(tmp) : NULL; + tmp = conninfo_getval(connOptions, "sslkey"); + conn->sslkey = tmp ? strdup(tmp) : NULL; + tmp = conninfo_getval(connOptions, "sslcert"); + conn->sslcert = tmp ? strdup(tmp) : NULL; + tmp = conninfo_getval(connOptions, "sslrootcert"); + conn->sslrootcert = tmp ? strdup(tmp) : NULL; + tmp = conninfo_getval(connOptions, "sslcrl"); + conn->sslcrl = tmp ? strdup(tmp) : NULL; #ifdef USE_SSL tmp = conninfo_getval(connOptions, "requiressl"); if (tmp && tmp[0] == '1') *************** *** 2032,2037 **** freePGconn(PGconn *conn) --- 2054,2067 ---- free(conn->sslmode); if (conn->sslverify) free(conn->sslverify); + if (conn->sslcert) + free(conn->sslcert); + if (conn->sslkey) + free(conn->sslkey); + if (conn->sslrootcert) + free(conn->sslrootcert); + if (conn->sslcrl) + free(conn->sslcrl); #if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI) if (conn->krbsrvname) free(conn->krbsrvname); *** a/src/interfaces/libpq/fe-secure.c --- b/src/interfaces/libpq/fe-secure.c *************** *** 568,574 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) } /* read the user certificate */ ! snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE); /* * OpenSSL <= 0.9.8 lacks error stack handling, which means it's likely to --- 568,577 ---- } /* read the user certificate */ ! if (conn->sslcert) ! strncpy(fnbuf, conn->sslcert, sizeof(fnbuf)); ! else ! snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE); /* * OpenSSL <= 0.9.8 lacks error stack handling, which means it's likely to *************** *** 618,677 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) BIO_free(bio); ! #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE) ! if (getenv("PGSSLKEY")) { ! /* read the user key from engine */ ! char *engine_env = getenv("PGSSLKEY"); ! char *engine_colon = strchr(engine_env, ':'); ! char *engine_str; ! ENGINE *engine_ptr; ! ! if (!engine_colon) { ! printfPQExpBuffer(&conn->errorMessage, ! libpq_gettext("invalid value of PGSSLKEY environment variable\n")); ! ERR_pop_to_mark(); ! return 0; ! } ! engine_str = malloc(engine_colon - engine_env + 1); ! strlcpy(engine_str, engine_env, engine_colon - engine_env + 1); ! engine_ptr = ENGINE_by_id(engine_str); ! if (engine_ptr == NULL) ! { ! char *err = SSLerrmessage(); ! printfPQExpBuffer(&conn->errorMessage, ! libpq_gettext("could not load SSL engine \"%s\": %s\n"), ! engine_str, err); ! SSLerrfree(err); ! free(engine_str); ! ERR_pop_to_mark(); ! return 0; ! } ! *pkey = ENGINE_load_private_key(engine_ptr, engine_colon + 1, ! NULL, NULL); ! if (*pkey == NULL) ! { ! char *err = SSLerrmessage(); ! printfPQExpBuffer(&conn->errorMessage, ! libpq_gettext("could not read private SSL key \"%s\" from engine \"%s\": %s\n"), ! engine_colon + 1, engine_str, err); ! SSLerrfree(err); free(engine_str); ! ERR_pop_to_mark(); ! return 0; } - free(engine_str); } else - #endif /* use PGSSLKEY */ { ! /* read the user key from file */ snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_KEY_FILE); if (stat(fnbuf, &buf) != 0) { printfPQExpBuffer(&conn->errorMessage, --- 621,698 ---- BIO_free(bio); ! /* ! * Read the SSL key. If a key is specified, treat it as an engine:key combination ! * if there is colon present - we don't support files with colon in the name. The ! * exception is if the second character is a colon, in which case it can be a Windows ! * filename with drive specification. ! */ ! 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] != ':' ! #endif ! ) { ! /* 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(); ! printfPQExpBuffer(&conn->errorMessage, ! libpq_gettext("could not load SSL engine \"%s\": %s\n"), ! engine_str, err); ! SSLerrfree(err); ! free(engine_str); ! ERR_pop_to_mark(); ! return 0; ! } ! *pkey = ENGINE_load_private_key(engine_ptr, engine_colon, ! NULL, NULL); ! if (*pkey == NULL) ! { ! char *err = SSLerrmessage(); ! ! printfPQExpBuffer(&conn->errorMessage, ! libpq_gettext("could not read private SSL key \"%s\" from engine \"%s\": %s\n"), ! engine_colon, engine_str, err); ! SSLerrfree(err); ! free(engine_str); ! ERR_pop_to_mark(); ! return 0; ! } free(engine_str); ! ! fnbuf[0] = '\0'; /* indicate we're not going to load from a file */ ! } ! else ! #endif /* support for SSL engines */ ! { ! /* PGSSLKEY is not an engine, treat it as a filename */ ! strncpy(fnbuf, conn->sslkey, sizeof(fnbuf)); } } else { ! /* No PGSSLKEY specified, load default file */ snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_KEY_FILE); + } + + if (fnbuf[0] != '\0') + { + /* read the user key from file */ + if (stat(fnbuf, &buf) != 0) { printfPQExpBuffer(&conn->errorMessage, *************** *** 948,954 **** initialize_SSL(PGconn *conn) /* Set up to verify server cert, if root.crt is present */ if (pqGetHomeDirectory(homedir, sizeof(homedir))) { ! snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CERT_FILE); if (stat(fnbuf, &buf) == 0) { X509_STORE *cvstore; --- 969,979 ---- /* Set up to verify server cert, if root.crt is present */ if (pqGetHomeDirectory(homedir, sizeof(homedir))) { ! if (conn->sslrootcert) ! strncpy(fnbuf, conn->sslrootcert, sizeof(fnbuf)); ! else ! snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CERT_FILE); ! if (stat(fnbuf, &buf) == 0) { X509_STORE *cvstore; *************** *** 966,973 **** initialize_SSL(PGconn *conn) if ((cvstore = SSL_CTX_get_cert_store(SSL_context)) != NULL) { /* setting the flags to check against the complete CRL chain */ ! if (X509_STORE_load_locations(cvstore, ROOT_CRL_FILE, NULL) != 0) /* OpenSSL 0.96 does not support X509_V_FLAG_CRL_CHECK */ #ifdef X509_V_FLAG_CRL_CHECK X509_STORE_set_flags(cvstore, --- 991,1003 ---- if ((cvstore = SSL_CTX_get_cert_store(SSL_context)) != NULL) { + if (conn->sslcrl) + strncpy(fnbuf, conn->sslcrl, sizeof(fnbuf)); + else + snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CRL_FILE); + /* setting the flags to check against the complete CRL chain */ ! if (X509_STORE_load_locations(cvstore, fnbuf, NULL) != 0) /* OpenSSL 0.96 does not support X509_V_FLAG_CRL_CHECK */ #ifdef X509_V_FLAG_CRL_CHECK X509_STORE_set_flags(cvstore, *** a/src/interfaces/libpq/libpq-int.h --- b/src/interfaces/libpq/libpq-int.h *************** *** 292,297 **** struct pg_conn --- 292,302 ---- char *pgpass; char *sslmode; /* SSL mode (require,prefer,allow,disable) */ char *sslverify; /* Verify server SSL certificate (none,chain,cn) */ + char *sslkey; /* client key filename */ + char *sslcert; /* client certificate filename */ + char *sslrootcert; /* root certificate filename */ + char *sslcrl; /* certificate revocation list filename */ + #if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI) char *krbsrvname; /* Kerberos service name */ #endif
Magnus Hagander wrote: > * Renamed PGROOTCERT to PGSSLROOTCERT > > + <primary><envar>PGROOTCERT</envar></primary> Looks like the old env name is still being used in the sgml docs. I like the flexibility this patch offers. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: > Magnus Hagander wrote: >> * Renamed PGROOTCERT to PGSSLROOTCERT >> >> + <primary><envar>PGROOTCERT</envar></primary> > > Looks like the old env name is still being used in the sgml docs. Yes - I did say I hadn't updated the docs yet :-) //Magnus
Magnus Hagander <magnus@hagander.net> writes:
> I would also like to look this over completely - we only support loading
> the KEY from the smartcard, but you still have to manually copy the
> certificate to your machine. I don't know exactly how you're supposed to
> do this in OpenSSL - some googling shows almost nobody else uses the
> functions quite the way we do. So I'd like to look over if we need to do
> more around this later, but this patch should make it possible to use
> keys from different files without breaking backwards compatibility with
> what we had before. So I'm considering that a separate step, that may
> not be done in time for 8.4.
I'm confused here. Are you proposing user-visible changes that might
not get done in time for 8.4? I don't much like the idea that the API
is going to remain a moving target --- once 8.4 is out you will have
backwards compatibility constraints with whatever it does. It would
be better to avoid extending the feature set beyond what 8.3 can do
until you are certain it's right.
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> I would also like to look this over completely - we only support loading >> the KEY from the smartcard, but you still have to manually copy the >> certificate to your machine. I don't know exactly how you're supposed to >> do this in OpenSSL - some googling shows almost nobody else uses the >> functions quite the way we do. So I'd like to look over if we need to do >> more around this later, but this patch should make it possible to use >> keys from different files without breaking backwards compatibility with >> what we had before. So I'm considering that a separate step, that may >> not be done in time for 8.4. > > I'm confused here. Are you proposing user-visible changes that might > not get done in time for 8.4? I don't much like the idea that the API > is going to remain a moving target --- once 8.4 is out you will have > backwards compatibility constraints with whatever it does. It would > be better to avoid extending the feature set beyond what 8.3 can do > until you are certain it's right. I'm not proposing anything yet - I haven't read up on it. If it does change, though, only the engine-specific stuff would change AFAICT. The new functionality in this patch is all around specifying filenames, so that would not change. And most likely it would not be a change in visible behavior if I get the time to "fix" that - it'll either just be an under-the-hood change, or more likely an extension to the parameters. I see no reason why it should have any user-visible change at all on the stuff that's in this patch. //Magnus
Why does pqGetHomeDirectory have to succeed to use conn->sslrootcert. Maybe this should be an OR of the two since sslrootcert is not dependent on homedir? around line 970 src/interfaces/libpq/fe-secure.c if (conn->sslrootcert || pqGetHomeDirectory(homedir, sizeof(homedir))) -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
On Sat, Dec 27, 2008 at 11:50, Andrew Chernow <ac@esilo.com> wrote: > Why does pqGetHomeDirectory have to succeed to use conn->sslrootcert. Maybe > this should be an OR of the two since sslrootcert is not dependent on > homedir? > > around line 970 src/interfaces/libpq/fe-secure.c > > if (conn->sslrootcert || pqGetHomeDirectory(homedir, sizeof(homedir))) Certainly, did we miss anywhere else?
Alex Hunsaker wrote: > On Sat, Dec 27, 2008 at 11:50, Andrew Chernow <ac@esilo.com> wrote: >> Why does pqGetHomeDirectory have to succeed to use conn->sslrootcert. Maybe >> this should be an OR of the two since sslrootcert is not dependent on >> homedir? >> >> around line 970 src/interfaces/libpq/fe-secure.c >> >> if (conn->sslrootcert || pqGetHomeDirectory(homedir, sizeof(homedir))) > > Certainly, did we miss anywhere else? I agree it looks strange. That said, have you actually seen a case where pqGetHomeDirectory() fails? Or did you just notice the code? //Magnus
Magnus Hagander wrote: > Alex Hunsaker wrote: >> On Sat, Dec 27, 2008 at 11:50, Andrew Chernow <ac@esilo.com> wrote: >>> Why does pqGetHomeDirectory have to succeed to use conn->sslrootcert. Maybe >>> this should be an OR of the two since sslrootcert is not dependent on >>> homedir? >>> >>> around line 970 src/interfaces/libpq/fe-secure.c >>> >>> if (conn->sslrootcert || pqGetHomeDirectory(homedir, sizeof(homedir)))>> >> Certainly, did we miss anywhere else? >> Yes, the homedir variable is used again later in the function. homedir could be invalid since pqGetHomeDirectory might not get called. Maybe something like below would do the trick: /* when used, it can't be an empty string. */ *homedir = 0; /* If either are NULL, homedir is needed */ if (!conn->sslrootcert || !conn->sslcrl) pqGetHomeDirectory(homedir, sizeof(homedir)); /* one of them must be valid */ if (conn->sslrootcert || *homedir) > I agree it looks strange. > > That said, have you actually seen a case where pqGetHomeDirectory() > fails? Or did you just notice the code? > It can fail. For non-windows systems, it can fail on pqGetpwuid; which equates to getpwuid or getpwuid_r failing. On windows, it can fail on SHGetFolderPath. I really have no idea how likely eitherfailure case is. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote:
> Magnus Hagander wrote:
>> Alex Hunsaker wrote:
>>> On Sat, Dec 27, 2008 at 11:50, Andrew Chernow <ac@esilo.com> wrote:
>>>> Why does pqGetHomeDirectory have to succeed to use
>>>> conn->sslrootcert. Maybe
>>>> this should be an OR of the two since sslrootcert is not dependent on
>>>> homedir?
>>>>
>>>> around line 970 src/interfaces/libpq/fe-secure.c
>>>>
>>>> if (conn->sslrootcert || pqGetHomeDirectory(homedir, sizeof(homedir)))
>>>
>>> Certainly, did we miss anywhere else?
>>>
>
> Yes, the homedir variable is used again later in the function. homedir
> could be invalid since pqGetHomeDirectory might not get called. Maybe
> something like below would do the trick:
>
> /* when used, it can't be an empty string. */
> *homedir = 0;
>
> /* If either are NULL, homedir is needed */
> if (!conn->sslrootcert || !conn->sslcrl)
> pqGetHomeDirectory(homedir, sizeof(homedir));
>
> /* one of them must be valid */
> if (conn->sslrootcert || *homedir)
How about this patch?
There's a lot of whitespace change due to indentation change, so I've
included a version without it for reference.
Also, it looks like we have the same problem with the private key, in
client_cert_cb(), agreed?
//Magnus
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index ca2d33e..6957542 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -964,76 +964,85 @@ initialize_SSL(PGconn *conn)
* If sslverify is set to anything other than "none", perform certificate
* verification. If set to "cn" we will also do further verifications after
* the connection has been completed.
+ *
+ * If we are going to look for either root certificate or CRL in the home directory,
+ * we need pqGetHomeDirectory() to succeed. In other cases, we don't need to
+ * get the home directory explicitly.
*/
-
- /* Set up to verify server cert, if root.crt is present */
- if (pqGetHomeDirectory(homedir, sizeof(homedir)))
+ if (!conn->sslrootcert || !conn->sslcrl)
{
- if (conn->sslrootcert)
- strncpy(fnbuf, conn->sslrootcert, sizeof(fnbuf));
- else
- snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CERT_FILE);
-
- if (stat(fnbuf, &buf) == 0)
+ if (!pqGetHomeDirectory(homedir, sizeof(homedir)))
{
- X509_STORE *cvstore;
-
- if (!SSL_CTX_load_verify_locations(SSL_context, fnbuf, NULL))
+ if (strcmp(conn->sslverify, "none") != 0)
{
- char *err = SSLerrmessage();
-
printfPQExpBuffer(&conn->errorMessage,
- libpq_gettext("could not read root certificate file \"%s\": %s\n"),
- fnbuf, err);
- SSLerrfree(err);
+ libpq_gettext("cannot find home directory to locate root certificate file"));
return -1;
}
+ }
+ }
+ else
+ {
+ homedir[0] = '\0';
+ }
- if ((cvstore = SSL_CTX_get_cert_store(SSL_context)) != NULL)
- {
- if (conn->sslcrl)
- strncpy(fnbuf, conn->sslcrl, sizeof(fnbuf));
- else
- snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CRL_FILE);
- /* setting the flags to check against the complete CRL chain */
- if (X509_STORE_load_locations(cvstore, fnbuf, NULL) != 0)
-/* OpenSSL 0.96 does not support X509_V_FLAG_CRL_CHECK */
-#ifdef X509_V_FLAG_CRL_CHECK
- X509_STORE_set_flags(cvstore,
- X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL);
- /* if not found, silently ignore; we do not require CRL */
-#else
- {
- char *err = SSLerrmessage();
- printfPQExpBuffer(&conn->errorMessage,
- libpq_gettext("SSL library does not support CRL certificates (file \"%s\")\n"),
- fnbuf);
- SSLerrfree(err);
- return -1;
- }
-#endif
- }
+ if (conn->sslrootcert)
+ strncpy(fnbuf, conn->sslrootcert, sizeof(fnbuf));
+ else
+ snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CERT_FILE);
+
+ if (stat(fnbuf, &buf) == 0)
+ {
+ X509_STORE *cvstore;
+
+ if (!SSL_CTX_load_verify_locations(SSL_context, fnbuf, NULL))
+ {
+ char *err = SSLerrmessage();
- SSL_CTX_set_verify(SSL_context, SSL_VERIFY_PEER, verify_cb);
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not read root certificate file \"%s\": %s\n"),
+ fnbuf, err);
+ SSLerrfree(err);
+ return -1;
}
- else
+
+ if ((cvstore = SSL_CTX_get_cert_store(SSL_context)) != NULL)
{
- if (strcmp(conn->sslverify, "none") != 0)
+ if (conn->sslcrl)
+ strncpy(fnbuf, conn->sslcrl, sizeof(fnbuf));
+ else
+ snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CRL_FILE);
+
+ /* setting the flags to check against the complete CRL chain */
+ if (X509_STORE_load_locations(cvstore, fnbuf, NULL) != 0)
+/* OpenSSL 0.96 does not support X509_V_FLAG_CRL_CHECK */
+#ifdef X509_V_FLAG_CRL_CHECK
+ X509_STORE_set_flags(cvstore,
+ X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL);
+ /* if not found, silently ignore; we do not require CRL */
+#else
{
+ char *err = SSLerrmessage();
+
printfPQExpBuffer(&conn->errorMessage,
- libpq_gettext("root certificate file (%s) not found"), fnbuf);
+ libpq_gettext("SSL library does not support CRL certificates (file \"%s\")\n"),
+ fnbuf);
+ SSLerrfree(err);
return -1;
}
+#endif
}
- }
+
+ SSL_CTX_set_verify(SSL_context, SSL_VERIFY_PEER, verify_cb);
+ } /* root certificate exists */
else
{
if (strcmp(conn->sslverify, "none") != 0)
{
printfPQExpBuffer(&conn->errorMessage,
- libpq_gettext("cannot find home directory to locate root certificate file"));
+ libpq_gettext("root certificate file (%s) not found"), fnbuf);
return -1;
}
}
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index ca2d33e..6957542 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -964,11 +964,30 @@ initialize_SSL(PGconn *conn)
* If sslverify is set to anything other than "none", perform certificate
* verification. If set to "cn" we will also do further verifications after
* the connection has been completed.
+ *
+ * If we are going to look for either root certificate or CRL in the home directory,
+ * we need pqGetHomeDirectory() to succeed. In other cases, we don't need to
+ * get the home directory explicitly.
*/
-
- /* Set up to verify server cert, if root.crt is present */
- if (pqGetHomeDirectory(homedir, sizeof(homedir)))
+ if (!conn->sslrootcert || !conn->sslcrl)
+ {
+ if (!pqGetHomeDirectory(homedir, sizeof(homedir)))
{
+ if (strcmp(conn->sslverify, "none") != 0)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("cannot find home directory to locate root certificate file"));
+ return -1;
+ }
+ }
+ }
+ else
+ {
+ homedir[0] = '\0';
+ }
+
+
+
if (conn->sslrootcert)
strncpy(fnbuf, conn->sslrootcert, sizeof(fnbuf));
else
@@ -1017,7 +1036,7 @@ initialize_SSL(PGconn *conn)
}
SSL_CTX_set_verify(SSL_context, SSL_VERIFY_PEER, verify_cb);
- }
+ } /* root certificate exists */
else
{
if (strcmp(conn->sslverify, "none") != 0)
@@ -1027,16 +1046,6 @@ initialize_SSL(PGconn *conn)
return -1;
}
}
- }
- else
- {
- if (strcmp(conn->sslverify, "none") != 0)
- {
- printfPQExpBuffer(&conn->errorMessage,
- libpq_gettext("cannot find home directory to locate root certificate file"));
- return -1;
- }
- }
/* set up mechanism to provide client certificate, if available */
SSL_CTX_set_client_cert_cb(SSL_context, client_cert_cb);
> > Also, it looks like we have the same problem with the private key, in > client_cert_cb(), agreed? > > > //Magnus > Yeah, same issue in that function. I missed that. My grep'n was obviously brain dead. It almost feels like there should be some util functions like get_sslrootcert(conn, path_buf, buf_size) for each of the SSL files. Isolate the logic behind a function with a success or failure return value. It would probably make the current code easier to read/follow. Only downside is that pqGetHomeDirectory would be called twice in some cases, but I really don't think that makes any noticeable performance difference. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
On Fri, Jan 2, 2009 at 03:13, Magnus Hagander <magnus@hagander.net> wrote: > Andrew Chernow wrote: >> Yes, the homedir variable is used again later in the function. homedir >> could be invalid since pqGetHomeDirectory might not get called. Maybe >> something like below would do the trick: > How about this patch? looks good to me (btw it was almost hard to believe the non-white space one was the same patch! lol)
Alex Hunsaker wrote: > On Fri, Jan 2, 2009 at 03:13, Magnus Hagander <magnus@hagander.net> wrote: >> Andrew Chernow wrote: >>> Yes, the homedir variable is used again later in the function. homedir >>> could be invalid since pqGetHomeDirectory might not get called. Maybe >>> something like below would do the trick: > >> How about this patch? > > looks good to me (btw it was almost hard to believe the non-white > space one was the same patch! lol) Applied, along with a fix for the second place that had the issue. //Magnus