Thread: 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_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
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
Attachment
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.
Attachment
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
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
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
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
Attachment
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.
Attachment
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
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.
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