SSL configure patch: review

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