Обсуждение: [HACKERS] Error-like LOG when connecting with SSL for password authentication

Hi all,

When attempting to connect using password authentication through SSL,
the backend will complain in its log with the following entry before
calling sendAuthRequest(), which asks the client for a password:
LOG:  could not receive data from client: Connection reset by peer

After a short talk with Heikki, it seems that be_tls_read() complains
on SSL_ERROR_ZERO_RETURN, which is documented here:
https://wiki.openssl.org/index.php/Manual:SSL_get_error(3)
The TLS/SSL connection has been closed. If the protocol version is SSL
3.0 or TLS 1.0, this result code is returned only if a closure alert
has occurred in the protocol, i.e. if the connection has been closed
cleanly. Note that in this case SSL_ERROR_ZERO_RETURN does not
necessarily indicate that the underlying transport has been closed.

As this is a clean shutdown of the SSL connection, shouldn't
be_tls_read() return 0 to the caller instead of -1? This would map
with what the non-SSL code path does for raw reads.

This is basically harmless, but the error message is confusing I
think, and there is no equivalent for the non-SSL code path.

Attached is an idea of patch.
Thoughts?
-- 
Michael

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 05/22/2017 03:10 AM, Michael Paquier wrote:
> Hi all,
>
> When attempting to connect using password authentication through SSL,
> the backend will complain in its log with the following entry before
> calling sendAuthRequest(), which asks the client for a password:
> LOG:  could not receive data from client: Connection reset by peer
>
> After a short talk with Heikki, it seems that be_tls_read() complains
> on SSL_ERROR_ZERO_RETURN, which is documented here:
> https://wiki.openssl.org/index.php/Manual:SSL_get_error(3)
> The TLS/SSL connection has been closed. If the protocol version is SSL
> 3.0 or TLS 1.0, this result code is returned only if a closure alert
> has occurred in the protocol, i.e. if the connection has been closed
> cleanly. Note that in this case SSL_ERROR_ZERO_RETURN does not
> necessarily indicate that the underlying transport has been closed.
>
> As this is a clean shutdown of the SSL connection, shouldn't
> be_tls_read() return 0 to the caller instead of -1? This would map
> with what the non-SSL code path does for raw reads.

Yeah. The be_tls_read() change looks OK to me.

Can SSL_ERROR_ZERO_RETURN also happen in a write? I suppose it can, but 
returning 0 from secure_write() seems iffy. My man page for send(2) 
doesn't say that it would return 0 if the connection has been closed by 
the peer, so that would be different from the non-SSL codepath. Looking 
at the only caller to secure_write(), it does check for 0, and would 
treat it correctly as EOF, so it would work. But it makes me a bit 
nervous, a comment in secure_write() at least would be in order, to 
point out that it can return 0 to indicate EOF, unlike the raw write(2) 
or send(2) system functions.

In libpq, do we want to set the "SSL connection has been closed 
unexpectedly" message? In the non-SSL case, pqsecure_raw_read() doesn't 
set any error message on EOF. Also, even though the comment in 
pgtls_read() says "... we should not report it as a server crash", 
looking at pqReadData, ISTM that returning 0 will in fact lead to the 
"server closed the connection unexpectedly" message. And it seems like a 
good assumption anyway, that the server did in fact terminate 
abnormally, if that happens. We don't expect the server to disconnect 
the client without notice, even though it's not unusual for the client 
to disconnect without notifying the server.

- Heikki

On 05/22/2017 10:11 PM, Vaishnavi Prabakaran wrote:
> On Mon, May 22, 2017 at 5:10 PM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>
>> If the protocol version is SSL
>> 3.0 or TLS 1.0, this result code is returned only if a closure alert
>> has occurred in the protocol, i.e. if the connection has been closed
>> cleanly. Note that in this case SSL_ERROR_ZERO_RETURN does not
>> necessarily indicate that the underlying transport has been closed.
>
> I guess this error code exist even for SSL2 protocol, In that case, don't
> we need to keep the current code for this error code?

If I understand correctly, with SSLv2, SSL_ERROR_ZERO_RETURN does mean 
that the underlying transport has been closed. Returning 0 seems 
appropriate in that case, too.

But the point is moot anyway, because PostgreSQL doesn't allow SSLv2 
anymore.

- Heikki

On Tue, May 23, 2017 at 6:36 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 05/22/2017 10:11 PM, Vaishnavi Prabakaran wrote:
>>
>> On Mon, May 22, 2017 at 5:10 PM, Michael Paquier
>> <michael.paquier@gmail.com>
>> wrote:
>>
>>> If the protocol version is SSL
>>> 3.0 or TLS 1.0, this result code is returned only if a closure alert
>>> has occurred in the protocol, i.e. if the connection has been closed
>>> cleanly. Note that in this case SSL_ERROR_ZERO_RETURN does not
>>> necessarily indicate that the underlying transport has been closed.
>>
>>
>> I guess this error code exist even for SSL2 protocol, In that case, don't
>> we need to keep the current code for this error code?
>
> If I understand correctly, with SSLv2, SSL_ERROR_ZERO_RETURN does mean that
> the underlying transport has been closed. Returning 0 seems appropriate in
> that case, too.

