Author: Alex Rousskov <rousskov@measurement-factory.com>
[request|reply]_header_* manglers fixes to handle custom headers
This patch fix the [request|reply]_header_[access|replace] configuration
parameters to support custom headers. Before this patch the user was able
to remove/replace/allow all custom headers using the "Other" as header name.
When '-k parse' is used deprecation notices and upgrade help messages etc
need to be bumped consistently up to level-0 and this macro will help
reducing the (?:) code mistakes.
Alex Rousskov [Sun, 1 Jul 2012 03:55:21 +0000 (21:55 -0600)]
Added ssl::bump_mode logformat code to log SslBump decisions.
Reshuffled (a little) how the bumping decision for CONNECT is made to
streamline the code. This was necessary to consistently distinguish '-' from
'none' logged modes.
Do not evaluate ssl_bump when we are going to respond to a CONNECT request
with an HTTP 407 (Proxy Authentication Required) or with a redirect response.
Such evaluation was pointless because the code never bumps in such cases
because, unlike regular errors, these responses cannot be delayed and served
later inside the bumped tunnel.
Alex Rousskov [Thu, 28 Jun 2012 18:26:44 +0000 (12:26 -0600)]
Removed FilledChecklist::checkCallback() as harmful and not needed.
The method was resetting the authentication state (auth_user_request) of a
connection just before notifying the caller of the async ACL check result. The
reset restarts authentication sequence, creating a 407 "loop" for auth schemes
that require more than one step (e.g., NTLM).
The method is no longer needed because we do not need to explicitly "unlock"
auth_user_request anymore. It is refcounted now.
There are other cases where connection authentication state () is reset from
within the ACL code. Some of those cases are not needed and some might even
cause similar bugs, but I did not risk misclassifying them in this commit.
Dmitry Kurochkin [Fri, 22 Jun 2012 03:49:38 +0000 (21:49 -0600)]
Fix build with GCC 4.7 (and probably other C++11 compilers).
User-defined literals introduced by C++11 break some previously valid
code, resulting in errors when building with GCC v4.7. For example:
error: unable to find string literal operator 'operator"" PRIu64'
In particular, whitespace is now needed after a string literal and
before something that could be a valid user-defined literal. See
"User-defined literals and whitespace" section at [1] for more details.
The patch adds spaces between string literals and macros.
Amos Jeffries [Tue, 19 Jun 2012 23:16:13 +0000 (11:16 +1200)]
Cleanup: disconnect Authentication and URL-rewrite callback handlers
The authentication handlers were for some reason using RH (rewrite helper)
callback typedef. But specifying it as a fatal error if the char*
parameter was used in auth.
Assign a new callback typedef AUTHCB for use by authentication callers.
This allows auth callers to use different parameters (none) and to avoid
possibly fatal mistakes when coding new auth modules.
Alex Rousskov [Mon, 18 Jun 2012 23:13:05 +0000 (17:13 -0600)]
Account for Store disk client quota when bandwidth-limiting the server.
It is not clear why the store client type matters when
MemObject::mostBytesAllowed() is trying to find the maximum delay pool
quota for reading from the next hop HTTP server. Whether the client(s)
are reading from disk or RAM, the corresponding server-side bandwidth
ought to be limited.
This code was removed as a part of bug 3462 investigation, but it is not
needed to fix bug 3462.
Julien Pinon [Mon, 18 Jun 2012 23:08:56 +0000 (17:08 -0600)]
Bug 3462: Delay Pools and ICAP
Allow StoreEntry::bytesWanted() API to ignore delay pools. Use that
feature when shoveling adapted response body from ICAP/eCAP BodyPipe
into Store.
If we do not ignore delay pools in
ServerStateData::handleMoreAdaptedBodyAvailable() context, and our pool
quota is exhausted, we will stop reading from the adaptation BodyPipe,
but there is no code/API to notify a BodyPipe reader when the quota
becomes available again. With no reads from the pipe, the ICAP service
stops sending more adapted body data and possibly stops reading the
virgin body data from Squid, resulting in a stuck ICAP RESPMOD and the
master HTTP transaction.
We do need to call StoreEntry::bytesWanted() in this context because
otherwise Squid may run out of RAM (Squid bug #2619). The fix for that
problem inadvertently created this bug when delay pools were enabled.
Long-term, a "kick BodyPipe consumer when there is quota" code should be
added, and delay pools should be enabled for ICAP responses, but with
enough knobs for admins to disable ICAP pooling where needed. Most ICAP
environments will probably want to disable bandwidth controls because
ICAP service traffic is usually "local".
Removed StoreEntry::bytesWanted() TRY_FWD_HDR_WAIT guard that disabled
delay pool application until the final HTTP response headers are
received (HttpStateData::haveParsedReplyHeaders() is called). Nobody
could fully explain why that condition was really needed (and we
obviously want to limit bandwidth consumption at all response processing
stages, in general). The now-removed guard was bypassing delay pool
checks for virgin HTTP response (until the ICAP server responded with
adapted headers), causing bandwidth overuse.
Possibly fixes or helps with Squid bug #2606 as well.
Bug 2976: squid reports ERR_INVALID_URL for transparently captured requests when reconfiguring
During reconfigure the configured AnyP::PortCfg objects in http_port_list
may deleted so it is not safe to use them while processing Http requests.
For this reason inside prepareTransparentURL (file client_side.cc) function
the protocol was hard-coded to "http" instead of read it from the related
AnyP::PortCfg object.
But this is breaks the intercepted https traffic.
This patch:
1. Inside prepareTransparentURL read the protocol from the related
AnyP::PortCfg object
2. add_http_port() locks the new port pointer before linking it.
3. parsePortCfg() locks the new port pointer before linking it.
4. free_PortCfg() unlock the old port pointer before unlinking
it. It does not delete the old pointer.
This patch also discussed in squid-dev user mailing list in
"Re: [PATCH] Squid host rewrite for intercepted https requests"
thread.
Alex Rousskov [Sat, 16 Jun 2012 15:03:46 +0000 (09:03 -0600)]
Fix several ACL-related bugs including broken default rules and ACCESS_DUNNO.
For example:
# broken when "goodGuys" matches (denies good guys)
acl_driven_option deny !goodGuys
and
# broken if badGuys fails to match or mismatch (allows bad guys)
acl_driven_option allow !badGuys
Fixing the above resulted in significant changes (and more fixes!)
detailed below.
* Revised ACLChecklist::fastCheck() and nonBlockingCheck() APIs to
clarify all possible outcomes and to specify that exceptional ACL
check outcomes (not ALLOW or DENIED) are not ignored/skipped but
result in the same exceptional final answer. I believe this is the
right behavior even if it is going to break some [already broken
IMHO] existing configurations. Skipping failed ACLs is insecure and
may lead to confusing results.
* Correctly handle cases where no rules were matched and, hence, the
keyword/action of the last seen rule (if any) has to be "reversed".
* Do not ignore non-allow/deny outcomes of rules in fastCheck().
* Move away from setting the "default" (and usually wrong) "current"
answer and then sometimes adjusting it. Set the answer only when we
know what it is. This is done using markFinished() call which now
accepts the [final] answer value and debugging reason for selecting
that answer.
* Streamline and better document ACLChecklist::matchAclList()
interface. Use it in a more consistent fashion.
* Rewrote ACLChecklist::matchAclList() implementation when it comes to
handling ACLList::matches() outcomes. Better document and restrict
supported outcomes. Assert on unsupported outcomes (for now).
* Removed ACLChecklist::lastACLResult(). It was doing nothing but
duplicating nodeMatched value as far as I could tell.
* Removed ProxyAuthNeeded class. It is an async state that does not
perform async operations and, hence, is not needed.
* Move IdentLookup::checkForAsync() connection check into
ACLIdent::match() to avoid creating an async state that is not
needed.
* Polished aclMatchExternal() and greatly simplified
ACLExternal::ExternalAclLookup() to avoid creating async state under
non-async conditions, to avoid checking for the same conditions
twice, to fix wrong debugging messages, and to simplify (and possibly
fix) the overall algorithm.
The aclMatchExternal() call now checks most of the corner cases,
discards stale cached entries, and schedules either a background
cache update or a regular external lookup as needed.
ACLExternal::ExternalAclLookup() code is now
ExternalACLLookup::Start(). It initiates an external lookup. It does
not deal with the cached entry at all. It relies on
aclMatchExternal() to check various preconditions.
Some of the old code made little sense to me, and this is the biggest
ACL-specific change in this project, with the highest probability of
new bugs or unintended side-effects.
My goal here was to prevent aclMatchExternal() from creating an async
state where none was needed because new ACLChecklist::matchAclList()
code prohibited such self-contradictory outcomes. However, I later
discovered that it is not possible to prohibit them without rewriting
how Squid DNS cache lookups are working -- ipcache_nbgethostbyname()
and similar code may call back immediately if the item is in the
cache. Since I did not want to rewrite DNS code as well, I ended up
relaxing the ACLChecklist::matchAclList() code requirements, going a
step back to where we sometimes call ACLList::matches() twice for the
same ACL node.
Thus, it is probably possible to undo most of the external_acl.cc
changes. I left them in because I think they improve the quality of
the code and possibly fix a bug or two.
* Adjusted ACLMaxUserIP::match(), ACLProxyAuth::match(), and
ACLExternal::match() to explicitly stop ACL processing when an
exceptional state is discovered instead of just setting the current
answer and proceeding as if more ACLs could be checked. On the other
hand, we now do not set the answer if the corresponding internal
matching code (e.g., AuthenticateAcl()) needs an async operation
because we do not know the answer yet.
* Fixed HttpStateData::handle1xx() and
HttpStateData::finishingBrokenPost() to correctly handle
fastCheck(void) return values. They were assuming that there are only
two possible return values (ACCESS_DENIED/ALLOWED), potentially
subjecting more messages to invasive adaptations than necessary.
TODO:
* Rename currentAnswer() to finalAnswer(). We probably never change the
"current" answer any more.
When an intercepted SSL connection matches "ssl_bump none" in
squid.conf, Squid correctly refuses to bump it and establishes a TCP
tunnel using a fake CONNECT request. Unfortunately, the HTTP client
terminates with an "unknown protocol" SSL error.
Also the client_dst_passthru does not work as expected for intercepted requests
This can occur due to long lists of unit tests needing a lot of local
variable state tracking. Essentially 'function too long' after CPPUNIT
macros have been expanded.
Break the large set of request-line unit tests into groups related to
sub-parts of the request-line.
Alex Rousskov [Sat, 2 Jun 2012 00:21:53 +0000 (18:21 -0600)]
Assume [] surround an IPv6 address and strip them
Browsers such as Firefox, Chromium, and Safari prefer bare IPv6 addresses in
CNs. They generate confusing errors when they see bracketed CNs. For example:
You attempted to reach [2001:470:1:18::120], but instead you actually reached
a server identifying itself as [2001:470:1:18::120]. Chromium can say for sure
that you reached [2001:470:1:18::120], but cannot verify that that is the same
site as [2001:470:1:18::120] which you intended to reach.
Alex Rousskov [Fri, 1 Jun 2012 22:01:43 +0000 (16:01 -0600)]
Fixed several ACL-related bugs, including:
# broken when "goodGuys" matches (denies good guys)
acl_driven_option deny !goodGuys
and
# broken if badGuys fails to match or mismatch (allows bad guys)
acl_driven_option allow !badGuys
Fixing the above resulted in significant changes (and more fixes!)
detailed below.
* Revised ACLChecklist::fastCheck() and nonBlockingCheck() APIs to
clarify all possible outcomes and to specify that exceptional ACL
check outcomes (not ALLOW or DENIED) are not ignored/skipped but
result in the same exceptional final answer. I believe this is the
right behavior even if it is going to break some [already broken
IMHO] existing configurations. Skipping failed ACLs is insecure and
may lead to confusing results.
* Correctly handle cases where no rules were matched and, hence, the
keyword/action of the last seen rule (if any) has to be "reversed".
* Do not ignore non-allow/deny outcomes of rules in fastCheck().
* Move away from setting the "default" (and usually wrong) "current"
answer and then sometimes adjusting it. Set the answer only when we
know what it is. This is done using markFinished() call which now
accepts the [final] answer value and debugging reason for selecting
that answer.
* Streamline and better document ACLChecklist::matchAclList()
interface. Use it in a more consistent fashion.
* Rewrote ACLChecklist::matchAclList() implementation when it comes to
handling ACLList::matches() outcomes. Better document and restrict
supported outcomes. Assert on unsupported outcomes (for now).
* Removed ACLChecklist::lastACLResult(). It was doing nothing but
duplicating nodeMatched value as far as I could tell.
* Removed ProxyAuthNeeded class. It is an async state that does not
perform async operations and, hence, is not needed.
* Move IdentLookup::checkForAsync() connection check into
ACLIdent::match() to avoid creating an async state that is not
needed.
* Polished aclMatchExternal() and greatly simplified
ACLExternal::ExternalAclLookup() to avoid creating async state under
non-async conditions, to avoid checking for the same conditions
twice, to fix wrong debugging messages, and to simplify (and possibly
fix) the overall algorithm.
The aclMatchExternal() call now checks most of the corner cases,
discards stale cached entries, and schedules either a background
cache update or a regular external lookup as needed.
ACLExternal::ExternalAclLookup() code is now
ExternalACLLookup::Start(). It initiates an external lookup. It does
not deal with the cached entry at all. It relies on
aclMatchExternal() to check various preconditions.
Some of the old code made little sense to me, and this is the biggest
ACL-specific change in this project, with the highest probability of
new bugs or unintended side-effects.
My goal here was to prevent aclMatchExternal() from creating an async
state where none was needed because new ACLChecklist::matchAclList()
code prohibited such self-contradictory outcomes. However, I later
discovered that it is not possible to prohibit them without rewriting
how Squid DNS cache lookups are working -- ipcache_nbgethostbyname()
and similar code may call back immediately if the item is in the
cache. Since I did not want to rewrite DNS code as well, I ended up
relaxing the ACLChecklist::matchAclList() code requirements, going a
step back to where we sometimes call ACLList::matches() twice for the
same ACL node.
Thus, it is probably possible to undo most of the external_acl.cc
changes. I left them in because I think they improve the quality of
the code and possibly fix a bug or two.
* Adjusted ACLMaxUserIP::match(), ACLProxyAuth::match(), and
ACLExternal::match() to explicitly stop ACL processing when an
exceptional state is discovered instead of just setting the current
answer and proceeding as if more ACLs could be checked. On the other
hand, we now do not set the answer if the corresponding internal
matching code (e.g., AuthenticateAcl()) needs an async operation
because we do not know the answer yet.
* Fixed HttpStateData::handle1xx() and
HttpStateData::finishingBrokenPost() to correctly handle
fastCheck(void) return values. They were assuming that there are only
two possible return values (ACCESS_DENIED/ALLOWED), potentially
subjecting more messages to invasive adaptations than necessary.
Amos Jeffries [Mon, 28 May 2012 02:40:52 +0000 (20:40 -0600)]
Define PRIuSIZE for displaying size_t
This allows us to avoid casting size_t to long long for printf.
./configure script auto-detects the supposedly standard %zu macro in case
it is missing and compat/types.h defines some alternatives for systems
which do not define it.
Alex Rousskov [Wed, 23 May 2012 23:34:49 +0000 (17:34 -0600)]
Fix protocol names in AnyP::PortCfg after http_port_list revamp in r12121.
The bug manifests itself when the URIs of intercepted requests are rewritten
into "https_port://..." strings, resulting in "Invalid port '0'" errors in
urlParse, followed by HTTP 400 (Bad Request) rejection.
There are other, more subtle cases where wrong PortCfg protocol matters.
Alex Rousskov [Wed, 23 May 2012 23:23:12 +0000 (17:23 -0600)]
Fix protocol names in AnyP::PortCfg after http_port_list revamp in r12121.
The bug manifests itself when the URIs of intercepted requests are rewritten
into "https_port://..." strings, resulting in "Invalid port '0'" errors in
urlParse followed by HTTP 400 (Bad Request) rejection.
There are other, more subtle cases where wrong PortCfg protocol matters.
Alex Rousskov [Tue, 8 May 2012 18:14:08 +0000 (12:14 -0600)]
Bug 3466: Adaptation stuck on last single-byte body piece
Changed StoreEntry::bytesWanted(range) to return range.end when the entry can
accommodate range.end bytes. This makes it possible to use that method for
single-byte ranges. Old code returned zero for such ranges, which was
difficult to distinguish from situations where no bytes were wanted at all.
TODO: The StoreEntry::bytesWanted(range) API is left undocumented because it
seems to be slightly broken and/or inconsistent with callers and with the
DelayId::bytesWanted(min, max) API. AFAICT, we should convert
StoreEntry::bytesWanted API from range-based to min/max-based or even just
max-based.
Store Entry API does not use the lower end of the range (except for the
now-removed assertion that the range is not empty). I suspect that Store API
was meant to be used with (first, last+1) "byte position" parameters (returning
the number of bytes wanted) while the DelayId API was meant to be used with
(min, max) "number of bytes" parameters. However, StoreEntry::bytesWanted
implementation does not follow this assumption so perhaps my speculation is
wrong and there are more problems, including this change.
Amos Jeffries [Sun, 6 May 2012 01:29:22 +0000 (19:29 -0600)]
Add support for TLSv1.1 and TLSv1.2 options and methods
When OpenSSL v1.0.1+ is being built against.
Also update the documentation for sslproxy_version which was not
mentioning what the supported version codes were.
Future work:
* make version config option(s) accept a set of named versions and
convert to codes internally.
* redesign how version and options are handled. Admin should be able to
just list the TLSv* wanted and Squid figure out the appropriate options
from there.
Alex Rousskov [Fri, 4 May 2012 22:21:44 +0000 (16:21 -0600)]
Re-enabled support for bump-client-first mode using enhanced ssl_bump option.
Even though bump-server-first is an overall better method, bumping the client
first is useful for backward compatibility and possibly for serving internal
Squid objects such as icons.
The code path implementing bump-client-first approach was preserved during the
bump-server-first changes, so we just needed to add a configuration option to
allow the admin to pick between two modes. We did that by using custom "mode"
keywords with the existing ssl_bump option. The old allow/deny pair of
standard keywords could not be used to select one of the two modes for an
"allowed" connection.
Alex Rousskov [Fri, 4 May 2012 22:17:15 +0000 (16:17 -0600)]
Allow for custom keywords in ACL lists (in addition to allow/deny).
Motivation: Ssl_bump used allow/deny keywords and standard ACL processing. We
wanted to change those keywords to specify how the connection should be bumped
(client-first, server-first, or not bumped at all).
We could implement that using a set of independent ACL lists, each with its
own keyword, similar to how request_header_access and many other options do
it. However, that would mean losing support for slow ACLs (or implementing
such support by hand). All current request_header_access-like options do not
support slow ACLs because generic ACL code does not preserve information about
which acl_access list has [slowly] matched and, hence, we cannot tell which
custom keyword (e.g., which header name) to use after the match.
To solve the problem, we converted allow_t enum into a class that can carry
the old matching result code (ACCESS_*) and a custom "match kind or keyword"
integer that the caller can optionally inspect if access is allowed (i.e., if
some acl_access list matched). When a squid.conf option is parsed into an
acl_access object, Squid sets the allow.kind field to the corresponding
keyword. When the ACL list is checked at runtime and there is a match, the
previously set allow.kind value is propagated to the final answer (XXX: not
implemented?) which the high-level caller can optionally check.
The old allow_t-using code does not need to be modified because the new
allow_t class allows for implicit conversion from and to ALLOW_* constants.
Caveat: When no ACLs match, the generic ACL code processing a chain of
acl_access rules will negate the last rule allow/deny keyword. Such "implicit"
rule would not carry any "matched keyword or kind" information (the _implicit_
last ACL list cannot have any explicit keywords). Thus, the high-level caller
should be careful when interpreting the answer to deal with such implicit
matches. Most likely, the high-level code must disclaim support for implicit
rules (in documentation) and treat any matches without keyword information as
if no rule has matched.
Eventually, the core ACL code can be reshaped so that the caller can control
whether implicit rules are allowed.
Bug fix: src ACL broken with sslproxy_cert_error, part2
Use the original CONNECT request instead of creating the fake request.
Set flags.sslPeek to mark the CONNECT request as the one used for peeking at
the origin server certificate. Forward.cc now use that flag when special
handling is needed.
This will allow as to use original request which includes CONNECT request
headers (eg X-Forwarded-For header) and other settings, with sslproxy_*
acl checks.
SourceLayout: port config and select-loop priority polishing
- renames http_port_list to AnyP::PortCfg
- de-duplicate https_port_list into AnyP::PortCfg
- shuffles related globals and defines into anyp/PortCfg.*
- renames MAXHTTPPORTS to MAXTCPLISTENPORTS to suit its actual coverage of HTTP and HTTPS ports.
- shuffled config port clone function into a method.
- rename ICP/HTCP/SNMP API functions to consistent *OpenPorts() and *ClosePorts()
NP:following applies to incoming_* and *_poll_cnt directives.
- renames *_icp_* to *_udp_*
- renames *_http_* to *_tcp_*
- shuffles duplicated struct SquidConf options into a shared structure
- shuffles related defines into comm/Loops.h
- documents options better
- various other cosmetic syntax tweaks and polish
One bug fix:
comm_dns_incoming was not being propigated in StatsHist copy/clone.
Now is. I seem to remember mention of something similar being zero before,
but can't find the bug report.
projects / thirdparty / squid.git / log
