Re: implement subject alternative names support for SSL connections

Greetings,

I'd like to propose a patch for checking subject alternative names entry in
the SSL certificate for DNS names during SSL authentication.

When the client PGSSLMODE is set to verify-full, the common name (CN) of
the PostgreSQL server in the certificate is matched against the actual
host name supplied by the client. A successful connection is only
established when those names match.

If a failover schema with a floating IP is used, a single DNS name may
point to different hosts after failover. In order to maintain the correct
pair of (client connection FQDN, FQDN in the certificate) the certificate
from the master should also be transferred to the slave. Unfortunately, it
makes failover more complicated, since the server restart is required when
the new certificate is installed.

The other option is to issue a single certificate covering all the hosts
that may participate in the failover.

So far the only way to cover more than one server name with a single
certificate is to use wildcards in the server common name, i.e.
*.db.example com. Unfortunately, this schema only works for names like
master.db.example.com, slave.db.example.com, but not for the example.com
and db-master.example.com and db-slave.example.com or other more complex
naming schemas.

The other way to issue a single certificate for multiple names is to set
the subject alternative names, described in the RFC 3280 4.2.1.7. SAN
allows binding multiple identities to the certificate, including DNS names.
For the names above the SAN with look like this:

X509v3 Subject Alternative Name:
DNS:db-master.example.com, DNS:db-slave.example.com.

At the moment PostgreSQL doesn't support SANs, but should, according to the
following comment in the code in verify_peer_name_matches_certificate:

/*
* Extract the common name from the certificate.
*
* XXX: Should support alternate names here
*/

The attached patch adds support for checking the client supplied names
against the dNSName-s in SAN, making it easier to set secure HA
environments using PostgreSQL. If SAN section is present, the DNS names
from that section are checked against the peer name in addition to checking
the common name (CN) from the certificate. The match is successful if at
least one of those names match the name supplied by the peer.

I gave it a spin and it works in our environment (Linux database servers,
Linux and Mac clients). I don't have either Windows or old versions of
OpenSSL, it's not tested against those systems.

I'd appreciate your feedback.

Sincerely,
--
Alexey Klyukin

On Fri, Jul 25, 2014 at 6:10 PM, Alexey Klyukin <alexk(at)hintbits(dot)com> wrote:
> Greetings,
>
> I'd like to propose a patch for checking subject alternative names entry in
> the SSL certificate for DNS names during SSL authentication.
>
> When the client PGSSLMODE is set to verify-full, the common name (CN) of the
> PostgreSQL server in the certificate is matched against the actual host
> name supplied by the client. A successful connection is only established
> when those names match.
>
> If a failover schema with a floating IP is used, a single DNS name may point
> to different hosts after failover. In order to maintain the correct pair of
> (client connection FQDN, FQDN in the certificate) the certificate from the
> master should also be transferred to the slave. Unfortunately, it makes
> failover more complicated, since the server restart is required when the new
> certificate is installed.
>
> The other option is to issue a single certificate covering all the hosts
> that may participate in the failover.
>
> So far the only way to cover more than one server name with a single
> certificate is to use wildcards in the server common name, i.e. *.db.example
> com. Unfortunately, this schema only works for names like
> master.db.example.com, slave.db.example.com, but not for the example.com and
> db-master.example.com and db-slave.example.com or other more complex naming
> schemas.
>
> The other way to issue a single certificate for multiple names is to set the
> subject alternative names, described in the RFC 3280 4.2.1.7. SAN allows
> binding multiple identities to the certificate, including DNS names. For the
> names above the SAN with look like this:
>
> X509v3 Subject Alternative Name:
> DNS:db-master.example.com, DNS:db-slave.example.com.
>
> At the moment PostgreSQL doesn't support SANs, but should, according to the
> following comment in the code in verify_peer_name_matches_certificate:
>
> /*
> * Extract the common name from the certificate.
> *
> * XXX: Should support alternate names here
> */

I believe that comment is mine, and yes, it should definitely be done.
So absolutely +1 on the feature :)

> The attached patch adds support for checking the client supplied names
> against the dNSName-s in SAN, making it easier to set secure HA environments
> using PostgreSQL. If SAN section is present, the DNS names from that section
> are checked against the peer name in addition to checking the common name
> (CN) from the certificate. The match is successful if at least one of those
> names match the name supplied by the peer.
>
> I gave it a spin and it works in our environment (Linux database servers,
> Linux and Mac clients). I don't have either Windows or old versions of
> OpenSSL, it's not tested against those systems.

We definitely need testing for older openssl. I don't think Windows
will make a difference, but openssl definitely may - there is a fairly
large chance we need a configure check.
>
> I'd appreciate your feedback.

I just took a very quick look at the code, and just noticed one thing:

Why keep looping once you've found a match? When you set result=true
you should break; from the loop I think. Not necessarily for
performance, but there might be something about a different extension
we can't parse for example, no need to fail in that case.

Please add it to the next CF - this was just a very quick review, and
it needs a proper one along with openssl version testing :)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Fri, Jul 25, 2014 at 6:34 PM, Magnus Hagander <magnus(at)hagander(dot)net>
wrote:

>
> I just took a very quick look at the code, and just noticed one thing:
>
> Why keep looping once you've found a match? When you set result=true
> you should break; from the loop I think. Not necessarily for
> performance, but there might be something about a different extension
> we can't parse for example, no need to fail in that case.
>

The for loop header is for (i = 0; i < alt_names_total && !result; i++), so
the loop
should terminate right when the result becomes true, which happens if the
pg_strcasecmp
finds a match between the given dNSName and the name supplied by the client.

