Basic PROXY protocol support for Squid-to-peer connections #281

eduard-bagdasaryan · 2025-01-16T15:02:20Z

No description provided.

src/FwdState.cc

rousskov · 2025-01-16T16:39:59Z

src/proxyp/Header.cc

 ProxyProtocol::Header::Header(const SBuf &ver, const Two::Command cmd):
    version_(ver),
    command_(cmd),
    ignoreAddresses_(false)
 {}

+void
+ProxyProtocol::Header::packInto(MemBuf &mb) const


If this code survives, it should become:

Suggested change

ProxyProtocol::Header::packInto(MemBuf &mb) const

ProxyProtocol::Header::packInto(Packable &out) const

However, for now, please continue to use MemBuf (and do not focus on the quality of this packing code -- as long as it produces the right serialization/bytes-on-the-wire). I am investigating a possible replacement/upgrade, but that will take some time, and there is no good reason to block your progress on that.

src/proxyp/Header.cc

src/proxyp/Header.h

This implementation is still untested. I also plan to remove the failed attempt at (hopefully premature) optimization to simplify further. This commit should fix PROXY protocol "tail" length packing. Also added code to pack TLVs. TLVs are currently absent, but ProxyProtocol::Header::pack() should not know that.

If we ever implement locking (removed in previous commit), this position will also help to "automatically" check the lock before any object updates. Also fixed method const-correctness.

rousskov

Please also fix "make check" when you get a chance.

src/proxyp/Header.cc

src/FwdState.cc

rousskov · 2025-01-16T23:26:04Z

src/FwdState.cc

+
+        BinaryPacker packer;
+        header.pack(packer);
+        const auto packed = packer.packed();


When you update this code to generate the PROXY protocol header before opening a connection, please store it as SBuf. Convert to MemBuf only when it is time to write it (using the freshly opened connection).

Also do not reset this field too early.

Also disclosed lack of support for multiple directives. We may need to relax that requirement later.

rousskov · 2025-01-30T14:57:01Z

src/format/Format.cc

