Re: Supporting Windows SChannel as OpenSSL replacement

On 07/11/2014 08:39 PM, Alvaro Herrera wrote:
> 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 also in
> be-secure-openssl.c. I think it should be removed from be-secure.c.
> Also, the hardcoded DH params are duplicated in be-secure.c, but they
> belong in -openssl.c only now.

Hmm. Once we add other SSL implementations, shouldn't they share the
hardcoded DH parameters? That would warrant keeping them in be-secure.c.

> * There is some mixup regarding USE_SSL and USE_OPENSSL in be-secure.c.
> I think anything that's OpenSSL-specific should be in
> be-secure-openssl.c only; any new SSL implementation will need to
> implement all those functions. For instance, be_tls_init().

Agreed.

> I imagine that if we select any SSL implementation, USE_SSL would get
> defined, and each SSL implementation would additionally define its own
> symbol.

Yeah, that was the idea.

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

Agreed.

> 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_SSL
> if (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.

I think you're missing a change to the way fe-secure-openssl.c now uses
the OpenSSL library: it defines custom read/write functions,
my_sock_read and my_sock_write, which in turn call
pqsecure_raw_read/write. So all the actual I/O now goes through
pqsecure_raw_read/write. I believe it's therefore enough to put do the
REMEMBER_EPIPE in pqsecure_raw_write. Come to think of it,
pqsecure_write() shouldn't be doing any SIGPIGE stuff at all anymore.

- Heikki