Re: Serverside SNI support in libpq - Mailing list pgsql-hackers

>> The attached version allows ssl_ca to be omitted from the pg_host config to
>> match the ssl_ca GUC.
>
> Aha! I think ssl_ca should be moved into the "Optional fields" section
> of `struct HostsLine` now.

Done, and removed the now unused default_host member from the struct as well
which I had missed in the previous version.

> We should probably check tokens->length to make sure that the user
> hasn't passed more than one token for each field, similar to how
> parse_hba_line() does it.

Done.

>>> Will anyone be mad at us for camping on the "no_sni" identifier? I
>>> know technically underscore isn't allowed in DNS hostnames, buuuut [1,
>>> 2]
>>
>> Maybe, but I think that regardless of what we do someone will be mad.  The
>> other option would be to use another single character like '?' or something.
>> Not sure that will improve readability though.
>
> Hm, I agree that's not readable. Especially since other famous server
> implementations use ? to match a single character in server alias
> names.
>
> Maybe we could enclose no_sni with something that's emphatically not
> DNS. Braces, brackets, etc.? If we had control over the lower level
> tokenizer, we could tell people to double-quote it to disambiguate,
> but I don't think we have access to that information at our level.

I've changed to /no_sni/ in the attached patch which should make it safer, but
it can easily be changed to braces or brackets or something else entirely.

>>> Should we support multiple hostname tokens in a single line, though,
>>> and just copy the settings that follow across all of them?
>>
>> I've been hesitant to add too much complexity, but perhaps just allowing a
>> comma separated list is a good middle ground to avoid going full regex?
>
> I think it could be a pretty good bump in usability. Wildcards seem
> ideal but the cost is much higher. Hopefully the cost of
> comma-separated hosts is just an extra inner loop in the parser, plus
> the extra tests?

I've added support for lists of hostnames along with tests and docs for the
same.  The limitation is that one cannot specify '*' or '/no_sni/' in a list,
it must be just hostnames.  I haven't added support for @hostnames.txt yet to
keep scope under control, but it can be added as well (in the future if this
patch is committed).

> I'm trying to put on my "what could we possibly regret" hat for these
> next ones. They may be uselessly speculative:

I really appreciate thinking about this!

> - If the goal is to eventually support wildcards, will the use of a
> bare catch-all asterisk conflict with your plans (if any)?

Possibly, I guess it depends on how we define a wildcard scheme.  One solution
could perhaps be to use an enclosed name like the non-SNI case, like /default/
or something similar.

> - What kind of normalization should we do? Currently, `example.com`
> will not match `example.COM` and it seems like that might be a problem
> for somebody.

The attached use case insensitive comparison.  RFC 952 makes it clear that
hostnames are case insensitive, and RFC 921/1035 does the same for DNS.

> - Do we need to consider IDNs and A-labels and U-labels? (Do we
> support the latter today, at all?)

There is nothing in the current patch which prevents supporting it in a future
update is there?

> A nice-to-have v2ish feature might be to warn if the host configured
> for a certificate cannot in fact match that certificate according to
> OpenSSL.

That would be quite nifty indeed.


I think the attached is pretty clear improvement over the previous version so
thanks for the review suggestions.  That being said, the test which was
reported to still fail upstream is failing here as well (it does the right
thing with the connection, but terminates the handshake in a different place).
In an attempt to fix that I moved to using the ClientHello callback which
OpenSSL document to be the right one (yet they use the servername callback
themselves), but it renders the same result.  I hope that your eagle eyes (or
someone elses) can figure out either what is wrong, or if this is a different
form of right.  The same failing test is added to 0001 to run it in a strictly
non-SNI config as well.

The attached also simplifies the tests you provided since there is no longer
any need to run the tests for different default values, as we no longer have
that mixed configfile handling it was intended to test.  The actual connection
tests remain though.

--
Daniel Gustafsson