Re: implement subject alternative names support for SSL connections

On Mon, Sep 8, 2014 at 8:04 PM, Heikki Linnakangas
<hlinnakangas@vmware.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