Skip site navigation (1) Skip section navigation (2)

SSL configure patch: review

From: Abhijit Menon-Sen <ams(at)oryx(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Cc: pgsql(at)mohawksoft(dot)com
Subject: SSL configure patch: review
Date: 2008-07-08 02:57:29
Message-ID: 20080708025729.GA18515@toroid.org (view raw or flat)
Thread:
Lists: pgsql-hackers
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

Responses

pgsql-hackers by date

Next:From: Abhijit Menon-SenDate: 2008-07-08 03:05:52
Subject: Re: SSL configure patch: review
Previous:From: Alvaro HerreraDate: 2008-07-08 02:18:20
Subject: Re: 8.1 index corruption woes

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group