implement subject alternative names support for SSL connections

On Fri, Jul 25, 2014 at 6:10 PM, Alexey Klyukin <alexk@hintbits.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/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jul 25, 2014 at 6:34 PM, Magnus Hagander <magnus@hagander.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@hintbits.com> wrote:

On Fri, Jul 25, 2014 at 6:34 PM, Magnus Hagander <magnus@hagander.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/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Magnus Hagander <magnus@hagander.net> writes:

On Fri, Jul 25, 2014 at 7:15 PM, Alexey Klyukin <alexk@hintbits.com> wrote:

On Fri, Jul 25, 2014 at 6:34 PM, Magnus Hagander <magnus@hagander.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas <
hlinnakangas@vmware.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@vmware.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Aug 25, 2014 at 12:02 PM, Heikki Linnakangas <
hlinnakangas@vmware.com> wrote:

On 08/24/2014 03:11 PM, Alexey Klyukin wrote:

On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas <
hlinnakangas@vmware.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@vmware.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@vmware.com> wrote:

On 08/24/2014 03:11 PM, Alexey Klyukin wrote:

On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas <
hlinnakangas@vmware.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Aug 29, 2014 at 11:22 AM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:

On 08/28/2014 07:28 PM, Alexey Klyukin wrote:

On Mon, Aug 25, 2014 at 12:02 PM, Heikki Linnakangas <
hlinnakangas@vmware.com> wrote:

On 08/24/2014 03:11 PM, Alexey Klyukin wrote:

On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas <
hlinnakangas@vmware.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Sep 1, 2014 at 10:39 AM, Alexey Klyukin <alexk@hintbits.com> wrote:

On Fri, Aug 29, 2014 at 11:22 AM, Heikki Linnakangas
<hlinnakangas@vmware.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@hintbits.com> wrote:

On Fri, Aug 29, 2014 at 11:22 AM, Heikki Linnakangas
<hlinnakangas@vmware.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Sep 3, 2014 at 11:50 AM, Heikki Linnakangas
<hlinnakangas@vmware.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 09/04/2014 10:33 AM, Alexey Klyukin wrote:

On Wed, Sep 3, 2014 at 11:50 AM, Heikki Linnakangas
<hlinnakangas@vmware.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Sep 4, 2014 at 10:23 AM, Heikki Linnakangas
<hlinnakangas@vmware.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@vmware.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