Re: [PATCH] MINOR: config: rhttp: Downgrade error on attach-srv name parsing
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.)
Re: [PATCH] MINOR: config: rhttp: Downgrade error on attach-srv name parsing
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
Re: [PATCH] MINOR: config: rhttp: Downgrade error on attach-srv name parsing
On Fri, Apr 12, 2024, at 4:01 PM, Amaury Denoyelle wrote: > I have a doubt though, will this kind of configuration really works ? I > though that for the moment if name parameter is specified, it is > mandatory to use a server with SSL+SNI. It may be mandatory according to the RFC, but I'm not using it that way. Usually it's RHTTP over SSL, and the incoming connection identifies itself securely using the SSL DN. The way I'm using it is RHTTP over HTTP CONNECT - and I'm validating the connection using the headers that came with the HTTP CONNECT. I have tcp server block that strips the HTTP CONNECT header and adds PROXY header instead with the connection pool name sent through using unique-id: listen connect_terminate mode tcp bind ... tcp-request inspect-delay 5s tcp-request content lua.terminate_http_connect # This allows us to send the hostname over the PROXY protocol: unique-id-format "%[var(txn.req_header.x_hostname)]" server srv 127.0.0.1:8001 send-proxy-v2 proxy-v2-options unique-id Then I use that unique id when adding the connection to the connection pool: frontend add_to_http_pool mode http bind 127.0.0.1:8001 proto h2 accept-proxy tcp-request session attach-srv rhttp_frontend/srv name fc_pp_unique_id It's a little roundabout (and this is the simplified version) but quite capable. I plan to use a similar technique to route multiple requests to different hostnames down the same RHTTP connection too. In that case I'll not be using sni req.hdr(host) either - but I haven't got that far yet. 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.)
Re: [PATCH] MINOR: config: rhttp: Downgrade error on attach-srv name parsing
On Fri, Apr 12, 2024 at 05:01:07PM +0200, Amaury Denoyelle wrote:
> On Fri, Apr 12, 2024 at 03:37:56PM +0200, Willy Tarreau wrote:
> > Hi!
> > 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.
> > > ---
> > > 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");
> > Well, I consider that any valid (and useful) configuration must be
> > writable without a warning. So if you have a valid use case with a
> > different expression, here you still have no way to express it without
> > the warning. In this case I'd suggest to use ha_diag_warning() instead,
> > it will only report this when starting with -dD (config diagnostic mode).
>
> I have a doubt though, will this kind of configuration really works ? I
> though that for the moment if name parameter is specified, it is
> mandatory to use a server with SSL+SNI.
If we receive the traffic with SSL already stripped by a front haproxy
and all the info presented in the proxy protocol, I think it should still
work. I must confess that it's blowing my mind a little bit to imagine
all these layers, but I tend to think that could be valid.
Willy
Re: [PATCH] MINOR: config: rhttp: Downgrade error on attach-srv name parsing
On Fri, Apr 12, 2024, at 2:37 PM, Willy Tarreau wrote: > Well, I consider that any valid (and useful) configuration must be > writable without a warning. So if you have a valid use case with a > different expression, here you still have no way to express it without > the warning. In this case I'd suggest to use ha_diag_warning() instead, > it will only report this when starting with -dD (config diagnostic mode). Thanks for taking a look. I've taken a slightly different approach now that I understand the warning better. Instead I've removed the requirement for SSL, but I keep the requirement that either name and sni are specified or neither are. This way it won't warn or error with my configuration, nor with the usual one, but it will still error if there is a mismatch between name and sni. 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.)
Re: [PATCH] MINOR: config: rhttp: Downgrade error on attach-srv name parsing
On Fri, Apr 12, 2024 at 03:37:56PM +0200, Willy Tarreau wrote:
> Hi!
> 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.
> > ---
> > 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");
> Well, I consider that any valid (and useful) configuration must be
> writable without a warning. So if you have a valid use case with a
> different expression, here you still have no way to express it without
> the warning. In this case I'd suggest to use ha_diag_warning() instead,
> it will only report this when starting with -dD (config diagnostic mode).
I have a doubt though, will this kind of configuration really works ? I
though that for the moment if name parameter is specified, it is
mandatory to use a server with SSL+SNI.
--
Amaury Denoyelle
Re: [PATCH] MINOR: config: rhttp: Downgrade error on attach-srv name parsing
Hi!
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.
> ---
> 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");
Well, I consider that any valid (and useful) configuration must be
writable without a warning. So if you have a valid use case with a
different expression, here you still have no way to express it without
the warning. In this case I'd suggest to use ha_diag_warning() instead,
it will only report this when starting with -dD (config diagnostic mode).
Willy
[PATCH] MINOR: config: rhttp: Downgrade error on attach-srv name parsing
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.
---
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
