SSL configure patch: review

Started by Abhijit Menon-Senover 17 years ago11 messages

These are my review comments on Mark Woodward's "SSL configure patch":

http://archives.postgresql.org/message-id/36152.64.119.130.186.1213364119.squirrel@mail.mohawksoft.com

(The patch is whitespace-damaged and the one fe-secure.c hunk doesn't
apply cleanly to the latest source, but I'm ignoring both problems for
the moment.)

1. To begin with, the patch should document the new connection options.

In fe-connect.c:

@@ -181,6 +181,19 @@
{"sslmode", "PGSSLMODE", DefaultSSLMode, NULL,
"SSL-Mode", "", 8}, /* sizeof("disable") == 8 */

+	{"sslcert", "PGSSLCERT", NULL, NULL,
+	"SSL-Client-Cert", "", 64},
+
+	{"sslkey", "PGSSLKEY", NULL, NULL,
+	"SSL-Client-Key", "", 64},
+
+	{"ssltrustcrt", "PGSSLKEY", NULL, NULL,
+	"SSL-Trusted-Keys", "", 64},
+
+	{"sslcrl", "PGSSLKEY", NULL, NULL,
+	"SSL-Revocation-List", "", 64},

2. You have "PGSSLKEY" for "ssltrustcrt" and "sslcrl". Cut and paste
error?

3. I'd suggest using the names "PGROOTCERT" and "sslrootcert" instead of
"ssltrustcrt", to better match the ROOT_CERT_FILE value you're trying
to configure.

In libpq-int.h:

@@ -293,6 +293,11 @@
char	   *pgpass;
bool		pgpass_from_client;	/* did password come from connect args? */
char	   *sslmode;		/* SSL mode (require,prefer,allow,disable) */
+        char       *sslkey;     /* ssl key file filename for call back */
+        char       *sslcert;    /* ssl certificate filename for call back */
+        char       *ssltrustcrt; /* Trusted certificuits */
+        char       *sslcrl;     /* certificates revoked by certificate authorities */
+

4. I'd say "SSL client key filename", "SSL client certificate filename",
"SSL root certificate filename", and "SSL certificate revocation list
filename" in the comments. (It's not clear what the "call back" is in
this context; nor does it need to be.)

(I like "certificuits". It makes me think of cookies. :-)

In fe-secure.c:

@@ -939,8 +950,13 @@

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, ROOT_CRL_FILE, NULL) != 0)
+				if (X509_STORE_load_locations(cvstore, fnbuf, NULL) != 0)

5. I notice you're adding "homedir/" to the ROOT_CRL_FILE. This looks
sensible, but I wanted to make sure it was intentional. Is this what
you (i.e. Mark) meant in another message when you said:

BTW: the revocation list probably never worked in the client.

Finally, I don't know enough (i.e. anything) about Windows to evaluate
the changes to libpq.rc, but the file that should be patched is really
libpq.rc.in.

-- ams

