Re: [PATCH] Support PROXY protocol
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.
> 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.
> +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"
> + 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.
> +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.
> +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.
> +if (!tcpVersion.cmp("UNKNOWN")) {
> +} else if (!tcpVersion.cmp("TCP",3)) {
Please use the length argument consistently if this code stays.
> +// 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.
> + 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 ':'.
> +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.
> +// 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!).
[N.B. prefix() should behave similarly on insufficient data, but it is
currently broken; I can fix after the
Re: [PATCH] Native FTP Relay
On 08/10/2014 06:11 AM, Amos Jeffries wrote:
> On 10/08/2014 2:44 p.m., Alex Rousskov wrote:
>> This part of the problem will be gone when HTTP-specific code will be
>> moved to HttpServer, but the ICAP compatibility part will not go away
>> regardless of any cleanup.
>
> The ICAP compatibility issue is a bug in ICAP or specific ICAP servers
> that needs fixing there.
Yes, of course, but fixing those ICAP services is often beyond Squid
admin control, and there is no pressing need to break compatibility with
those ICAP services _now_ (by using a different FTP "version" than 1.0
or 1.1).
> HTTP itself can already (or shortly will) have
> present "HTTP/2.0", "h2" or "h2-NN" message versions to be encapsulated.
I suspect we will receive requests to optionally mask those new versions
when sent over ICAP because some broken service that the admin cannot
fix cannot handle much more than /1.0 and /1.1.
>>> in src/Server.cc:
>>> * this change looks semantically wrong:
>>>-if (!doneWithServer()) {
>>>+if (mayReadVirginReplyBody()) {
>>> closeServer();
>>
>> The change itself does not make the code look semantically wrong. On the
>> semantics level, the code looks about the same before and after the change:
>>
>> Was: If we are not done with the server, close it.
>> Now: If we are still reading from the server, close it.
>
> It is now *apparently* skipping all the possible non-reply-reading
> reasons for closing the server connection. That is semantic change...
If you think the above change affects something on the semantics level,
then I do not have any new arguments to convince you otherwise.
Hopefully, the diverging opinions on the code looks are not important in
this case, because we seem to agree that the patch code is correct (at
least in a sense that it does not make the known problem any worse and
actually solves one sub-problem).
> Okay, I think I understand the cases now. So the old HTTP code was
> broken by not considering the request and Trailers cases as okay even
> when ICAP closed. You are just not fixing that bug.
Yes, I am not fixing any old bugs here. As for what exactly is broken
and by how much, I prefer not to discuss those details further (here and
now) because the patch does not change them.
> Would you mind adding a TODO to the new HTTP mayReadVirginReplyBody()
> that it should be only considering reply body reads, not overall server use?
Done. There was no HTTP mayReadVirginReplyBody(), but I added one.
> in src/cache_cf.cc:
> * the new parsePortProtocol() FTP case should return
> Ftp::ProtocolVersion().
Done.
> in src/cf.data.pre:
> * mentions of "ftp-track-dirs=on|off" need updating
Fixed.
> in src/ftp/Elements.cc:
> * Ftp::HttpReplyWrapper() has unnecessary:
> httpVersion = Http::ProtocolVersion(Ftp::ProtocolVersion().major,
> Ftp::ProtocolVersion().minor);
>
> - the current default Http::ProtocolVersion() constructor sets the
> right values for HTTP messages generated by Squid. The old code using
> Http::ProtocolVersion(1, 1) was incorrect/outdated.
>
> - same for Ftp::Server::parseOneRequest() in src/servers/FtpServer.cc
Left as is because the Http::ProtocolVersion default is irrelevant in
those two places.
We should set the wrapping message version to what FTP code wants to use
for HTTP wrapping. That version is defined by Ftp::ProtocolVersion().
The fact that the HTTP default currently matches the version used for
FTP wrapping is a coincidence that the code should not rely on. Imagine
changing the HTTP default to 2.0 tomorrow and then spending hours trying
to find all the places where FTP was _implicitly_ relying on that
default being 1.1...
> in src/clients/Makefile.am:
> * no need for forward.h in the SOURCES list to be separated from the
> other files.
> - same in src/servers/Makefile.am
Removed.
> * please include $(top_srcdir)/src/TestHeaders.am to run the .h header
> unit tests
> - same in src/ftp/Makefile.am and src/servers/Makefile.am
Done.
> in src/servers/forward.h:
> * missing pre-define for class ConnStateData
> - this would have been caught by the .h unit tests mentioned above.
Fixed.
> in src/servers/FtpServer.cc:
>
> + Ftp::Server::parseOneRequest() for all the following until "++"
>
> * please reference RFC 959 section 3.1.1.5.2 for the unusual definition
> of WhiteSpace CharacterSet.
Done. It was actually based on isspace(3) so not that "unusual" :-)
> * please use existing CharacterSet::LF instead of re-defining as local
> NewLine.
Done.
> * please use tok.prefix(CharacterSet::ALPHA) to parse the FTP command
> instead of BlackSpace. Then explicit delimiter check to validate the input.
> - RFC 959 is clear:
> "The command codes themselves are alphabetic characters terminated
>by the character (Space) if parameters follow and Telnet-EOL
>otherwise."
Not changed. I do not see a compelling reason for a general purpose
relay to police traffic by default. Squid itself does not care if
RELEASENOTES.html outdated
Hi, link of 'squid-3.4.6' points to outdeated 'http://www.squid-cache.org/Versions/v3/3.4/RELEASENOTES.html' which is version 3.4.5 seems that Releasenotes.html did not get updated to reflect 3.4.6. Cheers -- Christian - Please do not 'CC' me on list mails. Just reply to the list :) Der ultimative shop für Sportbekleidung und Zubehör http://www.sc24.de
Re: build failure squid 3.4.6
Hi, thanks :) --with-included-ltdl did the trick Am 10.08.2014 20:48, schrieb Kinkie: > I see. > You probably need to install libtool later than 2.4, or use the > --with-included-ltdl configure option. > > > > On Sun, Aug 10, 2014 at 8:31 PM, Christian wrote: >> Hi, >> >> rpm -qa | grep libtool >> libtool-2.2.6-2.131.1 >> >> OS: SuSE Linux Enterprise Server 11 SP3 >> >> Am 10.08.2014 20:25, schrieb Kinkie: >>> Hi, >>> what version of libtool are you using? What OS and version? >>> >>> On Sun, Aug 10, 2014 at 8:10 PM, Christian wrote: Hi, haveing Problems to build squid 3.4.6. Getting the Following: [ 554s] In file included from LoadableModule.cc:10: [ 554s] ../libltdl/ltdl.h:106: error: 'LT_DLSYM_CONST' does not name a type [ 554s] LoadableModule.cc: In constructor 'LoadableModule::LoadableModule(const String&)': [ 554s] LoadableModule.cc:25: error: 'lt__PROGRAM__LTX_preloaded_symbols' was not declared in this scope [ 554s] make[3]: *** [LoadableModule.o] Error 1 What can I do to fix this ? Thanks for help Cheers -- Christian - Please do not 'CC' me on list mails. Just reply to the list :) Der ultimative shop für Sportbekleidung und Zubehör http://www.sc24.de >>> >>> >>> >> >> >> -- >> >> Christian >> >>- Please do not 'CC' me on list mails. >> Just reply to the list :) >> >> Der ultimative shop für Sportbekleidung und Zubehör >> >> http://www.sc24.de >> > > > -- Christian - Please do not 'CC' me on list mails. Just reply to the list :) Der ultimative shop für Sportbekleidung und Zubehör http://www.sc24.de
Re: build failure squid 3.4.6
I see. You probably need to install libtool later than 2.4, or use the --with-included-ltdl configure option. On Sun, Aug 10, 2014 at 8:31 PM, Christian wrote: > Hi, > > rpm -qa | grep libtool > libtool-2.2.6-2.131.1 > > OS: SuSE Linux Enterprise Server 11 SP3 > > Am 10.08.2014 20:25, schrieb Kinkie: >> Hi, >> what version of libtool are you using? What OS and version? >> >> On Sun, Aug 10, 2014 at 8:10 PM, Christian wrote: >>> Hi, >>> >>> haveing Problems to build squid 3.4.6. >>> Getting the Following: >>> >>> [ 554s] In file included from LoadableModule.cc:10: >>> [ 554s] ../libltdl/ltdl.h:106: error: 'LT_DLSYM_CONST' does not name a type >>> [ 554s] LoadableModule.cc: In constructor >>> 'LoadableModule::LoadableModule(const String&)': >>> [ 554s] LoadableModule.cc:25: error: >>> 'lt__PROGRAM__LTX_preloaded_symbols' was not declared in this scope >>> [ 554s] make[3]: *** [LoadableModule.o] Error 1 >>> >>> What can I do to fix this ? >>> Thanks for help >>> >>> Cheers >>> -- >>> >>> Christian >>> >>>- Please do not 'CC' me on list mails. >>> Just reply to the list :) >>> >>> Der ultimative shop für Sportbekleidung und Zubehör >>> >>> http://www.sc24.de >>> >> >> >> > > > -- > > Christian > >- Please do not 'CC' me on list mails. > Just reply to the list :) > > Der ultimative shop für Sportbekleidung und Zubehör > > http://www.sc24.de > -- Francesco
Re: build failure squid 3.4.6
Hi, rpm -qa | grep libtool libtool-2.2.6-2.131.1 OS: SuSE Linux Enterprise Server 11 SP3 Am 10.08.2014 20:25, schrieb Kinkie: > Hi, > what version of libtool are you using? What OS and version? > > On Sun, Aug 10, 2014 at 8:10 PM, Christian wrote: >> Hi, >> >> haveing Problems to build squid 3.4.6. >> Getting the Following: >> >> [ 554s] In file included from LoadableModule.cc:10: >> [ 554s] ../libltdl/ltdl.h:106: error: 'LT_DLSYM_CONST' does not name a type >> [ 554s] LoadableModule.cc: In constructor >> 'LoadableModule::LoadableModule(const String&)': >> [ 554s] LoadableModule.cc:25: error: >> 'lt__PROGRAM__LTX_preloaded_symbols' was not declared in this scope >> [ 554s] make[3]: *** [LoadableModule.o] Error 1 >> >> What can I do to fix this ? >> Thanks for help >> >> Cheers >> -- >> >> Christian >> >>- Please do not 'CC' me on list mails. >> Just reply to the list :) >> >> Der ultimative shop für Sportbekleidung und Zubehör >> >> http://www.sc24.de >> > > > -- Christian - Please do not 'CC' me on list mails. Just reply to the list :) Der ultimative shop für Sportbekleidung und Zubehör http://www.sc24.de
Re: build failure squid 3.4.6
Hi, what version of libtool are you using? What OS and version? On Sun, Aug 10, 2014 at 8:10 PM, Christian wrote: > Hi, > > haveing Problems to build squid 3.4.6. > Getting the Following: > > [ 554s] In file included from LoadableModule.cc:10: > [ 554s] ../libltdl/ltdl.h:106: error: 'LT_DLSYM_CONST' does not name a type > [ 554s] LoadableModule.cc: In constructor > 'LoadableModule::LoadableModule(const String&)': > [ 554s] LoadableModule.cc:25: error: > 'lt__PROGRAM__LTX_preloaded_symbols' was not declared in this scope > [ 554s] make[3]: *** [LoadableModule.o] Error 1 > > What can I do to fix this ? > Thanks for help > > Cheers > -- > > Christian > >- Please do not 'CC' me on list mails. > Just reply to the list :) > > Der ultimative shop für Sportbekleidung und Zubehör > > http://www.sc24.de > -- Francesco
build failure squid 3.4.6
Hi, haveing Problems to build squid 3.4.6. Getting the Following: [ 554s] In file included from LoadableModule.cc:10: [ 554s] ../libltdl/ltdl.h:106: error: 'LT_DLSYM_CONST' does not name a type [ 554s] LoadableModule.cc: In constructor 'LoadableModule::LoadableModule(const String&)': [ 554s] LoadableModule.cc:25: error: 'lt__PROGRAM__LTX_preloaded_symbols' was not declared in this scope [ 554s] make[3]: *** [LoadableModule.o] Error 1 What can I do to fix this ? Thanks for help Cheers -- Christian - Please do not 'CC' me on list mails. Just reply to the list :) Der ultimative shop für Sportbekleidung und Zubehör http://www.sc24.de
Re: [PATCH] Native FTP Relay
On 10/08/2014 2:44 p.m., Alex Rousskov wrote:
> On 08/08/2014 11:13 AM, Amos Jeffries wrote:
>> On 9/08/2014 4:57 a.m., Alex Rousskov wrote:
>>> On 08/08/2014 09:48 AM, Amos Jeffries wrote:
Audit results (part 1):
* Please apply CharacterSet updates separately.
* Please apply Tokenizer API updates separately.
* please apply src/HttpHdrCc.h changes separately.
>
>>> Why? If the branch is merged, they will be applied with their own/
>>> existing commit messages.
>
>> They are updates on previous feature changesets unrelated to FTP which
>> will block future parser alterations (also unrelated to FTP) being
>> backported if this project takes long to stabilize.
>>
>> If they have their own commit messages then cherrypicking out of the
>> branch should be relatively easy.
>
> Thank you for explaining your rationale. To minimize overheads, let's
> come back to this issue if the branch is not merged soon.
>
>
* is FTP/1.1 really the protocol version of FTP parsed by Squid? (the
AnyP::ProtocolVersion parameters)
>
>> Okay, leave as-is but please document that as the reason for using that
>> version number
>
> Done. It was actually documented in the corresponding commit message. To
> provide _source code_ documentation in one place, I added
> Ftp::ProtocolVersion() and used that everywhere instead of hard-coded
> "1,1". It will be easier to track down all "1,1" users that way if
> somebody wants to change or polish this further later. See ftp/Elements.*
Thank you.
NP: I found two related spots that need converting as well. Highlighted
below in this round of audit notes.
>
> The resulting code required more changes than one may expect and is
> still a little awkward because Http::ProtocolVersion should have been
> just a function (not a class/type), and because PortCfg::setTransport
> internals did not belong to AnyP where they were placed. I did my best
> to avoid cascading changes required to fix all that by leaving
> Http::ProtocolVersion as is and moving PortCfg::setTransport() internals
> into cache_cf.cc where they can access FTP-specific code.
>
>
>> and a TODO about cleaning up the message processing to
>> stop assuming HTTP versions.
>
> Done in one of the many client_side.cc places where we still assume
> HTTP. Here is the new TODO:
>
>> /* RFC 2616 section 10.5.6 : handle unsupported HTTP major versions
>> cleanly. */
>> /* We currently only support 0.9, 1.0, 1.1 properly */
>> /* TODO: move HTTP-specific processing into servers/HttpServer and such
>> */
>> if ( (http_ver.major == 0 && http_ver.minor != 9) ||
>> (http_ver.major > 1) ) {
>
>
> This part of the problem will be gone when HTTP-specific code will be
> moved to HttpServer, but the ICAP compatibility part will not go away
> regardless of any cleanup.
The ICAP compatibility issue is a bug in ICAP or specific ICAP servers
that needs fixing there. HTTP itself can already (or shortly will) have
present "HTTP/2.0", "h2" or "h2-NN" message versions to be encapsulated.
>
>> in src/Server.cc:
>> * this change looks semantically wrong:
>>-if (!doneWithServer()) {
>>+if (mayReadVirginReplyBody()) {
>> closeServer();
>
> The change itself does not make the code look semantically wrong. On the
> semantics level, the code looks about the same before and after the change:
>
> Was: If we are not done with the server, close it.
> Now: If we are still reading from the server, close it.
It is now *apparently* skipping all the possible non-reply-reading
reasons for closing the server connection. That is semantic change...
>
>> in src/Server.h:
>> * I'm not sure the call order of doneWithServer() and
>> mayReadVirginReplyBody() is the right way around.
>
> Sorry, I am not sure what you mean by "call order" in src/Server.h
> context. That file does not determine the order of those two calls.
... but mayReadVirginReplyBody() is calling doneWithServer() in the HTTP
case. You clarified why its done below and I am certain now that it is
indeed wrong, although necessary as a temporary workaround until the
non-reply-reading cases are checked and fixed.
>
>> It seems like "being
>> done completely with a server" includes "whether more reply is coming"
>> as one sub-condition of several (see above), not the reverse.
>
> Correct. As the commit message quoted above tries to explain, FTP needed
> to distinguish two cases that the old code merged together:
>
> * The transaction may still be receiving the virgin response (the new
> mayReadVirginReplyBody method). This is when FTP and HTTP code has to
> end communication with the server. No other solution here right now (as
> the source code comment explains).
>
> * The transaction stopped receiving the virgin response but may still be
> able to communicate with the origin server. In this case, the FTP code
> wants to keep the [pinned] _control_ connection to the origin server
> open and, hence, does not want to c
Re: [PATCH] Kerberos configure patch + some cleanup
Apologies. I must have overlooked it. Here is the updated patch
Markus
"Amos Jeffries" wrote in message news:53e5c2df.3080...@treenet.co.nz...
On 9/08/2014 8:32 a.m., Markus Moeller wrote:
It should be in there or did I miss some ?
The original bits are still there in the patch copy mailed to the list.
Specifically in helpers/external_acl/kerberos_ldap_group/support_ldap.cc
get_bin_attributes() bits I can see :
* redux function setup:
+LDAPMessage *msg;
+char **attr_value = NULL;
+int *attr_len=NULL;
+size_t max_attr = 0;
+
+attr_value = *ret_value;
+attr_len = *ret_len;
should be:
+char **attr_value = *ret_value;
+int *attr_len = *ret_len;
+size_t max_attr = 0;
* main for loop:
- for (msg = ldap_first_entry
+ for (LDAPMessage *msg = ldap_first_entry
* drop these:
BerElement *b;
char *attr;
* switch case should be:
case LDAP_RES_SEARCH_ENTRY:
{
BerElement *b = NULL;
...
ber_free(b, 0);
} break;
* for loops inside that switch case should be:
- for (attr = ldap_first_attribute...
+ for (char *attr = ldap_first_attribute...
- int il; for (il = 0; ...
+ for (int il = 0; ...
Otherwise it looks okay.
Amos
Markus
-Original Message- From: Amos Jeffries Sent: Friday, August 08,
2014 1:28 PM To: squid-dev@squid-cache.org ; Markus Moeller Subject: Re:
[PATCH] Kerberos configure patch + some cleanup
On 8/08/2014 8:02 a.m., Markus Moeller wrote:
Are there any objections to this patch ?
The audit results from me I accidentally sent in private.
Do you have an updated patch with those fixes?
Amos
trunk_kerberos_cleanup_8.patch
Description: Binary data