>
> Please add it to the next CF - this was just a very quick review, and
> it needs a proper one along with openssl version testing :)
>

Done.

Sincerely,
--
Alexey Klyukin

On Fri, Jul 25, 2014 at 7:15 PM, Alexey Klyukin <alexk(at)hintbits(dot)com> wrote:
> On Fri, Jul 25, 2014 at 6:34 PM, Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
>>
>>
>> I just took a very quick look at the code, and just noticed one thing:
>>
>> Why keep looping once you've found a match? When you set result=true
>> you should break; from the loop I think. Not necessarily for
>> performance, but there might be something about a different extension
>> we can't parse for example, no need to fail in that case.
>
>
>
> The for loop header is for (i = 0; i < alt_names_total && !result; i++), so
> the loop
> should terminate right when the result becomes true, which happens if the
> pg_strcasecmp
> finds a match between the given dNSName and the name supplied by the client.

oh, ha. So yeah, that was too quick to count as a review - clearly :)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Fri, Jul 25, 2014 at 7:15 PM, Alexey Klyukin <alexk(at)hintbits(dot)com> wrote:
>> On Fri, Jul 25, 2014 at 6:34 PM, Magnus Hagander <magnus(at)hagander(dot)net>
>>> Why keep looping once you've found a match? When you set result=true
>>> you should break; from the loop I think. Not necessarily for
>>> performance, but there might be something about a different extension
>>> we can't parse for example, no need to fail in that case.

>> The for loop header is for (i = 0; i < alt_names_total && !result; i++), so
>> the loop
>> should terminate right when the result becomes true, which happens if the
>> pg_strcasecmp
>> finds a match between the given dNSName and the name supplied by the client.

> oh, ha. So yeah, that was too quick to count as a review - clearly :)

