On Thu, Apr 25, 2024, at 2:07 PM, Amaury Denoyelle wrote:
> Sorry for the delay. We have rediscussed this issue this morning and
> here is my answer on your patch.
Sorry for the even larger delay in responding :). Thanks for looking at this.
> 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.
I'll fix this.
> > ---
> > 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.
I agree. Did you see my replacement patch "MINOR: config: rhttp: Don't require
SSL when attach-srv name parsing"
https://www.mail-archive.com/haproxy@formilux.org/msg44826.html . It
implements this general idea, but with a nice specific error message depending
on whether sni or name is missing.
That replacement patch still needs the commit message corrected as you remarked
above, so I'll send a v2 of that patch.
Thanks
Will
---
William Manley
Stb-tester.com
Stb-tester.com Ltd is a company registered in England and Wales.
Registered number: 08800454. Registered office: 13B The Vale,
London, W3 7SH, United Kingdom (This is not a remittance address.)
