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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
pgconn-hostaddr-fix-poc.patchtext/x-patch; name=pgconn-hostaddr-fix-poc.patchDownload+42-5
On Fri, Nov 25, 2016 at 8:15 AM, Andreas Karlsson <andreas@proxel.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@postgresql.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
https://www.postgresql.org/docs/9.6/static/libpq-connect.html#LIBPQ-PARAMKEYWORDS.
Andreas
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Nov 25, 2016 at 10:41 AM, Tsunakawa, Takayuki <
tsunakawa.takay@jp.fujitsu.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@postgresql.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
https://www.postgresql.org/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:
https://www.postgresql.org/docs/devel/static/libpq-connect.html
Regards
Takayuki Tsunakawa
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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:
https://www.postgresql.org/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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Nov 25, 2016 at 12:03 PM, Andreas Karlsson <andreas@proxel.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@proxel.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:https://www.postgresql.org/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_ADDRESSThis 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson <andreas@proxel.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-12-01 14:22:19 -0500, Robert Haas wrote:
On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson <andreas@proxel.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Dec 1, 2016 at 2:40 PM, Andres Freund <andres@anarazel.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@proxel.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-12-01 14:43:04 -0500, Robert Haas wrote:
On Thu, Dec 1, 2016 at 2:40 PM, Andres Freund <andres@anarazel.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@proxel.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
:(
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Dec 1, 2016 at 2:45 PM, Andres Freund <andres@anarazel.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Nov 24, 2016 at 4:38 PM, Andreas Karlsson <andreas@proxel.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Dec 1, 2016 at 2:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Thu, Dec 1, 2016 at 2:56 PM, Tom Lane <tgl@sss.pgh.pa.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
