Hi William !
Sorry for the delay. We have rediscussed this issue this morning and
here is my answer on your patch.
It is definitely legitimate to want to be able to use reverse HTTP
without SSL on the server line. However, the way that haproxy currently
uses idle connection is that at least the SNI parameter alone must be
set to match the name parameter of the corresponding attach-srv rule. If
this is not the case, the connection will never be reused.
But in fact, when rereading haproxy code this morning, we found that it
is valid to have SNI parameter set even if SSL is not active, and this
will produce the desired effect. We are definitely okay with merging an
adjusted version of your patch for the moment, see my remarks below.
However, using SNI on a server line without SSL is something tricky.
Thus we plan to add a new keyword to replace it when SSL is not used to
have the same effect. When this will be done, you should update your
configuration to use it.
On Fri, Apr 12, 2024 at 02:29:30PM +0100, William Manley wrote:
> An attach-srv config line usually looks like this:
> tcp-request session attach-srv be/srv name ssl_c_s_dn(CN)
> The name is a key that is used when looking up connections in the
> connection pool. Without this patch you'd get an error if you passed
> anything other than "ssl_c_s_dn(CN)" as the name expression. Now you can
> pass arbitrary expressions and it will just warn you if you aren't
> producing a configuration that is RFC compliant.
> I'm doing this as I want to use `fc_pp_unique_id` as the name.
I'm not 100% okay with your description here. The current code condition
does not check that "ssl_c_s_dn(CN)" is used as expression, but rather
that if name is defined for an attach-srv rule, the targetted server
must have both SSL and SNI activated. I think this paragraph should be
reworded.
> ---
> src/tcp_act.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
> diff --git a/src/tcp_act.c b/src/tcp_act.c
> index a88fab4af..4d2a56c67 100644
> --- a/src/tcp_act.c
> +++ b/src/tcp_act.c
> @@ -522,8 +522,7 @@ static int tcp_check_attach_srv(struct act_rule *rule,
> struct proxy *px, char **
>
> if ((rule->arg.attach_srv.name && (!srv->use_ssl || !srv->sni_expr)) ||
> (!rule->arg.attach_srv.name && srv->use_ssl && srv->sni_expr)) {
> - memprintf(err, "attach-srv rule: connection will never be used;
> either specify name argument in conjunction with defined SSL SNI on targeted
> server or none of these");
> - return 0;
> + ha_warning("attach-srv rule: connection may never be used;
> usually name argument is defined SSL SNI on targeted server or none of
> these");
> }
>
> rule->arg.attach_srv.srv = srv;
> --
> 2.34.1
>
I think an alternative patch may be more desirable here. We could update
the condition with the following statement without removing the fatal
error :
> if ((rule->arg.attach_srv.name && !srv->sni_expr) ||
> (!rule->arg.attach_srv.name && srv->sni_expr)) {
This reflects the current mandatory usage of reverse-http : if name is
used on attach-srv, sni keyword must be specified on the server line.
Let me know your thoughts. If you're okay, can you adjust your patch
please ? If not, do not hesitate to tell me if there is something you
disagreeing with.
Thanks,
--
Amaury Denoyelle
