Re: new libpq SSL connection option - Mailing list pgsql-hackers

Alex Hunsaker wrote:
> On Fri, Dec 5, 2008 at 14:22, Andrew Chernow <ac@esilo.com> wrote:
>> Alex Hunsaker wrote:
>>> On Fri, Dec 5, 2008 at 13:58, Andrew Chernow <ac@esilo.com> wrote:
>>>> Who anyone be opposed to "ssldir = path" as a connection option?
>>>> Currently,
>>>> there is no way to change the homedir method ~/.postgresql ... or am I
>>>> missing something?  I am willing to supply a patch.
>>> You mean something like the
>>>
>>> http://archives.postgresql.org/message-id/34d269d40811202107q489e2be0h771762398dd9fcdb@mail.gmail.com.
>>>
>>> ?
>>>
>> yes, excately like that; apparently missed it.  What is the status of that
>> patch?  I see it was left in pending review  .. is the fest is over?
>
> I think all that is left is changing PGROOTCERT to PGSSLROOTCERT,
> agreeing to IFDEF the params out or not oh
>  and this little bit:
>
>> Magnus Hagander escribió:
>>> On Fri, Aug 1, 2008 at 13:31, Alvaro Herrera <alvherre(at)commandprompt(dot)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.


Here's a suggested update, which does *not* yet have documentation
updates. Changes from previous patch:

* Made all parameters available always and ignored for non-SSL connections
* Renamed PGROOTCERT to PGSSLROOTCERT
* Changes the way PGSSLKEY/sslkey is handled to this: When the string
does not contain a colon, it's treated as a filename. If it does contain
a colon (and on windows, if this colon is not in the second position
indicating a drive letter), it's treated as engine:key as before.

This should keep backwards compatibility.


I would also like to look this over completely - we only support loading
the KEY from the smartcard, but you still have to manually copy the
certificate to your machine. I don't know exactly how you're supposed to
do this in OpenSSL - some googling shows almost nobody else uses the
functions quite the way we do. So I'd like to look over if we need to do
more around this later, but this patch should make it possible to use
keys from different files without breaking backwards compatibility with
what we had before. So I'm considering that a separate step, that may
not be done in time for 8.4.


So. Comments?

//Magnus
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
***************
*** 318,323 ****
--- 318,367 ----
          </varlistentry>

          <varlistentry>
+          <term><literal>sslcert</literal></term>
+          <listitem>
+           <para>
+            This parameter specifies the file name of the client SSL
+            certificate.  This option is only available if
+            <productname>PostgreSQL</> is compiled with SSL support.
+           </para>
+          </listitem>
+         </varlistentry>
+
+         <varlistentry>
+          <term><literal>sslkey</literal></term>
+          <listitem>
+           <para>
+            This parameter specifies the file name of the client SSL key.
+            This option is only available if <productname>PostgreSQL</> is
+            compiled with SSL support.
+           </para>
+          </listitem>
+         </varlistentry>
+
+         <varlistentry>
+          <term><literal>sslrootcert</literal></term>
+          <listitem>
+           <para>
+            This parameter specifies the file name of the root SSL certificate.
+            This option is only available if <productname>PostgreSQL</> is
+            compiled with SSL support.
+           </para>
+          </listitem>
+         </varlistentry>
+
+         <varlistentry>
+          <term><literal>sslcrl</literal></term>
+          <listitem>
+           <para>
+            This parameter specifies the file name of the SSL certificate
+            revocation list (CRL).  This option is only available if
+            <productname>PostgreSQL</> is compiled with SSL support.
+           </para>
+          </listitem>
+         </varlistentry>
+
+         <varlistentry>
           <term><literal>krbsrvname</literal></term>
           <listitem>
            <para>
***************
*** 5778,5783 **** myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
--- 5822,5849 ----
      <listitem>
       <para>
        <indexterm>
