Re: implement subject alternative names support for SSL connections - Mailing list pgsql-hackers

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