Date: Mon, 31 Jan 2022 17:29:49 +0900 (JST)
Message-Id: <20220131.172949.370093237602326154.horikyota.ntt@gmail.com>
To: pchampion@vmware.com
Cc: pgsql-hackers@postgresql.org
Subject: Re: [PATCH] Accept IP addresses in server certificate SANs
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <f7d53c77e33b2e22e520f2b3aac7c5b863636b85.camel@vmware.com>
References: <20211217.165430.655740392355518222.horikyota.ntt@gmail.com>
	<ca9737c960afdf42f236a58c9759fb5d5b960cf9.camel@vmware.com>
	<f7d53c77e33b2e22e520f2b3aac7c5b863636b85.camel@vmware.com>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20220131.172949.370093237602326154.horikyota.ntt%40gmail.com>
Precedence: bulk

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