Thread: 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
Attachment
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
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/withouthostaddr specified.
However, I wonder whether the hostaddr parameter should also accept multiple IP addresses. Currently, it accepts only
oneaddress 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
isset, we're only going to try to * connect to that one particular address. If it's not, we'll use pghost, *
whichmay 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 https://www.postgresql.org/docs/9.6/static/libpq-connect.html#LIBPQ-PARAMKEYWORDS. Andreas
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.
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
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
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?
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.
--
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) returnconn->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@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
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
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
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 :(
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
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
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
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
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
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
On Thu, Dec 1, 2016 at 3:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > 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". 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.cy@enterprisedb.com> wrote: > 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. 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@gmail.com> wrote: > 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. + 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@gmail.com> wrote:
> On Fri, Nov 25, 2016 at 4:16 AM, Mithun Cy <mithun.cy@enterprisedb.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
producingapplications often use the reserved characters allowed in a segment to delimit scheme-specific or
dereference-handler-specificsubcomponents. For example, the semicolon (";") and equals ("=") reserved characters are
oftenused to delimit parameters and parameter values applicable to that segment. The comma (",") reserved character
isoften used for similar purposes. For example, one URI producer might use a segment such as "name;v=1.1" to
indicatea 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
specificto 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@gmail.com> wrote: > On Thu, Dec 1, 2016 at 3:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> 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". > > 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.paquier@gmail.com> wrote: > On Fri, Dec 2, 2016 at 5:17 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> 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. > > + 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@gmail.com> wrote: > On Thu, Dec 1, 2016 at 4:42 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Fri, Dec 2, 2016 at 5:17 AM, Robert Haas <robertmhaas@gmail.com> wrote: >>> 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. >> >> + 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.paquier@gmail.com> writes:
> On Fri, Dec 2, 2016 at 8:36 AM, Robert Haas <robertmhaas@gmail.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@sss.pgh.pa.us> wrote: > Michael Paquier <michael.paquier@gmail.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@gmail.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.
On Thu, Dec 1, 2016 at 9:31 PM, Michael Paquier <michael.paquier@gmail.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.cy@enterprisedb.com> wrote:
>On Fri, Dec 2, 2016 at 2:26 AM, Robert Haas <robertmhaas@gmail.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.
Attachment
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@iki.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@gmail.com> wrote: > On Mon, Dec 12, 2016 at 1:34 AM, Heikki Linnakangas <hlinnaka@iki.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