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

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