Re: Broken SSL tests in master

Hi,

The SSL test suite (src/test/ssl) is broken in the master since commit
9a1d0af4ad2cbd419115b453d811c141b80d872b, which is Robert's refactoring
of getting the server hostname for GSS, SSPI, and SSL in libpq.

The error we get in the test suite:

# Running: psql -X -A -t -c SELECT 'connected with user=ssltestuser
dbname=trustdb sslcert=invalid hostaddr=127.0.0.1
host=common-name.pg-ssltest.test sslrootcert=ssl/root+server_ca.crt
sslmode=verify-full' -d user=ssltestuser dbname=trustdb sslcert=invalid
hostaddr=127.0.0.1 host=common-name.pg-ssltest.test
sslrootcert=ssl/root+server_ca.crt sslmode=verify-full
psql: server certificate for "common-name.pg-ssltest.test" does not
match host name "127.0.0.1"

As you can see, after the patch libpq will now look at hostaddr rather
than host when validating the server certificate because that is what is
stored in the first (and only) entry of conn->connhost, and therefore
what PQhost() return.

To me it feels like the proper fix would be to make PQHost() return the
value of the host parameter rather than the hostaddr (maybe add a new
field in the pg_conn_host struct). But would be a behaviour change which
might break someones application. Thoughts?

Andreas

On 11/24/2016 10:38 PM, Andreas Karlsson wrote:
> To me it feels like the proper fix would be to make PQHost() return the
> value of the host parameter rather than the hostaddr (maybe add a new
> field in the pg_conn_host struct). But would be a behaviour change which
> might break someones application. Thoughts?

I have attached a proof of concept patch for this. Remaining work is
investigating all the callers of PQhost() and see if any of them are
negatively affected by this patch and cleaning up the code some.

Andreas

On Fri, Nov 25, 2016 at 8:15 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> On 11/24/2016 10:38 PM, Andreas Karlsson wrote:
>> To me it feels like the proper fix would be to make PQHost() return the
>> value of the host parameter rather than the hostaddr (maybe add a new
>> field in the pg_conn_host struct). But would be a behaviour change which
>> might break someones application. Thoughts?

Thanks for digging up the root cause. I can see the problem with HEAD as well.

> I have attached a proof of concept patch for this. Remaining work is
> investigating all the callers of PQhost() and see if any of them are
> negatively affected by this patch and cleaning up the code some.

This needs some thoughts, but first I need to understand the
whereabouts of this refactoring work in 9a1d0af4 as well as the
structures that 274bb2b3 has introduced. From what I can see you are
duplicating some logic parsing the pghost string when there is a
pghostaddr set, so my first guess is that you are breaking some of the
intentions behind this code by patching it this way... I am adding in
CC Robert, Mithun and Tsunakawa-san who worked on that. On my side,
I'll need some time to study and understand this new code, that's
necessary before looking at your patch in details, there could be a
more elegant solution. And we had better address this issue before
looking more into the SSL reload patch.
--
Michael

From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Andreas Karlsson
> On 11/24/2016 10:38 PM, Andreas Karlsson wrote:
> > To me it feels like the proper fix would be to make PQHost() return
> > the value of the host parameter rather than the hostaddr (maybe add a
> > new field in the pg_conn_host struct). But would be a behaviour change
> > which might break someones application. Thoughts?
>
> I have attached a proof of concept patch for this. Remaining work is
> investigating all the callers of PQhost() and see if any of them are
> negatively affected by this patch and cleaning up the code some.

I agree that pg_conn_host should have hostaddr in addition to host, and PQhost() return host when host is specified with/without hostaddr specified.

However, I wonder whether the hostaddr parameter should also accept multiple IP addresses. Currently, it accepts only one address as follows. I asked Robert and Mithun about this, but I forgot about that.

