Re: [PATCH] Accept IP addresses in server certificate SANs

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