net/haproxy: add SNI support in mapfile-based backend selection #4017
Conversation
cluck
changed the title
|
I'm currently investigating why Also need to swap |
cluck
changed the title
|
I found that multiple cascaded use_backend don't work as documented; only the first map ever seems to have any effect. Other People have documented the issue and a fix/workaround: I am trying to synthesize matching ACLs, but I totally fail to see where I could properly inject such ACLs from the code. PS: The currently released Haproxy plugin is also affected. |
There was a problem hiding this comment.
I found that multiple cascaded use_backend don't work as documented; only the first map ever seems to have any effect.
This should be reported on the HAProxy issue tracker:
https://github.com/haproxy/haproxy/issues/
| @@ -999,6 +1036,7 @@ global | |||
| {% if helpers.exists('OPNsense.HAProxy.general.tuning.maxDHSize') %} | |||
| tune.ssl.default-dh-param {{OPNsense.HAProxy.general.tuning.maxDHSize}} | |||
| {% endif %} | |||
| tune.ssl.capture-buffer-size 512 | |||
There was a problem hiding this comment.
Could you please make this option configurable?
I'd suggest to add two new options for this:
Enable Capture Buffer (default: disabled)
Capture Buffer Size (default: 512)
These should be added to Services: HAProxy: Settings -> Global Parameters.
There was a problem hiding this comment.
OK, but SNI parsing depends on the capture buffer being enabled to work, or it fails silently.
I guess this dependency should be added to the help text in both places then.
Maybe a warning during template generation could also be added.
| {% do action_options.append('use_backend %[req.hdr(host),lower,map_dom(' ~ mapfile_path ~ defaultbackend_option ~ ')]') %} | ||
| {% set backend_file_hostname_snisupport = action_data.map_use_backend_file_snisupport|default("hostonly") %} | ||
| {% if backend_file_hostname_snisupport in ("sniandhost", "snionly") %} | ||
| {% set backend_file_global_options = ['# NOTE: Map files SNI support', 'tcp-request inspect-delay 200ms', 'tcp-request content accept if { req.ssl_hello_type 1 }', ''] %} |
There was a problem hiding this comment.
This tcp-request inspect-delay would be nice to be either configurable or at least a lot higher than 200ms.
There was a problem hiding this comment.
Do you have an usecase where this is required? I was led by the impression that SNI comes with the very first packet, thus any delay would indicate a non-SNI connection anyway.
PS: the 500 bytes capture buffer chosen above is below the minimum MTU required by IP/IPv6 for the same reason.
There was a problem hiding this comment.
Using hardcoded values is always something we should try to avoid. It may work for one use-case, but fail in other scenarios.
However, I don't think this option should silently be added here. Instead it should be documented that this feature relies on it and that the user needs to add it (because it's already available in os-haproxy with a customizable inspection delay).
This is now reported at haproxy/haproxy#2698 |
|
@cluck I'd like to merge this. Would you please have a look at the two review comments? Thanks! |
|
Meanwhile haproxy/haproxy#2698 was decided to be a wontfix, thus, pairs of multiple use_backend commands really need to be guarded by matching ACLs. I was looking for advice on where to synthesize the required ACLs in the template. I really can't find my way around there. |
Could you provide a example haproxy.conf to demonstrate how this should be "guarded by ACLs"? Then I may be able to offer advice regarding it's implementation in os-haproxy. 😊 |
This adds two options to the "Map domains to backend pools using a map file".
One of the options is addressing #3641.
The first option adds all map_dom, map_str, map_beg, map_end and map_reg (regex) support.
The second option adds SNI support.
Defaults are chosen such that existing setups don't change behavior.
I am currently testing this in our lab, but the more testing this gets the better.