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

At Tue, 15 Mar 2022 21:41:49 +0000, Jacob Champion <pchampion@vmware.com> wrote in 
> 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
unknownca'
 
> > while running 'psql -XAtq -d sslrootcert=ssl/root+server_ca.crt sslmode=require dbname=trustdb hostaddr=127.0.0.1
user=ssltestuserhost=localhost -f - -v ON_ERROR_STOP=1' at
/home/horiguti/work/worktrees/ipsan/src/test/ssl/../../../src/test/perl/PostgreSQL/Test/Cluster.pmline 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.... I'm not sure how come I didn't noticed that, master also fails
for me fo the same reason.  In the past that may fail when valid
clinent-certs exists in the users ~/.postgresql but I believe that has
been fixed.


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

I'm not discussing on the meaning. Purely on the logical equivalancy
of the two ways to decide whether to visit CN.

Concretely, the equivalancy between this:

=====
 check_cn = true;
 rc = 0;
 for (i < san_len)
 {
   if (type matches)  check_cn = false;
   if (some error happens)  rc = nonzero;
 
   if (rc != 0)
     break;
 }
!if ((rc == 0) && check_cn) {}
=====

and this.

=====
 check_cn = true;
 rc = 0;
 for (i < san_len)
 {
   if (type matches)  check_cn = false;
   if (some error happens)  rc = nonzero;

   if (rc != 0)
   {
!    check_cn = false;
     break;
   }
 }
!if (check_cn) {}
=====

The two are equivalant to me. And if it is, the latter form smees
simpler and clearner to me.

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

Sorry, I was confused here. Please ignore that. I shoudl have said
something like the following instead.

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

Yes, in that case we don't visit CN because check_cn is false even if
we don't exit by (rc != 0) and that behavior is not changed by my
proposal.

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

Agreed.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center