In reply to: Abhijit Menon-Sen (#1)
1 attachment(s)
Re: SSL configure patch: review

At 2008-07-08 08:27:29 +0530, ams@oryx.com wrote:

(The patch is whitespace-damaged and the one fe-secure.c hunk doesn't
apply cleanly to the latest source, but I'm ignoring both problems for
the moment.)

It wasn't hard to fix those, so I've attached an updated patch here.

Finally, I don't know enough (i.e. anything) about Windows to evaluate
the changes to libpq.rc, but the file that should be patched is really
libpq.rc.in.

(But I didn't touch libpq.rc.in. I'm not sure if it even needs to be
changed any more.)

-- ams

Attachments:

ssl.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 307b805..6b53e37 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -181,6 +181,19 @@ static const PQconninfoOption PQconninfoOptions[] = {
 	{"sslmode", "PGSSLMODE", DefaultSSLMode, NULL,
 	"SSL-Mode", "", 8},			/* sizeof("disable") == 8 */
 
+	{"sslcert", "PGSSLCERT", NULL, NULL,
+	"SSL-Client-Cert", "", 64},
+
+	{"sslkey", "PGSSLKEY", NULL, NULL,
+	"SSL-Client-Key", "", 64},
+
+	{"sslrootcert", "PGROOTCERT", 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,
@@ -413,6 +426,14 @@ connectOptions1(PGconn *conn, const char *conninfo)
 	conn->connect_timeout = tmp ? strdup(tmp) : NULL;
 	tmp = conninfo_getval(connOptions, "sslmode");
 	conn->sslmode = 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')
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index a61dbfb..a4fdba1 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -599,7 +599,11 @@ client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey)
 	}
 
 	/* read the user certificate */
-	snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE);
+
+	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
@@ -650,7 +654,7 @@ 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"))
+	if (getenv("PGSSLKEY") && !conn->sslkey)
 	{
 		/* read the user key from engine */
 		char	   *engine_env = getenv("PGSSLKEY");
@@ -702,7 +706,11 @@ client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey)
 #endif   /* use PGSSLKEY */
 	{
 		/* read the user key from file */
-		snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_KEY_FILE);
+		if (conn->sslkey)
+			strncpy(fnbuf, conn->sslkey, sizeof(fnbuf));
+		else
+			snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_KEY_FILE);
+
 		if (stat(fnbuf, &buf) != 0)
 		{
 			printfPQExpBuffer(&conn->errorMessage,
@@ -904,7 +912,11 @@ 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 (conn->ssltrustcrt)
+			strncpy(fnbuf, conn->ssltrustcrt, sizeof(fnbuf));
+		else
+			snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CERT_FILE);
+
 		if (stat(fnbuf, &buf) == 0)
 		{
 			X509_STORE *cvstore;
@@ -922,8 +934,13 @@ initialize_SSL(PGconn *conn)
 
 			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, ROOT_CRL_FILE, NULL) != 0)
+				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,
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 4af1388..f47e50e 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -293,6 +293,11 @@ struct pg_conn
 	char	   *pgpass;
 	bool		pgpass_from_client;	/* did password come from connect args? */
 	char	   *sslmode;		/* SSL mode (require,prefer,allow,disable) */
+	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
#3Alvaro Herrera
alvherre@commandprompt.com
In reply to: Abhijit Menon-Sen (#2)
1 attachment(s)
Re: SSL configure patch: review

Abhijit Menon-Sen wrote:

At 2008-07-08 08:27:29 +0530, ams@oryx.com wrote:

(The patch is whitespace-damaged and the one fe-secure.c hunk doesn't
apply cleanly to the latest source, but I'm ignoring both problems for
the moment.)

It wasn't hard to fix those, so I've attached an updated patch here.

Finally, I don't know enough (i.e. anything) about Windows to evaluate
the changes to libpq.rc, but the file that should be patched is really
libpq.rc.in.

(But I didn't touch libpq.rc.in. I'm not sure if it even needs to be
changed any more.)

It doesn't look like it needs changing.

I've hacked up a couple of SGML paragraphs to serve as documentation.
The patch is attached. I'll revise it (and make sure it compiles
properly) and see about committing it later today.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Attachments:

ssl-new-params.patchtext/x-diff; charset=us-asciiDownload
Index: doc/src/sgml/libpq.sgml
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.260
diff -c -p -r1.260 libpq.sgml
*** doc/src/sgml/libpq.sgml	27 Jun 2008 02:44:31 -0000	1.260
--- doc/src/sgml/libpq.sgml	1 Aug 2008 18:52:07 -0000
***************
*** 281,286 ****
--- 281,324 ----
          </varlistentry>
  
          <varlistentry>