+Format::Format::hasPercentCode() const
+{
+    for (auto t = format; t; t = t->next) {
+        if (t->type != LFT_STRING)


We also need to handle LFT_PERCENT specially, right? And LFT_NONE AFAICT (e.g., for a logformat that is an empty string).

Suggested change

if (t->type != LFT_STRING)

if (t->type != LFT_NONE && t->type != LFT_STRING && t->type != LFT_PERCENT) // other codes require ALE

... but the above is also inaccurate because %byte does not require ALE. We need to decide whether this method is really "this logformat has a logformat %code" or "this logformat requires ALE" to finalize this.

There are several other codes not requiring ALE, such as LFT_TIME_SECONDS_SINCE_EPOCH, LFT_TIME_SUBSECOND, LFT_SEQUENCE_NUMBER. We can check them all in a similar method (e.g., requiresALE()) but how are we going to maintain this condition, i.e., if some of these codes start using ALE or others stop? At least we probably need adding some Format::Token::requiresAle flag and assert in Format::assemble() if the flag does not match the passed ALE pointer.

There are several other codes not requiring ALE, such as LFT_TIME_SECONDS_SINCE_EPOCH, LFT_TIME_SUBSECOND, LFT_SEQUENCE_NUMBER. We can check them all in a similar method (e.g., requiresALE())

Let's move from hasPercentCode() to isConstant():

Do not include LFT_TIME_SECONDS_SINCE_EPOCH and LFT_TIME_SUBSECOND. They will change with time and, depending on formatting, those changes may affect the validity of port numbers or even IP addresses produced using those %codes.

Do not include LFT_SEQUENCE_NUMBER. It will change from one transaction to another, and, depending on formatting, those changes may affect the validity of port numbers or even IP addresses produced using that %code.

but how are we going to maintain this condition, i.e., if some of these codes start using ALE or others stop?

It is the right question to ask, but let's not solve that problem in this PR. A proper solution requires significant out-of-scope Format refactoring, and I am not even sure it is worth doing because, after all, we are talking about a feature that may be called an "optimization" -- we want to detect misconfigurations earlier.

Done (e045f61).

rousskov · 2025-01-30T15:02:20Z

src/proxyp/OutgoingHttpConfig.cc

+void
+ProxyProtocol::OutgoingHttpConfig::fill(ProxyProtocol::Header &header, const AccessLogEntryPointer &al)
+{
+    fillAddresses(header.sourceAddress, header.destinationAddress, al);


If we are guaranteed to have ALE here, we should at least assert that (but it may be better to change the parameter type from a pointer to a reference). Otherwise, we should not call assemble() (down the stack) without ALE because that method (at least) assumes that ALE is available AFAICT.

rousskov · 2025-01-30T16:16:51Z

src/proxyp/OutgoingHttpConfig.h

+{
+public:
+    Option(const char *aName, const char *aVal, bool quoted);
+    virtual ~Option() { delete valueFormat; }


Please try to avoid including format/Format.h into this header by moving destructor definition to .cc file (and including format/forward.h instead).

rousskov · 2025-01-30T16:19:19Z

src/proxyp/OutgoingHttpConfig.cc

+{
+    char *key = nullptr;
+    char *value = nullptr;
+    if(!ConfigParser::NextKvPair(key, value))


Please use optionalKvPair() if possible.

rousskov · 2025-01-30T16:21:40Z

src/proxyp/OutgoingHttpConfig.cc

+ProxyProtocol::OutgoingHttpConfig::parseOptions(ConfigParser &parser)
+{
+    // required options
+    srcAddr = new AddrOption("src_addr", requiredValue("src_addr"), ConfigParser::LastTokenWasQuoted());


If possible, please use parser.Foo() instead of ConfigParser::Foo(). We want to get rid of LegacyParser global (eventually), but I do not recall whether parser.Foo() produces warnings for static Foo() methods. If it does trigger warnings, then "if possible" precondition cannot be satisfied, of course.

rousskov · 2025-01-30T16:44:15Z

src/proxyp/OutgoingHttpConfig.cc

+    }
+}
+
+static unsigned short


Suggested change

static unsigned short

static uint16_t

rousskov · 2025-01-30T16:47:59Z

src/proxyp/OutgoingHttpConfig.cc

+    Parser::Tokenizer tok(val);
+    int64_t p = -1;
+    if (!tok.int64(p, 10, false) || (p > std::numeric_limits<uint16_t>::max()))
+        throw TextException(ToSBuf("Could not parse '", val, "' as an IP port"), Here());


IP (i.e. the protocol) has no ports.

Suggested change

throw TextException(ToSBuf("Could not parse '", val, "' as an IP port"), Here());

throw TextException(ToSBuf("Could not parse '", val, "' as an address port"), Here());

... but we may be able to improve this wording by making this function a class method, to gain access to the option "name".

rousskov · 2025-01-30T16:49:06Z

src/proxyp/OutgoingHttpConfig.cc

+{
+    Parser::Tokenizer tok(val);
+    int64_t p = -1;
+    if (!tok.int64(p, 10, false) || (p > std::numeric_limits<uint16_t>::max()))


To get better errors and improve code quality, please use udec64() instead of int64(), making p constant.

I could not use it because udec64() throws InsufficientInput() in our case.

rousskov · 2025-01-30T16:54:03Z

src/proxyp/OutgoingHttpConfig.h

+    Port port(const AccessLogEntryPointer &al) const;
+
+protected:
+    Port port_; ///< parsed address (for options without logformat %macros)


Suggested change

Port port_; ///< parsed address (for options without logformat %macros)

Port port_; ///< transaction-independent source or destination address port

rousskov · 2025-01-30T17:00:56Z

src/proxyp/OutgoingHttpConfig.cc

+
+ProxyProtocol::AddrOption::AddrOption(const char *aName, const char *aVal, bool quoted) : Option(aName, aVal, quoted)
+{
+    if (!valueFormat || !valueFormat->hasPercentCode())


Please see if we can refactor so that we do not store valueFormat when it is used only once (here). Same for storing theValue when valueFormat has %codes, actually. In other words, try to avoid storing anything we are not going to use after construction.

We need valueFormat after addressing another request, that removed the raw theValue field.

We need valueFormat

I changed this after addressing the new requirement to treat "-" as as an 'undefined' value. In this special case valueFormat is not created.

rousskov · 2025-01-30T18:07:11Z

src/proxyp/OutgoingHttpConfig.h

+    void dump(std::ostream &);
+
+    SBuf theName;  ///< Configured option name
+    SBuf theValue; ///< Configured option value, possibly with %macros.


After thinking about complexities in current PR code a bit more, I realized that we should make one significant simplification: There should be only one Option::value data member. It's type should be [a pointer to] Format. The specs should be adjusted to always interpret configured option values as logformat strings, regardless of value quoting (but surrounding quotes, if any, should be automatically removed) and regardless of whether the value contains %codes.

If an admin configures a constant value (i.e. a value without %codes), it is up to Format class to decide whether to optimize handling of that constant logformat value. This PR code has nothing to do with that optional optimization; it is Format business.

Format::dump() can be used to obtain raw/uncompiled logformat value where needed (e.g., for dumping new directive configuration itself).

I believe the above ideas supersede any conflicting suggestions in the previous review.

This PR code should still reject configurations where

"constant" address values are malformed or have mismatching families

"constant" port values are malformed

two TLVs are identical

Format::dump() can be used to obtain raw/uncompiled logformat value where needed

Format::Format::dump(StoreEntry * entry,...) is incompatible with our Configuration::Component<ProxyProtocol::OutgoingHttpConfig*>::Print(std::ostream &os, ...). Do you suggest refactoring Format::Format::dump() here to provide this compatibility?

No, I do not. I would probably start by adding Token::print(ostream) (to dump the format value; all tokens in the invasive list) and use that to implement the inner loop inside existing Format::dump(). PROXY protocol call can then print Format::format member using Token::print().

Done (0f26c31).

rousskov · 2025-01-30T18:15:07Z

src/cf.data.pre

+	configuration errors.
+
+	Quoted (using "double quotes") header field values are supported. Squid
+	removes quotes before sending the field value. Quoted header field values


Suggested change

removes quotes before sending the field value. Quoted header field values

removes quotes before using the field value.

A header field value, whether quoted or not,

eduard-bagdasaryan

Partially done (50096e6).

eduard-bagdasaryan · 2025-02-03T15:00:24Z

src/cf.data.pre

+	configuration errors.
+
+	Quoted (using "double quotes") header field values are supported. Squid
+	removes quotes before sending the field value. Quoted header field values


eduard-bagdasaryan · 2025-02-03T15:00:43Z

src/proxyp/OutgoingHttpConfig.cc

+void
+ProxyProtocol::Option::dump(std::ostream &os)
+{
+    os << theName << '=' << ConfigParser::QuoteString(SBufToString(theValue));


eduard-bagdasaryan · 2025-02-03T15:00:48Z

src/proxyp/OutgoingHttpConfig.cc

+    valueFormat = new Format::Format(theName.c_str());
+    if (!valueFormat->parse(theValue.c_str())) {
+        delete valueFormat;


eduard-bagdasaryan · 2025-02-03T15:01:04Z

src/proxyp/OutgoingHttpConfig.cc

+    }
+}
+
+static unsigned short


eduard-bagdasaryan · 2025-02-03T15:01:44Z

src/proxyp/OutgoingHttpConfig.cc

+{
+    Parser::Tokenizer tok(val);
+    int64_t p = -1;
+    if (!tok.int64(p, 10, false) || (p > std::numeric_limits<uint16_t>::max()))


I could not use it because udec64() throws InsufficientInput() in our case.

eduard-bagdasaryan · 2025-02-03T15:02:08Z

src/proxyp/OutgoingHttpConfig.cc

+    srcAddr->dump(os);
+    os << separator;
+    dstAddr->dump(os);
+    os << separator;
+    srcPort->dump(os);
+    os << separator;
+    dstPort->dump(os);
+    for (const auto &t : tlvOptions) {
+        os << separator;
+        t->dump(os);
+    }


eduard-bagdasaryan · 2025-02-03T15:02:16Z

src/proxyp/OutgoingHttpConfig.cc

+    auto p = srcPort->port(al);
+    src.port(p ? *p : 0);


eduard-bagdasaryan · 2025-02-03T15:02:23Z

src/proxyp/OutgoingHttpConfig.cc

+ProxyProtocol::OutgoingHttpConfig::parseOptions(ConfigParser &parser)
+{
+    // required options
+    srcAddr = new AddrOption("src_addr", requiredValue("src_addr"), ConfigParser::LastTokenWasQuoted());


eduard-bagdasaryan · 2025-02-03T15:02:30Z

src/proxyp/OutgoingHttpConfig.h

+{
+public:
+    Option(const char *aName, const char *aVal, bool quoted);
+    virtual ~Option() { delete valueFormat; }


eduard-bagdasaryan · 2025-02-03T15:02:36Z

src/proxyp/OutgoingHttpConfig.h

+    Port port(const AccessLogEntryPointer &al) const;
+
+protected:
+    Port port_; ///< parsed address (for options without logformat %macros)


This complication is required to let admin configure Squid to withheld address or port information without generating cache.log warnings. Other minor polishing touches.

and adjusted Format::Format::dump() accordingly. No functionality changes.

Use Option::valueFormat to get both raw and formatted option values (the raw value is obtained via Token::print(ostream)).

eduard-bagdasaryan

Done (up to e045f61).

eduard-bagdasaryan · 2025-02-05T13:05:28Z

src/proxyp/OutgoingHttpConfig.h

+    void dump(std::ostream &);
+
+    SBuf theName;  ///< Configured option name
+    SBuf theValue; ///< Configured option value, possibly with %macros.


Done (0f26c31).

eduard-bagdasaryan · 2025-02-05T13:06:26Z

src/proxyp/OutgoingHttpConfig.cc

+
+ProxyProtocol::AddrOption::AddrOption(const char *aName, const char *aVal, bool quoted) : Option(aName, aVal, quoted)
+{
+    if (!valueFormat || !valueFormat->hasPercentCode())


We need valueFormat after addressing another request, that removed the raw theValue field.

eduard-bagdasaryan · 2025-02-05T13:07:05Z

src/proxyp/OutgoingHttpConfig.cc

+{
+    char *key = nullptr;
+    char *value = nullptr;
+    if(!ConfigParser::NextKvPair(key, value))


eduard-bagdasaryan · 2025-02-05T13:07:18Z

src/proxyp/OutgoingHttpConfig.cc

+    srcAddr = new AddrOption("src_addr", requiredValue("src_addr"), ConfigParser::LastTokenWasQuoted());
+    dstAddr = new AddrOption("dst_addr", requiredValue("dst_addr"), ConfigParser::LastTokenWasQuoted());
+    srcPort = new PortOption("src_addr", requiredValue("src_port"), ConfigParser::LastTokenWasQuoted());
+    dstPort = new PortOption("dst_addr", requiredValue("dst_port"), ConfigParser::LastTokenWasQuoted());


eduard-bagdasaryan · 2025-02-05T17:16:55Z

src/format/Format.cc

+Format::Format::hasPercentCode() const
+{
+    for (auto t = format; t; t = t->next) {
+        if (t->type != LFT_STRING)


Done (e045f61).

having same type values but configured in decimal and hex

Initial implementation

e2130d1

rousskov requested changes Jan 16, 2025

View reviewed changes

rousskov added 4 commits January 16, 2025 16:14

Revert tunnel.cc changes until FwdState changes become more stable

41a71bb

fixup: Removed failed attempt at (hopefully premature) optimization

464bbc5

fixup: Report packing state _before_ assertions

0392fde

If we ever implement locking (removed in previous commit), this position will also help to "automatically" check the lock before any object updates. Also fixed method const-correctness.

rousskov requested changes Jan 16, 2025

View reviewed changes

src/proxyp/Header.cc Outdated Show resolved Hide resolved

src/FwdState.cc Outdated Show resolved Hide resolved

rousskov reviewed Jan 16, 2025

View reviewed changes

eduard-bagdasaryan and others added 19 commits January 20, 2025 17:35

Generate a PROXY protocol header before opening a connection

bf3f105

Moved Magic() functions to Elements.cc

e3c4dab

Made FwdState::proxyProtocolHeader optional<>

267ba2a

Also do not reset this field too early.

Added missing headers

9fb2bb5

Detailed decision making time in directive docs

42c5ef5

Also disclosed lack of support for multiple directives. We may need to relax that requirement later.

Added pseudo header and TLV configuration documentation

fb8efa2

Fixed the condition passed to cs->allowPersistent()

f0ceb1f

Autoformatted

8434e72

Basic options support for http_outgoing_proxy_protocol

9ac8bfc

Refactored with AddrOption, PortOption and TlvOption classes

5aa51b3

Refactored, added some descriptions and performed some tests

dfe7934

Added RefCount for Options

3d6262f

Autoformatted

1a90e3b

Do not dereference a nil valueFormat

49e50c0

Simplified and reduced duplication

b848cb7

Minor polishing

0a1cf1f

Implemented OutgoingHttpConfig::dump()

32742cb

Added a missing header

58963f2

Added a negative configuration test

0f209f3

rousskov requested changes Jan 30, 2025

View reviewed changes

Improved with format/forward.h

c856b67

eduard-bagdasaryan added 3 commits February 3, 2025 16:37

Addressing code review comments

b460c57

Simplified by moving the parsing into the Option constructor

fa64314

Covered other exception paths in OutgoingHttpConfig::adjustAddresses()

50096e6

eduard-bagdasaryan commented Feb 3, 2025

View reviewed changes

rousskov and others added 14 commits February 3, 2025 16:26

Introduced special handling for "-" header fields

2968fcd

This complication is required to let admin configure Squid to withheld address or port information without generating cache.log warnings. Other minor polishing touches.

Introduced Token::print(ostream)

d3835c6

and adjusted Format::Format::dump() accordingly. No functionality changes.

Remove Option::theValue

0f26c31

Use Option::valueFormat to get both raw and formatted option values (the raw value is obtained via Token::print(ostream)).

OutgoingHttpConfig::adjustAddresses()

46f04ea

Merged from head

97c059f

Removed and unused method

208b71b

Format::needsAle() should cover all codes that does not need ALE

72303fb

Configure an unspecified port/address as a single dash

3717a74

Let the adjustAddresses() decide whether the problem is fatal

b1aebf3

Added a couple of tlvs to the negative configuration test

04142e8

Reject tlvs with a length greater than 65535

9d069ee

Code cleanup

ed07241

Autoformatted

2b537ce

Renamed to Format::isConstant(), addressing a request

e045f61

eduard-bagdasaryan commented Feb 5, 2025

View reviewed changes

eduard-bagdasaryan added 2 commits February 5, 2025 21:16

Reject port options with leftovers

93cb024

Avoid duplicate tlvs

c2338c5

having same type values but configured in decimal and hex

	ProxyProtocol::Header::packInto(MemBuf &mb) const
	ProxyProtocol::Header::packInto(Packable &out) const

	if (t->type != LFT_STRING)
	if (t->type != LFT_NONE && t->type != LFT_STRING && t->type != LFT_PERCENT) // other codes require ALE

	throw TextException(ToSBuf("Could not parse '", val, "' as an IP port"), Here());
	throw TextException(ToSBuf("Could not parse '", val, "' as an address port"), Here());

	Port port_; ///< parsed address (for options without logformat %macros)
	Port port_; ///< transaction-independent source or destination address port

-	removes quotes before sending the field value. Quoted header field values
+	removes quotes before using the field value.
+	A header field value, whether quoted or not,

Basic PROXY protocol support for Squid-to-peer connections #281

Are you sure you want to change the base?

Basic PROXY protocol support for Squid-to-peer connections #281

Conversation

eduard-bagdasaryan commented Jan 16, 2025

Choose a reason for hiding this comment

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment