Thread: 9.5: Can't connect with PGSSLMODE=require on Windows

Hi,

With 9.5 alpha 2 on Windows 8 (64-bit), trying to require SSL results
in a blocking error:

pg_hba.conf:
hostssl   postgres   postgres   0.0.0.0/0   trust

postgresql.conf:
ssl=on


C:\Program Files\PostgreSQL\9.5\bin>SET PGSSLMODE=require

C:\Program Files\PostgreSQL\9.5\bin>psql.exe -p 5432 -U postgres -h
localhost postgres
psql: SSL SYSCALL error: Operation would block (0x00002733/10035)



Doing the same thing on Linux doesn't result in the error (I'm using
latest Git master here):

thom@swift:~/Development/data$ PGSSLMODE=require psql -U thom -p 5488
-d postgres -h 127.0.0.1
psql (9.6devel)
SSL connection (protocol: TLSv1.2, cipher:
ECDHE-RSA-AES256-GCM-SHA384, bits: 256, compression: off)
Type "help" for help.

postgres=#

Thom

On Tue, Sep 22, 2015 at 9:54 AM, Thom Brown <thom@linux.com> wrote:
> Hi,
>
> With 9.5 alpha 2 on Windows 8 (64-bit), trying to require SSL results
> in a blocking error:
>
> pg_hba.conf:
> hostssl   postgres   postgres   0.0.0.0/0   trust
>
> postgresql.conf:
> ssl=on
>
>
> C:\Program Files\PostgreSQL\9.5\bin>SET PGSSLMODE=require
>
> C:\Program Files\PostgreSQL\9.5\bin>psql.exe -p 5432 -U postgres -h
> localhost postgres
> psql: SSL SYSCALL error: Operation would block (0x00002733/10035)
>
>
>
> Doing the same thing on Linux doesn't result in the error (I'm using
> latest Git master here):
>
> thom@swift:~/Development/data$ PGSSLMODE=require psql -U thom -p 5488
> -d postgres -h 127.0.0.1
> psql (9.6devel)
> SSL connection (protocol: TLSv1.2, cipher:
> ECDHE-RSA-AES256-GCM-SHA384, bits: 256, compression: off)
> Type "help" for help.
>
> postgres=#

This, by the way, was reported internally to EDB by our QA team.  My
understanding is that it doesn't happen on 9.4, but nobody knows yet
which 9.5 commit broke it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 09/22/2015 10:57 AM, Robert Haas wrote:
> On Tue, Sep 22, 2015 at 9:54 AM, Thom Brown <thom@linux.com> wrote:
>> Hi,
>>
>> With 9.5 alpha 2 on Windows 8 (64-bit), trying to require SSL results
>> in a blocking error:
>>
>> pg_hba.conf:
>> hostssl   postgres   postgres   0.0.0.0/0   trust
>>
>> postgresql.conf:
>> ssl=on
>>
>>
>> C:\Program Files\PostgreSQL\9.5\bin>SET PGSSLMODE=require
>>
>> C:\Program Files\PostgreSQL\9.5\bin>psql.exe -p 5432 -U postgres -h
>> localhost postgres
>> psql: SSL SYSCALL error: Operation would block (0x00002733/10035)
>>
>>
>>
>> Doing the same thing on Linux doesn't result in the error (I'm using
>> latest Git master here):
>>
>> thom@swift:~/Development/data$ PGSSLMODE=require psql -U thom -p 5488
>> -d postgres -h 127.0.0.1
>> psql (9.6devel)
>> SSL connection (protocol: TLSv1.2, cipher:
>> ECDHE-RSA-AES256-GCM-SHA384, bits: 256, compression: off)
>> Type "help" for help.
>>
>> postgres=#
> This, by the way, was reported internally to EDB by our QA team.  My
> understanding is that it doesn't happen on 9.4, but nobody knows yet
> which 9.5 commit broke it.
>

"git bisect" is your friend.

