These are my review comments on Mark Woodward's "SSL configure patch":
http://archives.postgresql.org/message-id/[EMAIL PROTECTED] (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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
