>> 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
v14-0001-Serverside-SNI-support-for-libpq.patch
Description: Binary data
v14-0002-Review-comments.patch
Description: Binary data
