SSL configure patch: review - Mailing list 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_FILEvalue 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
certificaterevocation list  filename" in the comments. (It's not clear what the "call back" is in  this context; nor
doesit 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