Re: [PATCH] Accept IP addresses in server certificate SANs - Mailing list pgsql-hackers

(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