Re: [HACKERS] new libpq SSL connection option
Alex Hunsaker wrote: On Fri, Jan 2, 2009 at 03:13, Magnus Hagander mag...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] new libpq SSL connection option
Andrew Chernow wrote:
Magnus Hagander wrote:
Alex Hunsaker wrote:
On Sat, Dec 27, 2008 at 11:50, Andrew Chernow a...@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
Re: [HACKERS] new libpq SSL connection option
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] new libpq SSL connection option
On Fri, Jan 2, 2009 at 03:13, Magnus Hagander mag...@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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] new libpq SSL connection option
On Sat, Dec 27, 2008 at 11:50, Andrew Chernow a...@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? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] new libpq SSL connection option
Alex Hunsaker wrote: On Sat, Dec 27, 2008 at 11:50, Andrew Chernow a...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] new libpq SSL connection option
Magnus Hagander wrote: Alex Hunsaker wrote: On Sat, Dec 27, 2008 at 11:50, Andrew Chernow a...@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 either failure case is. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] new libpq SSL connection option
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] new libpq SSL connection option
Alex Hunsaker wrote: On Fri, Dec 5, 2008 at 14:22, Andrew Chernow [EMAIL PROTECTED] wrote: Alex Hunsaker wrote: On Fri, Dec 5, 2008 at 13:58, Andrew Chernow [EMAIL PROTECTED] 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/[EMAIL PROTECTED] ? 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 + termliteralsslcert/literal/term + listitem + para +This parameter specifies the file name of the client SSL +certificate. This option is only available if +productnamePostgreSQL/ is compiled with SSL support. + /para + /listitem + /varlistentry + + varlistentry + termliteralsslkey/literal/term + listitem + para +This parameter specifies the file name of the client SSL key. +This option is only available if productnamePostgreSQL/ is +compiled with SSL support. + /para + /listitem + /varlistentry + + varlistentry + termliteralsslrootcert/literal/term + listitem + para +This parameter specifies the file name of the root SSL certificate. +This option is only available if productnamePostgreSQL/ is +compiled with SSL support. + /para + /listitem + /varlistentry + + varlistentry + termliteralsslcrl/literal/term + listitem + para +This parameter specifies the file name of the SSL certificate +revocation list (CRL). This option is only available if +productnamePostgreSQL/ is compiled with SSL support. + /para + /listitem + /varlistentry + + varlistentry termliteralkrbsrvname/literal/term listitem para *** *** 5778,5783 myEventProc(PGEventId evtId, void *evtInfo, void *passThrough) --- 5822,5849 listitem para indexterm +primaryenvarPGROOTCERT/envar/primary + /indexterm + envarPGROOTCERT/envar specifies the file name where the SSL + root certificate is stored. This can be overridden by the + literalsslrootcert/literal connection parameter. + /para + /listitem + + listitem + para + indexterm +primaryenvarPGSSLCRL/envar/primary + /indexterm + envarPGSSLCRL/envar specifies the file name where the SSL certificate + revocation list is
Re: [HACKERS] new libpq SSL connection option
Magnus Hagander wrote: * Renamed PGROOTCERT to PGSSLROOTCERT +primaryenvarPGROOTCERT/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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] new libpq SSL connection option
Andrew Chernow wrote: Magnus Hagander wrote: * Renamed PGROOTCERT to PGSSLROOTCERT +primaryenvarPGROOTCERT/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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] new libpq SSL connection option
Magnus Hagander [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] new libpq SSL connection option
Tom Lane wrote: Magnus Hagander [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] new libpq SSL connection option
On Fri, Dec 5, 2008 at 13:58, Andrew Chernow [EMAIL PROTECTED] 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/[EMAIL PROTECTED] ? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] new libpq SSL connection option
Alex Hunsaker wrote: On Fri, Dec 5, 2008 at 13:58, Andrew Chernow [EMAIL PROTECTED] 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/[EMAIL PROTECTED] ? 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] new libpq SSL connection option
On Fri, Dec 5, 2008 at 14:22, Andrew Chernow [EMAIL PROTECTED] wrote: Alex Hunsaker wrote: On Fri, Dec 5, 2008 at 13:58, Andrew Chernow [EMAIL PROTECTED] 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/[EMAIL PROTECTED] ? 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
