Hello Geoff,

On Wed, Aug 21, 2019 at 05:33:18PM +0200, Geoff Simmons wrote:
> Hello to readers of the haproxy list,
> 
> This is my first-ever mail to the list,

Thus welcome to the list :-)

> to propose my first-ever
> contribution to the project, so I apologize in advance if anything here
> runs afoul of your customs. I hope I've come close enough to make it
> worth your while.

No problem, and thanks for the precision.

> What I'm proposing has been coded up here:
> 
> https://github.com/slimhazard/haproxy/commit/2093dc54f0d5f7d0fb30c918c2c7689b8764bec1
> 
> And of course I can send a git-formatted patch to the list (but my
> understanding is that an RFC discussion is preferred first).

If you want to discuss the concept first the patches are not needed
indeed, though when the code exists sometimes it helps understand your
choices. With this said, discussing away from the code sometimes helps
one think again about certain choices and that's not bad.

> The idea is probably best understood from a sample config:
> 
>     listen fe
>       # ...
>       server s0 0.0.0.0:0 ssl verify none forward-dst-sni
> 
> This is only relevant for the case of "forwarding" the client's
> destination address, by using 0.0.0.0 or * in the server config. When
> the new option 'forward-dst-sni' is enabled for such a server, then for
> clients who send an authority TLV in the PROXY header, the SNI for the
> backend is set to the value of the TLV.

OK.

> The use case is for clients who want to use haproxy's "address
> forwarding" feature, but also need to communicate with backends via
> haproxy that use SNI for "virtual hosing with SSL". With this feature
> they can do that by using PROXYv2 and TLV.

Just to know a bit more about your use case, thus your client speaks
in clear to haproxy by prepending a PPv2 header and lets haproxy serve
as a TLS "onloader" if I can call it like this, that's it ? If so, this
is pretty close to what is also being done in Varnish to allow it to
use haproxy to gateway to TLS servers :

    https://github.com/varnishcache/varnish-cache/pull/2957

> The option is a NOP for servers that don't have the 0.0.0.0/* config.
> When it's enabled for such a server, and when the authority TLV is sent,
> then the authority value overrides any SNI set in the client TLS
> handshake. The rationale is that if a client goes to the trouble of
> sending the TLV, then they want that to be used for the SNI (otherwise
> they can just let the SNI be passed on from the client TLS).

I understand your point, I have an objection but will explain below :-)

> I considered adding a test in reg-tests, but I don't see a way to set a
> PROXY TLV value using VTest and haproxy (happy to be corrected if I'm
> wrong). But we do have some varnishtest VTCs for it here, for a PR that
> my colleague and I are working on for Varnish:
> 
> https://github.com/slimhazard/varnish-cache/blob/proxy_via_authority/bin/varnishtest/tests/c00100.vtc
> 
> https://github.com/slimhazard/varnish-cache/blob/proxy_via_authority/bin/varnishtest/tests/c00101.vtc

I'm not aware of a current solution to use the PROXY proto in vtest either.

> As for the coding, I suspect that my newbie status with haproxy shows
> mostly in the way space is allocated for the authority string, 

It's not dramatic, as long as you're interested in learning to improve,
you will, like about all of us here.

> which the
> patch adds as a new field to struct connection. Since the feature is
> likely to be used only rarely, it doesn't seem sensible to foresee all
> of the space for a host name right in the struct.

Indeed. And in fact it was the main reason why it was not implemented
since the field was specified. But very recently (in 2.1-dev) we moved
a few fields so that addresses are dynamically allocated (for the same
reason which is that allocating 128 bytes for a sockaddr_storage to
store a destination address that nobody uses is a waste of RAM). And I'd
really like that you do the same with the SNI extracted from the PPv2
header, so that it's not wasted when not used.

> In the patch I have it
> as a pointer to struct buffer, which is NULL unless the authority TLV is
> read from the PROXY header.

Well, buffer :-)  Some people (namely those playing with WAFs) can use
huge buffers, 1 MB or more. So as you can see it's not suited, better
create a new pool for this.

> It seemed to make sense to use a memory pool for the allocation, but it
> also felt like overkill to add a new memory pool just for this feature.

No it's not overkill, pools are extremely cheap and they are fused when
they have very close sizes. I just had a quick look and saw that appctx
is 248 bytes si it will end up being fused with your pool, so by adding
a pool, your entries will cost exactly zero. So really, don't be shy on
this.

> So in the patch I'm just using the "trash pool".

Got it, but please, no :-)

> Aside from the fact that the name "trash pool" makes me think that it
> might not be meant to be used this way, if I understand correctly the
> default allocation size is 16KiB, and users are advised not to reduce
> it. That's far too large for a host name (usually max 255 bytes). So I
> suspect that this may not be best practice for haproxy. Maybe define a
> dedicated pool after all?

Yep :-)

> I'd be grateful for any feedback about the feature and the code (or
> anything else). And of course I can send a patch to the list if you're
> interested.

So I'm having a small disagreement on the way to configure this, but
there is a nice solution that should not change much of what you've
done. The issue for me is double :
  - passing the SNI from client side PPv2 to server side without ability
    to adjust it. You can be certain that in less than 2 years someone
    will say "too bad we can't pass it via a map or add a subdomain or
    strip a subdomain". And it makes sense, people generally install
    a proxy to manipulate the traffic, so they expect to manipulate
    anything ;

  - restricting this to servers having 0.0.0.0 as their address is
    another problem because it will create corner cases for situations
    that neither you nor me have thought about yet and people will get
    trapped by this.

Another issue is that we already have a directive to send an SNI to the
server, and nobody will know which one is used when the two are active.

But the solution is actually very simple. The current "sni" directive
takes a sample expression. You "just" have to write a sample fetch to
return the SNI extracted from the PP header. Just call it "fc_sni", we
already have a number of other "fc_*" sample fetch functions for stuff
extracted from the front connection. In your case you'll simply have
to add "sni fc_sni" on your server lines and they will magically work.
Other people will continue to use "sni ssl_fc_sni" to use it from the
front TLS field instead. I was wondering whether fc_sni() should do a
fallback to ssl_fc_sni() if the former is empty. I'm not that much
convinced as it creates an inter-dependency that is not necessarily
welcome. Those who want to use any of them can simply set a variable
from the ssl_fc_sni or fc_sni field depending on the case.

Also one thing I think we're missing is fc_has_pp() or something like
this to indicate that the front connection used the proxy protocol.
And in this case it can prove very useful.

Cheers,
Willy

Reply via email to