+        <primary><envar>PGROOTCERT</envar></primary>
+       </indexterm>
+       <envar>PGROOTCERT</envar> specifies the file name where the SSL
+       root certificate is stored.  This can be overridden by the
+       <literal>sslrootcert</literal> connection parameter.
+      </para>
+     </listitem>
+
+     <listitem>
+      <para>
+       <indexterm>
+        <primary><envar>PGSSLCRL</envar></primary>
+       </indexterm>
+       <envar>PGSSLCRL</envar> specifies the file name where the SSL certificate
+       revocation list is stored.  This can be overridden by the
+       <literal>sslcrl</literal> connection parameter.
+      </para>
+     </listitem>
+
+     <listitem>
+      <para>
+       <indexterm>
         <primary><envar>PGKRBSRVNAME</envar></primary>
        </indexterm>
        <envar>PGKRBSRVNAME</envar> sets the Kerberos service name to use
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
***************
*** 177,184 **** static const PQconninfoOption PQconninfoOptions[] = {
  #endif

      /*
!      * "sslmode" option is allowed even without client SSL support because the
!      * client can still handle SSL modes "disable" and "allow".
       */
      {"sslmode", "PGSSLMODE", DefaultSSLMode, NULL,
      "SSL-Mode", "", 8},            /* sizeof("disable") == 8 */
--- 177,186 ----
  #endif

      /*
!      * ssl options are allowed even without client SSL support because the
!      * client can still handle SSL modes "disable" and "allow". Other parameters
!      * have no effect on non-SSL connections, so there is no reason to exclude them
!      * since none of them are mandatory.
       */
      {"sslmode", "PGSSLMODE", DefaultSSLMode, NULL,
      "SSL-Mode", "", 8},            /* sizeof("disable") == 8 */
***************
*** 186,191 **** static const PQconninfoOption PQconninfoOptions[] = {
--- 188,205 ----
      {"sslverify", "PGSSLVERIFY", DefaultSSLVerify, NULL,
      "SSL-Verify", "", 5},        /* sizeof("chain") == 5 */

+     {"sslcert", "PGSSLCERT", NULL, NULL,
+     "SSL-Client-Cert", "", 64},
+
+     {"sslkey", "PGSSLKEY", NULL, NULL,
+     "SSL-Client-Key", "", 64},
+
+     {"sslrootcert", "PGSSLROOTCERT", NULL, NULL,
+     "SSL-Root-Certificate", "", 64},
+
+     {"sslcrl", "PGSSLCRL", NULL, NULL,
+     "SSL-Revocation-List", "", 64},
+
  #if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
      /* Kerberos and GSSAPI authentication support specifying the service name */
      {"krbsrvname", "PGKRBSRVNAME", PG_KRB_SRVNAM, NULL,
***************
*** 419,424 **** connectOptions1(PGconn *conn, const char *conninfo)
--- 433,446 ----
      conn->sslmode = tmp ? strdup(tmp) : NULL;
      tmp = conninfo_getval(connOptions, "sslverify");
      conn->sslverify = tmp ? strdup(tmp) : NULL;
+     tmp = conninfo_getval(connOptions, "sslkey");
+     conn->sslkey = tmp ? strdup(tmp) : NULL;
+     tmp = conninfo_getval(connOptions, "sslcert");
+     conn->sslcert = tmp ? strdup(tmp) : NULL;
+     tmp = conninfo_getval(connOptions, "sslrootcert");
+     conn->sslrootcert = tmp ? strdup(tmp) : NULL;
+     tmp = conninfo_getval(connOptions, "sslcrl");
+     conn->sslcrl = tmp ? strdup(tmp) : NULL;
  #ifdef USE_SSL
      tmp = conninfo_getval(connOptions, "requiressl");
      if (tmp && tmp[0] == '1')
***************
*** 2032,2037 **** freePGconn(PGconn *conn)
--- 2054,2067 ----
          free(conn->sslmode);
      if (conn->sslverify)
          free(conn->sslverify);
+     if (conn->sslcert)
+         free(conn->sslcert);
+     if (conn->sslkey)
+         free(conn->sslkey);
+     if (conn->sslrootcert)
+         free(conn->sslrootcert);
+     if (conn->sslcrl)
+         free(conn->sslcrl);
  #if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
      if (conn->krbsrvname)
          free(conn->krbsrvname);
*** a/src/interfaces/libpq/fe-secure.c
--- b/src/interfaces/libpq/fe-secure.c
***************
*** 568,574 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey)
      }

      /* read the user certificate */
!     snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE);

      /*
       * OpenSSL <= 0.9.8 lacks error stack handling, which means it's likely to
--- 568,577 ----
      }

      /* read the user certificate */
!     if (conn->sslcert)
!         strncpy(fnbuf, conn->sslcert, sizeof(fnbuf));
!     else
!         snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE);

      /*
       * OpenSSL <= 0.9.8 lacks error stack handling, which means it's likely to
***************
*** 618,677 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey)

      BIO_free(bio);

! #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
!     if (getenv("PGSSLKEY"))
      {
!         /* read the user key from engine */
!         char       *engine_env = getenv("PGSSLKEY");
!         char       *engine_colon = strchr(engine_env, ':');
!         char       *engine_str;
!         ENGINE       *engine_ptr;
!
!         if (!engine_colon)
          {
!             printfPQExpBuffer(&conn->errorMessage,
!                               libpq_gettext("invalid value of PGSSLKEY environment variable\n"));
!             ERR_pop_to_mark();
!             return 0;
!         }

!         engine_str = malloc(engine_colon - engine_env + 1);
!         strlcpy(engine_str, engine_env, engine_colon - engine_env + 1);
!         engine_ptr = ENGINE_by_id(engine_str);
!         if (engine_ptr == NULL)
!         {
!             char       *err = SSLerrmessage();

!             printfPQExpBuffer(&conn->errorMessage,
!                      libpq_gettext("could not load SSL engine \"%s\": %s\n"),
!                               engine_str, err);
!             SSLerrfree(err);
!             free(engine_str);
!             ERR_pop_to_mark();
!             return 0;
!         }

!         *pkey = ENGINE_load_private_key(engine_ptr, engine_colon + 1,
!                                         NULL, NULL);
!         if (*pkey == NULL)
!         {
!             char       *err = SSLerrmessage();

!             printfPQExpBuffer(&conn->errorMessage,
!                               libpq_gettext("could not read private SSL key \"%s\" from engine \"%s\": %s\n"),
!                               engine_colon + 1, engine_str, err);
!             SSLerrfree(err);
              free(engine_str);
!             ERR_pop_to_mark();
!             return 0;
          }
-         free(engine_str);
      }
      else
- #endif   /* use PGSSLKEY */
      {
!         /* read the user key from file */
          snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_KEY_FILE);
          if (stat(fnbuf, &buf) != 0)
          {
              printfPQExpBuffer(&conn->errorMessage,
--- 621,698 ----

      BIO_free(bio);

!     /*
!      * Read the SSL key. If a key is specified, treat it as an engine:key combination
!      * if there is colon present - we don't support files with colon in the name. The
!      * exception is if the second character is a colon, in which case it can be a Windows
!      * filename with drive specification.
!      */
!     if (conn->sslkey && strlen(conn->sslkey) > 0)
      {
! #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
!         if (strchr(conn->sslkey, ':')
! #ifdef WIN32
!             && conn->sslkey[1] != ':'
! #endif
!            )
          {
!             /* Colon, but not in second character, treat as engine:key */
!             ENGINE       *engine_ptr;
!             char       *engine_str = strdup(conn->sslkey);
!             char       *engine_colon = strchr(engine_str, ':');

!             *engine_colon = '\0';    /* engine_str now has engine name */
!             engine_colon++;            /* engine_colon now has key name */

!             engine_ptr = ENGINE_by_id(engine_str);
!             if (engine_ptr == NULL)
!             {
!                 char       *err = SSLerrmessage();

!                 printfPQExpBuffer(&conn->errorMessage,
!                                   libpq_gettext("could not load SSL engine \"%s\": %s\n"),
!                                   engine_str, err);
!                 SSLerrfree(err);
!                 free(engine_str);
!                 ERR_pop_to_mark();
!                 return 0;
!             }

!             *pkey = ENGINE_load_private_key(engine_ptr, engine_colon,
!                                             NULL, NULL);
!             if (*pkey == NULL)
!             {
!                 char       *err = SSLerrmessage();
!
!                 printfPQExpBuffer(&conn->errorMessage,
!                                   libpq_gettext("could not read private SSL key \"%s\" from engine \"%s\": %s\n"),
!                                   engine_colon, engine_str, err);
!                 SSLerrfree(err);
!                 free(engine_str);
!                 ERR_pop_to_mark();
!                 return 0;
!             }
              free(engine_str);
!
!             fnbuf[0] = '\0'; /* indicate we're not going to load from a file */
!         }
!         else
! #endif /* support for SSL engines */
!         {
!             /* PGSSLKEY is not an engine, treat it as a filename */
!             strncpy(fnbuf, conn->sslkey, sizeof(fnbuf));
          }
      }
      else
      {
!         /* No PGSSLKEY specified, load default file */
          snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_KEY_FILE);
+     }
+
+     if (fnbuf[0] != '\0')
+     {
+         /* read the user key from file */
+
          if (stat(fnbuf, &buf) != 0)
          {
              printfPQExpBuffer(&conn->errorMessage,
***************
*** 948,954 **** initialize_SSL(PGconn *conn)
      /* Set up to verify server cert, if root.crt is present */
      if (pqGetHomeDirectory(homedir, sizeof(homedir)))
      {
!         snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CERT_FILE);
          if (stat(fnbuf, &buf) == 0)
          {
              X509_STORE *cvstore;
--- 969,979 ----
      /* Set up to verify server cert, if root.crt is present */
      if (pqGetHomeDirectory(homedir, sizeof(homedir)))
      {
!         if (conn->sslrootcert)
!             strncpy(fnbuf, conn->sslrootcert, sizeof(fnbuf));
!         else
!             snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CERT_FILE);
!
          if (stat(fnbuf, &buf) == 0)
          {
              X509_STORE *cvstore;
***************
*** 966,973 **** initialize_SSL(PGconn *conn)

              if ((cvstore = SSL_CTX_get_cert_store(SSL_context)) != NULL)
              {
                  /* setting the flags to check against the complete CRL chain */
!                 if (X509_STORE_load_locations(cvstore, ROOT_CRL_FILE, NULL) != 0)
  /* OpenSSL 0.96 does not support X509_V_FLAG_CRL_CHECK */
  #ifdef X509_V_FLAG_CRL_CHECK
                      X509_STORE_set_flags(cvstore,
--- 991,1003 ----

              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, fnbuf, NULL) != 0)
  /* OpenSSL 0.96 does not support X509_V_FLAG_CRL_CHECK */
  #ifdef X509_V_FLAG_CRL_CHECK
                      X509_STORE_set_flags(cvstore,
*** a/src/interfaces/libpq/libpq-int.h
--- b/src/interfaces/libpq/libpq-int.h
***************
*** 292,297 **** struct pg_conn
--- 292,302 ----
      char       *pgpass;
      char       *sslmode;        /* SSL mode (require,prefer,allow,disable) */
      char       *sslverify;        /* Verify server SSL certificate (none,chain,cn) */
+     char       *sslkey;            /* client key filename */
+     char       *sslcert;        /* client certificate filename */
+     char       *sslrootcert;    /* root certificate filename */
+     char       *sslcrl;            /* certificate revocation list filename */
+
  #if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
      char       *krbsrvname;        /* Kerberos service name */
  #endif