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

On Mon, 2022-03-14 at 15:30 +0900, Kyotaro Horiguchi wrote:
> 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.

Hmm, the sslinfo tests are failing? I wouldn't have expected that based
on the patch changes. Just to confirm -- they pass for you without the
patch?

> > > 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.

Unmatched names, you mean? I'm not sure I understand.

If it helps, the two tests that will fail if check_cn is unset only at
the break are

- certificate with both a CN and SANs ignores CN
- certificate with both an IP CN and IP SANs ignores CN

because none of the SANs would match in that case.

> > > 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

Sounds good. I'll work on adding tests for the current behavior, and if
the committers don't like it, we can change it.

> > > 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.)

I see what you mean now. Yes, we deviate there as well (and have done
so for a while now). I think breaking compatibility there would
probably not go over well.

> Thanks. All behaviors and theier reasons is now clear. So....
>
> Let's leave them for committers for now.

Thank you for the review!

--Jacob