+          <term><literal>sslcert</literal></term>
+          <listitem>
+           <para>
+            This parameter specifies the file name of the client SSL
+            certificate.
+           </para>
+          </listitem>
+         </varlistentry>
+ 
+         <varlistentry>
+          <term><literal>sslkey</literal></term>
+          <listitem>
+           <para>
+            This parameter specifies the file name of the client SSL key.
+           </para>
+          </listitem>
+         </varlistentry>
+ 
+         <varlistentry>
+          <term><literal>sslrootcert</literal></term>
+          <listitem>
+           <para>
+            This parameter specifies the file name of the root SSL certificate.
+           </para>
+          </listitem>
+         </varlistentry>
+ 
+         <varlistentry>
+          <term><literal>sslcrl</literal></term>
+          <listitem>
+           <para>
+            This parameter specifies the file name of the SSL certificate
+            revocation list (CRL)
+           </para>
+          </listitem>
+         </varlistentry>
+ 
+         <varlistentry>
           <term><literal>krbsrvname</literal></term>
           <listitem>
            <para>
*************** defaultNoticeProcessor(void *arg, const 
*** 4911,4916 ****
--- 4949,4976 ----
      <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
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.359
diff -c -p -r1.359 fe-connect.c
*** src/interfaces/libpq/fe-connect.c	29 May 2008 22:02:44 -0000	1.359
--- src/interfaces/libpq/fe-connect.c	1 Aug 2008 14:44:50 -0000
*************** static const PQconninfoOption PQconninfo
*** 181,186 ****
--- 181,198 ----
  	{"sslmode", "PGSSLMODE", DefaultSSLMode, NULL,
  	"SSL-Mode", "", 8},			/* sizeof("disable") == 8 */
  
+ 	{"sslcert", "PGSSLCERT", NULL, NULL,
+ 	"SSL-Client-Cert", "", 64},
+ 
+ 	{"sslkey", "PGSSLKEY", NULL, NULL,
+ 	"SSL-Client-Key", "", 64},
+ 
+ 	{"sslrootcert", "PGROOTCERT", 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,
*************** connectOptions1(PGconn *conn, const char
*** 413,418 ****
--- 425,438 ----
  	conn->connect_timeout = tmp ? strdup(tmp) : NULL;
  	tmp = conninfo_getval(connOptions, "sslmode");
  	conn->sslmode = 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')
Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.105
diff -c -p -r1.105 fe-secure.c
*** src/interfaces/libpq/fe-secure.c	16 May 2008 18:30:53 -0000	1.105
--- src/interfaces/libpq/fe-secure.c	1 Aug 2008 14:44:00 -0000
*************** client_cert_cb(SSL *ssl, X509 **x509, EV
*** 599,605 ****
  	}
  
  	/* 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
--- 599,608 ----
  	}
  
  	/* 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
*************** client_cert_cb(SSL *ssl, X509 **x509, EV
*** 650,656 ****
  	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");
--- 653,659 ----
  	BIO_free(bio);
  
  #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
! 	if (getenv("PGSSLKEY") && !conn->sslkey)
  	{
  		/* read the user key from engine */
  		char	   *engine_env = getenv("PGSSLKEY");
*************** client_cert_cb(SSL *ssl, X509 **x509, EV
*** 702,708 ****
  #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,
--- 705,715 ----
  #endif   /* use PGSSLKEY */
  	{
  		/* read the user key from file */
! 		if (conn->sslkey)
! 			strncpy(fnbuf, conn->sslkey, sizeof(fnbuf));
! 		else
! 			snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_KEY_FILE);
! 
  		if (stat(fnbuf, &buf) != 0)
  		{
  			printfPQExpBuffer(&conn->errorMessage,
*************** initialize_SSL(PGconn *conn)
*** 904,910 ****
  	/* 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;
--- 911,921 ----
  	/* Set up to verify server cert, if root.crt is present */
  	if (pqGetHomeDirectory(homedir, sizeof(homedir)))
  	{
! 		if (conn->ssltrustcrt)
! 			strncpy(fnbuf, conn->ssltrustcrt, sizeof(fnbuf));
! 		else
! 			snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CERT_FILE);
! 
  		if (stat(fnbuf, &buf) == 0)
  		{
  			X509_STORE *cvstore;
*************** initialize_SSL(PGconn *conn)
*** 922,929 ****
  
  			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,
--- 933,945 ----
  
  			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,
Index: src/interfaces/libpq/libpq-int.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/interfaces/libpq/libpq-int.h,v
retrieving revision 1.131
diff -c -p -r1.131 libpq-int.h
*** src/interfaces/libpq/libpq-int.h	29 May 2008 22:02:44 -0000	1.131
--- src/interfaces/libpq/libpq-int.h	1 Aug 2008 14:34:15 -0000
*************** struct pg_conn
*** 293,298 ****
--- 293,303 ----
  	char	   *pgpass;
  	bool		pgpass_from_client;	/* did password come from connect args? */
  	char	   *sslmode;		/* SSL mode (require,prefer,allow,disable) */
+ 	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
#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#3)
Re: SSL configure patch: review

Alvaro Herrera <alvherre@commandprompt.com> writes:

I've hacked up a couple of SGML paragraphs to serve as documentation.
The patch is attached. I'll revise it (and make sure it compiles
properly) and see about committing it later today.

Weren't there a couple of other points in ams' review that we were
waiting to hear from Mark about?

regards, tom lane

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#3)
Re: SSL configure patch: review

BTW, doesn't this patch leak memory at freePGconn() time? I also
think that more of it should be inside #ifdef USE_SSL --- ie,
the options should be treated like requiressl not sslmode, and
not exist in a non-SSL build.

regards, tom lane

#6Alvaro Herrera
alvherre@commandprompt.com
In reply to: Tom Lane (#4)
Re: SSL configure patch: review

Tom Lane wrote:

Alvaro Herrera <alvherre@commandprompt.com> writes:

I've hacked up a couple of SGML paragraphs to serve as documentation.
The patch is attached. I'll revise it (and make sure it compiles
properly) and see about committing it later today.

Weren't there a couple of other points in ams' review that we were
waiting to hear from Mark about?

Yes; I based mine on ams', which already had them corrected AFAICT.

I, too, am unhappy about Mark's unresponsiveness anyway.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

#7Alvaro Herrera
alvherre@commandprompt.com
In reply to: Tom Lane (#5)
1 attachment(s)
Re: SSL configure patch: review

Tom Lane wrote:

BTW, doesn't this patch leak memory at freePGconn() time?

Doh -- right, fixed.

I also think that more of it should be inside #ifdef USE_SSL --- ie,
the options should be treated like requiressl not sslmode, and not
exist in a non-SSL build.

I wondered about that too, and couldn't make up my mind about it.
This patch does things that way.

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.

Obviously one still wants to be able to specify a different file name
from the default; I tried to see if there's any way to load an engine
that would load the key from a file, but could not extract any sense
from the man page:
http://www.openssl.org/docs/crypto/engine.html

Maybe this means that we should provide separate parameters, say
"sslkey" and "sslkeyfile", and a new env var PGSSLKEYFILE.

Thoughts? Am I overengineering this stuff?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Attachments:

ssl-new-params-2.patchtext/x-diff; charset=us-asciiDownload
Index: doc/src/sgml/libpq.sgml
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.260
diff -c -p -r1.260 libpq.sgml
*** doc/src/sgml/libpq.sgml	27 Jun 2008 02:44:31 -0000	1.260
--- doc/src/sgml/libpq.sgml	1 Aug 2008 20:29:04 -0000
***************
*** 281,286 ****
--- 281,330 ----
          </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>
*************** defaultNoticeProcessor(void *arg, const 
*** 4911,4916 ****
--- 4955,4982 ----
      <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
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.359
diff -c -p -r1.359 fe-connect.c
*** src/interfaces/libpq/fe-connect.c	29 May 2008 22:02:44 -0000	1.359
--- src/interfaces/libpq/fe-connect.c	1 Aug 2008 20:13:49 -0000
*************** static const PQconninfoOption PQconninfo
*** 181,186 ****
--- 181,201 ----
  	{"sslmode", "PGSSLMODE", DefaultSSLMode, NULL,
  	"SSL-Mode", "", 8},			/* sizeof("disable") == 8 */
  
+ 	/* These parameters are only present in an SSL-enabled build. */
+ #ifdef USE_SSL
+ 	{"sslcert", "PGSSLCERT", NULL, NULL,
+ 	"SSL-Client-Cert", "", 64},
+ 
+ 	{"sslkey", "PGSSLKEY", NULL, NULL,
+ 	"SSL-Client-Key", "", 64},
+ 
+ 	{"sslrootcert", "PGROOTCERT", NULL, NULL,
+ 	"SSL-Root-Certificate", "", 64},
+ 
+ 	{"sslcrl", "PGSSLCRL", NULL, NULL,
+ 	"SSL-Revocation-List", "", 64},
+ #endif
+ 
  #if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
  	/* Kerberos and GSSAPI authentication support specifying the service name */
  	{"krbsrvname", "PGKRBSRVNAME", PG_KRB_SRVNAM, NULL,
*************** connectOptions1(PGconn *conn, const char
*** 414,419 ****
--- 429,442 ----
  	tmp = conninfo_getval(connOptions, "sslmode");
  	conn->sslmode = tmp ? strdup(tmp) : NULL;
  #ifdef USE_SSL
+ 	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;
  	tmp = conninfo_getval(connOptions, "requiressl");
  	if (tmp && tmp[0] == '1')
  	{
*************** freePGconn(PGconn *conn)
*** 1996,2001 ****
--- 2019,2034 ----
  		free(conn->pgpass);
  	if (conn->sslmode)
  		free(conn->sslmode);
+ #ifdef USE_SSL
+ 	if (conn->sslcert)
+ 		free(conn->sslcert);
+ 	if (conn->sslkey)
+ 		free(conn->sslkey);
+ 	if (conn->sslrootcert)
+ 		free(conn->sslrootcert);
+ 	if (conn->sslcrl)
+ 		free(conn->sslcrl);
+ #endif
  #if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
  	if (conn->krbsrvname)
  		free(conn->krbsrvname);
Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.105
diff -c -p -r1.105 fe-secure.c
*** src/interfaces/libpq/fe-secure.c	16 May 2008 18:30:53 -0000	1.105
--- src/interfaces/libpq/fe-secure.c	1 Aug 2008 20:31:25 -0000
*************** client_cert_cb(SSL *ssl, X509 **x509, EV
*** 599,605 ****
  	}
  
  	/* 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
--- 599,608 ----
  	}
  
  	/* 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
*************** client_cert_cb(SSL *ssl, X509 **x509, EV
*** 650,656 ****
  	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");
--- 653,659 ----
  	BIO_free(bio);
  
  #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
! 	if (getenv("PGSSLKEY") && !conn->sslkey)
  	{
  		/* read the user key from engine */
  		char	   *engine_env = getenv("PGSSLKEY");
*************** client_cert_cb(SSL *ssl, X509 **x509, EV
*** 702,708 ****
  #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,
--- 705,715 ----
  #endif   /* use PGSSLKEY */
  	{
  		/* read the user key from file */
! 		if (conn->sslkey)
! 			strncpy(fnbuf, conn->sslkey, sizeof(fnbuf));
! 		else
! 			snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_KEY_FILE);
! 
  		if (stat(fnbuf, &buf) != 0)
  		{
  			printfPQExpBuffer(&conn->errorMessage,
*************** initialize_SSL(PGconn *conn)
*** 904,910 ****
  	/* 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;
--- 911,921 ----
  	/* 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;
*************** initialize_SSL(PGconn *conn)
*** 922,929 ****
  
  			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,
--- 933,945 ----
  
  			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,
Index: src/interfaces/libpq/libpq-int.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/interfaces/libpq/libpq-int.h,v
retrieving revision 1.131
diff -c -p -r1.131 libpq-int.h
*** src/interfaces/libpq/libpq-int.h	29 May 2008 22:02:44 -0000	1.131
--- src/interfaces/libpq/libpq-int.h	1 Aug 2008 20:30:24 -0000
*************** struct pg_conn
*** 293,298 ****
--- 293,305 ----
  	char	   *pgpass;
  	bool		pgpass_from_client;	/* did password come from connect args? */
  	char	   *sslmode;		/* SSL mode (require,prefer,allow,disable) */
+ #ifdef USE_SSL
+ 	char       *sslkey;			/* client key filename */
+ 	char       *sslcert;		/* client certificate filename */
+ 	char       *sslrootcert;	/* root certificate filename */
+ 	char       *sslcrl;			/* certificate revocation list filename */
+ #endif
+ 
  #if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
  	char	   *krbsrvname;		/* Kerberos service name */
  #endif
#8Alex Hunsaker
badalex@gmail.com
In reply to: Alvaro Herrera (#7)
1 attachment(s)
Re: SSL configure patch: review

On Fri, Aug 1, 2008 at 13:31, Alvaro Herrera <alvherre@commandprompt.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.

Obviously one still wants to be able to specify a different file name
from the default; I tried to see if there's any way to load an engine
that would load the key from a file, but could not extract any sense
from the man page:
http://www.openssl.org/docs/crypto/engine.html

Maybe this means that we should provide separate parameters, say
"sslkey" and "sslkeyfile", and a new env var PGSSLKEYFILE.

Thoughts? Am I overengineering this stuff?

I think the patch as it stands is ok for now (mainly because I don't
have any hardware either so I can't test or attest to how it should be
done i.e. if those params would be enough)

Should PGROOTCERT be PGSSLROOTCERT to be more congruent with all the
other ssl params?

Find attached an updated patch against HEAD (no other changes). I
also gave it a look over and tested it to make sure it worked as
described.

Attachments:

ssl-new-params-3.patchtext/x-patch; name=ssl-new-params-3.patchDownload
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
***************
*** 309,314 ****
--- 309,358 ----
          </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>
***************
*** 5767,5772 **** myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
--- 5811,5838 ----
      <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
***************
*** 186,191 **** static const PQconninfoOption PQconninfoOptions[] = {
--- 186,206 ----
  	{"sslverify", "PGSSLVERIFY", DefaultSSLVerify, NULL,
  	"SSL-Verify", "", 5},		/* sizeof("chain") == 5 */
  
+ 	/* These parameters are only present in an SSL-enabled build. */
+ #ifdef USE_SSL
+ 	{"sslcert", "PGSSLCERT", NULL, NULL,
+ 	"SSL-Client-Cert", "", 64},
+ 
+ 	{"sslkey", "PGSSLKEY", NULL, NULL,
+ 	"SSL-Client-Key", "", 64},
+ 
+ 	{"sslrootcert", "PGROOTCERT", NULL, NULL,
+ 	"SSL-Root-Certificate", "", 64},
+ 
+ 	{"sslcrl", "PGSSLCRL", NULL, NULL,
+ 	"SSL-Revocation-List", "", 64},
+ #endif
+ 
  #if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
  	/* Kerberos and GSSAPI authentication support specifying the service name */
  	{"krbsrvname", "PGKRBSRVNAME", PG_KRB_SRVNAM, NULL,
***************
*** 423,428 **** connectOptions1(PGconn *conn, const char *conninfo)
--- 438,451 ----
  	tmp = conninfo_getval(connOptions, "sslverify");
  	conn->sslverify = tmp ? strdup(tmp) : NULL;
  #ifdef USE_SSL
+ 	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;
  	tmp = conninfo_getval(connOptions, "requiressl");
  	if (tmp && tmp[0] == '1')
  	{
***************
*** 2035,2040 **** freePGconn(PGconn *conn)
--- 2058,2073 ----
  		free(conn->sslmode);
  	if (conn->sslverify)
  		free(conn->sslverify);
+ #ifdef USE_SSL
+ 	if (conn->sslcert)
+ 		free(conn->sslcert);
+ 	if (conn->sslkey)
+ 		free(conn->sslkey);
+ 	if (conn->sslrootcert)
+ 		free(conn->sslrootcert);
+ 	if (conn->sslcrl)
+ 		free(conn->sslcrl);
+ #endif
  #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
***************
*** 509,515 **** 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
--- 509,518 ----
  	}
  
  	/* 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
***************
*** 560,566 **** 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");
--- 563,569 ----
  	BIO_free(bio);
  
  #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
! 	if (getenv("PGSSLKEY") && !conn->sslkey)
  	{
  		/* read the user key from engine */
  		char	   *engine_env = getenv("PGSSLKEY");
***************
*** 612,618 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey)
  #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,
--- 615,625 ----
  #endif   /* use PGSSLKEY */
  	{
  		/* read the user key from file */
! 		if (conn->sslkey)
! 			strncpy(fnbuf, conn->sslkey, sizeof(fnbuf));
! 		else
! 			snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_KEY_FILE);
! 
  		if (stat(fnbuf, &buf) != 0)
  		{
  			printfPQExpBuffer(&conn->errorMessage,
***************
*** 820,826 **** 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;
--- 827,837 ----
  	/* 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;
***************
*** 838,845 **** 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,
--- 849,861 ----
  
  			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,304 ----
  	char	   *pgpass;
  	char	   *sslmode;		/* SSL mode (require,prefer,allow,disable) */
  	char	   *sslverify;		/* Verify server SSL certificate (none,chain,cn) */
+ #ifdef USE_SSL
+ 	char	   *sslkey;			/* client key filename */
+ 	char	   *sslcert;		/* client certificate filename */
+ 	char	   *sslrootcert;	/* root certificate filename */
+ 	char	   *sslcrl;			/* certificate revocation list filename */
+ #endif
+ 
  #if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
  	char	   *krbsrvname;		/* Kerberos service name */
  #endif
#9Magnus Hagander
magnus@hagander.net
In reply to: Alex Hunsaker (#8)
Re: SSL configure patch: review

Alex Hunsaker wrote:

On Fri, Aug 1, 2008 at 13:31, Alvaro Herrera <alvherre@commandprompt.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.

Obviously one still wants to be able to specify a different file name
from the default; I tried to see if there's any way to load an engine
that would load the key from a file, but could not extract any sense
from the man page:
http://www.openssl.org/docs/crypto/engine.html

Maybe this means that we should provide separate parameters, say
"sslkey" and "sslkeyfile", and a new env var PGSSLKEYFILE.

Thoughts? Am I overengineering this stuff?

I think the patch as it stands is ok for now (mainly because I don't
have any hardware either so I can't test or attest to how it should be
done i.e. if those params would be enough)

Should PGROOTCERT be PGSSLROOTCERT to be more congruent with all the
other ssl params?

Yes, I think that'd be a good idea.

Find attached an updated patch against HEAD (no other changes). I
also gave it a look over and tested it to make sure it worked as
described.

I still think the parameters should be available for non-SSL builds, but
ignored. None of them put any restrictions in place, so it's not an
issue to ignore them if we are not going to obey them (if SSL isn't used
anyway), and it'll make life easier for autogenerated connection strings.

I also think we should rename the variables internally. conn->sslcert
sounds very much to me like it'd be the certificate in use for the
connection, which it isn't. How about "sslcertname", "sslcrlname" etc?
(no need to change the user visible stuff, just in the code to make it
easier to follow elsewhere)

We still have the issue with the environment variable not matching the
connection string one that needs to be resolved.

Thoughts?

(I have added this to the commitfest page, as it was lost)

//Magnus

#10Alvaro Herrera
alvherre@commandprompt.com
In reply to: Magnus Hagander (#9)
Re: SSL configure patch: review

Magnus Hagander escribi�:

Alex Hunsaker wrote:

On Fri, Aug 1, 2008 at 13:31, Alvaro Herrera <alvherre@commandprompt.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.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

#11Magnus Hagander
magnus@hagander.net
In reply to: Alvaro Herrera (#10)
Re: SSL configure patch: review

Alvaro Herrera wrote:

Magnus Hagander escribi�:

Alex Hunsaker wrote:

On Fri, Aug 1, 2008 at 13:31, Alvaro Herrera <alvherre@commandprompt.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.

.. that's the one I was referring to in my mail ...

It should definitely be made consistent.

//MAgnus