cheers

andrew

On Tue, Sep 22, 2015 at 11:23 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
> "git bisect" is your friend.

Yeah, but finding someone who has a working Windows build environment
and a lot of time to run this down is my enemy.  We're trying, but if
anyone else has a clue, that would be much appreciated.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 23 September 2015 at 13:10, Michael Paquier
<michael.paquier@gmail.com> wrote:
>
>
> On Wed, Sep 23, 2015 at 2:15 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> On Tue, Sep 22, 2015 at 11:23 AM, Andrew Dunstan <andrew@dunslane.net>
>> wrote:
>> > "git bisect" is your friend.
>>
>> Yeah, but finding someone who has a working Windows build environment
>> and a lot of time to run this down is my enemy.  We're trying, but if
>> anyone else has a clue, that would be much appreciated.
>
>
> That's not cool. I have added this problem in the list of open items for
> 9.5.

This appears that it might be related to the version of OpenSSL that's
been packaged with PostgreSQL 9.5 alpha 2.  When swapping this out for
the version that's shipped with 9.4, it works.  I don't have the
specific OpenSSL versions to hand, but I'll report back anything as I
learn more.

-- 
Thom

Asif Naeem wrote:
> I have spent sometime to investigate the issue, it is reproduciable. In
> case of Windows, when pqsecure_raw_read() function error code
> WSAEWOULDBLOCK (EWOULDBLOCK) when no data queued to be read from the non
> blocking socket there is a need to log retry flag. Related error code can
> be retrieved via Windows WSAGetLastError() instead of errno, preprocessor
> SOCK_ERRNO handle it gracefully.

Hmm, wow.  I think you should also change my_sock_write.

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

Asif Naeem <anaeem.it@gmail.com> writes:
> I have spent sometime to investigate the issue, it is reproduciable. In
> case of Windows, when pqsecure_raw_read() function error code
> WSAEWOULDBLOCK (EWOULDBLOCK) when no data queued to be read from the non
> blocking socket there is a need to log retry flag. Related error code can
> be retrieved via Windows WSAGetLastError() instead of errno, preprocessor
> SOCK_ERRNO handle it gracefully. PFA patch, it resolve the issue i.e.

