Re: [RFC] setting the backend SNI from the client's authority TLV, when the target address was forwarded
> Le 22 août 2019 à 14:40, Willy Tarreau a écrit : > > On Thu, Aug 22, 2019 at 11:36:00AM +0200, Geoff Simmons wrote: > >> I suspect that there are other ways that the authority TLV can be useful >> for haproxy besides the specific Varnish case. Someone connecting via >> TLS, for example, might want to send the TLV to "override" the SNI for >> certain backends. > > It's definitely useful for other cases, which is why I'm interested in > seeing it addressed properly. Right now we do emit a few fields in the > PPv2 but we ignore them on receipt, which is a bit sad. For example, if > you use multi-process to chain two haproxy instances, one with TLS > offloading, passing to the next level using PPv2, you currently lose the > SNI information. With your change it would finally work. > Indeed, the receipt part of PPv2 has been postponed until needed. With authority received from PPv2 (Geoff patch), i see use cases (one is to test configuration like chaining tow haproxy :-) ) To generalize the usage of authority, i will see a get_authority func in xprt_ops. With a ssl connection: return the sni (aka ssl authority). With a tcp connection: return the ppv2 authority. So a « send-proxy-v2 proxy-v2-options authority » server line will work in both cases. To override the authority, is like « sni » for server line, we could used « authority ». « send-proxy-v2 authority hdr(Host) proxy-v2-options authority » For server line with ssl, i thought that « authority » could replace « sni » but we can have both: « ssl sni hdr(Host),lower send-proxy-v2 authority hdr(Host) proxy-v2-options authority » Note: in my test « sni hdr(Host),lower » work but « sni var(req.host) » doesn’t (with http-request content set-var(req.host) hdr(host),lower ) What do you think? Manu
Re: [RFC] setting the backend SNI from the client's authority TLV, when the target address was forwarded
On 8/26/19 18:03, Emmanuel Hocdet wrote: > > Great to see TLS onloader continue. Working on it ... > About the TLS onloader configuration. If i understand the principle of > servers set to 0.0.0.0 and stick table: > The server configuration will look like: >server s0 0.0.0.0:0 ssl sni fc_pp_authority >[…] Yes, I'm currently testing a new patch, and the config looks very much like that. Real-world use cases may want to implement the fallback logic that we were talking about earlier in the thread, since fc_pp_authority may or may not have been present in the PROXY header. "Set SNI to fc_pp_authority if it was sent, otherwise set it to ssl_fc_sni". Best, Geoff -- ** * * UPLEX - Nils Goroll Systemoptimierung Scheffelstraße 32 22301 Hamburg Tel +49 40 2880 5731 Mob +49 176 636 90917 Fax +49 40 42949753 http://uplex.de signature.asc Description: OpenPGP digital signature
Re: [RFC] setting the backend SNI from the client's authority TLV, when the target address was forwarded
HI Geoff, Willy Great to see TLS onloader continue. > Le 22 août 2019 à 16:33, Geoff Simmons a écrit : > > On 8/22/19 14:40, Willy Tarreau wrote: >> >>> I would suggest naming it something like fc_authority or >>> fc_pp_authority, to be specific about where it came from. > > Since you used fc_pp_authority in an example further down, I'll take > that as the choice (unless somebody yells). Seems better to me, since > just "authority" could refer to a number of things. > fc_pp_authority seems ok. (fc_)authority could refer to ssl_fc_sni for ssl connection or host header for http connection. About the TLS onloader configuration. If i understand the principle of servers set to 0.0.0.0 and stick table: The server configuration will look like: server s0 0.0.0.0:0 ssl sni fc_pp_authority […] For stick part, to correctly reused TLS connection, destination IP + authority should be used. Regards Manu
Re: [RFC] setting the backend SNI from the client's authority TLV, when the target address was forwarded
On Thu, Aug 22, 2019 at 04:33:13PM +0200, Geoff Simmons wrote: > On 8/22/19 14:40, Willy Tarreau wrote: > > > >> I would suggest naming it something like fc_authority or > >> fc_pp_authority, to be specific about where it came from. > > Since you used fc_pp_authority in an example further down, I'll take > that as the choice (unless somebody yells). Seems better to me, since > just "authority" could refer to a number of things. OK. > All right, I think we've covered enough that I can take another go at > coding it up. And I believe I can mail a patch next time, if there are > no objections. Since the patch will be adding a fetch, I'd say the > regression risk is MINOR, as no one could have ever used it before. If it's really *that* minor (we'll see in the end), we could even think about backporting it to 2.0. We do occasionally backport a few sample fetches and converters when they are totally harmless. And if your work on Varnish is about to be completed, you could benefit from the full support from a stable version. > Willy, thanks again for the feedback, the first impression about working > with haproxy development has been very pleasant. Thanks. Willy
Re: [RFC] setting the backend SNI from the client's authority TLV, when the target address was forwarded
On 8/22/19 14:40, Willy Tarreau wrote: > >> I would suggest naming it something like fc_authority or >> fc_pp_authority, to be specific about where it came from. Since you used fc_pp_authority in an example further down, I'll take that as the choice (unless somebody yells). Seems better to me, since just "authority" could refer to a number of things. > You don't need to have the extra boolean because our sample fetch functions > already return this information for you. The "-m found" match method > indicates whether or not the requested information was present, regardless > of its possible emptiness. > >> I assume the fetch for the authority TLV should return the empty string >> if none was read; correct? > > No it returns a "not found" (return 0 technically speaking). Ah, OK thx. >> - Use a dedicated memory pool to allocate the name from TLV, if there is >> one in the proxy header. > > Yep. Please name it "authority" then. At least if we need an authority > anywhere else we know we can share it. OK All right, I think we've covered enough that I can take another go at coding it up. And I believe I can mail a patch next time, if there are no objections. Since the patch will be adding a fetch, I'd say the regression risk is MINOR, as no one could have ever used it before. Willy, thanks again for the feedback, the first impression about working with haproxy development has been very pleasant. Best, Geoff -- ** * * UPLEX - Nils Goroll Systemoptimierung Scheffelstraße 32 22301 Hamburg Tel +49 40 2880 5731 Mob +49 176 636 90917 Fax +49 40 42949753 http://uplex.de signature.asc Description: OpenPGP digital signature
Re: [RFC] setting the backend SNI from the client's authority TLV, when the target address was forwarded
On Thu, Aug 22, 2019 at 11:36:00AM +0200, Geoff Simmons wrote: > Spot on, that's the PR that I'm working on with my colleague Nils. Mine > is a PR to his PR, if that makes any sense; after review (assuming he > approves), the part about setting the authority TLV will go into the > Varnish PR. OK, thanks for the context. > I suspect that there are other ways that the authority TLV can be useful > for haproxy besides the specific Varnish case. Someone connecting via > TLS, for example, might want to send the TLV to "override" the SNI for > certain backends. It's definitely useful for other cases, which is why I'm interested in seeing it addressed properly. Right now we do emit a few fields in the PPv2 but we ignore them on receipt, which is a bit sad. For example, if you use multi-process to chain two haproxy instances, one with TLS offloading, passing to the next level using PPv2, you currently lose the SNI information. With your change it would finally work. > I thought there could be an objection to special-casing just the one > class of TLV, since there's a variety of them. A general solution might > be to have the field point to a table of TLVs read from the connection, > if there were any. That would complicate the allocation, though. > > My instinct is to not generalize from a single use case, especially if > it introduces new complexities before it's clear whether anyone wants to > use them. If haproxy users find that they like accessing the authority > TLV and want more, that can be accommodated when the time comes. I totally agree with this, let's just create a pool for this one at the moment, and who knows what we'll see next. > I would suggest naming it something like fc_authority or > fc_pp_authority, to be specific about where it came from. If it's > available as a fetch, then SNI doesn't have to be its only purpose. Hmmm you're totally right. I remind a conversation, I think it was with Amos Jeffries of Squid, who was insisting on the fact that we name it authority and not SNI because authority is the functional description and is always valid regardless of the underlying protocol, while SNI is only the technical way to convery the authority over TLS. Thus I agree with this, it's just that I didn't remember the field's name in the PP :-) > Conceivably someone could use it to set a Host header, define an ACL, or > do other things we haven't considered yet. Absolutely. > And I think there would have to be a boolean like fc_rcvd_authority, to > find out if the TLV had been sent at all. That way, the fallback logic > can be made explicit in the config: set SNI from fc_authority if > present, otherwise set it from fc_sni. You don't need to have the extra boolean because our sample fetch functions already return this information for you. The "-m found" match method indicates whether or not the requested information was present, regardless of its possible emptiness. > I assume the fetch for the authority TLV should return the empty string > if none was read; correct? No it returns a "not found" (return 0 technically speaking). If used to set a textual value somewhere, this will be turned to an empty string, but as much as possible it will indicate "no value" and will lead to some operations not being performed. Typically if you use "sni fc_pp_authority" and this authority is not there, then no SNI will be sent. > > 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. > > Isn't that what fc_rcvd_proxy says? I didn't remember about it and I responded without reading the doc :-) > So to summarize my takeaways thus far: > > - Use a dedicated memory pool to allocate the name from TLV, if there is > one in the proxy header. Yep. Please name it "authority" then. At least if we need an authority anywhere else we know we can share it. > - Introduce a fetch to use it in configuration. Yes. > - I suggest naming the fetch to identify it specifically as the > authority TLV from PP, Yes. > and also introducing a boolean fetch that says > whether any such was read. Not needed as indicated above. > Thanks again, this has been an encouraging start. %^) You're welcome, thanks for your pretty clear approach. Willy
Re: [RFC] setting the backend SNI from the client's authority TLV, when the target address was forwarded
On 8/21/19 21:50, Willy Tarreau wrote: > > Thus welcome to the list :-) Thanks for the informative and welcoming response. %^) > 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 Spot on, that's the PR that I'm working on with my colleague Nils. Mine is a PR to his PR, if that makes any sense; after review (assuming he approves), the part about setting the authority TLV will go into the Varnish PR. I suspect that there are other ways that the authority TLV can be useful for haproxy besides the specific Varnish case. Someone connecting via TLS, for example, might want to send the TLV to "override" the SNI for certain backends. >> 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. > >> Maybe define a >> dedicated pool after all? > > Yep :-) All right then, a new memory pool will be introduced. I thought there could be an objection to special-casing just the one class of TLV, since there's a variety of them. A general solution might be to have the field point to a table of TLVs read from the connection, if there were any. That would complicate the allocation, though. My instinct is to not generalize from a single use case, especially if it introduces new complexities before it's clear whether anyone wants to use them. If haproxy users find that they like accessing the authority TLV and want more, that can be accommodated when the time comes. > 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. [...] > > 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. I agree, the fetch looks like the better solution. I would suggest naming it something like fc_authority or fc_pp_authority, to be specific about where it came from. If it's available as a fetch, then SNI doesn't have to be its only purpose. Conceivably someone could use it to set a Host header, define an ACL, or do other things we haven't considered yet. And I think there would have to be a boolean like fc_rcvd_authority, to find out if the TLV had been sent at all. That way, the fallback logic can be made explicit in the config: set SNI from fc_authority if present, otherwise set it from fc_sni. I assume the fetch for the authority TLV should return the empty string if none was read; correct? > 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. Isn't that what fc_rcvd_proxy says? So to summarize my takeaways thus far: - Use a dedicated memory pool to allocate the name from TLV, if there is one in the proxy header. - Introduce a fetch to use it in configuration. - I suggest naming the fetch to identify it specifically as the authority TLV from PP, and also introducing a boolean fetch that says whether any such was read. Thanks again, this has been an encouraging start. %^) Best, Geoff -- ** * * UPLEX - Nils Goroll Systemoptimierung Scheffelstraße 32 22301 Hamburg Tel +49 40 2880 5731 Mob +49 176 636 90917 Fax +49 40 42949753 http://uplex.de signature.asc Description: OpenPGP digital signature
Re: [RFC] setting the backend SNI from the client's authority TLV, when the target address was forwarded
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
[RFC] setting the backend SNI from the client's authority TLV, when the target address was forwarded
Hello to readers of the haproxy list, This is my first-ever mail 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. 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). 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. 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. 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 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 As for the coding, I suspect that my newbie status with haproxy shows mostly in the way space is allocated for the authority string, 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. 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. 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. So in the patch I'm just using the "trash pool". 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? 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. Thanks, Geoff Simmons -- ** * * UPLEX - Nils Goroll Systemoptimierung Scheffelstraße 32 22301 Hamburg Tel +49 40 2880 5731 Mob +49 176 636 90917 Fax +49 40 42949753 http://uplex.de signature.asc Description: OpenPGP digital signature
