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