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

On Tue, 2022-02-15 at 15:16 +0900, Kyotaro Horiguchi wrote:
> (This needs rebasing)

Done in v6, attached.

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

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

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

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

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

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

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

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

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