Jenkins build is back to normal : 3.HEAD-amd64-centos-6 #429

[noc]

See 

Build failed in Jenkins: 3.HEAD-amd64-centos-7 #5

[noc]

See 

Changes:

[Amos Jeffries] Fix unwrapped adaptation code in rev.13528

--
[...truncated 38344 lines...]
make[3]: Nothing to be done for `uninstall'.
make[3]: Leaving directory 
`
Making uninstall in adaptation
make[3]: Entering directory 
`
Making uninstall in icap
make[4]: Entering directory 
`
make[4]: Nothing to be done for `uninstall'.
make[4]: Leaving directory 
`
make[4]: Entering directory 
`
make[4]: Nothing to be done for `uninstall-am'.
make[4]: Leaving directory 
`
make[3]: Leaving directory 
`
Making uninstall in esi
make[3]: Entering directory 
`
make[3]: Nothing to be done for `uninstall'.
make[3]: Leaving directory 
`
make[3]: Entering directory 
`
 ( cd 
'/tmp/am-dc-18013/
 && rm -f diskd unlinkd )
 ( cd 
'/tmp/am-dc-18013/
 && rm -f mib.txt )
 /usr/bin/rm -f -f 
/tmp/am-dc-18013/
 ( cd 
'/tmp/am-dc-18013/
 && rm -f squid.8 )
 /usr/bin/rm -f -f 
/tmp/am-dc-18013/
 ( cd 
'/tmp/am-dc-18013/
 && rm -f squid )
 ( cd 
'/tmp/am-dc-18013/
 && rm -f squid.conf.default squid.conf.documented mime.conf.default )
make[3]: Leaving directory 
`
make[2]: Leaving directory 
`
Making uninstall in tools
make[2]: Entering directory 
`
Making uninstall in purge
make[3]: Entering directory 
`
 ( cd 
'/tmp/am-dc-18013/
 && rm -f purge )
make[3]: Leaving directory 
`
Making uninstall in squidclient
make[3]: Entering directory 
`
make[4]: Entering directory 
`
 ( cd 
'/tmp/am-dc-18013/
 && rm -f squidclient )
 ( cd 
'/tmp/am-dc-18013/
 && rm -f squidclient.1 )
make[4]: Leaving directory 
`
make[3]: Leaving directory 
`
make[3]: Entering directory 
`

Re: [PATCH] Support PROXY protocol

[Amos Jeffries]