static bool
connectOptions2(PGconn *conn)
{
/*
* Allocate memory for details about each host to which we might possibly
* try to connect. If pghostaddr is set, we're only going to try to
* connect to that one particular address. If it's not, we'll use pghost,
* which may contain multiple, comma-separated names.
*/

Regards
Takayuki Tsunakawa

On 11/25/2016 06:11 AM, Tsunakawa, Takayuki wrote:
> However, I wonder whether the hostaddr parameter should also accept multiple IP addresses.

Yeah, I too thought about if we should fix that. I feel like it would
make sense to add support for multiple hostaddrs. For consitency's sake
if nothing else.

By the way is comma separated hosts documented somewhere? It is not
included in
/docs/9.6/static/libpq-connect.html#LIBPQ-PARAMKEYWORDS.

Andreas

On Fri, Nov 25, 2016 at 10:41 AM, Tsunakawa, Takayuki <
tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com> wrote:
> I agree that pg_conn_host should have hostaddr in addition to host, and
PQhost() return host when host is specified with/without hostaddr specified.

typedef struct pg_conn_host
+{
*+ char *host; /* host name or address, or socket path */*
*+ pg_conn_host_type type; /* type of host */*
+ char *port; /* port number for this host; if not NULL,
+ * overrrides the PGConn's pgport */
+ char *password; /* password for this host, read from the
+ * password file. only set if the PGconn's
+ * pgpass field is NULL. */
+ struct addrinfo *addrlist; /* list of possible backend addresses */
+} pg_conn_host;

+typedef enum pg_conn_host_type
+{
+ CHT_HOST_NAME,
+ CHT_HOST_ADDRESS,
+ CHT_UNIX_SOCKET
+} pg_conn_host_type;

host parameter stores both hostname and hostaddr, and we already have
parameter "type" to identify same.
I think we should not be using PQHost() directly in
verify_peer_name_matches_certificate_name (same holds good for GSS, SSPI).
Instead proceed only if "conn->connhost[conn->whichhost]" is a
"CHT_HOST_NAME".
Also further old PQHost() did not produce CHT_HOST_ADDRESS as its output so
we might need to revert back to old behaviour.

>However, I wonder whether the hostaddr parameter should also accept
multiple IP addresses. Currently, it accepts only one address as follows.
I >asked Robert and Mithun about this, but I forgot about that.

As far as I know only pghost allowed to have multiple host. And, pghostaddr
takes only one numeric address.

--
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

From: pgsql-hackers-owner(at)postgresql(dot)org
> sense to add support for multiple hostaddrs. For consitency's sake if
> nothing else.

Yes, consistency and performance. The purpose of hostaddr is to speed up connection by eliminating DNS lookup, isn't it? Then, some users should want to specify multiple IP addresses for hostaddr and omit host.

> By the way is comma separated hosts documented somewhere? It is not included
> in
> /docs/9.6/static/libpq-connect.html#LIBPQ-PA
> RAMKEYWORDS.

Specifying multiple hosts is a new feature to be introduced in v10, so that's here:

/docs/devel/static/libpq-connect.html

Regards
Takayuki Tsunakawa

On 11/25/2016 07:19 AM, Tsunakawa, Takayuki wrote:
> Specifying multiple hosts is a new feature to be introduced in v10, so that's here:
>
> /docs/devel/static/libpq-connect.html

Thanks, I had missed that patch. If we add support for multiple hosts I
think we should also allow for specifying the corresponding multiple
hostaddrs.

Another thought about this code: should we not check if it is a unix
socket first before splitting the host? While I doubt that it is common
to have a unix socket in a directory with comma in the path it is a bit
annoying that we no longer support this.

Andreas

On Fri, Nov 25, 2016 at 12:03 PM, Andreas Karlsson <andreas(at)proxel(dot)se>
wrote:
> Another thought about this code: should we not check if it is a unix
socket first before splitting the host? While I doubt that it is common to
have a unix >socket in a directory with comma in the path it is a bit
annoying that we no longer support this.

I think it is a bug.

*Before this feature:*

./psql postgres://%2fhome%2fmithun%2f%2c
psql: could not connect to server: No such file or directory
Is the server running locally and accepting
connections on Unix domain socket "*/home/mithun/,/.*s.PGSQL.5444"?

*After this feature:*
./psql postgres://%2fhome%2fmithun%2f%2c
psql: could not connect to server: No such file or directory
Is the server running locally and accepting
connections on Unix domain socket "*/home/mithun//.*s.PGSQL.5432"?
*could not connect to server: Connection refused*
* Is the server running on host "" (::1) and accepting*
* TCP/IP connections on port 5432?*
*could not connect to server: Connection refused*
* Is the server running on host "" (127.0.0.1) and accepting*
* TCP/IP connections on port 5432?*

So comma (%2c) is misinterpreted as separator not as part of UDS path.

Reason is we first decode the URI(percent encoded character) then try to
split the string into multiple host assuming they are separated by *','*. I
think we need to change the order here. Otherwise difficult the say whether
*','* is part of USD path or a separator.

--
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

On Fri, Nov 25, 2016 at 3:33 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> On 11/25/2016 07:19 AM, Tsunakawa, Takayuki wrote:
>>
>> Specifying multiple hosts is a new feature to be introduced in v10, so
>> that's here:
>>
>> /docs/devel/static/libpq-connect.html
>
>
> Thanks, I had missed that patch. If we add support for multiple hosts I
> think we should also allow for specifying the corresponding multiple
> hostaddrs.
>
> Another thought about this code: should we not check if it is a unix socket
> first before splitting the host? While I doubt that it is common to have a
> unix socket in a directory with comma in the path it is a bit annoying that
> we no longer support this.

I had a look at the proposed patch as well as the multi-host
infrastructure that Robert has introduced, and as far as I can see the
contract of PQHost() is broken because of this code in
connectOptions2:
if (conn->pghostaddr != NULL && conn->pghostaddr[0] != '\0')
{
conn->connhost[0].host = strdup(conn->pghostaddr);
if (conn->connhost[0].host == NULL)
goto oom_error;
conn->connhost[0].type = CHT_HOST_ADDRESS;
}
This fills in the array of hosts arbitrarily a host IP. And this
breaks when combined with this code in PQhost():
if (!conn)
return NULL;
if (conn->connhost != NULL)
return conn->connhost[conn->whichhost].host;
else if (conn->pghost != NULL && conn->pghost[0] != '\0')
return conn->pghost;
So this makes PQhost return an address number even if a name is
available, which is not correct per what PQhost should do. If connhost
has multiple entries, it is definitely right to return the one
whichhost is pointing to and not fallback to pghost which may be a
list of names separated by commas. But if pghostaddr is defined *and*
there is a name available, we had better return the name that
whichhost is pointing to. The most correct fix in my opinion asdasd

- conn->connhost[0].host = strdup(conn->pghostaddr);
- if (conn->connhost[0].host == NULL)
+ if (conn->pghost != NULL && conn->pghost[0] != '\0')
+ {
+ char *e = conn->pghost;
+
+ /*
+ * Search for the end of the first hostname; a comma or
+ * end-of-string acts as a terminator.
+ */
+ while (*e != '\0' && *e != ',')
+ ++e;
+
+ /* Copy the hostname whose bounds we just identified. */
+ conn->connhost[0].host =
+ (char *) malloc(sizeof(char) * (e - conn->pghost + 1));
+ if (conn->connhost[0].host == NULL)
+ goto oom_error;
+ memcpy(conn->connhost[0].host, conn->pghost, e - conn->pghost);
+ conn->connhost[0].host[e - conn->pghost] = '\0';
+ }
+ else
+ {
+ conn->connhost[0].host = strdup(conn->pghostaddr);
+ if (conn->connhost[0].host == NULL)
+ goto oom_error;
+ }
+
+ conn->connhost[0].hostaddr = strdup(conn->pghostaddr);
+ if (conn->connhost[0].hostaddr == NULL)
goto oom_error;
conn->connhost[0].type = CHT_HOST_ADDRESS

This breaks the case where users specify both host and hostaddr in a
connection string as this would force users to do an extra lookup at
which IP a host name is mapping, which is not good.

As we know that if hostaddr is specified, the number of entries in the
connhost array will be one, wouldn't the most correct fix, based on
the current implementation of multi-hosts and its limitations, be to
tweak PQhost() so as the first element in pghost is returned back to
the caller instead of an entry of type CHT_HOST_ADDRESS? And if pghost
is NULL, we should return PG_DEFAULT_HOST which is the same way of
doing things as before multi-host was implemented. We definitely need
to make PQhost() not return any host IPs if host names are available,
but not forget the case where a host can be specified as an IP itself.

Robert, others, what do you think?
--
Michael

On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> The SSL test suite (src/test/ssl) is broken in the master since commit
> 9a1d0af4ad2cbd419115b453d811c141b80d872b, which is Robert's refactoring of
> getting the server hostname for GSS, SSPI, and SSL in libpq.

So, we have no buildfarm coverage for this test suite? And make
check-world doesn't run it? Sigh.

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

On 2016-12-01 14:22:19 -0500, Robert Haas wrote:
> On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> > The SSL test suite (src/test/ssl) is broken in the master since commit
> > 9a1d0af4ad2cbd419115b453d811c141b80d872b, which is Robert's refactoring of
> > getting the server hostname for GSS, SSPI, and SSL in libpq.
>
> So, we have no buildfarm coverage for this test suite? And make
> check-world doesn't run it? Sigh.

The story behind that is that it opens the server over tcp to everyone
who has a copy of our test CA. Which is oh-so-secretly stored in our git
repo..

Andres

On Thu, Dec 1, 2016 at 2:40 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-12-01 14:22:19 -0500, Robert Haas wrote:
>> On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>> > The SSL test suite (src/test/ssl) is broken in the master since commit
>> > 9a1d0af4ad2cbd419115b453d811c141b80d872b, which is Robert's refactoring of
>> > getting the server hostname for GSS, SSPI, and SSL in libpq.
>>
>> So, we have no buildfarm coverage for this test suite? And make
>> check-world doesn't run it? Sigh.
>
> The story behind that is that it opens the server over tcp to everyone
> who has a copy of our test CA. Which is oh-so-secretly stored in our git
> repo..

I get that, but this is the second time in very recent history that
I've broken something because there was code that wasn't compiled or
tests that weren't run by 'make check-world'. I do run that for many
of my commits even though it takes 15 minutes. It's frustrating to me
that it leaves random stuff out and there's no alternative target that
includes that stuff; I don't like breaking things. Of course if my
code reviewing were perfect it wouldn't matter, but I haven't figured
out how to do that yet.

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

On 2016-12-01 14:43:04 -0500, Robert Haas wrote:
> On Thu, Dec 1, 2016 at 2:40 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2016-12-01 14:22:19 -0500, Robert Haas wrote:
> >> On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> >> > The SSL test suite (src/test/ssl) is broken in the master since commit
> >> > 9a1d0af4ad2cbd419115b453d811c141b80d872b, which is Robert's refactoring of
> >> > getting the server hostname for GSS, SSPI, and SSL in libpq.
> >>
> >> So, we have no buildfarm coverage for this test suite? And make
> >> check-world doesn't run it? Sigh.
> >
> > The story behind that is that it opens the server over tcp to everyone
> > who has a copy of our test CA. Which is oh-so-secretly stored in our git
> > repo..
>
> I get that, but this is the second time in very recent history that
> I've broken something because there was code that wasn't compiled or
> tests that weren't run by 'make check-world'. I do run that for many
> of my commits even though it takes 15 minutes. It's frustrating to me
> that it leaves random stuff out and there's no alternative target that
> includes that stuff; I don't like breaking things. Of course if my
> code reviewing were perfect it wouldn't matter, but I haven't figured
> out how to do that yet.

Well, I don't quite know what the alternative is. For some reason, which
I don't quite understand personally, people care about security during
regression tests runs. So we can't run the test automatedly. And nobody
has added a buildfarm module to run it manually on their servers either
:(

On Thu, Dec 1, 2016 at 2:45 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Well, I don't quite know what the alternative is. For some reason, which
> I don't quite understand personally, people care about security during
> regression tests runs. So we can't run the test automatedly. And nobody
> has added a buildfarm module to run it manually on their servers either
> :(

Well, if people are unwilling to add test suites to 'make
check-world', we can add 'make check-universe' and I'll run that
instead. And that can come with a big shiny disclaimer. I just want
a way to compile and run EVERYTHING that people care about not
breaking, which I think is frankly a pretty reasonable request!

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

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2016-12-01 14:43:04 -0500, Robert Haas wrote:
>> I get that, but this is the second time in very recent history that
>> I've broken something because there was code that wasn't compiled or
>> tests that weren't run by 'make check-world'.

> Well, I don't quite know what the alternative is. For some reason, which
> I don't quite understand personally, people care about security during
> regression tests runs. So we can't run the test automatedly. And nobody
> has added a buildfarm module to run it manually on their servers either
> :(

I don't think there's much substitute for knowing what tests we have
available.

In the particular case at hand, I wonder if we could generate new test
certificates every time (or at least have an option to do that) rather
than relying on premade ones. But I don't think it's realistic to imagine
that check-world will ever automatedly invoke every possible test.

regards, tom lane

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Well, if people are unwilling to add test suites to 'make
> check-world', we can add 'make check-universe' and I'll run that
> instead. And that can come with a big shiny disclaimer. I just want
> a way to compile and run EVERYTHING that people care about not
> breaking, which I think is frankly a pretty reasonable request!

Really? How are you going to test Windows-specific (or any-other-
platform-but-yours-specific) code? How about WORDS_BIGENDIAN code,
or code that is sensitive to alignment rules? Or code that breaks
in locales you haven't got, or depends on compile options you don't
use?

check-world isn't a magic bullet.

regards, tom lane

On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> As you can see, after the patch libpq will now look at hostaddr rather than
> host when validating the server certificate because that is what is stored
> in the first (and only) entry of conn->connhost, and therefore what PQhost()
> return.
>
> To me it feels like the proper fix would be to make PQHost() return the
> value of the host parameter rather than the hostaddr (maybe add a new field
> in the pg_conn_host struct). But would be a behaviour change which might
> break someones application. Thoughts?

I think that the blame here is on the original commit,
274bb2b3857cc987cfa21d14775cae9b0dababa5, which inadvertently changed
the behavior of PQhost. Prior to that commit, even if "hostaddr" was
used, PQhost would still return whatever value was associated with the
"host" parameter, but now it ignores "host" and returns "hostaddr"
instead. That's busted. I've pushed a trivial fix, and the SSL tests
now pass for me.

It might be that (as suggested downthread) we should consider
supporting multiple IPs in the hostaddr string as well, but that
requires some thought. For example, what happens if, for example, the
host and hostaddr lists are of unequal length? Would we accept one
host and >1 hostaddrs? Probably makes sense to just apply the host to
every hostaddr. >1 host and 1 hostaddr? Probably doesn't make sense,
but I guess you could argue for it. Equal length lists definitely
make sense.

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

On Thu, Dec 1, 2016 at 2:56 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Well, if people are unwilling to add test suites to 'make
>> check-world', we can add 'make check-universe' and I'll run that
>> instead. And that can come with a big shiny disclaimer. I just want
>> a way to compile and run EVERYTHING that people care about not
>> breaking, which I think is frankly a pretty reasonable request!
>
> Really? How are you going to test Windows-specific (or any-other-
> platform-but-yours-specific) code? How about WORDS_BIGENDIAN code,
> or code that is sensitive to alignment rules? Or code that breaks
> in locales you haven't got, or depends on compile options you don't
> use?
>
> check-world isn't a magic bullet.

No, but deliberately leaving things out that could be run isn't
helping anything either.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Dec 1, 2016 at 2:56 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> check-world isn't a magic bullet.

> No, but deliberately leaving things out that could be run isn't
> helping anything either.

Tests that open security holes while running aren't in my list of "things
that could be run automatically".

In any case, you're in a poor position to whine about this given that
parallel query is entirely unamenable to automated testing, and you've
resisted past proposals for improving that situation.

regards, tom lane

On Thu, Dec 1, 2016 at 3:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Thu, Dec 1, 2016 at 2:56 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> check-world isn't a magic bullet.
>
>> No, but deliberately leaving things out that could be run isn't
>> helping anything either.
>
> Tests that open security holes while running aren't in my list of "things
> that could be run automatically".

If we create a new target that runs them automatically, you don't have
to use it.

> In any case, you're in a poor position to whine about this given that
> parallel query is entirely unamenable to automated testing, and you've
> resisted past proposals for improving that situation.

Really?

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

On Fri, Nov 25, 2016 at 4:16 AM, Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com> wrote:
> On Fri, Nov 25, 2016 at 12:03 PM, Andreas Karlsson <andreas(at)proxel(dot)se>
> wrote:
>> Another thought about this code: should we not check if it is a unix
>> socket first before splitting the host? While I doubt that it is common to
>> have a unix >socket in a directory with comma in the path it is a bit
>> annoying that we no longer support this.
>
> I think it is a bug.
>
> Before this feature:
>
> ./psql postgres://%2fhome%2fmithun%2f%2c
> psql: could not connect to server: No such file or directory
> Is the server running locally and accepting
> connections on Unix domain socket "/home/mithun/,/.s.PGSQL.5444"?
>
> After this feature:
> ./psql postgres://%2fhome%2fmithun%2f%2c
> psql: could not connect to server: No such file or directory
> Is the server running locally and accepting
> connections on Unix domain socket "/home/mithun//.s.PGSQL.5432"?
> could not connect to server: Connection refused
> Is the server running on host "" (::1) and accepting
> TCP/IP connections on port 5432?
> could not connect to server: Connection refused
> Is the server running on host "" (127.0.0.1) and accepting
> TCP/IP connections on port 5432?
>
> So comma (%2c) is misinterpreted as separator not as part of UDS path.
>
> Reason is we first decode the URI(percent encoded character) then try to
> split the string into multiple host assuming they are separated by ','. I
> think we need to change the order here. Otherwise difficult the say whether
> ',' is part of USD path or a separator.

Yeah, we should change that. Are you going to write a patch?

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

On Fri, Dec 2, 2016 at 5:17 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>> As you can see, after the patch libpq will now look at hostaddr rather than
>> host when validating the server certificate because that is what is stored
>> in the first (and only) entry of conn->connhost, and therefore what PQhost()
>> return.
>>
>> To me it feels like the proper fix would be to make PQHost() return the
>> value of the host parameter rather than the hostaddr (maybe add a new field
>> in the pg_conn_host struct). But would be a behaviour change which might
>> break someones application. Thoughts?
>
> I think that the blame here is on the original commit,
> 274bb2b3857cc987cfa21d14775cae9b0dababa5, which inadvertently changed
> the behavior of PQhost. Prior to that commit, even if "hostaddr" was
> used, PQhost would still return whatever value was associated with the
> "host" parameter, but now it ignores "host" and returns "hostaddr"
> instead. That's busted. I've pushed a trivial fix, and the SSL tests
> now pass for me.

+ if (conn->connhost != NULL &&
+ conn->connhost[conn->whichhost].type != CHT_HOST_ADDRESS)
return conn->connhost[conn->whichhost].host
I think that's still incorrect. If a connection string defines a
comma-separated list of host, and hostaddr is defined as well,
PQhost() would return the comma-separated list, not the IP of the host
it is connected to. Am I reading that incorrectly?
--
Michael

On Fri, Dec 2, 2016 at 5:56 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Nov 25, 2016 at 4:16 AM, Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com> wrote:
>> Reason is we first decode the URI(percent encoded character) then try to
>> split the string into multiple host assuming they are separated by ','. I
>> think we need to change the order here. Otherwise difficult the say whether
>> ',' is part of USD path or a separator.
>
> Yeah, we should change that. Are you going to write a patch?

The interesting bit in rfc3986:

Aside from dot-segments in hierarchical paths, a path segment is
considered opaque by the generic syntax. URI producing applications
often use the reserved characters allowed in a segment to delimit
scheme-specific or dereference-handler-specific subcomponents. For
example, the semicolon (";") and equals ("=") reserved characters are
often used to delimit parameters and parameter values applicable to
that segment. The comma (",") reserved character is often used for
similar purposes. For example, one URI producer might use a segment
such as "name;v=1.1" to indicate a reference to version 1.1 of
"name", whereas another might use a segment such as "name,1.1" to
indicate the same. Parameter types may be defined by scheme-specific
semantics, but in most cases the syntax of a parameter is specific to
the implementation of the URI's dereferencing algorithm.

So not being able to distinguish commas in names and separators is
clearly a bug.
--
Michael

On Fri, Dec 2, 2016 at 5:46 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Dec 1, 2016 at 3:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Thu, Dec 1, 2016 at 2:56 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> check-world isn't a magic bullet.
>>
>>> No, but deliberately leaving things out that could be run isn't
>>> helping anything either.
>>
>> Tests that open security holes while running aren't in my list of "things
>> that could be run automatically".
>
> If we create a new target that runs them automatically, you don't have
> to use it.

Having a new target would make sense, if flagged as security-unsafe.
The reason why the SSL test suite is not in check-world is that SSL
cannot be used with unix domain sockets, making it unfit in shared
environments.
--
Michael

On Thu, Dec 1, 2016 at 4:42 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Dec 2, 2016 at 5:17 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>>> As you can see, after the patch libpq will now look at hostaddr rather than
>>> host when validating the server certificate because that is what is stored
>>> in the first (and only) entry of conn->connhost, and therefore what PQhost()
>>> return.
>>>
>>> To me it feels like the proper fix would be to make PQHost() return the
>>> value of the host parameter rather than the hostaddr (maybe add a new field
>>> in the pg_conn_host struct). But would be a behaviour change which might
>>> break someones application. Thoughts?
>>
>> I think that the blame here is on the original commit,
>> 274bb2b3857cc987cfa21d14775cae9b0dababa5, which inadvertently changed
>> the behavior of PQhost. Prior to that commit, even if "hostaddr" was
>> used, PQhost would still return whatever value was associated with the
>> "host" parameter, but now it ignores "host" and returns "hostaddr"
>> instead. That's busted. I've pushed a trivial fix, and the SSL tests
>> now pass for me.
>
> + if (conn->connhost != NULL &&
> + conn->connhost[conn->whichhost].type != CHT_HOST_ADDRESS)
> return conn->connhost[conn->whichhost].host
> I think that's still incorrect. If a connection string defines a
> comma-separated list of host, and hostaddr is defined as well,
> PQhost() would return the comma-separated list, not the IP of the host
> it is connected to. Am I reading that incorrectly?

Correct, but I'm defining that as user error. If hostaddr is
specified, host is not used to decide what to connect to, so it makes
no sense for it to be a string of multiple host names. If we allowed
multiple hostaddrs, as has been proposed, then we'd need to be more
clever about this.

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

On Fri, Dec 2, 2016 at 8:36 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Dec 1, 2016 at 4:42 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Fri, Dec 2, 2016 at 5:17 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>>>> As you can see, after the patch libpq will now look at hostaddr rather than
>>>> host when validating the server certificate because that is what is stored
>>>> in the first (and only) entry of conn->connhost, and therefore what PQhost()
>>>> return.
>>>>
>>>> To me it feels like the proper fix would be to make PQHost() return the
>>>> value of the host parameter rather than the hostaddr (maybe add a new field
>>>> in the pg_conn_host struct). But would be a behaviour change which might
>>>> break someones application. Thoughts?
>>>
>>> I think that the blame here is on the original commit,
>>> 274bb2b3857cc987cfa21d14775cae9b0dababa5, which inadvertently changed
>>> the behavior of PQhost. Prior to that commit, even if "hostaddr" was
>>> used, PQhost would still return whatever value was associated with the
>>> "host" parameter, but now it ignores "host" and returns "hostaddr"
>>> instead. That's busted. I've pushed a trivial fix, and the SSL tests
>>> now pass for me.
>>
>> + if (conn->connhost != NULL &&
>> + conn->connhost[conn->whichhost].type != CHT_HOST_ADDRESS)
>> return conn->connhost[conn->whichhost].host
>> I think that's still incorrect. If a connection string defines a
>> comma-separated list of host, and hostaddr is defined as well,
>> PQhost() would return the comma-separated list, not the IP of the host
>> it is connected to. Am I reading that incorrectly?
>
> Correct, but I'm defining that as user error. If hostaddr is
> specified, host is not used to decide what to connect to, so it makes
> no sense for it to be a string of multiple host names. If we allowed
> multiple hostaddrs, as has been proposed, then we'd need to be more
> clever about this.

Would it be better to return NULL instead then. Falling back to a
comma-separated list of hosts, or the default values does not sound
much appealing either.
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On Fri, Dec 2, 2016 at 8:36 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Correct, but I'm defining that as user error. If hostaddr is
>> specified, host is not used to decide what to connect to, so it makes
>> no sense for it to be a string of multiple host names. If we allowed
>> multiple hostaddrs, as has been proposed, then we'd need to be more
>> clever about this.

> Would it be better to return NULL instead then.

That would likely just result in application core dumps.
See notes for commit 490cb21f7. I think we've established an expectation
that only a NULL argument will elicit a NULL return from PQhost.

regards, tom lane

On Fri, Dec 2, 2016 at 10:26 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> Would it be better to return NULL instead then.
>
> That would likely just result in application core dumps.
> See notes for commit 490cb21f7.

That's 40cb21f7 actually. Thanks for the pointer.

> I think we've established an expectation
> that only a NULL argument will elicit a NULL return from PQhost.

OK, the current behavior wins then.
--
Michael

On 12/1/16 4:53 PM, Michael Paquier wrote:
> The reason why the SSL test suite is not in check-world is that SSL
> cannot be used with unix domain sockets, making it unfit in shared
> environments.

If that is it, that could be changed.

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

On Thu, Dec 01, 2016 at 03:17:34PM -0500, Robert Haas wrote:
> It might be that (as suggested downthread) we should consider
> supporting multiple IPs in the hostaddr string as well, but that
> requires some thought. For example, what happens if, for example, the
> host and hostaddr lists are of unequal length? Would we accept one
> host and >1 hostaddrs? Probably makes sense to just apply the host to
> every hostaddr. >1 host and 1 hostaddr? Probably doesn't make sense,
> but I guess you could argue for it. Equal length lists definitely
> make sense.

That would make the current code a huge plate of spagetthi for sanity
checks considering the multiple interations between port, host and
hostaddr. It seems to me that the current approach of supporting only
port and host is simple enough and will satisfy most of the user's
need plently. So +1 for simplicity.
--
Michael

On Fri, Dec 2, 2016 at 2:26 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>Yeah, we should change that. Are you going to write a patch?

Thanks, will work on this will produce a patch to patch to fix.

--
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

On Thu, Dec 1, 2016 at 9:31 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Thu, Dec 01, 2016 at 03:17:34PM -0500, Robert Haas wrote:
>> It might be that (as suggested downthread) we should consider
>> supporting multiple IPs in the hostaddr string as well, but that
>> requires some thought. For example, what happens if, for example, the
>> host and hostaddr lists are of unequal length? Would we accept one
>> host and >1 hostaddrs? Probably makes sense to just apply the host to
>> every hostaddr. >1 host and 1 hostaddr? Probably doesn't make sense,
>> but I guess you could argue for it. Equal length lists definitely
>> make sense.
>
> That would make the current code a huge plate of spagetthi for sanity
> checks considering the multiple interations between port, host and
> hostaddr. It seems to me that the current approach of supporting only
> port and host is simple enough and will satisfy most of the user's
> need plently. So +1 for simplicity.

I don't think it'd be ridiculously complicated to make it work and I
don't mind if someone wants to try. However, it wasn't interesting to
me, so I didn't spend time on it. A lot of these parameters are
intertwined, and I wanted to avoid trying to boil the ocean. But I'm
not allergic to follow-on patches.

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

On Fri, Dec 2, 2016 at 9:18 AM, Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com>
wrote:
>On Fri, Dec 2, 2016 at 2:26 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>Yeah, we should change that. Are you going to write a patch?
> Thanks, will work on this will produce a patch to patch to fix.

This is more complicated than I thought, I tried to fix same by delaying
the decoding of URI encoded hostname till connectOptions2(patch attached),
but that bring side effect for PQconninfoParse and PQconninfo which will
still show encoded string for "host" (also note %2x is a regular chars in
non-uri connection string). So I think even with uri connection string we
will be not able to use "," in hostname. I think probably just as in
regular connection string comma can only be used as separator.

--
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

On 12/01/2016 09:45 PM, Andres Freund wrote:
> And nobody has added a buildfarm module to run it manually on their
> servers either :(

I just added a module to run "make -C src/test/ssl check" in chipmunk.
So at least that's covered now.

http://pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=chipmunk&dt=2016-12-10%2012%3A08%3A31&stg=test-ssl-check

- Heikki

On Mon, Dec 12, 2016 at 1:34 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 12/01/2016 09:45 PM, Andres Freund wrote:
>>
>> And nobody has added a buildfarm module to run it manually on their
>> servers either :(
>
> I just added a module to run "make -C src/test/ssl check" in chipmunk. So at
> least that's covered now.
>
> http://pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=chipmunk&dt=2016-12-10%2012%3A08%3A31&stg=test-ssl-check

Thanks.

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

On Tue, Dec 13, 2016 at 10:07 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Dec 12, 2016 at 1:34 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>> On 12/01/2016 09:45 PM, Andres Freund wrote:
>>>
>>> And nobody has added a buildfarm module to run it manually on their
>>> servers either :(
>>
>> I just added a module to run "make -C src/test/ssl check" in chipmunk. So at
>> least that's covered now.
>>
>> http://pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=chipmunk&dt=2016-12-10%2012%3A08%3A31&stg=test-ssl-check
>
> Thanks.

Could you publish the module or send a pull request to Andrew?
--
Michael