Re: Supporting Windows SChannel as OpenSSL replacement
От | Heikki Linnakangas |
---|---|
Тема | Re: Supporting Windows SChannel as OpenSSL replacement |
Дата | |
Msg-id | 53DBD3F3.3030107@vmware.com обсуждение исходный текст |
Ответ на | Re: Supporting Windows SChannel as OpenSSL replacement (Alvaro Herrera <alvherre@2ndquadrant.com>) |
Список | pgsql-hackers |
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
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Heikki LinnakangasДата:
Сообщение: Re: Supporting Windows SChannel as OpenSSL replacement
Следующее
От: Heikki LinnakangasДата:
Сообщение: Re: Supporting Windows SChannel as OpenSSL replacement