Am I reading the docs incorrectly then? I understand that with SSLv2
the transport may not be closed after SSL_ERROR_ZERO_RETURN.

> But the point is moot anyway, because PostgreSQL doesn't allow SSLv2
> anymore.

And SSL_OP_NO_SSLv2 is enforced anyway.

Side note.. Looking at the openssl docs, I am just noticing that
SSLv23_method has been marked as deprecated in 1.1.0:
https://www.openssl.org/docs/man1.1.0/ssl/SSLv23_method.html
And has been replaced by TLS_method. Something to keep in mind.
-- 
Michael

On Tue, May 23, 2017 at 6:26 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Yeah. The be_tls_read() change looks OK to me.
>
> Can SSL_ERROR_ZERO_RETURN also happen in a write? I suppose it can, but
> returning 0 from secure_write() seems iffy.

It seems to me that it could be the case, the man page of SSL_write
tells me that:
       0   The write operation was not successful. Probably the
underlying connection was closed. Call SSL_get_error() with the return
value ret to find out, whether an error occurred or the connection was
shut down cleanly (SSL_ERROR_ZERO_RETURN).

> My man page for send(2) doesn't
> say that it would return 0 if the connection has been closed by the peer, so
> that would be different from the non-SSL codepath.

errno maps to ECONNRESET in this case. So send() can return 0 only if
the caller has specified 0 as the number of bytes to send?

> Looking at the only
> caller to secure_write(), it does check for 0, and would treat it correctly
> as EOF, so it would work. But it makes me a bit nervous, a comment in
> secure_write() at least would be in order, to point out that it can return 0
> to indicate EOF, unlike the raw write(2) or send(2) system functions.

Yep. Agreed. Hopefully improved in the attached.

> In libpq, do we want to set the "SSL connection has been closed
> unexpectedly" message?

Perhaps not.

> In the non-SSL case, pqsecure_raw_read() doesn't set
> any error message on EOF. Also, even though the comment in pgtls_read() says
> "... we should not report it as a server crash", looking at pqReadData, ISTM
> that returning 0 will in fact lead to the "server closed the connection
> unexpectedly" message. And it seems like a good assumption anyway, that the
> server did in fact terminate abnormally, if that happens. We don't expect
> the server to disconnect the client without notice, even though it's not
> unusual for the client to disconnect without notifying the server.

Yes.

Attached is an updated patch.
-- 
Michael

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 05/24/2017 05:29 PM, Michael Paquier wrote:
> Attached is an updated patch.

Thanks, I've pushed the backend read part of this patch. That's enough 
to fix the original complaint with password authentication. I think the 
rest was a bit dubious, and I'm hesitant to commit that (or at least to 
backport):

* Backend write: you wouldn't really expect a client to disconnect, 
while the backend is trying to send something to it. You'll get an error 
in the non-SSL case too, although I guess the error message would be 
different.

* Frontend read: pqReadData has this, after calling pqsecure_read:

>     /*
>      * A return value of 0 could mean just that no data is now available, or
>      * it could mean EOF --- that is, the server has closed the connection.
>      * Since we have the socket in nonblock mode, the only way to tell the
>      * difference is to see if select() is saying that the file is ready.
>      * Grumble.  Fortunately, we don't expect this path to be taken much,
>      * since in normal practice we should not be trying to read data unless
>      * the file selected for reading already.
>      *
>      * In SSL mode it's even worse: SSL_read() could say WANT_READ and then
>      * data could arrive before we make the pqReadReady() test, but the second
>      * SSL_read() could still say WANT_READ because the data received was not
>      * a complete SSL record.  So we must play dumb and assume there is more
>      * data, relying on the SSL layer to detect true EOF.
>      */
>
> #ifdef USE_SSL
>     if (conn->ssl_in_use)
>         return 0;
> #endif

* Frontend write: Same as in the backend, I think having the server 
disconnect while you're trying to send to it is not expected. Not sure 
if the error message is here again different, but seems best to just 
leave it alone.

- Heikki

On Mon, Jul 3, 2017 at 9:02 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Thanks, I've pushed the backend read part of this patch.

Thanks.
-- 
Michael

Hi Michael,

I tried to apply your patch to HEAD and it failed.  But I think that's because some version of it has already been
committed,correct?  I see some of your ECONNRESET and "SSL connection has been closed unexpectedly" code already in
HEAD.

Best,
Ryan

The new status of this patch is: Waiting on Author

On Thu, Jul 6, 2017 at 12:45 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:
> I tried to apply your patch to HEAD and it failed.
> But I think that's because some version of it has already been committed, correct?  I see some of your ECONNRESET and
"SSLconnection has been closed unexpectedly" code already in HEAD.
 

Thanks Ryan for the reminder. Heikki has pushed a minimum patch set as
b93827c7. So I have updated the CF app accordingly.
-- 
Michael