Thread: [PATCH] Accept IP addresses in server certificate SANs
Hello all, libpq currently supports server certificates with a single IP address in the Common Name. It's fairly brittle; as far as I can tell, the single name you choose has to match the client's address exactly. Attached is a patch for libpq to support IP addresses in the server's Subject Alternative Names, which would allow admins to issue certs for multiple IP addresses, both IPv4 and IPv6, and mix them with alternative DNS hostnames. These addresses are compared bytewise instead of stringwise, so the client can contact the server via alternative spellings of the same IP address. This patch arose because I was writing tests for the NSS implementation that used a server cert with both DNS names and IP addresses, and then they failed when I ran those tests against the OpenSSL implementation. NSS supports this functionality natively. Anecdotally, I've heard from at least one client group who is utilizing IP-based certificates in their cloud deployments. It seems uncommon but still useful. There are two open questions I have; they're based on NSS implementation details that I did not port here: - NSS allows an IPv4 SAN to match an IPv6 mapping of that same address, and vice-versa. I chose not to implement that behavior, figuring it is easy enough for people to issue a certificate with both addresses. Is that okay? - If a certificate contains only iPAddress SANs, and none of them match, I fall back to check the certificate Common Name. OpenSSL will not do this (its X509_check_ip considers only SANs). NSS will only do this if the client's address is itself a DNS name. The spec says that we can't fall back to Common Name if the SANs contain any DNS entries, but it's silent on the subject of IP addresses. What should the behavior be? The patchset roadmap: - 0001 moves inet_net_pton() to src/port, since libpq will need it. - 0002 implements the new functionality and adds tests. WDYT? Thanks, --Jacob
Attachment
At Thu, 16 Dec 2021 01:13:57 +0000, Jacob Champion <pchampion@vmware.com> wrote in > This patch arose because I was writing tests for the NSS implementation > that used a server cert with both DNS names and IP addresses, and then > they failed when I ran those tests against the OpenSSL implementation. > NSS supports this functionality natively. Anecdotally, I've heard from > at least one client group who is utilizing IP-based certificates in > their cloud deployments. It seems uncommon but still useful. > > There are two open questions I have; they're based on NSS > implementation details that I did not port here: > > - NSS allows an IPv4 SAN to match an IPv6 mapping of that same address, > and vice-versa. I chose not to implement that behavior, figuring it > is easy enough for people to issue a certificate with both addresses. > Is that okay? > - If a certificate contains only iPAddress SANs, and none of them > match, I fall back to check the certificate Common Name. OpenSSL will > not do this (its X509_check_ip considers only SANs). NSS will only do > this if the client's address is itself a DNS name. The spec says that > we can't fall back to Common Name if the SANs contain any DNS > entries, but it's silent on the subject of IP addresses. What should > the behavior be? > > The patchset roadmap: > > - 0001 moves inet_net_pton() to src/port, since libpq will need it. > - 0002 implements the new functionality and adds tests. > > WDYT? In RFC2828 and 6125, > In some cases, the URI is specified as an IP address rather than a > hostname. In this case, the iPAddress subjectAltName must be present > in the certificate and must exactly match the IP in the URI. It seems like saying that we must search for iPAddress and mustn't use CN nor dNSName if the client connected using IP address. Otherwise, if the host name is a domain name, we use only dNSName if present, and use CN otherwise. That behavior seems agreeing to what you wrote as NSS's behavior. That being said it seems to me we should preserve that behavior at least for OpenSSL as an established behavior. In short, I think the current behavior of the patch is the direction we would go but some documentation is may be needed. I'm not sure about ipv4 comptible addresses. However, I think we can identify ipv4 compatible address easily. + * pg_inet_net_pton() will accept CIDR masks, which we don't want to + * match, so skip the comparison if the host string contains a slash. + */ + if (!strchr(host, '/') + && pg_inet_net_pton(PGSQL_AF_INET6, host, addr, -1) == 128) If a cidr is given, pg_inet_net_pton returns a number less than 128 so we don't need to check '/' explicity? (I'm not sure '/128' is sensible but doesn't harm..) regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On 12/15/21 20:13, Jacob Champion wrote: > Hello all, > > libpq currently supports server certificates with a single IP address > in the Common Name. It's fairly brittle; as far as I can tell, the > single name you choose has to match the client's address exactly. > > Attached is a patch for libpq to support IP addresses in the server's > Subject Alternative Names, which would allow admins to issue certs for > multiple IP addresses, both IPv4 and IPv6, and mix them with > alternative DNS hostnames. These addresses are compared bytewise > instead of stringwise, so the client can contact the server via > alternative spellings of the same IP address. Good job, this is certainly going to be useful. > > This patch arose because I was writing tests for the NSS implementation > that used a server cert with both DNS names and IP addresses, and then > they failed when I ran those tests against the OpenSSL implementation. > NSS supports this functionality natively. Anecdotally, I've heard from > at least one client group who is utilizing IP-based certificates in > their cloud deployments. It seems uncommon but still useful. > > There are two open questions I have; they're based on NSS > implementation details that I did not port here: > > - NSS allows an IPv4 SAN to match an IPv6 mapping of that same address, > and vice-versa. I chose not to implement that behavior, figuring it > is easy enough for people to issue a certificate with both addresses. > Is that okay? Sure. > > - If a certificate contains only iPAddress SANs, and none of them > match, I fall back to check the certificate Common Name. OpenSSL will > not do this (its X509_check_ip considers only SANs). NSS will only do > this if the client's address is itself a DNS name. The spec says that > we can't fall back to Common Name if the SANs contain any DNS > entries, but it's silent on the subject of IP addresses. What should > the behavior be? I don't think we should fall back on the CN. It would seem quite odd to do so for IP addresses but not for DNS names. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Thu, 2021-12-16 at 14:54 +0900, Kyotaro Horiguchi wrote: > In RFC2828 and 6125, > > > In some cases, the URI is specified as an IP address rather than a > > hostname. In this case, the iPAddress subjectAltName must be present > > in the certificate and must exactly match the IP in the URI. Ah, right, I misremembered. Disregard my statement that the spec is "silent on the subject", sorry. > It seems like saying that we must search for iPAddress and mustn't use > CN nor dNSName if the client connected using IP address. Otherwise, if > the host name is a domain name, we use only dNSName if present, and > use CN otherwise. That behavior seems agreeing to what you wrote as > NSS's behavior. NSS departs slightly from the spec and will additionally try to match an IP address against the CN, but only if there are no iPAddresses in the SAN. It roughly matches the logic for DNS names. Here's the description of the NSS behavior and some of the reasoning behind it, quoted from a developer on Bugzilla [1]: > Elsewhere in RFC 2818, it says > > If a subjectAltName extension of type dNSName is present, that MUST > be used as the identity. Otherwise, the (most specific) Common Name > field in the Subject field of the certificate MUST be used. > > Notice that this section is not conditioned upon the URI being a hostname > and not an IP address. So this statement conflicts with the one cited > above. > > I implemented this policy: > > if the URI contains a host name > if the subject alt name is present and has one or more DNS names > use the DNS names in that extension as the server identity > else > use the subject common name as the server identity > else if the URI contains an IP address > if the subject alt name is present and has one or more IP addresses > use the IP addresses in that extension as the server identity > else > compare the URI IP address string with the subject common name. It sounds like both you and Andrew might be comfortable with that same behavior? I think it looks like a sane solution, so I'll implement that and we can see what it looks like. (My work on this will be paused over the end-of-year holidays.) > That being said it seems to me we should preserve > that behavior at least for OpenSSL as an established behavior. That part is interesting. I'll talk more about that in my reply to Andrew. > In short, I think the current behavior of the patch is the direction > we would go but some documentation is may be needed. Great! > I'm not sure about ipv4 comptible addresses. However, I think we can > identify ipv4 compatible address easily. Yeah, it would probably not be a difficult feature to add later. > + * pg_inet_net_pton() will accept CIDR masks, which we don't want to > + * match, so skip the comparison if the host string contains a slash. > + */ > + if (!strchr(host, '/') > + && pg_inet_net_pton(PGSQL_AF_INET6, host, addr, -1) == 128) > > If a cidr is given, pg_inet_net_pton returns a number less than 128 so > we don't need to check '/' explicity? (I'm not sure '/128' is > sensible but doesn't harm..) Personally I think that, if someone wants your libpq to connect to a server with a hostname of "some:ipv6::address/128", then they are trying to pull something (evading a poorly coded blocklist, perhaps?) and we should not allow that to match an IP. Thoughts? Thanks for the review! --Jacob [1] https://bugzilla.mozilla.org/show_bug.cgi?id=103752
On Thu, 2021-12-16 at 10:50 -0500, Andrew Dunstan wrote: > Good job, this is certainly going to be useful. Thanks! > I don't think we should fall back on the CN. It would seem quite odd to > do so for IP addresses but not for DNS names. So there's at least one compatibility concern with disabling the fallback, in that there could be existing users that are happily using a certificate with an IP address CN, and libpq is just ignoring any iPAddress SANs that the certificate has. Once libpq becomes aware of those, it will stop accepting the CN and the certificate might stop working. Personally I think that's acceptable, but it would probably warrant a release note or some such. I will work on implementing behavior that's modeled off of the NSS matching logic (see my reply to Horiguchi-san), which will at least make it more logically consistent, and we can see what that looks like? Thanks for the review! --Jacob
At Thu, 16 Dec 2021 18:44:54 +0000, Jacob Champion <pchampion@vmware.com> wrote in > On Thu, 2021-12-16 at 14:54 +0900, Kyotaro Horiguchi wrote: > > It seems like saying that we must search for iPAddress and mustn't use > > CN nor dNSName if the client connected using IP address. Otherwise, if > > the host name is a domain name, we use only dNSName if present, and > > use CN otherwise. That behavior seems agreeing to what you wrote as > > NSS's behavior. > > NSS departs slightly from the spec and will additionally try to match > an IP address against the CN, but only if there are no iPAddresses in > the SAN. It roughly matches the logic for DNS names. OpenSSL seems different. X509_check_host() tries SAN then CN iff SAN doesn't exist. X509_check_ip() tries SAN and completely ignores iPAdress and CN. > Here's the description of the NSS behavior and some of the reasoning > behind it, quoted from a developer on Bugzilla [1]: > > > Elsewhere in RFC 2818, it says > > > > If a subjectAltName extension of type dNSName is present, that MUST > > be used as the identity. Otherwise, the (most specific) Common Name > > field in the Subject field of the certificate MUST be used. > > > > Notice that this section is not conditioned upon the URI being a hostname > > and not an IP address. So this statement conflicts with the one cited > > above. > > > > I implemented this policy: > > > > if the URI contains a host name > > if the subject alt name is present and has one or more DNS names > > use the DNS names in that extension as the server identity > > else > > use the subject common name as the server identity > > else if the URI contains an IP address > > if the subject alt name is present and has one or more IP addresses > > use the IP addresses in that extension as the server identity > > else > > compare the URI IP address string with the subject common name. (Wow. The article is 20-years old.) *I* am fine with it. > It sounds like both you and Andrew might be comfortable with that same > behavior? I think it looks like a sane solution, so I'll implement that > and we can see what it looks like. (My work on this will be paused over > the end-of-year holidays.) > > I'm not sure about ipv4 comptible addresses. However, I think we can > > identify ipv4 compatible address easily. > > Yeah, it would probably not be a difficult feature to add later. I agree. > > + * pg_inet_net_pton() will accept CIDR masks, which we don't want to > > + * match, so skip the comparison if the host string contains a slash. > > + */ > > + if (!strchr(host, '/') > > + && pg_inet_net_pton(PGSQL_AF_INET6, host, addr, -1) == 128) > > > > If a cidr is given, pg_inet_net_pton returns a number less than 128 so > > we don't need to check '/' explicity? (I'm not sure '/128' is > > sensible but doesn't harm..) > > Personally I think that, if someone wants your libpq to connect to a > server with a hostname of "some:ipv6::address/128", then they are > trying to pull something (evading a poorly coded blocklist, perhaps?) > and we should not allow that to match an IP. Thoughts? If the client could connect to the network-address, it could be said that we can assume that address is the name:p Just kidding. As the name suggests, the function reads a network address. And the only user is network_in(). I think we should provide pg_inet_pton() instead of abusing pg_inet_net_pton(). inet_net_pton_*() functions can be modified to reject /cidr part without regression so we are able to have pg_inet_pton() with a small amount of change. - inet_net_pton_ipv4(const char *src, u_char *dst) + inet_net_pton_ipv4_internal(const char *src, u_char *dst, bool netaddr) + inet_net_pton_ipv4(const char *src, u_char *dst) (calls inet_net_pton_ipv4_internal(src, dst, true)) + inet_pton_ipv4(const char *src, u_char *dst) (calls inet_net_pton_ipv4_internal(src, dst, false)) > Thanks for the review! > --Jacob > > [1] https://bugzilla.mozilla.org/show_bug.cgi?id=103752 -- Kyotaro Horiguchi NTT Open Source Software Center
Sorry for the silly mistake. At Fri, 17 Dec 2021 15:40:10 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > > NSS departs slightly from the spec and will additionally try to match > > an IP address against the CN, but only if there are no iPAddresses in > > the SAN. It roughly matches the logic for DNS names. > > OpenSSL seems different. X509_check_host() tries SAN then CN iff SAN > doesn't exist. X509_check_ip() tries SAN and completely ignores > iPAdress and CN. OpenSSL seems different. X509_check_host() tries SAN then CN iff SAN doesn't exist. X509_check_ip() tries iPAddress and completely ignores CN. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Hi, On 2021-12-16 01:13:57 +0000, Jacob Champion wrote: > Attached is a patch for libpq to support IP addresses in the server's > Subject Alternative Names, which would allow admins to issue certs for > multiple IP addresses, both IPv4 and IPv6, and mix them with > alternative DNS hostnames. These addresses are compared bytewise > instead of stringwise, so the client can contact the server via > alternative spellings of the same IP address. This fails to build on windows: https://cirrus-ci.com/task/6734650927218688?logs=build#L1029 [14:33:28.277] network.obj : error LNK2019: unresolved external symbol pg_inet_net_pton referenced in function network_in[c:\cirrus\postgres.vcxproj] Greetings, Andres Freund
On Fri, 2021-12-17 at 16:54 +0900, Kyotaro Horiguchi wrote: > Sorry for the silly mistake. > > At Fri, 17 Dec 2021 15:40:10 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > > > NSS departs slightly from the spec and will additionally try to match > > > an IP address against the CN, but only if there are no iPAddresses in > > > the SAN. It roughly matches the logic for DNS names. > > > > OpenSSL seems different. X509_check_host() tries SAN then CN iff SAN > > doesn't exist. X509_check_ip() tries SAN and completely ignores > > iPAdress and CN. > > OpenSSL seems different. X509_check_host() tries SAN then CN iff SAN > doesn't exist. X509_check_ip() tries iPAddress and completely ignores > CN. Right. On Fri, 2021-12-17 at 15:40 +0900, Kyotaro Horiguchi wrote: > > > + * pg_inet_net_pton() will accept CIDR masks, which we don't want to > > > + * match, so skip the comparison if the host string contains a slash. > > > + */ > > > + if (!strchr(host, '/') > > > + && pg_inet_net_pton(PGSQL_AF_INET6, host, addr, -1) == 128) > > > > > > If a cidr is given, pg_inet_net_pton returns a number less than 128 so > > > we don't need to check '/' explicity? (I'm not sure '/128' is > > > sensible but doesn't harm..) > > > > Personally I think that, if someone wants your libpq to connect to a > > server with a hostname of "some:ipv6::address/128", then they are > > trying to pull something (evading a poorly coded blocklist, perhaps?) > > and we should not allow that to match an IP. Thoughts? > > If the client could connect to the network-address, it could be said > that we can assume that address is the name:p Just kidding. > > As the name suggests, the function reads a network address. And the > only user is network_in(). I think we should provide pg_inet_pton() > instead of abusing pg_inet_net_pton(). inet_net_pton_*() functions > can be modified to reject /cidr part without regression so we are able > to have pg_inet_pton() with a small amount of change. > > - inet_net_pton_ipv4(const char *src, u_char *dst) > + inet_net_pton_ipv4_internal(const char *src, u_char *dst, bool netaddr) > > + inet_net_pton_ipv4(const char *src, u_char *dst) > (calls inet_net_pton_ipv4_internal(src, dst, true)) > + inet_pton_ipv4(const char *src, u_char *dst) > (calls inet_net_pton_ipv4_internal(src, dst, false)) Sounds good, I will make that change. Thanks for the feedback! --Jacob
On Sun, 2022-01-02 at 13:29 -0800, Andres Freund wrote: > Hi, > > On 2021-12-16 01:13:57 +0000, Jacob Champion wrote: > > Attached is a patch for libpq to support IP addresses in the server's > > Subject Alternative Names, which would allow admins to issue certs for > > multiple IP addresses, both IPv4 and IPv6, and mix them with > > alternative DNS hostnames. These addresses are compared bytewise > > instead of stringwise, so the client can contact the server via > > alternative spellings of the same IP address. > > This fails to build on windows: > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcirrus-ci.com%2Ftask%2F6734650927218688%3Flogs%3Dbuild%23L1029&data=04%7C01%7Cpchampion%40vmware.com%7C2b2171168f3c4935e89f08d9ce36f790%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637767557770534489%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=JtfsPtershSljU1oDGrkL8bQiHYB3iMfUgTqlh%2B4wbs%3D&reserved=0 > > [14:33:28.277] network.obj : error LNK2019: unresolved external symbol pg_inet_net_pton referenced in function network_in[c:\cirrus\postgres.vcxproj] Thanks for the heads up; I'll fix that while I'm implementing the internal API. --Jacob
On Thu, 2021-12-16 at 18:44 +0000, Jacob Champion wrote: > It sounds like both you and Andrew might be comfortable with that same > behavior? I think it looks like a sane solution, so I'll implement that > and we can see what it looks like. (My work on this will be paused over > the end-of-year holidays.) v2 implements the discussed CN/SAN fallback behavior and should fix the build on Windows. Still TODO is the internal pg_inet_pton() refactoring that you asked for; I'm still deciding how best to approach it. Changes only in since-v1.diff.txt. Thanks, --Jacob
Attachment
On Mon, 2022-01-03 at 16:19 +0000, Jacob Champion wrote: > On Fri, 2021-12-17 at 15:40 +0900, Kyotaro Horiguchi wrote: > > > > + inet_net_pton_ipv4(const char *src, u_char *dst) > > (calls inet_net_pton_ipv4_internal(src, dst, true)) > > + inet_pton_ipv4(const char *src, u_char *dst) > > (calls inet_net_pton_ipv4_internal(src, dst, false)) > > Sounds good, I will make that change. Thanks for the feedback! v3 implements a pg_inet_pton(), but for IPv6 instead of IPv4 as presented above (since we only need inet_pton() for IPv6 in this case). It's split into a separate patch (0003) for ease of review. Thanks! --Jacob
Attachment
At Thu, 6 Jan 2022 00:02:27 +0000, Jacob Champion <pchampion@vmware.com> wrote in > On Mon, 2022-01-03 at 16:19 +0000, Jacob Champion wrote: > > On Fri, 2021-12-17 at 15:40 +0900, Kyotaro Horiguchi wrote: > > > > > > + inet_net_pton_ipv4(const char *src, u_char *dst) > > > (calls inet_net_pton_ipv4_internal(src, dst, true)) > > > + inet_pton_ipv4(const char *src, u_char *dst) > > > (calls inet_net_pton_ipv4_internal(src, dst, false)) > > > > Sounds good, I will make that change. Thanks for the feedback! > > v3 implements a pg_inet_pton(), but for IPv6 instead of IPv4 as > presented above (since we only need inet_pton() for IPv6 in this case). > It's split into a separate patch (0003) for ease of review. 0001 looks fine as it is in the almost same shape withinet_net_pton about PGSQL_AF_INET and PGSQL_AF_INET6. I'm not sure about the difference on how to handle AF_INET6 between pg_inet_net_pton and ntop but that's not a matter of this patch. However, 0002, +/* + * In a frontend build, we can't include inet.h, but we still need to have + * sensible definitions of these two constants. Note that pg_inet_net_ntop() + * assumes that PGSQL_AF_INET is equal to AF_INET. + */ +#define PGSQL_AF_INET (AF_INET + 0) +#define PGSQL_AF_INET6 (AF_INET + 1) + Now we have the same definition thrice in frontend code. Coulnd't we define them in, say, libpq-fe.h or inet-fe.h (nonexistent) then include it from the three files? +$node->connect_fails( + "$common_connstr host=192.0.2.2", + "host not matching an IPv4 address (Subject Alternative Name 1)", It is not the real IP address of the server. https://datatracker.ietf.org/doc/html/rfc6125 > In some cases, the URI is specified as an IP address rather than a > hostname. In this case, the iPAddress subjectAltName must be > present in the certificate and must exactly match the IP in the URI. When IP address is embedded in URI, it won't be translated to another IP address. Concretely https://192.0.1.5/hoge cannot reach to the host 192.0.1.8. On the other hand, as done in the test, libpq allows that when "host=192.0.1.5 hostaddr=192.0.1.8". I can't understand what we are doing in that case. Don't we need to match the SAN IP address with hostaddr instead of host? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Mon, 2022-01-31 at 17:29 +0900, Kyotaro Horiguchi wrote: > However, 0002, > > +/* > + * In a frontend build, we can't include inet.h, but we still need to have > + * sensible definitions of these two constants. Note that pg_inet_net_ntop() > + * assumes that PGSQL_AF_INET is equal to AF_INET. > + */ > +#define PGSQL_AF_INET (AF_INET + 0) > +#define PGSQL_AF_INET6 (AF_INET + 1) > + > > Now we have the same definition thrice in frontend code. Coulnd't we > define them in, say, libpq-fe.h or inet-fe.h (nonexistent) then > include it from the three files? I started down the inet-fe.h route, and then realized I didn't know where that should go. Does it need to be included in (or part of) port.h? And should it be installed as part of the logic in src/include/Makefile? > +$node->connect_fails( > + "$common_connstr host=192.0.2.2", > + "host not matching an IPv4 address (Subject Alternative Name 1)", > > It is not the real IP address of the server. > > https://datatracker.ietf.org/doc/html/rfc6125 > > In some cases, the URI is specified as an IP address rather than a > > hostname. In this case, the iPAddress subjectAltName must be > > present in the certificate and must exactly match the IP in the URI. > > When IP address is embedded in URI, it won't be translated to another > IP address. Concretely https://192.0.1.5/hoge cannot reach to the host > 192.0.1.8. On the other hand, as done in the test, libpq allows that > when "host=192.0.1.5 hostaddr=192.0.1.8". I can't understand what we > are doing in that case. Don't we need to match the SAN IP address > with hostaddr instead of host? I thought that host, not hostaddr, was the part that corresponded to the URI. So in a hypothetical future where postgresqls:// exists, the two URIs postgresqls://192.0.2.2:5432/db postgresqls://192.0.2.2:5432/db?hostaddr=127.0.0.1 should both be expecting the same certificate. That seems to match the libpq documentation as well. (Specifying a host parameter is also allowed... that seems like it could cause problems for a hypothetical postgresqls:// scheme, but it's probably not relevant for this thread.) --Jacob
At Wed, 2 Feb 2022 19:46:13 +0000, Jacob Champion <pchampion@vmware.com> wrote in > On Mon, 2022-01-31 at 17:29 +0900, Kyotaro Horiguchi wrote: > > +#define PGSQL_AF_INET (AF_INET + 0) > > +#define PGSQL_AF_INET6 (AF_INET + 1) > > + > > > > Now we have the same definition thrice in frontend code. Coulnd't we > > define them in, say, libpq-fe.h or inet-fe.h (nonexistent) then > > include it from the three files? > > I started down the inet-fe.h route, and then realized I didn't know > where that should go. Does it need to be included in (or part of) > port.h? And should it be installed as part of the logic in > src/include/Makefile? I don't think it should be a part of port.h. Though I suggested frontend-only header file by the name, isn't it enough to separate out the definitions from utils/inet.h to common/inet-common.h then include the inet-common.h from inet.h? > > When IP address is embedded in URI, it won't be translated to another > > IP address. Concretely https://192.0.1.5/hoge cannot reach to the host > > 192.0.1.8. On the other hand, as done in the test, libpq allows that > > when "host=192.0.1.5 hostaddr=192.0.1.8". I can't understand what we > > are doing in that case. Don't we need to match the SAN IP address > > with hostaddr instead of host? > > I thought that host, not hostaddr, was the part that corresponded to > the URI. So in a hypothetical future where postgresqls:// exists, the > two URIs > > postgresqls://192.0.2.2:5432/db > postgresqls://192.0.2.2:5432/db?hostaddr=127.0.0.1 > > should both be expecting the same certificate. That seems to match the > libpq documentation as well. Hmm. Well, considering that the objective for the validation is to check if the server is actually the client is intending to connect, it is fine. Sorry for the noise. > (Specifying a host parameter is also allowed... that seems like it > could cause problems for a hypothetical postgresqls:// scheme, but it's > probably not relevant for this thread.) Yeah. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, 2022-02-03 at 16:23 +0900, Kyotaro Horiguchi wrote: > At Wed, 2 Feb 2022 19:46:13 +0000, Jacob Champion <pchampion@vmware.com> wrote in > > On Mon, 2022-01-31 at 17:29 +0900, Kyotaro Horiguchi wrote: > > > +#define PGSQL_AF_INET (AF_INET + 0) > > > +#define PGSQL_AF_INET6 (AF_INET + 1) > > > + > > > > > > Now we have the same definition thrice in frontend code. Coulnd't we > > > define them in, say, libpq-fe.h or inet-fe.h (nonexistent) then > > > include it from the three files? > > > > I started down the inet-fe.h route, and then realized I didn't know > > where that should go. Does it need to be included in (or part of) > > port.h? And should it be installed as part of the logic in > > src/include/Makefile? > > I don't think it should be a part of port.h. Though I suggested > frontend-only header file by the name, isn't it enough to separate out > the definitions from utils/inet.h to common/inet-common.h then include > the inet-common.h from inet.h? That works a lot better than what I had in my head. Done that way in v4. Thanks! --Jacob
Attachment
At Fri, 4 Feb 2022 17:06:53 +0000, Jacob Champion <pchampion@vmware.com> wrote in
> That works a lot better than what I had in my head. Done that way in
> v4. Thanks!
Thanks!
0002:
+#define PGSQL_AF_INET (AF_INET + 0)
+#define PGSQL_AF_INET6 (AF_INET + 1)
..
-#define PGSQL_AF_INET (AF_INET + 0)
-#define PGSQL_AF_INET6 (AF_INET + 1)
I feel this should be a part of 0001. (But the patches will be
finally merged so maybe no need to bother moving it).
> * The use of inet_aton() instead of inet_pton() is deliberate; the
> * latter cannot handle alternate IPv4 notations ("numbers-and-dots").
I think we should be consistent in handling IP addresses. We have
both inet_pton and inet_aton to parse IPv4 addresses.
We use inet_pton in the inet type (network_in).
We use inet_aton in server addresses.
# Hmm. I'm surprised to see listen_addresses accepts "0x7f.1".
# I think we should accept the same by network_in but it is another
# issue.
So, inet_aton there seems to be the right choice but the comment
doesn't describe the reason for that behavior. I think we should add
an explanation about the reason for the behavior, maybe something like
this:
> We accept alternative IPv4 address notations that are accepted by
> inet_aton but not by inet_pton as server address.
+ * GEN_IPADD is an OCTET STRING containing an IP address in network byte
+ * order.
+ /* OK to cast from unsigned to plain char, since it's all ASCII. */
+ return pq_verify_peer_name_matches_certificate_ip(conn, (const char *) addrdata, len, store_name);
Aren't the two comments contradicting each other? The retruned general
name looks like an octet array, which is not a subset of ASCII
string. So pq_verify_peer_name_matches_certificate_ip should receive
addrdata as "const unsigned char *", without casting.
+ if (name->type == host_type)
+ check_cn = false;
Don't we want a concise coment for this?
- if (*names_examined == 0)
+ if ((rc == 0) && check_cn)
To me, it seems a bit hard to understand. We can set false to
check_cn in the rc != 0 path in the loop on i, like this:
> if (rc != 0)
> + {
> + /*
> + * don't fall back to CN when we have a match or have an error
> + */
> + check_cn = false;
> break;
> + }
...
> - if ((rc == 0) && check_cn)
> + if (check_cn)
The following existing code (CN fallback)
> rc = openssl_verify_peer_name_matches_certificate_name(conn,
> X509_NAME_ENTRY_get_data(X509_NAME_get_entry(subject_name, cn_index)),
> first_name);
is expecting that first_name has not been set when it is visited.
However, with this patch, first_name can be set when the cert has any
SAN of unmatching type (DNS/IPADD) and the already-set name leaks. We
need to avoid that memory leak since the path can be visited multiple
times from the user-program of libpq. I came up with two directions.
1. Completely ignore type-unmatching entries. first_name is not set by
such entries. Such unmatching entreis doesn't increase
*names_examined.
2. Avoid overwriting first_name there.
I like 1, but since we don't make distinction between DNS and IPADDR
in the error message emited by the caller, we would take 2?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Mon, 2022-02-07 at 17:29 +0900, Kyotaro Horiguchi wrote:
> At Fri, 4 Feb 2022 17:06:53 +0000, Jacob Champion <pchampion@vmware.com> wrote in
> > That works a lot better than what I had in my head. Done that way in
> > v4. Thanks!
>
> Thanks!
>
> 0002:
>
> +#define PGSQL_AF_INET (AF_INET + 0)
> +#define PGSQL_AF_INET6 (AF_INET + 1)
> ..
> -#define PGSQL_AF_INET (AF_INET + 0)
> -#define PGSQL_AF_INET6 (AF_INET + 1)
>
> I feel this should be a part of 0001. (But the patches will be
> finally merged so maybe no need to bother moving it).
Okay. I can move it easily if you feel like it would help review, but
for now I've kept it in 0002.
> > * The use of inet_aton() instead of inet_pton() is deliberate; the
> > * latter cannot handle alternate IPv4 notations ("numbers-and-dots").
>
> I think we should be consistent in handling IP addresses. We have
> both inet_pton and inet_aton to parse IPv4 addresses.
>
> We use inet_pton in the inet type (network_in).
> We use inet_aton in server addresses.
>
> # Hmm. I'm surprised to see listen_addresses accepts "0x7f.1".
> # I think we should accept the same by network_in but it is another
> # issue.
Yeah, that's an interesting inconsistency.
> So, inet_aton there seems to be the right choice but the comment
> doesn't describe the reason for that behavior. I think we should add
> an explanation about the reason for the behavior, maybe something like
> this:
>
> > We accept alternative IPv4 address notations that are accepted by
> > inet_aton but not by inet_pton as server address.
I've pulled this wording into the comment in v5, attached.
> + * GEN_IPADD is an OCTET STRING containing an IP address in network byte
> + * order.
>
> + /* OK to cast from unsigned to plain char, since it's all ASCII. */
> + return pq_verify_peer_name_matches_certificate_ip(conn, (const char *) addrdata, len, store_name);
>
> Aren't the two comments contradicting each other? The retruned general
> name looks like an octet array, which is not a subset of ASCII
> string. So pq_verify_peer_name_matches_certificate_ip should receive
> addrdata as "const unsigned char *", without casting.
Bad copy-paste on my part; thanks for the catch. Fixed.
> + if (name->type == host_type)
> + check_cn = false;
>
> Don't we want a concise coment for this?
Added one; see what you think.
> - if (*names_examined == 0)
> + if ((rc == 0) && check_cn)
>
> To me, it seems a bit hard to understand. We can set false to
> check_cn in the rc != 0 path in the loop on i, like this:
>
> > if (rc != 0)
> > + {
> > + /*
> > + * don't fall back to CN when we have a match or have an error
> > + */
> > + check_cn = false;
> > break;
> > + }
> ...
> > - if ((rc == 0) && check_cn)
> > + if (check_cn)
If I understand right, that's not quite equivalent (and the new tests
fail if I implement it that way). We have to disable fallback if the
SAN exists, whether it matches or not.
> The following existing code (CN fallback)
>
> > rc = openssl_verify_peer_name_matches_certificate_name(conn,
> > X509_NAME_ENTRY_get_data(X509_NAME_get_entry(subject_name, cn_index)),
> > first_name);
>
> is expecting that first_name has not been set when it is visited.
> However, with this patch, first_name can be set when the cert has any
> SAN of unmatching type (DNS/IPADD) and the already-set name leaks. We
> need to avoid that memory leak since the path can be visited multiple
> times from the user-program of libpq. I came up with two directions.
>
> 1. Completely ignore type-unmatching entries. first_name is not set by
> such entries. Such unmatching entreis doesn't increase
> *names_examined.
>
> 2. Avoid overwriting first_name there.
>
> I like 1, but since we don't make distinction between DNS and IPADDR
> in the error message emited by the caller, we would take 2?
Great catch, thanks! I implemented option 2 to start. Option 1 might
make things difficult to debug if you're connecting to a server by IP
address but its certificate only has DNS names.
Thanks!
--Jacob
Attachment
(This needs rebasing)
At Wed, 9 Feb 2022 00:52:48 +0000, Jacob Champion <pchampion@vmware.com> wrote in
> On Mon, 2022-02-07 at 17:29 +0900, Kyotaro Horiguchi wrote:
> > I feel this should be a part of 0001. (But the patches will be
> > finally merged so maybe no need to bother moving it).
>
> Okay. I can move it easily if you feel like it would help review, but
> for now I've kept it in 0002.
Thanks.
> > So, inet_aton there seems to be the right choice but the comment
> > doesn't describe the reason for that behavior. I think we should add
> > an explanation about the reason for the behavior, maybe something like
> > this:
> >
> > > We accept alternative IPv4 address notations that are accepted by
> > > inet_aton but not by inet_pton as server address.
>
> I've pulled this wording into the comment in v5, attached.
> > + if (name->type == host_type)
> > + check_cn = false;
> >
> > Don't we want a concise coment for this?
>
> Added one; see what you think.
That's fine with me.
> > > if (rc != 0)
> > > + {
> > > + /*
> > > + * don't fall back to CN when we have a match or have an error
> > > + */
> > > + check_cn = false;
> > > break;
> > > + }
> > ...
> > > - if ((rc == 0) && check_cn)
> > > + if (check_cn)
>
> If I understand right, that's not quite equivalent (and the new tests
> fail if I implement it that way). We have to disable fallback if the
> SAN exists, whether it matches or not.
# I forgot to mention that, the test fails for me even without the
# change. I didn't checked what is wrong there, though.
Mmm. after the end of the loop, rc is non-zero only when the loop has
exited by the break and otherwise rc is zero. Why isn't it equivalent
to setting check_cn to false at the break?
Anyway, apart from that detail, I reconfirmed the spec the patch is
going to implement.
* If connhost contains a DNS name, and the certificate's SANs contain any
* dNSName entries, then we'll ignore the Subject Common Name entirely;
* otherwise, we fall back to checking the CN. (This behavior matches the
* RFC.)
Sure.
* If connhost contains an IP address, and the SANs contain iPAddress
* entries, we again ignore the CN. Otherwise, we allow the CN to match,
* EVEN IF there is a dNSName in the SANs. (RFC 6125 prohibits this: "A
* client MUST NOT seek a match for a reference identifier of CN-ID if the
* presented identifiers include a DNS-ID, SRV-ID, URI-ID, or any
* application-specific identifier types supported by the client.")
Actually the patch searches for a match of IP address connhost from
dNSName SANs even if iPAddress SANs exist. I think we've not
explicitly defined thebehavior in that case. I supposed that we only
be deviant in the case "IP address connhost and no SANs of any type
exists". What do you think about it?
- For the certificate that have only dNSNames or no SANs presented, we
serach for a match from all dNSNames if any or otherwise try CN
regardless of the type of connhost.
- Otherwise (the cert has at least one iPAddress SANs) we follow the RFCs.
- For IP-addr connhost, we search only the iPAddress SANs.
- For DNSName connhost, we search only dNSName SANs if any or
otherwise try CN.
Honestly I didn't consider to that detail. On second thought, with
this specification we cannot decide the behavior unless we scanned all
SANs. Maybe we can find an elegant implement but I don't think people
here would welcome even that level of complexity needed only for that
dubious existing use case.
What do you think about this? And I'd like to hear from others.
> Great catch, thanks! I implemented option 2 to start. Option 1 might
> make things difficult to debug if you're connecting to a server by IP
> address but its certificate only has DNS names.
Looks fine. Thanks!
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Tue, 2022-02-15 at 15:16 +0900, Kyotaro Horiguchi wrote: > (This needs rebasing) Done in v6, attached. > # I forgot to mention that, the test fails for me even without the > # change. I didn't checked what is wrong there, though. Ah. We should probably figure that out, then -- what failures do you see? > Mmm. after the end of the loop, rc is non-zero only when the loop has > exited by the break and otherwise rc is zero. Why isn't it equivalent > to setting check_cn to false at the break? check_cn can be false if rc is zero, too; it means that we found a SAN of the correct type but it didn't match. > Anyway, apart from that detail, I reconfirmed the spec the patch is > going to implement. > > * If connhost contains a DNS name, and the certificate's SANs contain any > * dNSName entries, then we'll ignore the Subject Common Name entirely; > * otherwise, we fall back to checking the CN. (This behavior matches the > * RFC.) > > Sure. > > * If connhost contains an IP address, and the SANs contain iPAddress > * entries, we again ignore the CN. Otherwise, we allow the CN to match, > * EVEN IF there is a dNSName in the SANs. (RFC 6125 prohibits this: "A > * client MUST NOT seek a match for a reference identifier of CN-ID if the > * presented identifiers include a DNS-ID, SRV-ID, URI-ID, or any > * application-specific identifier types supported by the client.") > > Actually the patch searches for a match of IP address connhost from > dNSName SANs even if iPAddress SANs exist. I think we've not > explicitly defined thebehavior in that case. That's a good point; I didn't change the prior behavior. I feel more comfortable leaving that check, since it is technically possible to push something that looks like an IP address into a dNSName SAN. We should probably make an explicit decision on that, as you say. But I don't think that contradicts the code comment, does it? The comment is just talking about CN fallback scenarios. If you find a match in a dNSName, there's no reason to fall back to the CN. > I supposed that we only > be deviant in the case "IP address connhost and no SANs of any type > exists". What do you think about it? We fall back in the case of "IP address connhost and dNSName SANs exist", which is prohibited by that part of RFC 6125. I don't think we deviate in the case you described; can you explain further? > - For the certificate that have only dNSNames or no SANs presented, we > serach for a match from all dNSNames if any or otherwise try CN > regardless of the type of connhost. Correct. (I don't find that way of dividing up the cases very intuitive, though.) > - Otherwise (the cert has at least one iPAddress SANs) we follow the RFCs. > > - For IP-addr connhost, we search only the iPAddress SANs. We search the dNSNames as well, as you pointed out above. But we don't fall back to the CN. > - For DNSName connhost, we search only dNSName SANs if any or > otherwise try CN. Effectively, yes. (We call the IP address verification functions too, to get alt_name, but they can't match. If that's too confusing, we'd need to pull the alt_name handling up out of the verification layer.) > Honestly I didn't consider to that detail. On second thought, with > this specification we cannot decide the behavior unless we scanned all > SANs. Right. > Maybe we can find an elegant implement but I don't think people > here would welcome even that level of complexity needed only for that > dubious existing use case. Which use case do you mean? > What do you think about this? And I'd like to hear from others. I think we need to decide whether or not to keep the current "IP address connhost can match a dNSName SAN" behavior, and if so I need to add it to the test cases. (And we need to figure out why the tests are failing in your build, of course.) Thanks! --Jacob
Attachment
At Thu, 17 Feb 2022 17:29:15 +0000, Jacob Champion <pchampion@vmware.com> wrote in > On Tue, 2022-02-15 at 15:16 +0900, Kyotaro Horiguchi wrote: > > (This needs rebasing) > > Done in v6, attached. Thanks! > > # I forgot to mention that, the test fails for me even without the > > # change. I didn't checked what is wrong there, though. > > Ah. We should probably figure that out, then -- what failures do you > see? I forgot the detail but v6 still fails for me. I think it is that. t/003_sslinfo.pl ... 1/? # Tests were run but no plan was declared and done_testing() was not seen. # Looks like your test exited with 29 just after 6. t/003_sslinfo.pl ... Dubious, test returned 29 (wstat 7424, 0x1d00) All 6 subtests passed ... Result: FAIL The script complains like this: ok 6 - ssl_client_cert_present() for connection with cert connection error: 'psql: error: connection to server at "127.0.0.1", port 62656 failed: SSL error: tlsv1 alert unknown ca' while running 'psql -XAtq -d sslrootcert=ssl/root+server_ca.crt sslmode=require dbname=trustdb hostaddr=127.0.0.1 user=ssltestuserhost=localhost -f - -v ON_ERROR_STOP=1' at /home/horiguti/work/worktrees/ipsan/src/test/ssl/../../../src/test/perl/PostgreSQL/Test/Cluster.pmline 1873. So, psql looks like disliking the ca certificate. I also will dig into that. > > Mmm. after the end of the loop, rc is non-zero only when the loop has > > exited by the break and otherwise rc is zero. Why isn't it equivalent > > to setting check_cn to false at the break? > > check_cn can be false if rc is zero, too; it means that we found a SAN > of the correct type but it didn't match. Don't we count unmatched certs as "existed"? In that case I think we don't go to CN. > > Anyway, apart from that detail, I reconfirmed the spec the patch is > > going to implement. > > > > * If connhost contains a DNS name, and the certificate's SANs contain any > > * dNSName entries, then we'll ignore the Subject Common Name entirely; > > * otherwise, we fall back to checking the CN. (This behavior matches the > > * RFC.) > > > > Sure. > > > > * If connhost contains an IP address, and the SANs contain iPAddress > > * entries, we again ignore the CN. Otherwise, we allow the CN to match, > > * EVEN IF there is a dNSName in the SANs. (RFC 6125 prohibits this: "A > > * client MUST NOT seek a match for a reference identifier of CN-ID if the > > * presented identifiers include a DNS-ID, SRV-ID, URI-ID, or any > > * application-specific identifier types supported by the client.") > > > > Actually the patch searches for a match of IP address connhost from > > dNSName SANs even if iPAddress SANs exist. I think we've not > > explicitly defined thebehavior in that case. > > That's a good point; I didn't change the prior behavior. I feel more > comfortable leaving that check, since it is technically possible to > push something that looks like an IP address into a dNSName SAN. We > should probably make an explicit decision on that, as you say. > > But I don't think that contradicts the code comment, does it? The > comment is just talking about CN fallback scenarios. If you find a > match in a dNSName, there's no reason to fall back to the CN. The comment explains the spec correctly. From a practical view, the behavior above doesn't seem to make things insecure. So I don't have a strong opinion on the choice of the behaviors. The only thing I'm concerned here is the possibility that the decision corners us to some uncomfortable state between the RFC and our spec in future. On the other hand, changing the behavior can immediately make someone uncomfortable. So, I'd like to leave it to committers:p > > I supposed that we only > > be deviant in the case "IP address connhost and no SANs of any type > > exists". What do you think about it? > > We fall back in the case of "IP address connhost and dNSName SANs > exist", which is prohibited by that part of RFC 6125. I don't think we > deviate in the case you described; can you explain further? In that case, i.e., connhost is IP address and no SANs exist at all, we go to CN. On the other hand in RFC6125: rfc6125> In some cases, the URI is specified as an IP address rather rfc6125> than a hostname. In this case, the iPAddress subjectAltName rfc6125> must be present in the certificate and must exactly match the rfc6125> IP in the URI. It (seems to me) denies that behavior. Regardless of the existence of other types of SANs, iPAddress is required if connname is an IP address. (That is, it doesn't seem to me that there's any context like "if any SANs exists", but I'm not so sure I read it perfectly.) > > - For the certificate that have only dNSNames or no SANs presented, we > > serach for a match from all dNSNames if any or otherwise try CN > > regardless of the type of connhost. > > Correct. (I don't find that way of dividing up the cases very > intuitive, though.) Yeah, it's the same decision to the above. It doesn't matter in the security view (if the cert issuer is sane) but we could be cornerd in future. > > - Otherwise (the cert has at least one iPAddress SANs) we follow the RFCs. > > > > - For IP-addr connhost, we search only the iPAddress SANs. > > We search the dNSNames as well, as you pointed out above. But we don't > fall back to the CN. Ur. Yes. > > - For DNSName connhost, we search only dNSName SANs if any or > > otherwise try CN. > > Effectively, yes. (We call the IP address verification functions too, > to get alt_name, but they can't match. If that's too confusing, we'd > need to pull the alt_name handling up out of the verification layer.) Yes, I meant that. > > Honestly I didn't consider to that detail. On second thought, with > > this specification we cannot decide the behavior unless we scanned all > > SANs. > > Right. > > > Maybe we can find an elegant implement but I don't think people > > here would welcome even that level of complexity needed only for that > > dubious existing use case. > > Which use case do you mean? "dubious". So I meant that the use case where dNSNames is expected to match with an IP address. > > What do you think about this? And I'd like to hear from others. > > I think we need to decide whether or not to keep the current "IP > address connhost can match a dNSName SAN" behavior, and if so I need to > add it to the test cases. (And we need to figure out why the tests are > failing in your build, of course.) Thanks. All behaviors and theier reasons is now clear. So.... Let's leave them for committers for now. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Mon, 2022-03-14 at 15:30 +0900, Kyotaro Horiguchi wrote: > t/003_sslinfo.pl ... 1/? # Tests were run but no plan was declared and done_testing() was not seen. > # Looks like your test exited with 29 just after 6. > t/003_sslinfo.pl ... Dubious, test returned 29 (wstat 7424, 0x1d00) > All 6 subtests passed > ... > Result: FAIL > > The script complains like this: > > ok 6 - ssl_client_cert_present() for connection with cert > connection error: 'psql: error: connection to server at "127.0.0.1", port 62656 failed: SSL error: tlsv1 alert unknownca' > while running 'psql -XAtq -d sslrootcert=ssl/root+server_ca.crt sslmode=require dbname=trustdb hostaddr=127.0.0.1 user=ssltestuserhost=localhost -f - -v ON_ERROR_STOP=1' at /home/horiguti/work/worktrees/ipsan/src/test/ssl/../../../src/test/perl/PostgreSQL/Test/Cluster.pmline 1873. > > So, psql looks like disliking the ca certificate. I also will dig > into that. Hmm, the sslinfo tests are failing? I wouldn't have expected that based on the patch changes. Just to confirm -- they pass for you without the patch? > > > Mmm. after the end of the loop, rc is non-zero only when the loop has > > > exited by the break and otherwise rc is zero. Why isn't it equivalent > > > to setting check_cn to false at the break? > > > > check_cn can be false if rc is zero, too; it means that we found a SAN > > of the correct type but it didn't match. > > Don't we count unmatched certs as "existed"? In that case I think we > don't go to CN. Unmatched names, you mean? I'm not sure I understand. If it helps, the two tests that will fail if check_cn is unset only at the break are - certificate with both a CN and SANs ignores CN - certificate with both an IP CN and IP SANs ignores CN because none of the SANs would match in that case. > > > Actually the patch searches for a match of IP address connhost from > > > dNSName SANs even if iPAddress SANs exist. I think we've not > > > explicitly defined thebehavior in that case. > > > > That's a good point; I didn't change the prior behavior. I feel more > > comfortable leaving that check, since it is technically possible to > > push something that looks like an IP address into a dNSName SAN. We > > should probably make an explicit decision on that, as you say. > > > > But I don't think that contradicts the code comment, does it? The > > comment is just talking about CN fallback scenarios. If you find a > > match in a dNSName, there's no reason to fall back to the CN. > > The comment explains the spec correctly. From a practical view, the > behavior above doesn't seem to make things insecure. So I don't have > a strong opinion on the choice of the behaviors. > > The only thing I'm concerned here is the possibility that the decision > corners us to some uncomfortable state between the RFC and our spec in > future. On the other hand, changing the behavior can immediately make > someone uncomfortable. > > So, I'd like to leave it to committers:p Sounds good. I'll work on adding tests for the current behavior, and if the committers don't like it, we can change it. > > > I supposed that we only > > > be deviant in the case "IP address connhost and no SANs of any type > > > exists". What do you think about it? > > > > We fall back in the case of "IP address connhost and dNSName SANs > > exist", which is prohibited by that part of RFC 6125. I don't think we > > deviate in the case you described; can you explain further? > > In that case, i.e., connhost is IP address and no SANs exist at all, > we go to CN. On the other hand in RFC6125: > > rfc6125> In some cases, the URI is specified as an IP address rather > rfc6125> than a hostname. In this case, the iPAddress subjectAltName > rfc6125> must be present in the certificate and must exactly match the > rfc6125> IP in the URI. > > It (seems to me) denies that behavior. Regardless of the existence of > other types of SANs, iPAddress is required if connname is an IP > address. (That is, it doesn't seem to me that there's any context > like "if any SANs exists", but I'm not so sure I read it perfectly.) I see what you mean now. Yes, we deviate there as well (and have done so for a while now). I think breaking compatibility there would probably not go over well. > Thanks. All behaviors and theier reasons is now clear. So.... > > Let's leave them for committers for now. Thank you for the review! --Jacob
On Tue, 2022-03-15 at 21:41 +0000, Jacob Champion wrote: > Sounds good. I'll work on adding tests for the current behavior, and if > the committers don't like it, we can change it. Done in v7, attached. --Jacob
Attachment
At Tue, 15 Mar 2022 21:41:49 +0000, Jacob Champion <pchampion@vmware.com> wrote in
> On Mon, 2022-03-14 at 15:30 +0900, Kyotaro Horiguchi wrote:
> > t/003_sslinfo.pl ... 1/? # Tests were run but no plan was declared and done_testing() was not seen.
> > # Looks like your test exited with 29 just after 6.
> > t/003_sslinfo.pl ... Dubious, test returned 29 (wstat 7424, 0x1d00)
> > All 6 subtests passed
> > ...
> > Result: FAIL
> >
> > The script complains like this:
> >
> > ok 6 - ssl_client_cert_present() for connection with cert
> > connection error: 'psql: error: connection to server at "127.0.0.1", port 62656 failed: SSL error: tlsv1 alert
unknownca'
> > while running 'psql -XAtq -d sslrootcert=ssl/root+server_ca.crt sslmode=require dbname=trustdb hostaddr=127.0.0.1
user=ssltestuserhost=localhost -f - -v ON_ERROR_STOP=1' at
/home/horiguti/work/worktrees/ipsan/src/test/ssl/../../../src/test/perl/PostgreSQL/Test/Cluster.pmline 1873.
> >
> > So, psql looks like disliking the ca certificate. I also will dig
> > into that.
>
> Hmm, the sslinfo tests are failing? I wouldn't have expected that based
> on the patch changes. Just to confirm -- they pass for you without the
> patch?
Mmm.... I'm not sure how come I didn't noticed that, master also fails
for me fo the same reason. In the past that may fail when valid
clinent-certs exists in the users ~/.postgresql but I believe that has
been fixed.
> > > > Mmm. after the end of the loop, rc is non-zero only when the loop has
> > > > exited by the break and otherwise rc is zero. Why isn't it equivalent
> > > > to setting check_cn to false at the break?
> > >
> > > check_cn can be false if rc is zero, too; it means that we found a SAN
> > > of the correct type but it didn't match.
I'm not discussing on the meaning. Purely on the logical equivalancy
of the two ways to decide whether to visit CN.
Concretely, the equivalancy between this:
=====
check_cn = true;
rc = 0;
for (i < san_len)
{
if (type matches) check_cn = false;
if (some error happens) rc = nonzero;
if (rc != 0)
break;
}
!if ((rc == 0) && check_cn) {}
=====
and this.
=====
check_cn = true;
rc = 0;
for (i < san_len)
{
if (type matches) check_cn = false;
if (some error happens) rc = nonzero;
if (rc != 0)
{
! check_cn = false;
break;
}
}
!if (check_cn) {}
=====
The two are equivalant to me. And if it is, the latter form smees
simpler and clearner to me.
> > > check_cn can be false if rc is zero, too; it means that we found a SAN
> > > of the correct type but it didn't match.
>
> > Don't we count unmatched certs as "existed"? In that case I think we
> > don't go to CN.
>
> Unmatched names, you mean? I'm not sure I understand.
Sorry, I was confused here. Please ignore that. I shoudl have said
something like the following instead.
> check_cn can be false if rc is zero, too; it means that we found a SAN
> of the correct type but it didn't match.
Yes, in that case we don't visit CN because check_cn is false even if
we don't exit by (rc != 0) and that behavior is not changed by my
proposal.
> > > > I supposed that we only
> > > > be deviant in the case "IP address connhost and no SANs of any type
> > > > exists". What do you think about it?
> > >
> > > We fall back in the case of "IP address connhost and dNSName SANs
> > > exist", which is prohibited by that part of RFC 6125. I don't think we
> > > deviate in the case you described; can you explain further?
> >
> > In that case, i.e., connhost is IP address and no SANs exist at all,
> > we go to CN. On the other hand in RFC6125:
> >
> > rfc6125> In some cases, the URI is specified as an IP address rather
> > rfc6125> than a hostname. In this case, the iPAddress subjectAltName
> > rfc6125> must be present in the certificate and must exactly match the
> > rfc6125> IP in the URI.
> >
> > It (seems to me) denies that behavior. Regardless of the existence of
> > other types of SANs, iPAddress is required if connname is an IP
> > address. (That is, it doesn't seem to me that there's any context
> > like "if any SANs exists", but I'm not so sure I read it perfectly.)
>
> I see what you mean now. Yes, we deviate there as well (and have done
> so for a while now). I think breaking compatibility there would
> probably not go over well.
Agreed.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
At Wed, 16 Mar 2022 15:56:02 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > Mmm.... I'm not sure how come I didn't noticed that, master also fails > for me fo the same reason. In the past that may fail when valid > clinent-certs exists in the users ~/.postgresql but I believe that has > been fixed. And finally my fear found to be true.. The test doesn't fail after removing files under ~/.postgresql, which should not happen. I'll fix it apart from this. Sorry for the confusion. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Wed, 2022-03-16 at 15:56 +0900, Kyotaro Horiguchi wrote: > At Tue, 15 Mar 2022 21:41:49 +0000, Jacob Champion <pchampion@vmware.com> wrote in > > Hmm, the sslinfo tests are failing? I wouldn't have expected that based > > on the patch changes. Just to confirm -- they pass for you without the > > patch? > > Mmm.... I'm not sure how come I didn't noticed that, master also fails > for me fo the same reason. In the past that may fail when valid > clinent-certs exists in the users ~/.postgresql but I believe that has > been fixed. Good to know; I was worried that I'd messed up something well outside the code I'd touched. > > > > > Mmm. after the end of the loop, rc is non-zero only when the loop has > > > > > exited by the break and otherwise rc is zero. Why isn't it equivalent > > > > > to setting check_cn to false at the break? > > > > > > > > check_cn can be false if rc is zero, too; it means that we found a SAN > > > > of the correct type but it didn't match. > > I'm not discussing on the meaning. Purely on the logical equivalancy > of the two ways to decide whether to visit CN. > > Concretely, the equivalancy between this: [snip] Thank you for the explanation -- the misunderstanding was all on my end. I thought you were asking me to move the check_cn assignment instead of copying it to the end. I agree that your suggestion is much clearer, and I'll make that change tomorrow. Thanks! --Jacob
On Wed, 2022-03-16 at 23:49 +0000, Jacob Champion wrote: > Thank you for the explanation -- the misunderstanding was all on my > end. I thought you were asking me to move the check_cn assignment > instead of copying it to the end. I agree that your suggestion is much > clearer, and I'll make that change tomorrow. Done in v8. Thanks again for your suggestions (and for your perseverance when I didn't get it)! --Jacob
Attachment
At Thu, 17 Mar 2022 21:55:07 +0000, Jacob Champion <pchampion@vmware.com> wrote in > On Wed, 2022-03-16 at 23:49 +0000, Jacob Champion wrote: > > Thank you for the explanation -- the misunderstanding was all on my > > end. I thought you were asking me to move the check_cn assignment > > instead of copying it to the end. I agree that your suggestion is much > > clearer, and I'll make that change tomorrow. > > Done in v8. Thanks again for your suggestions (and for your > perseverance when I didn't get it)! Thanks! .. and some nitpicks..(Sorry) fe-secure-common.c doesn't need netinet/in.h. +++ b/src/include/utils/inet.h .. +#include "common/inet-common.h" I'm not sure about the project policy on #include practice, but I think it is the common practice not to include headers that are not required by the file itself. In this case, fe-secure-common.h itself doesn't need the include. Instead, fe-secure-openssl.c and fe-secure-common.c needs the include. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Fri, 18 Mar 2022 16:38:57 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > At Thu, 17 Mar 2022 21:55:07 +0000, Jacob Champion <pchampion@vmware.com> wrot> Thanks! .. and some nitpicks..(Sorry) > > fe-secure-common.c doesn't need netinet/in.h. > > > +++ b/src/include/utils/inet.h > .. > +#include "common/inet-common.h" > > I'm not sure about the project policy on #include practice, but I > think it is the common practice not to include headers that are not > required by the file itself. In this case, fe-secure-common.h itself > doesn't need the include. Instead, fe-secure-openssl.c and > fe-secure-common.c needs the include. I noticed that this doesn't contain doc changes. https://www.postgresql.org/docs/current/libpq-ssl.html > In verify-full mode, the host name is matched against the > certificate's Subject Alternative Name attribute(s), or against the > Common Name attribute if no Subject Alternative Name of type dNSName > is present. If the certificate's name attribute starts with an > asterisk (*), the asterisk will be treated as a wildcard, which will > match all characters except a dot (.). This means the certificate will > not match subdomains. If the connection is made using an IP address > instead of a host name, the IP address will be matched (without doing > any DNS lookups). This refers to dNSName, so we should revise this so that it describes the new behavior. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Tue, 2022-03-22 at 13:32 +0900, Kyotaro Horiguchi wrote: > At Fri, 18 Mar 2022 16:38:57 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > > > > fe-secure-common.c doesn't need netinet/in.h. > > > > > > +++ b/src/include/utils/inet.h > > .. > > +#include "common/inet-common.h" > > > > I'm not sure about the project policy on #include practice, but I > > think it is the common practice not to include headers that are not > > required by the file itself. In this case, fe-secure-common.h itself > > doesn't need the include. Instead, fe-secure-openssl.c and > > fe-secure-common.c needs the include. Thanks, looks like I had some old header dependencies left over from several versions ago. Fixed in v9. > I noticed that this doesn't contain doc changes. > > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.postgresql.org%2Fdocs%2Fcurrent%2Flibpq-ssl.html&data=04%7C01%7Cpchampion%40vmware.com%7Cb25566c0f0124a30221908da0bbcec13%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637835203290105956%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=sZuKc9UxmW1oZQij%2F%2F91rkEF57BZiQebkXtvEt%2FdROU%3D&reserved=0 > > > In verify-full mode, the host name is matched against the > > certificate's Subject Alternative Name attribute(s), or against the > > Common Name attribute if no Subject Alternative Name of type dNSName > > is present. If the certificate's name attribute starts with an > > asterisk (*), the asterisk will be treated as a wildcard, which will > > match all characters except a dot (.). This means the certificate will > > not match subdomains. If the connection is made using an IP address > > instead of a host name, the IP address will be matched (without doing > > any DNS lookups). > > This refers to dNSName, so we should revise this so that it describes > the new behavior. v9 contains the bare minimum but I don't think it's quite enough. How much of the behavior (and edge cases) do you think we should detail here? All of it? Thanks, --Jacob
Attachment
At Tue, 22 Mar 2022 20:42:37 +0000, Jacob Champion <pchampion@vmware.com> wrote in
> Thanks, looks like I had some old header dependencies left over from
> several versions ago. Fixed in v9.
Thanks! Looks perfect.
> v9 contains the bare minimum but I don't think it's quite enough. How
> much of the behavior (and edge cases) do you think we should detail
> here? All of it?
I tried to write out the doc part. What do you think about it?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 3998b1781b..13e3e63768 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -8342,16 +8342,31 @@ ldap://ldap.acme.com/cn=dbserver,cn=hosts?pgconnectinfo?base?(objectclass=*)
<para>
In <literal>verify-full</literal> mode, the host name is matched against the
- certificate's Subject Alternative Name attribute(s), or against the
- Common Name attribute if no Subject Alternative Name of type <literal>dNSName</literal> is
+ certificate's Subject Alternative Name attribute(s) (SAN), or against the
+ Common Name attribute if no SAN of type <literal>dNSName</literal> is
present. If the certificate's name attribute starts with an asterisk
(<literal>*</literal>), the asterisk will be treated as
a wildcard, which will match all characters <emphasis>except</emphasis> a dot
(<literal>.</literal>). This means the certificate will not match subdomains.
If the connection is made using an IP address instead of a host name, the
- IP address will be matched (without doing any DNS lookups).
+ IP address will be matched (without doing any DNS lookups) against SANs of
+ type <literal>iPAddress</literal> or <literal>dNSName</literal>. If no
+ <literal>ipAddress</literal> SAN is present and no
+ matching <literal>dNSName</literal> SAN is present, the host IP address is
+ matched against the Common Name attribute.
</para>
+ <note>
+ <para>
+ For backward compatibility with earlier versions of PostgreSQL, the host
+ IP address is verified in a manner different
+ from <ulink url="https://tools.ietf.org/html/rfc6125">RFC 6125</ulink>.
+ The host IP address is always matched against <literal>dNSName</literal>
+ SANs as well as <literal>iPAdress</literal> SANs, and can be matched
+ against the Common Name attribute for a certain condition.
+ </para>
+ </note>
+
<para>
To allow server certificate verification, one or more root certificates
must be placed in the file <filename>~/.postgresql/root.crt</filename>
On Wed, 2022-03-23 at 14:20 +0900, Kyotaro Horiguchi wrote: > I tried to write out the doc part. What do you think about it? I like it, thanks! I've applied that in v10, with a tweak to two iPAddress spellings and a short expansion of the condition in the Note, and I've added you as a co-author to 0002. --Jacob
Attachment
At Wed, 23 Mar 2022 23:52:06 +0000, Jacob Champion <pchampion@vmware.com> wrote in
> On Wed, 2022-03-23 at 14:20 +0900, Kyotaro Horiguchi wrote:
> > I tried to write out the doc part. What do you think about it?
>
> I like it, thanks! I've applied that in v10, with a tweak to two
> iPAddress spellings and a short expansion of the condition in the Note,
> and I've added you as a co-author to 0002.
I'm fine with it. Thanks. I marked it as Ready-for-Commiter.
Note for the patch set:
0001 is preliminary patch to move inet_pton out of src/backend tree.
0002 is the main patch of this patchset
0003 is optional, which introduces pg_inet_pton() only works for IPv6
addresses. 0002 gets the same effect by the following use of
pg_inet_net_pton().
> if (!strchr(host, '/')
> && pg_inet_net_pton(PGSQL_AF_INET6, host, addr, -1) == 128)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Thu, 2022-03-24 at 17:10 +0900, Kyotaro Horiguchi wrote: > I'm fine with it. Thanks. I marked it as Ready-for-Commiter. Thank you for the reviews and feedback! --Jacob
Jacob Champion <pchampion@vmware.com> writes:
> [ v10-0001-Move-inet_net_pton-to-src-port.patch etc ]
There is something broken about the ssl tests as modified by
this patch. The cfbot doesn't provide a lot of evidence about
why it's failing, but I applied the patchset locally and what
I see is
...
ok 47 - mismatch between host name and server certificate sslmode=verify-full: m
atches
Odd number of elements in hash assignment at /home/postgres/pgsql/src/test/ssl/t
/SSL/Server.pm line 288.
Use of uninitialized value in concatenation (.) or string at /home/postgres/pgsq
l/src/test/ssl/t/SSL/Backend/OpenSSL.pm line 178.
Use of uninitialized value in concatenation (.) or string at /home/postgres/pgsq
l/src/test/ssl/t/SSL/Backend/OpenSSL.pm line 178.
### Restarting node "primary"
# Running: pg_ctl -w -D /home/postgres/pgsql/src/test/ssl/tmp_check/t_001_ssltes
ts_primary_data/pgdata -l /home/postgres/pgsql/src/test/ssl/tmp_check/log/001_ss
ltests_primary.log restart
waiting for server to shut down.... done
server stopped
waiting for server to start.... stopped waiting
pg_ctl: could not start server
The tail end of the server log is
2022-03-27 17:13:11.482 EDT [551720] FATAL: could not load server certificate file ".crt": No such file or directory
so it seems pretty clear that something's fouling up computation of
a certificate file name. This may be caused by 9ca234bae or
4a7e964fc.
regards, tom lane
> On 27 Mar 2022, at 23:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> This may be caused by 9ca234bae or 4a7e964fc.
I'd say 4a7e964fc is the culprit here. From a quick skim the the
switch_server_cert() calls need to be changed along the lines of:
from: switch_server_cert($node, 'server-ip-in-dnsname');
to: switch_server_cert($node, certfile => 'server-ip-in-dnsname');
There migth be more changes required, that was the one that stood out. Unless
someone beats me to it I'll take a look at fixing up the test in this patch
tomorrow.
--
Daniel Gustafsson https://vmware.com/
Hi, On 2022-03-27 17:19:27 -0400, Tom Lane wrote: > The cfbot doesn't provide a lot of evidence about > why it's failing, but I applied the patchset locally and what > I see is FWIW - and I agree that's not nice user interface wise - just below the cpu / memory graphs there's a "directory browser", allowing you to navigate to the saved log files. Navigating to log/src/test/ssl/tmp_check/log allows you to download https://api.cirrus-ci.com/v1/artifact/task/5261015175659520/log/src/test/ssl/tmp_check/log/regress_log_001_ssltests https://api.cirrus-ci.com/v1/artifact/task/5261015175659520/log/src/test/ssl/tmp_check/log/001_ssltests_primary.log Greetings, Andres Freund
> On 28 Mar 2022, at 00:44, Daniel Gustafsson <daniel@yesql.se> wrote: > I'll take a look at fixing up the test in this patch tomorrow. Fixing up the switch_server_cert() calls and using default_ssl_connstr makes the test pass for me. The required fixes are in the supplied 0004 diff, I kept them separate to allow the original author to incorporate them without having to dig them out to see what changed (named to match the git format-patch output since I think the CFBot just applies the patches in alphabetical order). -- Daniel Gustafsson https://vmware.com/
Attachment
On Mon, 28 Mar 2022 at 05:17, Daniel Gustafsson <daniel@yesql.se> wrote: > > named to match the git format-patch output > since I think the CFBot just applies the patches in alphabetical order). The first patch doesn't seem to actually apply though so it doesn't get to the subsequent patches. http://cfbot.cputube.org/patch_37_3458.log === Applying patches on top of PostgreSQL commit ID e26114c817b610424010cfbe91a743f591246ff1 === === applying patch ./v10-0001-Move-inet_net_pton-to-src-port.patch patching file src/backend/utils/adt/Makefile Hunk #1 FAILED at 44. 1 out of 1 hunk FAILED -- saving rejects to file src/backend/utils/adt/Makefile.rej patching file src/include/port.h patching file src/include/utils/builtins.h patching file src/port/Makefile patching file src/port/inet_net_pton.c (renamed from src/backend/utils/adt/inet_net_pton.c) patching file src/tools/msvc/Mkvcbuild.pm -- greg
On Mon, 2022-03-28 at 11:17 +0200, Daniel Gustafsson wrote: > Fixing up the switch_server_cert() calls and using default_ssl_connstr makes > the test pass for me. The required fixes are in the supplied 0004 diff, I kept > them separate to allow the original author to incorporate them without having > to dig them out to see what changed (named to match the git format-patch output > since I think the CFBot just applies the patches in alphabetical order). Thanks! Those changes look good to me; I've folded them into v11. This is rebased on a newer HEAD so it should fix the apply failures that Greg pointed out. --Jacob
Attachment
On 28.03.22 22:21, Jacob Champion wrote: > On Mon, 2022-03-28 at 11:17 +0200, Daniel Gustafsson wrote: >> Fixing up the switch_server_cert() calls and using default_ssl_connstr makes >> the test pass for me. The required fixes are in the supplied 0004 diff, I kept >> them separate to allow the original author to incorporate them without having >> to dig them out to see what changed (named to match the git format-patch output >> since I think the CFBot just applies the patches in alphabetical order). > > Thanks! Those changes look good to me; I've folded them into v11. This > is rebased on a newer HEAD so it should fix the apply failures that > Greg pointed out. I'm not happy about how inet_net_pton.o is repurposed here. That code is clearly meant to support backend data types with specifically. Code like + /* + * pg_inet_net_pton() will accept CIDR masks, which we don't want to + * match, so skip the comparison if the host string contains a slash. + */ indicates that we are fighting against the API. Also, if someone ever wants to change how those backend data types work, we then have to check a bunch of other code as well. I think we should be using inet_ntop()/inet_pton() directly here. We can throw substitute implementations into libpgport if necessary, that's not so difficult.
On Wed, 2022-03-30 at 13:37 +0200, Peter Eisentraut wrote: > On 28.03.22 22:21, Jacob Champion wrote: > > On Mon, 2022-03-28 at 11:17 +0200, Daniel Gustafsson wrote: > > > Fixing up the switch_server_cert() calls and using default_ssl_connstr makes > > > the test pass for me. The required fixes are in the supplied 0004 diff, I kept > > > them separate to allow the original author to incorporate them without having > > > to dig them out to see what changed (named to match the git format-patch output > > > since I think the CFBot just applies the patches in alphabetical order). > > > > Thanks! Those changes look good to me; I've folded them into v11. This > > is rebased on a newer HEAD so it should fix the apply failures that > > Greg pointed out. > > I'm not happy about how inet_net_pton.o is repurposed here. That code > is clearly meant to support backend data types with specifically. Code like > > + /* > + * pg_inet_net_pton() will accept CIDR masks, which we don't want to > + * match, so skip the comparison if the host string contains a slash. > + */ > > indicates that we are fighting against the API. Horiguchi-san had the same concern upthread, I think. I replaced that code in the next patch, but it was enough net-new stuff that I kept the patches separate instead of merging them. I can change that if it's not helpful for review. > Also, if someone ever > wants to change how those backend data types work, we then have to check > a bunch of other code as well. > > I think we should be using inet_ntop()/inet_pton() directly here. We > can throw substitute implementations into libpgport if necessary, that's > not so difficult. Is this request satisfied by the implementation of pg_inet_pton() in patch 0003? If not, what needs to change? Thanks, --Jacob
On 30.03.22 18:17, Jacob Champion wrote: >> Also, if someone ever >> wants to change how those backend data types work, we then have to check >> a bunch of other code as well. >> >> I think we should be using inet_ntop()/inet_pton() directly here. We >> can throw substitute implementations into libpgport if necessary, that's >> not so difficult. > Is this request satisfied by the implementation of pg_inet_pton() in > patch 0003? If not, what needs to change? Why add a (failry complicated) pg_inet_pton() when a perfectly reasonable inet_pton() exists? I would get rid of all that refactoring and just have your code call inet_pton()/inet_ntop() directly. If you're worried about portability, and you don't want to go through the effort of proving libpgport substitutes, just have your code raise an error in the "#else" code paths. We can fill that in later if there is demand.
On Thu, 2022-03-31 at 16:32 +0200, Peter Eisentraut wrote: > Why add a (failry complicated) pg_inet_pton() when a perfectly > reasonable inet_pton() exists? I think it was mostly just that inet_aton() and pg_inet_net_ntop() both had ports, and I figured I might as well port the other one since we already had the implementation. (I don't have a good intuition yet for the community's preference for port vs dependency.) > I would get rid of all that refactoring and just have your code call > inet_pton()/inet_ntop() directly. > > If you're worried about portability, and you don't want to go through > the effort of proving libpgport substitutes, just have your code raise > an error in the "#else" code paths. We can fill that in later if there > is demand. Switched to inet_pton() in v12, with no #if/else for now. I think this should work with Winsock as-is; let's see if the bot agrees... Thanks, --Jacob
Attachment
On 31.03.22 20:15, Jacob Champion wrote: > On Thu, 2022-03-31 at 16:32 +0200, Peter Eisentraut wrote: >> Why add a (failry complicated) pg_inet_pton() when a perfectly >> reasonable inet_pton() exists? > > I think it was mostly just that inet_aton() and pg_inet_net_ntop() both > had ports, and I figured I might as well port the other one since we > already had the implementation. (I don't have a good intuition yet for > the community's preference for port vs dependency.) > >> I would get rid of all that refactoring and just have your code call >> inet_pton()/inet_ntop() directly. >> >> If you're worried about portability, and you don't want to go through >> the effort of proving libpgport substitutes, just have your code raise >> an error in the "#else" code paths. We can fill that in later if there >> is demand. > > Switched to inet_pton() in v12, with no #if/else for now. I think this > should work with Winsock as-is; let's see if the bot agrees... I have committed this. I have removed the inet header refactoring that you had. That wasn't necessary, since pg_inet_net_ntop() can use the normal AF_INET* constants. The PGSQL_AF_INET* constants are only for the internal storage of the inet/cidr types. I have added a configure test for inet_pton(). We can check in the build farm if it turns out to be necessary.
On Fri, 2022-04-01 at 16:07 +0200, Peter Eisentraut wrote: > I have committed this. > > I have removed the inet header refactoring that you had. That wasn't > necessary, since pg_inet_net_ntop() can use the normal AF_INET* > constants. The PGSQL_AF_INET* constants are only for the internal > storage of the inet/cidr types. > > I have added a configure test for inet_pton(). We can check in the > build farm if it turns out to be necessary. Thanks! --Jacob