FWIW, I find that type of loop coding to be extremely poor style,
precisely because it's not too readable. A break in the loop body is
*far* more obvious to the reader. (Not to mention that it doesn't
add overhead to the loop on iterations where you can't break.)

regards, tom lane

On 07/25/2014 07:10 PM, Alexey Klyukin wrote:
> Greetings,
>
> I'd like to propose a patch for checking subject alternative names entry in
> the SSL certificate for DNS names during SSL authentication.

Thanks! I just ran into this missing feature last week, while working on
my SSL test suite. So +1 for having the feature.

This patch needs to be rebased over current master branch, thanks to my
refactoring that moved all OpenSSL-specific stuff to be-secure-openssl.c.

- Heikki

On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas <
hlinnakangas(at)vmware(dot)com> wrote:

> On 07/25/2014 07:10 PM, Alexey Klyukin wrote:
>
>> Greetings,
>>
>> I'd like to propose a patch for checking subject alternative names entry
>> in
>> the SSL certificate for DNS names during SSL authentication.
>>
>
> Thanks! I just ran into this missing feature last week, while working on
> my SSL test suite. So +1 for having the feature.
>
> This patch needs to be rebased over current master branch, thanks to my
> refactoring that moved all OpenSSL-specific stuff to be-secure-openssl.c.

The patch is rebased against fe-secure-openssl.c (that's where
verify_peer_name_matches_certificate appeared in the master branch), I've
changed the condition in the for loop to be less confusing (thanks to
comments from Magnus and Tom), making an explicit break once a match is
detected.

Note that It generates a lot of OpenSSL related warnings on my system (66
total) with clang, complaining about
$X is deprecated: first deprecated in OS X 10.7
[-Wdeprecated-declarations], but it does so for most other SSL functions,
so I don't think it's a problem introduced by this patch.

Sincerely,
Alexey.

On 08/24/2014 03:11 PM, Alexey Klyukin wrote:
> On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas <
> hlinnakangas(at)vmware(dot)com> wrote:
>
>> On 07/25/2014 07:10 PM, Alexey Klyukin wrote:
>>
>>> Greetings,
>>>
>>> I'd like to propose a patch for checking subject alternative names entry
>>> in
>>> the SSL certificate for DNS names during SSL authentication.
>>>
>>
>> Thanks! I just ran into this missing feature last week, while working on
>> my SSL test suite. So +1 for having the feature.
>>
>> This patch needs to be rebased over current master branch, thanks to my
>> refactoring that moved all OpenSSL-specific stuff to be-secure-openssl.c.
>
>
> The patch is rebased against fe-secure-openssl.c (that's where
> verify_peer_name_matches_certificate appeared in the master branch), I've
> changed the condition in the for loop to be less confusing (thanks to
> comments from Magnus and Tom), making an explicit break once a match is
> detected.

The patch doesn't seem to support wildcards in alternative names. Is
that on purpose?

It would be good to add a little helper function that does the
NULL-check, straight comparison, and wildcard check, for a single name.
And then use that for the Common Name and all the Alternatives. That'll
ensure that all the same rules apply whether the name is the Common Name
or an Alternative (assuming that the rules are supposed to be the same;
I don't know if that's true).

But actually, I wonder if we should delegate the whole hostname matching
to OpenSSL? There's a function called X509_check_host for that, although
it's new in OpenSSL 1.1.0 so we'd need to add a configure test for that
and keep the current code to handle older versions.

- Heikki

On 2014-08-25 13:02:50 +0300, Heikki Linnakangas wrote:
> But actually, I wonder if we should delegate the whole hostname matching to
> OpenSSL? There's a function called X509_check_host for that, although it's
> new in OpenSSL 1.1.0 so we'd need to add a configure test for that and keep
> the current code to handle older versions.

Given that we're about to add support for other SSL implementations I'm
not sure that that's a good idea. IIRC there exist quite a bit of
different interpretations about what denotes a valid cert between the
libraries. Doesn't sound fun to me.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 08/25/2014 01:07 PM, Andres Freund wrote:
> On 2014-08-25 13:02:50 +0300, Heikki Linnakangas wrote:
>> But actually, I wonder if we should delegate the whole hostname matching to
>> OpenSSL? There's a function called X509_check_host for that, although it's
>> new in OpenSSL 1.1.0 so we'd need to add a configure test for that and keep
>> the current code to handle older versions.
>
> Given that we're about to add support for other SSL implementations I'm
> not sure that that's a good idea. IIRC there exist quite a bit of
> different interpretations about what denotes a valid cert between the
> libraries.

Really? That sounds scary. I can imagine that some libraries support
more complicated stuff like Internationalized Domain Names, while others
don't, but as long as they all behave the same with the basic stuff, I
think that's acceptable.

> Doesn't sound fun to me.

As long as just this patch is concerned, I agree it's easier to just
implement it ourselves, but if we want to start implementing more
complicated rules, then I'd rather not get into that business at all,
and let the SSL library vendor deal with the bugs and CVEs.

I guess we'll go ahead with this patch for now, but keep this in mind if
someone wants to complicate the rules further in the future.

- Heikki

On Mon, Aug 25, 2014 at 12:02 PM, Heikki Linnakangas <
hlinnakangas(at)vmware(dot)com> wrote:

> On 08/24/2014 03:11 PM, Alexey Klyukin wrote:
>
>> On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas <
>> hlinnakangas(at)vmware(dot)com> wrote:
>>
>>>
>>> The patch doesn't seem to support wildcards in alternative names. Is
> that on purpose?
>

Not really, we just did not have any use case for them, but it seems that
RFC 5280 does not disallow them:

" Finally, the semantics of subject alternative names that include
wildcard characters (e.g., as a placeholder for a set of names) are
not addressed by this specification. Applications with specific
requirements MAY use such names, but they must define the semantics."

I've added support for them in the next iteration of the patch attached to
this email.

>
> It would be good to add a little helper function that does the NULL-check,
> straight comparison, and wildcard check, for a single name. And then use
> that for the Common Name and all the Alternatives. That'll ensure that all
> the same rules apply whether the name is the Common Name or an Alternative
> (assuming that the rules are supposed to be the same; I don't know if
> that's true).
>

Thanks, common code has been moved into a separate new function.

Another question is how should we treat the certificates with no CN and
non-empty SAN?

Current code just bails out right after finding no CN section present , but
the RFC (5280) says:
"
Further, if the only subject identity included in the certificate is
an alternative name form (e.g., an electronic mail address), then the
subject distinguished name MUST be empty (an empty sequence), and the
subjectAltName extension MUST be present.
"
which to me sounds like the possibility of coming across such certificates
in the wild, although I personally see little use in them.

Regards,
--
Alexey Klyukin

On Mon, Aug 25, 2014 at 12:33 PM, Heikki Linnakangas <
hlinnakangas(at)vmware(dot)com> wrote:

> On 08/25/2014 01:07 PM, Andres Freund wrote:
>
>> On 2014-08-25 13:02:50 +0300, Heikki Linnakangas wrote:
>>
>>> But actually, I wonder if we should delegate the whole hostname matching
>>> to
>>> OpenSSL? There's a function called X509_check_host for that, although
>>> it's
>>> new in OpenSSL 1.1.0 so we'd need to add a configure test for that and
>>> keep
>>> the current code to handle older versions.
>>>
>>
>> Given that we're about to add support for other SSL implementations I'm
>> not sure that that's a good idea. IIRC there exist quite a bit of
>> different interpretations about what denotes a valid cert between the
>> libraries.
>>
>
>
> As long as just this patch is concerned, I agree it's easier to just
> implement it ourselves, but if we want to start implementing more
> complicated rules, then I'd rather not get into that business at all, and
> let the SSL library vendor deal with the bugs and CVEs.
>

Sounds reasonable.

>
> I guess we'll go ahead with this patch for now, but keep this in mind if
> someone wants to complicate the rules further in the future.

+1

--
Regards,
Alexey Klyukin

On 08/28/2014 07:28 PM, Alexey Klyukin wrote:
> On Mon, Aug 25, 2014 at 12:02 PM, Heikki Linnakangas <
> hlinnakangas(at)vmware(dot)com> wrote:
>
>> On 08/24/2014 03:11 PM, Alexey Klyukin wrote:
>>
>>> On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas <
>>> hlinnakangas(at)vmware(dot)com> wrote:
>>>
>>>> The patch doesn't seem to support wildcards in alternative names. Is
>> that on purpose?
>
> Not really, we just did not have any use case for them, but it seems that
> RFC 5280 does not disallow them:
>
> " Finally, the semantics of subject alternative names that include
> wildcard characters (e.g., as a placeholder for a set of names) are
> not addressed by this specification. Applications with specific
> requirements MAY use such names, but they must define the semantics."
>
> I've added support for them in the next iteration of the patch attached to
> this email.

Hmm. So wildcards MAY be supported, but should we? I think we should
follow the example of common browsers here, or OpenSSL or other SSL
libraries; what do they do?

RFC 6125 section 6.4.4 Checking of Common Names says:

> As noted, 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.

So, to conform to that we shouldn't check the Common Name at all, if an
alternative subject field is present.

(Relying on OpenSSL's hostname-checking function is starting feel more
and more appetizing. At least it won't be our problem then.)

>> It would be good to add a little helper function that does the NULL-check,
>> straight comparison, and wildcard check, for a single name. And then use
>> that for the Common Name and all the Alternatives. That'll ensure that all
>> the same rules apply whether the name is the Common Name or an Alternative
>> (assuming that the rules are supposed to be the same; I don't know if
>> that's true).
>
> Thanks, common code has been moved into a separate new function.
>
> Another question is how should we treat the certificates with no CN and
> non-empty SAN?
>
> Current code just bails out right after finding no CN section present , but
> the RFC (5280) says:
> "
> Further, if the only subject identity included in the certificate is
> an alternative name form (e.g., an electronic mail address), then the
> subject distinguished name MUST be empty (an empty sequence), and the
> subjectAltName extension MUST be present.
> "
> which to me sounds like the possibility of coming across such certificates
> in the wild, although I personally see little use in them.

Yeah, I think a certificate without CN should be supported. See also RFC
6125, section 4.1. "Rules" [for issuers of certificates]:

> 5. Even though many deployed clients still check for the CN-ID
> within the certificate subject field, certification authorities
> are encouraged to migrate away from issuing certificates that
> represent the server's fully qualified DNS domain name in a
> CN-ID. Therefore, the certificate SHOULD NOT include a CN-ID
> unless the certification authority issues the certificate in
> accordance with a specification that reuses this one and that
> explicitly encourages continued support for the CN-ID identifier
> type in the context of a given application technology.

Certificates without a CN-ID are probably rare today, but they might
start to appear in the future.

BTW, should we also support alternative subject names in the server, in
client certificates? And if there are multiple names, which one takes
effect? Perhaps better to leave that for a separate patch.

- Heikki

On Fri, Aug 29, 2014 at 11:22 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
>
> On 08/28/2014 07:28 PM, Alexey Klyukin wrote:
>>
>> On Mon, Aug 25, 2014 at 12:02 PM, Heikki Linnakangas <
>> hlinnakangas(at)vmware(dot)com> wrote:
>>
>>> On 08/24/2014 03:11 PM, Alexey Klyukin wrote:
>>>
>>>> On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas <
>>>> hlinnakangas(at)vmware(dot)com> wrote:
>>>>
>>>>> The patch doesn't seem to support wildcards in alternative names. Is
>>>
>>> that on purpose?
>>
>>
>> Not really, we just did not have any use case for them, but it seems that
>> RFC 5280 does not disallow them:
>>
>> " Finally, the semantics of subject alternative names that include
>> wildcard characters (e.g., as a placeholder for a set of names) are
>> not addressed by this specification. Applications with specific
>> requirements MAY use such names, but they must define the semantics."
>>
>> I've added support for them in the next iteration of the patch attached to
>> this email.
>
>
> Hmm. So wildcards MAY be supported, but should we? I think we should follow the example of common browsers here, or OpenSSL or other SSL libraries; what do they do?

Yes, they seem to be supported. The function you've mentioned above
(X509_check_host) specifically mentions wildcards in SANs at
https://www.openssl.org/docs/crypto/X509_check_host.html:

'X509_check_host() checks if the certificate Subject Alternative Name
(SAN) or Subject CommonName (CN) matches the specified host name,
which must be encoded in the preferred name syntax described in
section 3.5 of RFC 1034. By default, wildcards are supported and they
match only in the left-most label; but they may match part of that
label with an explicit prefix or suffix. For example, by default, the
host name ``www.example.com'' would match a certificate with a SAN or
CN value of ``*.example.com'', ``w*.example.com'' or
``*w.example.com''.'

>
> RFC 6125 section 6.4.4 Checking of Common Names says:
>
>> As noted, 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.
>
>
> So, to conform to that we shouldn't check the Common Name at all, if an alternative subject field is present.

While the RFC indeed says so, the OpenSSL implementation includes
X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT flag (which, as far as I can see,
is set to 1 by default), which makes it check for the CN even if DNS
names in SAN are present. I'm not sure what is the reason behind
section 6.4.4, and I think it makes sense to check CN as well, since
it does not lead to the case of false matches.

>
>
> Yeah, I think a certificate without CN should be supported. See also RFC 6125, section 4.1. "Rules" [for issuers of certificates]:
>
>> 5. Even though many deployed clients still check for the CN-ID
>> within the certificate subject field, certification authorities
>> are encouraged to migrate away from issuing certificates that
>> represent the server's fully qualified DNS domain name in a
>> CN-ID. Therefore, the certificate SHOULD NOT include a CN-ID
>> unless the certification authority issues the certificate in
>> accordance with a specification that reuses this one and that
>> explicitly encourages continued support for the CN-ID identifier
>> type in the context of a given application technology.
>
>
> Certificates without a CN-ID are probably rare today, but they might start to appear in the future.

Ok, I will change a patch to add support for this clause.

>
>
> BTW, should we also support alternative subject names in the server, in client certificates? And if there are multiple names, which one takes effect? Perhaps better to leave that for a separate patch.

Good question. The RFC 5280 section 4.2.1.6 does not restrict the
certificates to be used only server-side, so the same rules should be
applicable to the client certs. I'm wondering if there is an
equivalent of RFC 6125 for the clients?

PostgreSQL, however, checks different things on the backends and the
clients, the formers are checked against the DNS name, while on the
clients only the user name is checked. For this, I think, a SAN
section
with some custom identity type should be issued (the 4.2.1.6 does not
list user names as a pre-defined identify type). Note that PostgreSQL
can already support clients with multiple names via the user maps, so
support for SAN is not that urgent there.

On the other hand, should we, in addition to verification of client
user names, verify the client names supplied during connections
against the DNS entries in their certificates? Are there use cases for
this?

I do agree this should be the subject of a separate patch.

Regards,
Alexey

On Mon, Sep 1, 2014 at 10:39 AM, Alexey Klyukin <alexk(at)hintbits(dot)com> wrote:
> On Fri, Aug 29, 2014 at 11:22 AM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
>> Yeah, I think a certificate without CN should be supported. See also RFC 6125, section 4.1. "Rules" [for issuers of certificates]:
>>
>>> 5. Even though many deployed clients still check for the CN-ID
>>> within the certificate subject field, certification authorities
>>> are encouraged to migrate away from issuing certificates that
>>> represent the server's fully qualified DNS domain name in a
>>> CN-ID. Therefore, the certificate SHOULD NOT include a CN-ID
>>> unless the certification authority issues the certificate in
>>> accordance with a specification that reuses this one and that
>>> explicitly encourages continued support for the CN-ID identifier
>>> type in the context of a given application technology.
>>
>>
>> Certificates without a CN-ID are probably rare today, but they might start to appear in the future.
>
> Ok, I will change a patch to add support for this clause.

Attached is a new version. I've changed the logic to check for the SAN
names first, and only check the common name if there is no match. The
error when the common name is missing is only shown if SAN section
does not contain any DNS names as well. The tricky part is the error
message if no match was found: initially, it only listed a single
common name, but now tracking all DNS names just for the sake of the
error message makes the code more bloated, so I'm wondering if simply
stating that there was no match, as implemented in the attached patch,
would be good enough?

--
Regards,
Alexey Klyukin

On 09/01/2014 09:14 PM, Alexey Klyukin wrote:
> On Mon, Sep 1, 2014 at 10:39 AM, Alexey Klyukin <alexk(at)hintbits(dot)com> wrote:
>> On Fri, Aug 29, 2014 at 11:22 AM, Heikki Linnakangas
>> <hlinnakangas(at)vmware(dot)com> wrote:
>>> Yeah, I think a certificate without CN should be supported. See also RFC 6125, section 4.1. "Rules" [for issuers of certificates]:
>>>
>>>> 5. Even though many deployed clients still check for the CN-ID
>>>> within the certificate subject field, certification authorities
>>>> are encouraged to migrate away from issuing certificates that
>>>> represent the server's fully qualified DNS domain name in a
>>>> CN-ID. Therefore, the certificate SHOULD NOT include a CN-ID
>>>> unless the certification authority issues the certificate in
>>>> accordance with a specification that reuses this one and that
>>>> explicitly encourages continued support for the CN-ID identifier
>>>> type in the context of a given application technology.
>>>
>>>
>>> Certificates without a CN-ID are probably rare today, but they might start to appear in the future.
>>
>> Ok, I will change a patch to add support for this clause.
>
> Attached is a new version. I've changed the logic to check for the SAN
> names first, and only check the common name if there is no match. The
> error when the common name is missing is only shown if SAN section
> does not contain any DNS names as well.

* It's ugly that the caller does the malloc and memcpy, and the
certificate_name_entry_validate_match function then modifies its name
argument. Move the malloc+memcpy inside the function.

* The error message in certificate_name_entry_validate_match says "SSL
certificate's common name contains embedded null" even though it's also
used for SANs.

> The tricky part is the error
> message if no match was found: initially, it only listed a single
> common name, but now tracking all DNS names just for the sake of the
> error message makes the code more bloated, so I'm wondering if simply
> stating that there was no match, as implemented in the attached patch,
> would be good enough?

Hmm. It would still be nice to say something about the certificate that
was received. How about:

server certificate with common name "%s" does not match host name "%s"

?

- Heikki

On Wed, Sep 3, 2014 at 11:50 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:

> * It's ugly that the caller does the malloc and memcpy, and the
> certificate_name_entry_validate_match function then modifies its name
> argument. Move the malloc+memcpy inside the function.

For the case of CN the caller has to do the malloc anyway, since
X509_NAME_get_text_by_NID expects the already allocated buffer.
This means that 'movable' malloc + memcpy occurs only once, and while
it would still make sense to move it into the function, it would also
mean we would do an unnecessary malloc for the case of CN.

>
> * The error message in certificate_name_entry_validate_match says "SSL
> certificate's common name contains embedded null" even though it's also used
> for SANs.

Will fix, thank you.

>
>
>> The tricky part is the error
>> message if no match was found: initially, it only listed a single
>> common name, but now tracking all DNS names just for the sake of the
>> error message makes the code more bloated, so I'm wondering if simply
>> stating that there was no match, as implemented in the attached patch,
>> would be good enough?
>
>
> Hmm. It would still be nice to say something about the certificate that was
> received. How about:
>
> server certificate with common name "%s" does not match host name "%s"

We cannot guarantee at this point that the common name is present in
the certificate. And if it is not, which name should we pick instead?
One way to solve this is to take the first non-NULL name, but it if
there is no common name, but then the wording of the error message
would depend on the availability of CN.
Another is to show all available names, but I do not like collecting
them just for the sake of displaying in the error message.
And last one is to just show the error without mentioning names,
that's what I've chosen to be the most consistent.

Regards,
--
Alexey Klyukin

On 09/04/2014 10:33 AM, Alexey Klyukin wrote:
> On Wed, Sep 3, 2014 at 11:50 AM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
>
>> * It's ugly that the caller does the malloc and memcpy, and the
>> certificate_name_entry_validate_match function then modifies its name
>> argument. Move the malloc+memcpy inside the function.
>
> For the case of CN the caller has to do the malloc anyway, since
> X509_NAME_get_text_by_NID expects the already allocated buffer.
> This means that 'movable' malloc + memcpy occurs only once, and while
> it would still make sense to move it into the function, it would also
> mean we would do an unnecessary malloc for the case of CN.

Hmm. Perhaps we should use X509_NAME_get_index_by_NID +
X509_NAME_get_entry instead of X509_NAME_get_text_by_NID. You could then
pass the ASN1_STRING object to the
certificate_name_entry_validate_match() function, and have it do the
ASN1_STRING_length() and ASN1_STRING_data() calls too.

>>> The tricky part is the error
>>> message if no match was found: initially, it only listed a single
>>> common name, but now tracking all DNS names just for the sake of the
>>> error message makes the code more bloated, so I'm wondering if simply
>>> stating that there was no match, as implemented in the attached patch,
>>> would be good enough?
>>
>> Hmm. It would still be nice to say something about the certificate that was
>> received. How about:
>>
>> server certificate with common name "%s" does not match host name "%s"
>
> We cannot guarantee at this point that the common name is present in
> the certificate. And if it is not, which name should we pick instead?
> One way to solve this is to take the first non-NULL name, but it if
> there is no common name, but then the wording of the error message
> would depend on the availability of CN.
> Another is to show all available names, but I do not like collecting
> them just for the sake of displaying in the error message.
> And last one is to just show the error without mentioning names,
> that's what I've chosen to be the most consistent.

I think we should:

1. Check if there's a common name, and if so, print that
2. Check if there is exactly one SAN, and if so, print that
3. Just print an error without mentioning names.

There's a lot of value in printing the name if possible, so I'd really
like to keep that. But I agree that printing all the names if there are
several would get complicated and the error message could become very
long. Yeah, the error message might need to be different for cases 1 and
2. Or maybe phrase it "server certificate's name \"%s\" does not match
host name \"%s\"", which would be reasonable for both 1. and 2.

- Heikki

On Thu, Sep 4, 2014 at 10:23 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
>
> Hmm. Perhaps we should use X509_NAME_get_index_by_NID + X509_NAME_get_entry
> instead of X509_NAME_get_text_by_NID. You could then pass the ASN1_STRING
> object to the certificate_name_entry_validate_match() function, and have it
> do the ASN1_STRING_length() and ASN1_STRING_data() calls too.
...
> I think we should:
>
> 1. Check if there's a common name, and if so, print that
> 2. Check if there is exactly one SAN, and if so, print that
> 3. Just print an error without mentioning names.
>
> There's a lot of value in printing the name if possible, so I'd really like
> to keep that. But I agree that printing all the names if there are several
> would get complicated and the error message could become very long. Yeah,
> the error message might need to be different for cases 1 and 2. Or maybe
> phrase it "server certificate's name \"%s\" does not match host name
> \"%s\"", which would be reasonable for both 1. and 2.

Thank you, I've implemented both suggestions in the attached new
version of the patch.
On the error message, I've decided to show either a single name, or
the first examined name and the number of other names present in the
certificate, i.e:

> psql: server name "example.com" and 2 other names from server SSL certificate do not match host name "example-foo.com"

The error does not state whether the names comes from the CN or from
the SAN section.

This version also checks for the availability of the subject name, it
looks like RFC 5280 does not require it at all.

4.1.2.6. Subject

The subject field identifies the entity associated with the public
key stored in the subject public key field. The subject name MAY be
carried in the subject field and/or the subjectAltName extension.

The pattern of allocating the name in the subroutine and then
reporting it (and releasing when necessary) in the main function is a
little bit ugly, but it looks to me the least ugly of anything else I
could come up (i.e. extracting those names at the time the error
message is shown).

Regards,

--
Alexey Klyukin

On 09/05/2014 07:30 PM, Alexey Klyukin wrote:
> On Thu, Sep 4, 2014 at 10:23 AM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
>>
>> Hmm. Perhaps we should use X509_NAME_get_index_by_NID + X509_NAME_get_entry
>> instead of X509_NAME_get_text_by_NID. You could then pass the ASN1_STRING
>> object to the certificate_name_entry_validate_match() function, and have it
>> do the ASN1_STRING_length() and ASN1_STRING_data() calls too.
> ...
>> I think we should:
>>
>> 1. Check if there's a common name, and if so, print that
>> 2. Check if there is exactly one SAN, and if so, print that
>> 3. Just print an error without mentioning names.
>>
>> There's a lot of value in printing the name if possible, so I'd really like
>> to keep that. But I agree that printing all the names if there are several
>> would get complicated and the error message could become very long. Yeah,
>> the error message might need to be different for cases 1 and 2. Or maybe
>> phrase it "server certificate's name \"%s\" does not match host name
>> \"%s\"", which would be reasonable for both 1. and 2.
>
> Thank you, I've implemented both suggestions in the attached new
> version of the patch.
> On the error message, I've decided to show either a single name, or
> the first examined name and the number of other names present in the
> certificate, i.e:
>
>> psql: server name "example.com" and 2 other names from server SSL certificate do not match host name "example-foo.com"
>
> The error does not state whether the names comes from the CN or from
> the SAN section.

I'd reword that slightly, to:

psql: server certificate for "example.com" (and 2 other names) does not
match host name "example-foo.com"

I never liked the current wording, "server name "foo"" very much. I
think it's important to use the word "server certificate" in the error
message, to make it clear where the problem is.

For translations, that message should be "pluralized", but there is no
libpq_ngettext macro equivalent to ngettext(), like libpq_gettext. I
wonder if that was left out on purpose, or if we just haven't needed
that in libpq before. Anyway, I think we need to add that for this.

> This version also checks for the availability of the subject name, it
> looks like RFC 5280 does not require it at all.
>
> 4.1.2.6. Subject
>
> The subject field identifies the entity associated with the public
> key stored in the subject public key field. The subject name MAY be
> carried in the subject field and/or the subjectAltName extension.

Ok.

> The pattern of allocating the name in the subroutine and then
> reporting it (and releasing when necessary) in the main function is a
> little bit ugly, but it looks to me the least ugly of anything else I
> could come up (i.e. extracting those names at the time the error
> message is shown).

I reworked that a bit, see attached. What do you think?

I think this is ready for commit, except for two things:

1. The pluralization of the message for translation

2. I still wonder if we should follow the RFC 6125 and not check the
Common Name at all, if SANs are present. I don't really understand the
point of that rule, and it seems unlikely to pose a security issue in
practice, because surely a CA won't sign a certificate with a
bogus/wrong CN, because an older client that doesn't look at the SANs at
all would use the CN anyway. But still... what does a Typical Web
Browser do?

- Heikki

On Mon, Sep 8, 2014 at 8:04 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> On 09/05/2014 07:30 PM, Alexey Klyukin wrote:

>> The error does not state whether the names comes from the CN or from
>> the SAN section.
>
>
> I'd reword that slightly, to:
>
> psql: server certificate for "example.com" (and 2 other names) does not
> match host name "example-foo.com"
>
> I never liked the current wording, "server name "foo"" very much. I think
> it's important to use the word "server certificate" in the error message, to
> make it clear where the problem is.

+1

>
> For translations, that message should be "pluralized", but there is no
> libpq_ngettext macro equivalent to ngettext(), like libpq_gettext. I wonder
> if that was left out on purpose, or if we just haven't needed that in libpq
> before. Anyway, I think we need to add that for this.

Well, the translation guidelines in the documentation suggest avoiding
plurals if possible, but I don't like rephrasing the sentence with
'names total: %d', so attached is my attempt to add the function.
I have also moved the one-time library binding code to the function of its own.

>
>> This version also checks for the availability of the subject name, it
>> looks like RFC 5280 does not require it at all.
>>
>> 4.1.2.6. Subject
>>
>> The subject field identifies the entity associated with the public
>> key stored in the subject public key field. The subject name MAY be
>> carried in the subject field and/or the subjectAltName extension.
>
>
> Ok.
>
>> The pattern of allocating the name in the subroutine and then
>> reporting it (and releasing when necessary) in the main function is a
>> little bit ugly, but it looks to me the least ugly of anything else I
>> could come up (i.e. extracting those names at the time the error
>> message is shown).
>
>
> I reworked that a bit, see attached. What do you think?

Thank you, I like your approach of unconditionally allocating and
freeing memory, it makes the code easier to read.
2 minor changes I've made in v7 (in addition to adding the libpq_ngettext):

- the verify_peer_name_matches_certificate does not try to return -1
(since it returns bool, it would be misinterpreted as true)
- removed the !got_error && !found_match check from the for loop
header to the loop body per style comment from Tom in the beginning of
this thread.

>
> I think this is ready for commit, except for two things:
>
> 1. The pluralization of the message for translation
>
> 2. I still wonder if we should follow the RFC 6125 and not check the Common
> Name at all, if SANs are present. I don't really understand the point of
> that rule, and it seems unlikely to pose a security issue in practice,
> because surely a CA won't sign a certificate with a bogus/wrong CN, because
> an older client that doesn't look at the SANs at all would use the CN
> anyway. But still... what does a Typical Web Browser do?
>

At least Firefox and Safari seem to follow RFC strictly, according to
some anecdotical evidences (I haven't got enough time to dig into the
source code yet):

http://superuser.com/questions/230136/primary-common-name-in-subjectaltname
https://bugzilla.mozilla.org/show_bug.cgi?id=238142

Chrome seem to follow them, so the major open-source browsers are
ignoring the Common Name if the SubjectAltName is present.
Still, I see no win in ignoring CN (except for the shorter code), it
just annoys some users that forgot to put the CN name also in the SAN
section, so my 5 cents is that we should check both.

Regards,
--
Alexey Klyukin

On 09/11/2014 08:46 PM, Alexey Klyukin wrote:
> On Mon, Sep 8, 2014 at 8:04 PM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
>> 2. I still wonder if we should follow the RFC 6125 and not check the Common
>> Name at all, if SANs are present. I don't really understand the point of
>> that rule, and it seems unlikely to pose a security issue in practice,
>> because surely a CA won't sign a certificate with a bogus/wrong CN, because
>> an older client that doesn't look at the SANs at all would use the CN
>> anyway. But still... what does a Typical Web Browser do?
>
> At least Firefox and Safari seem to follow RFC strictly, according to
> some anecdotical evidences (I haven't got enough time to dig into the
> source code yet):
>
> http://superuser.com/questions/230136/primary-common-name-in-subjectaltname
> https://bugzilla.mozilla.org/show_bug.cgi?id=238142
>
> Chrome seem to follow them, so the major open-source browsers are
> ignoring the Common Name if the SubjectAltName is present.
> Still, I see no win in ignoring CN (except for the shorter code), it
> just annoys some users that forgot to put the CN name also in the SAN
> section, so my 5 cents is that we should check both.

Hmm. If that's what the browsers do, I think we should also err on the
side of caution here. Ignoring the CN is highly unlikely to cause anyone
a problem; a CA worth its salt should not issue a certificate with a CN
that's not also listed in the SAN section. But if you have such a
certificate anyway for some reason, it shouldn't be too difficult to get
a new certificate. Certificates expire every 1-3 years anyway, so there
must be a procedure to renew them anyway.

- Heikki

On 09/12/2014 01:30 PM, Heikki Linnakangas wrote:
> On 09/11/2014 08:46 PM, Alexey Klyukin wrote:
>> On Mon, Sep 8, 2014 at 8:04 PM, Heikki Linnakangas
>> <hlinnakangas(at)vmware(dot)com> wrote:
>>> 2. I still wonder if we should follow the RFC 6125 and not check the Common
>>> Name at all, if SANs are present. I don't really understand the point of
>>> that rule, and it seems unlikely to pose a security issue in practice,
>>> because surely a CA won't sign a certificate with a bogus/wrong CN, because
>>> an older client that doesn't look at the SANs at all would use the CN
>>> anyway. But still... what does a Typical Web Browser do?
>>
>> At least Firefox and Safari seem to follow RFC strictly, according to
>> some anecdotical evidences (I haven't got enough time to dig into the
>> source code yet):
>>
>> http://superuser.com/questions/230136/primary-common-name-in-subjectaltname
>> https://bugzilla.mozilla.org/show_bug.cgi?id=238142
>>
>> Chrome seem to follow them, so the major open-source browsers are
>> ignoring the Common Name if the SubjectAltName is present.
>> Still, I see no win in ignoring CN (except for the shorter code), it
>> just annoys some users that forgot to put the CN name also in the SAN
>> section, so my 5 cents is that we should check both.
>
> Hmm. If that's what the browsers do, I think we should also err on the
> side of caution here. Ignoring the CN is highly unlikely to cause anyone
> a problem; a CA worth its salt should not issue a certificate with a CN
> that's not also listed in the SAN section. But if you have such a
> certificate anyway for some reason, it shouldn't be too difficult to get
> a new certificate. Certificates expire every 1-3 years anyway, so there
> must be a procedure to renew them anyway.

Committed, with that change, ie. the CN is not checked if SANs are present.

Thanks for bearing through all these iterations!

- Heikki

On Fri, Sep 12, 2014 at 4:20 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:

>> Hmm. If that's what the browsers do, I think we should also err on the
>> side of caution here. Ignoring the CN is highly unlikely to cause anyone
>> a problem; a CA worth its salt should not issue a certificate with a CN
>> that's not also listed in the SAN section. But if you have such a
>> certificate anyway for some reason, it shouldn't be too difficult to get
>> a new certificate. Certificates expire every 1-3 years anyway, so there
>> must be a procedure to renew them anyway.
>
>
> Committed, with that change, ie. the CN is not checked if SANs are present.
>
> Thanks for bearing through all these iterations!

Great news! Thank you very much for devoting your time and energy to
the review and providing such a useful feedback!
On the CN thing, I don't have particularly strong arguments for either
of the possible behaviors, so sticking to RFC makes sense here

Sincerely,
--
Alexey Klyukin

On Mon, Sep 15, 2014 at 10:23 AM, Alexey Klyukin <alexk(at)hintbits(dot)com> wrote:
> On Fri, Sep 12, 2014 at 4:20 PM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
>
>>> Hmm. If that's what the browsers do, I think we should also err on the
>>> side of caution here. Ignoring the CN is highly unlikely to cause anyone
>>> a problem; a CA worth its salt should not issue a certificate with a CN
>>> that's not also listed in the SAN section. But if you have such a
>>> certificate anyway for some reason, it shouldn't be too difficult to get
>>> a new certificate. Certificates expire every 1-3 years anyway, so there
>>> must be a procedure to renew them anyway.
>>
>>
>> Committed, with that change, ie. the CN is not checked if SANs are present.

Actually, I disagree with the way the patch ignores the CN. Currently,
it skips the
CN unconditionally if the SubjectAltName section is present. But what
RFC 6125 says
is:

"If a subjectAltName extension of type dNSName is present, that MUST
be used as the identity. Otherwise, the (most specific) Common Name
field in the Subject field of the certificate MUST be used."

This means that we have to check that at least one dNSName resource is
present before
rejecting to examine the CN. Attached is a one-liner (excluding
comments) that fixes this.

Regards,
Alexey

On 09/15/2014 01:44 PM, Alexey Klyukin wrote:
>>> Committed, with that change, ie. the CN is not checked if SANs are present.
>
> Actually, I disagree with the way the patch ignores the CN. Currently,
> it skips the
> CN unconditionally if the SubjectAltName section is present. But what
> RFC 6125 says
> is:
>
> "If a subjectAltName extension of type dNSName is present, that MUST
> be used as the identity. Otherwise, the (most specific) Common Name
> field in the Subject field of the certificate MUST be used."
>
> This means that we have to check that at least one dNSName resource is
> present before
> rejecting to examine the CN. Attached is a one-liner (excluding
> comments) that fixes this.

Ok, good catch. Fixed.

- Heikki