On 11/08/2014 4:32 p.m., Alex Rousskov wrote:
> On 08/05/2014 08:31 PM, Amos Jeffries wrote:
> 
>> I am adding proxy_protocol_access as the first access control, reverting
>> follow_x_forwarded_for for the second. 
> 
> Great. I think this is a much simpler/cleaner design.
> 
> 
>> +} else if (strcmp(token, "require-proxy-header") == 0) {
>> +s->flags.proxySurrogate = true;
>> +debugs(3, DBG_IMPORTANT, "Disabling TPROXY Spoofing on port " << 
>> s->s << " (require-proxy-header enabled)");
>> +
> 
> The IMPORTANT message should probably be printed only if TPROXY spoofing
> on that port was actually configured/requested.
> 
> And, at that point, I would actually make the apparently illegal
> combination a fatal misconfiguration error, but I suspect you do not
> like that approach, and I will not insist on it.
> 

Done.

>> But retaining the new description texts.
> 
>> -DEFAULT_DOC: X-Forwarded-For header will be ignored.
>> +DEFAULT_DOC: indirect client IP will not be accepted.
> 
> The old documentation line was much more clear IMO. If it was incorrect,
> can you rephrase the correction please? Perhaps you meant to say
> something like "Any client IP specified in X-Forwarded-For header will
> be ignored"? If yes, the current version still sounds better to me.
> Adding Forwarded header to those description is a good idea if correct,
> of course.
> 

Done.

>> +DEFAULT_DOC: all TCP connections will be denied
> 
> I would clarify that with either
> 
> * "all TCP connections to ports with require-proxy-header will be denied" or
> 
> * "proxy_protocol_access deny all"
> 

Done

>> +Any host for which we accept client IP details can place
>> +incorrect information in the relevant header, and Squid
> 
> I would s/for/from/ but perhaps I misunderstand how this works.
> 
> 
>> +tok.skip(Proxy1p0magic);
> 
> We already know the magic is there. If you want to optimize this, then
> skip() in ConnStateData::parseProxyProtocolHeader() and pass the
> Tokenizer object to the version-specific parsers. I do not insist on
> this change, but you may want to add a corresponding TODO if you do not
> want to optimize this.
> 

This is done with the parser method Tokenizer so that if we happen not
to receive the whole header in one read for any reason we can easily
undo and retry on the following read(2).
 Only if the Tokenizer parses completely and fully is the I/O buffer
updated.

We could generate a temporary SBuf and setup the Tokenizer with that.
But it's simpler just to setup the Tokenizer with the existing buffer
and unconditionally skip the magic we already know exists.

> 
>> +if (!tok.prefix(tcpVersion, CharacterSet::ALPHA+CharacterSet::DIGIT))
> 
> That dynamic creation, addition, and destruction of a 256-element vector
> is a significant performance penalty. Please create a static version of
> that alphanumeric(?) CharacterSet instead of computing it at least once
> for every PROXY connection.
> 
> 
>> +const CharacterSet ipChars =  CharacterSet("IP Address",".:") + 
>> CharacterSet::HEXDIG;
> 
> Same here, made worse by adding creation of an intermediate set.
> 
> Please check for other occurrences.

Fixed, and checked.


>> +if (!tok.prefix(tcpVersion, CharacterSet::ALPHA+CharacterSet::DIGIT))
>> +return proxyProtocolError(tok.atEnd()?"PROXY/1.0 error: invalid 
>> protocol family":NULL);
> 
> I recommend removing that code and using
> 
>   if (tok.skip("UNKNOWN")) { ... your UNKNOWN code here ...}
>   else if (tok.skip("TCP")) { ... your TCP code here ...}
> 
> instead.
> 
> 
>> +if (!tcpVersion.cmp("UNKNOWN")) {
>> +} else if (!tcpVersion.cmp("TCP",3)) {
> 
> Please test for the more likely/common condition first.
> 

Agreed. Done.

>> +// skip to first LF (assumes it is part of CRLF)
>> +const SBuf::size_type pos = in.buf.findFirstOf(CharacterSet::LF);
>> +if (pos != SBuf::npos) {
>> +if (in.buf[pos-1] != '\r')
> 
> It would be better to use Tokenizer for this IMO. If you insist on
> manual parsing, consider at least skipping the magic header in the
> findFirstOf() call.
> 

Waiting on the FTP change that makes it easy to settle in trunk.

> 
>> + return proxyProtocolError(tok.atEnd()?"PROXY/1.0 error: missing SP":NULL);
> 
> Please surround the ? and : parts of the operator with spaces for
> readability, especially since the string itself contains ':'.
> 

Done.

> 
>> +if (!tok.prefix(ipa, ipChars) || !tok.skip(' ') ||
>> +   !tok.prefix(ipb, ipChars) || !tok.skip(' ') ||
>> +   !tok.int64(porta) || !tok.skip(' ') ||
>> +   !tok.int64(portb) || !tok.skip('\r') || !tok.skip('\n'))
> 
> This code would be a lot clearer if you rewrite it in a positive way:
> 
> const bool parsedAll = tok.prefix() && tok.skip() && ...
> if (!parsedAll)
> ...
> 
> I do not insist on this change.
> 

Makes some sense. Done.

> 
>> +// parse  src-IP SP dst-IP SP src-

[PATCH] Better handling of huge native FTP requests

[Alex Rousskov]

Hello,

The attached patch avoids assertions on large FTP commands and
cleans up both general and FTP-specific error handling code during
request parsing. Please see the patch preamble for technical details.


Thank you,

Alex.
Do not assert on native FTP ERR_TOO_BIG. Do not check for ERR_TOO_BIG twice.

The assertion occurred because both the FTP request parser and the generic
ConnStateData::checkHeaderLimits() code would try to write their own error
message to the user. Reworked all error reporting code in the FTP parser to
avoid writing early responses (that were bypassing the overall transaction
flow with various negative side effects such as lack of logging).

Removed ConnStateData::checkHeaderLimits(): We already have protocol-specific
checks for huge HTTP and FTP requests. There is no point in duplicating them.
Centralizing them sounds like a good idea, but a general checkHeaderLimits()
cannot produce protocol-specific errors messages that we need, so it hurts
more than it helps. Moreover, checkHeaderLimits() was handling errors
differently than protocol parsing code, making the code more complex overall.

All that remains from the checkHeaderLimits() code now is a single Must(),
checking that the protocol parsers did what they were supposed to do: Return
NULL to request more data after checking any applicable limits. If parsers do
not (a Squid bug!), the ConnStateData job gets killed (and connection gets
closed) as the last resort.

Added clientReplyContext::setReplyToReply() and
StoreEntry::storeErrorResponse() to handle storing of a response to an FTP
command parsing error. The old code was using ErrorState to store parsing
errors, but ErrorState is still HTTP-specific and cannot relay the right FTP
codes/reasons to the user. The setReplyToReply() sounds silly but it matches
the existing setReplyTo*() naming scheme well.

Make sure parsed native FTP command tokens are not even close to the String
buffer limit. These checks are not a firm guarantee, but are better than
nothing until we replace String.

Handle ClientSocketContext registration centrally because all parsers need it.

Call quitAfterError() on fatal native FTP errors. Probably not necessary due
to fssError handling code that closes the FTP control connection, but adds
helpful debugging and brings us closer to the HTTP error handling code.

Described ConnStateData::clientParseRequests().

=== modified file 'src/Store.h'
--- src/Store.h	2014-06-25 00:14:36 +
+++ src/Store.h	2014-08-12 17:42:51 +
@@ -80,40 +80,42 @@ public:
 StoreEntry();
 virtual ~StoreEntry();
 
 virtual HttpReply const *getReply() const;
 virtual void write (StoreIOBuffer);
 
 /** Check if the Store entry is emtpty
  * \retval true   Store contains 0 bytes of data.
  * \retval false  Store contains 1 or more bytes of data.
  * \retval false  Store contains negative content !!
  */
 virtual bool isEmpty() const {
 assert (mem_obj);
 return mem_obj->endOffset() == 0;
 }
 virtual bool isAccepting() const;
 virtual size_t bytesWanted(Range const aRange, bool ignoreDelayPool = false) const;
 virtual void complete();
 virtual store_client_t storeClientType() const;
 virtual char const *getSerialisedMetaData();
+/// Store a prepared error response. MemObject locks the reply object.
+void storeErrorResponse(HttpReply *reply);
 void replaceHttpReply(HttpReply *, bool andStartWriting = true);
 void startWriting(); ///< pack and write reply headers and, maybe, body
 /// whether we may start writing to disk (now or in the future)
 virtual bool mayStartSwapOut();
 virtual void trimMemory(const bool preserveSwappable);
 
 // called when a decision to cache in memory has been made
 void memOutDecision(const bool willCacheInRam);
 // called when a decision to cache on disk has been made
 void swapOutDecision(const MemObject::SwapOut::Decision &decision);
 
 void abort();
 void unlink();
 void makePublic();
 void makePrivate();
 void setPublicKey();
 void setPrivateKey();
 void expireNow();
 void releaseRequest();
 void negativeCache();

=== modified file 'src/client_side.cc'
--- src/client_side.cc	2014-08-11 16:09:06 +
+++ src/client_side.cc	2014-08-12 23:20:15 +
@@ -180,41 +180,40 @@ public:
 private:
 AnyP::PortCfgPointer portCfg;   ///< from HttpPortList
 Ipc::FdNoteId portTypeNote;///< Type of IPC socket being opened
 Subscription::Pointer sub; ///< The handler to be subscribed for this connetion listener
 };
 
 static void clientListenerConnectionOpened(AnyP::PortCfgPointer &s, const Ipc::FdNoteId portTypeNote, const Subscription::Pointer &sub);
 
 /* our socket-related context */
 
 CBDATA_CLASS_INIT(ClientSocketContext);
 
 /* Local functions */
 static IOCB clientWriteComplete;
 static IOCB clientWriteBodyComplete;
 static IOACB httpAccept;
 #if USE_OPENSSL
 static IOACB httpsAcce

Re: [PATCH] Support PROXY protocol

[Alex Rousskov]

On 08/12/2014 10:17 AM, Amos Jeffries wrote:
> On 11/08/2014 4:32 p.m., Alex Rousskov wrote:
>> On 08/05/2014 08:31 PM, Amos Jeffries wrote:
>>> +tok.skip(Proxy1p0magic);
>>
>> We already know the magic is there. If you want to optimize this, then
>> skip() in ConnStateData::parseProxyProtocolHeader() and pass the
>> Tokenizer object to the version-specific parsers. I do not insist on
>> this change, but you may want to add a corresponding TODO if you do not
>> want to optimize this.
>>
> 
> This is done with the parser method Tokenizer so that if we happen not
> to receive the whole header in one read for any reason we can easily
> undo and retry on the following read(2).
>  Only if the Tokenizer parses completely and fully is the I/O buffer
> updated.

I know.


> We could generate a temporary SBuf and setup the Tokenizer with that.
> But it's simpler just to setup the Tokenizer with the existing buffer
> and unconditionally skip the magic we already know exists.

No temporary SBuf is needed AFAICT. You just _move_ the Tokenizer
creation into ConnStateData::parseProxyProtocolHeader() where you can
skip() the magic instead of doing startsWith() tests. Any buffer updates
happen when and where they happen today.

This is just an optional optimization, nothing more, of course. Not
required (although it would have helped avoid the missing skip() bug).


>>> +// parse  src-IP SP dst-IP SP src-port SP dst-port CRLF
>>> +if (!tok.prefix(ipa, ipChars) || !tok.skip(' ') ||
>>> +   !tok.prefix(ipb, ipChars) || !tok.skip(' ') ||
>>> +   !tok.int64(porta) || !tok.skip(' ') ||
>>> +   !tok.int64(portb) || !tok.skip('\r') || !tok.skip('\n'))
>>> +return proxyProtocolError(!tok.atEnd()?"PROXY/1.0 error: 
>>> invalid syntax":NULL);
>>
>> AFAICT, atEnd() may be false just because we have not received the whole
>> PROXY header yet, not because we received something invalid. For
>> example, int64() returns false not just on failures but on "insufficient
>> data" (as it should!).
>>
> 
> I am not worried about those failure cases particularly, allowing for
> sender emitting one field per packet is a tolerance optimization to
> begin with. The entire header is designed to be sent in one packet and
> the spec explicitly requires it to be sent in a single write(2).

The PROXY RFC says "A receiver may reject an incomplete line which does
not contain the CRLF sequence in the first atomic read operation". If
this ever gets to the IETF review, I hope this requirement will be
stricken down. An "atomic read" is a bogus concept in this context; the
whole requirement violates the spirit of a TCP-compatible protocol and
will lead to interoperability problems.

It does not matter what the sender does because a lot of things can
happen to a packet after the write(2) call. IMO, we MUST accept any
valid PROXY header regardless of the packet and I/O segmentation at
lower levels. Fortunately, it is not very difficult to do in Squid.


>> [N.B. prefix() should behave similarly on insufficient data, but it is
>> currently broken; I can fix after the FTP Relay project is done.]

That Tokenizer::prefix() fix is also in trunk now.


>>> +if ((in.buf[0] & 0xF0) != 0x20) // version == 2 is mandatory
>>> +return proxyProtocolError("PROXY/2.0 error: invalid version");
>>> +
>>> +const char command = (in.buf[0] & 0x0F);
>>> +if ((command & 0xFE) != 0x00) // values other than 0x0-0x1 are invalid
>>> +return proxyProtocolError("PROXY/2.0 error: invalid command");
>>
>> These and similar v2 checks do not make sense to me because we know for
>> sure that in.buf starts with Proxy2p0magic. Why test parts of a
>> hard-coded constant? I am guessing you meant to skip magic before these
>> tests...
>>
>> Which forces me to ask whether the PROXY v2 path was tested recently?
> 
> Willy reported it working before the audit,

Strange. I do not see how that code could have worked, but that is not
important, I guess. The RFC already claims Squid support! :-)


>>> +const char *clen = in.buf.rawContent() + Proxy2p0magic.length() + 2;
>>> +const uint16_t len = ntohs(*(reinterpret_cast>> *>(clen)));
>>
>> Could this cast violate integer alignment rules on some platforms? If
>> yes, we may have to memcpy those two bytes first. I do not think we can
>> guarantee any specific alignment for in.buf.rawContent() and, hence, we
>> cannot guarantee any specific alignment for clen.
>>
> 
> Unsure. The uint16_t should be compiled as 8-bit aligned AFAIK. In
> theory we only encounter that type of problem with the more complex
> types (like pax below).
> 
> I dont like casting anyway, so if you have any better way to do it
> without a double data copy I am interested. Note that ntohs() is
> effectively one data copy.

Your call, but I would memcpy() before interpreting that clen value to
be on the safe side. It is a very fast copy. If you do not memcpy(),
please add a comment. For example:

/