Re: Supporting Windows SChannel as OpenSSL replacement - Mailing list pgsql-hackers

Heikki Linnakangas wrote:

> I did again the refactoring you did back in 2006, patch attached.
> One thing I did differently: I moved the raw, non-encrypted,
> read/write functions to separate functions: pqsecure_raw_read and
> pqsecure_raw_write. Those functions encapsulate the SIGPIPE
> handling. The OpenSSL code implements a custom BIO, which calls to
> pqsecure_raw_read/write to do the low-level I/O.  Similarly in the
> server-side, there are be_tls_raw_read and pg_tls_raw_write
> functions, which do the
> prepare_for_client_read()/client_read_ended() dance, so that the SSL
> implementation doesn't need to know about that.

I'm skimming over this patch (0001).  There are some issues:

* You duplicated the long comment under the IDENTIFICATION tag that was in be-secure.c; it's now both in that file and
alsoin be-secure-openssl.c.  I think it should be removed from be-secure.c. Also, the hardcoded DH params are
duplicatedin be-secure.c, but they belong in -openssl.c only now.
 

* There is some mixup regarding USE_SSL and USE_OPENSSL in be-secure.c. I think anything that's OpenSSL-specific should
bein be-secure-openssl.c only; any new SSL implementation will need to implement all those functions.  For instance,
be_tls_init().I imagine that if we select any SSL implementation, USE_SSL would get defined, and each SSL
implementationwould additionally define its own symbol.  Unless the idea is to get rid of USE_OPENSSL completely, and
useonly the Makefile bit to decide which implementation to use?  If so, then USE_OPENSSL as a preprocessor symbol is
useless...
 

* ssl_renegotiation_limit is also duplicated.  But removing this one is probably not going to be as easy as deleting a
linefrom be-secure.c, because guc.c depends on that one.  I think that variable should be defined in be-secure.c (i.e.
deleteit from -openssl) and make sure that all SSL implementations enforce it on their own somehow.
 

The DISABLE_SIGPIPE thingy looks wrong in pqsecure_write.  I think it
should be like this:

ssize_t
pqsecure_write(PGconn *conn, const void *ptr, size_t len)
{ssize_t        n;

#ifdef USE_SSLif (conn->ssl_in_use){    DECLARE_SIGPIPE_INFO(spinfo);
    DISABLE_SIGPIPE(conn, spinfo, return -1);
    n = pgtls_write(conn, ptr, len);
    RESTORE_SIGPIPE(spinfo);}else 
#endif   /* USE_OPENSSL */{    n = pqsecure_raw_write(conn, ptr, len);}
return n;
}

You are missing the restore call, and I moved the declaration inside the
ssl_in_use block since otherwise it's not useful.  The other path is
fine since pqsecure_raw_write disables and restores the flag by itself.
Also, you're missing DECLARE/DISABLE/RESTORE in the ssl_in_use block in
pqsecure_read.  (The original code does not have that code in the
non-SSL path.  I assume, without checking, that that's okay.)  I also
assume without checking that all SSL implementations would be fine with
this SIGPIPE handling.

Another thing that seems wrong is the REMEMBER_EPIPE stuff.  The
fe-secure-openssl.c code should be setting the flag, but AFAICS only the
non-SSL code is doing it.


Thanks for working on this -- I'm sure many distributors will be happy
to be able to enable other, less license-broken TLS implementations.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services