> @@ -1601,7 +1601,7 @@ my_sock_read(BIO *h, char *buf, int size)
>      int            save_errno;
>  
>      res = pqsecure_raw_read((PGconn *) h->ptr, buf, size);
> -    save_errno = errno;
> +    save_errno = SOCK_ERRNO;
>      BIO_clear_retry_flags(h);
>      if (res < 0)
>      {


Great detective work!  But if that's broken, then surely the identical
code in my_sock_write is as well; and the reassignment to errno at the
bottom of my_sock_read needs to be SOCK_ERRNO_SET(); and why doesn't
my_sock_write have a reassignment at all?
        regards, tom lane

On 2015-09-28 16:57:24 -0400, Tom Lane wrote:
> Asif Naeem <anaeem.it@gmail.com> writes:
> > I have spent sometime to investigate the issue, it is reproduciable. In
> > case of Windows, when pqsecure_raw_read() function error code
> > WSAEWOULDBLOCK (EWOULDBLOCK) when no data queued to be read from the non
> > blocking socket there is a need to log retry flag. Related error code can
> > be retrieved via Windows WSAGetLastError() instead of errno, preprocessor
> > SOCK_ERRNO handle it gracefully. PFA patch, it resolve the issue i.e.
> 
> > @@ -1601,7 +1601,7 @@ my_sock_read(BIO *h, char *buf, int size)
> >      int            save_errno;
> >  
> >      res = pqsecure_raw_read((PGconn *) h->ptr, buf, size);
> > -    save_errno = errno;
> > +    save_errno = SOCK_ERRNO;
> >      BIO_clear_retry_flags(h);
> >      if (res < 0)
> >      {
> 
> 
> Great detective work!  But if that's broken, then surely the identical
> code in my_sock_write is as well; and the reassignment to errno at the
> bottom of my_sock_read needs to be SOCK_ERRNO_SET(); and why doesn't
> my_sock_write have a reassignment at all?

I wonder if we couldn't remove saving/restoring errno entirely from
my_sock_*. We didn't do so before 680513ab79 and I can't see a reason
why we'd need to now.

What I do find curious is that afaics before 680513ab79 the code also
looked at errno, not SOCK_ERRNO. And apparently things worked back then?
I guess the difference is that pgsecure_raw_read now unconditionally
does SOCK_ERRNO_SET(result_errno).

Greetings,

Andres Freund

I wrote:
> ... and the reassignment to errno at the
> bottom of my_sock_read needs to be SOCK_ERRNO_SET(); and why doesn't
> my_sock_write have a reassignment at all?

Comparison to the backend versions of these routines, which have been
through quite a few releases, suggests that the reassignment to errno at
the bottom of my_sock_read is simply bogus/unnecessary.  There is no
reason to believe that BIO_clear_retry_flags, BIO_set_retry_read, or
BIO_set_retry_write will munge errno.  Hence we should remove that flight
of fantasy rather than clone it into my_sock_write.
        regards, tom lane

Andres Freund <andres@anarazel.de> writes:
> On 2015-09-28 16:57:24 -0400, Tom Lane wrote:
>> Great detective work!  But if that's broken, then surely the identical
>> code in my_sock_write is as well; and the reassignment to errno at the
>> bottom of my_sock_read needs to be SOCK_ERRNO_SET(); and why doesn't
>> my_sock_write have a reassignment at all?

> I wonder if we couldn't remove saving/restoring errno entirely from
> my_sock_*. We didn't do so before 680513ab79 and I can't see a reason
> why we'd need to now.

Agreed, see my comment to the same effect.

> What I do find curious is that afaics before 680513ab79 the code also
> looked at errno, not SOCK_ERRNO. And apparently things worked back then?

No; AFAICS, before that commit, libpq did not use a custom BIO at all.
That commit cloned the backend's custom BIO, but did not correctly
adjust the backend's errno handling for the libpq environment.

Will go fix it.
        regards, tom lane

On 2015-09-28 17:28:48 -0400, Tom Lane wrote:

> > What I do find curious is that afaics before 680513ab79 the code also
> > looked at errno, not SOCK_ERRNO. And apparently things worked back then?
> 
> No; AFAICS, before that commit, libpq did not use a custom BIO at all.
> That commit cloned the backend's custom BIO, but did not correctly
> adjust the backend's errno handling for the libpq environment.

Oh, yea.

> Will go fix it.

We now probably could remove* XXX OpenSSL 1.0.1e considers many more errcodes than just EINTR as reasons* to retry; do
weneed to adopt their logic for that?
 
since we now actually check for more tahn just EINTR.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> We now probably could remove
>  * XXX OpenSSL 1.0.1e considers many more errcodes than just EINTR as reasons
>  * to retry; do we need to adopt their logic for that?
> since we now actually check for more tahn just EINTR.

Well, that comment is cloned from the backend which is already checking
for all three of these errno codes.  I am too lazy to go look at the
OpenSSL code right now, but my recollection is that they checked for
some truly weird stuff, not just the expected spellings of EINTR.
        regards, tom lane

Thom Brown <thom@linux.com> writes:
> With 9.5 alpha 2 on Windows 8 (64-bit), trying to require SSL results
> in a blocking error:

I've pushed a patch for this; can you verify it on Windows?
        regards, tom lane

Asif Naeem <anaeem.it@gmail.com> writes:
> Thank you Tom. The issue seems not reproducible anymore with latest PG95
> source code (commit 60fcee9e5e77dc748a9787fae34328917683b95e) Windows build

Thanks for testing!  I've marked this issue resolved in the 9.5 open-items
list.
        regards, tom lane