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

At Thu, 17 Feb 2022 17:29:15 +0000, Jacob Champion <pchampion(at)vmware(dot)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=ssltestuser host=localhost -f - -v ON_ERROR_STOP=1' at /home/horiguti/work/worktrees/ipsan/src/test/ssl/../../../src/test/perl/PostgreSQL/Test/Cluster.pm line 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