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.
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.
The following Squid configuration uses src ACL with sslproxy_cert_error:
acl me src 172.16.101.51
sslproxy_cert_error allow me
Cache log shows that the source IP address is missing when the 'me' ACL
is checked for sslproxy_cert_error:
| ACL::checklistMatches: checking 'me'
| aclIpAddrNetworkCompare: compare: *[::]/[ff...ff] ([::])* vs ...
| aclIpMatchIp: '[::]' NOT found
The problem is that the HttpRequest::client_addr is not set, for the fake
HTTPS request created to initiate the bump-server-first procedure.
This patch trying to handle at least the following three cases when we are
reporting error details to the user and logging error details:
1) Shallow error: The same code discovers the error and creates the
error page. The request details will be in sync with the error page
details because they are discovered at the same time, by the same code.
2) Honored deep error: Somewhere deep inside, say, ICAP or DNS code, an
error was detected and detailed. The error condition/answer slowly and
asynchronously propagated to the place where the error page is being
created. We want to preserve that original deep detail if any or provide
the current high-level detail if no deep detail is available.
3) Bypassed deep error1 followed by error2: Somewhere deep inside, say,
ICAP or DNS code, error1 was detected and detailed. The error1 condition
started propagating up but the ICAP or DNS transaction was eventually
successfully retried. Later, a deep or shallow error2 was discovered.
The error1 detail becomes irrelevant when we started to retry the failed
transaction.
This patch:
- Reset the error details when ICAP transactions retried, adaptation services
retried or replaced by the fail-over service and when the forwarding code
retry the connection to the destication servers.
- On SSLBump errors the error details, logged in both master CONNECT request
plus the first tunnelled GET request. To achieve this the error details
of the bump-server-first fake request saved to the CONNECT HttpRequest
object, and the logging of CONNECT request delayed until we have the
bump-server-first answer (freeAllContexts called after SSL-Server answered).
- Fix the cases where we set custom error codes (internal squid error codes)
to ErrorState::xerrno member. This member is only for system errors.
- We should not set ErrorState::xerrno to system err number unless we know
that the current system error triggered the error page generation.
This patch sets this member only to the system errorno passed by squid
API (eg AsyncCalls API).
This is also fix a possible bug in gopher.cc subsystem.
- We are setting the HttpRequest:detailError inside ErrorState::BuildHttpReply
method, where we have all the information required to corrently build
HttpRequest error details.
Alex Rousskov [Fri, 20 Apr 2012 17:18:17 +0000 (11:18 -0600)]
Polished sslproxy_cert_sign and sslproxy_cert_adapt documentation.
Most importantly, we now explicitly document that sslproxy_cert_adapt stops
searching for other ACL matches within the same adaptation algorithm group
once the first matching sslproxy_cert_adapt is found within an adaptation
algorithm group.
Ssl certificate domain mismatch errors on IP-based URLs
The ssl::certDomainMismatch acl can not be used with ip-based urls. For example
let's assume that a user enters https://74.125.65.99/, using a Google IP addressin the URL instead of www.google.com. If the sslBump used with
"sslproxy_cert_error allow all" and "sslproxy_cert_adapt setCommonName ssl::certDomainMismatch"
the browser displays a browser "Server's certificate does not match the URL"
error.
This is because for all cases we have the ip address instead of the hostname
we are detecting the cert domain mismatch errors when the first GET request
comes. At the time the sslproxy_cert_adatp access list processed the error is
not detected yet.
For intercepted connections this is the desired behaviour.
This patch fix the ssl-bump-first to check for domain-mismatch errors while
retrieving the SSL certificate from the server, hoping that CONNECT is using
a user-entered address (a host name or a user-entered IP).
Someone can hit this assertion when have two "sslproxy_cert_adapt setCommonName"
configuration lines the first with parameter but the second without parameter:
sslproxy_cert_adapt setCommonName{toThisOne} AN_IP
sslproxy_cert_adapt setCommonName AN_IP
Inside ConnStateData::buildSslCertGenerationParams method inside the loop
for (sslproxy_cert_adapt *ca = Config.ssl_client.cert_adapt; ....) {...}
the second time the loop entered the param is NULL and never set because
certProperties.setCommonName is already set, so we hit the assertion.
* relay "Permanent Redirect" message on status line
* MAY cache these responses with heuristics
* accept this status as a redirect status from URL redirectors
Alex Rousskov [Tue, 10 Apr 2012 04:26:14 +0000 (22:26 -0600)]
Bug 3441: Part 3: Replace corrupted v1 swap.state with new v2 format.
A fix for bug 3408 changed the offset at which we start writing dirty
swap.state entries from StoreSwapLogHeader::record_size to StoreSwapLogHeader
size. However, the log-reading code still read the entries starting from the
old offset (which is required to remain compatible with how a clean swap.state
is written).
Wrong starting offset essentially means that the vast majority of read
swap.state entries were bogus. They could only match some real entry when 64*n
is divisible by 12 and perhaps when their random data just happened to match a
real entry. Part 2 of this bug fix (trunk r11995) started to pad the [dirty]
swap.state header to start entry writing at StoreSwapLogHeader::record_size
boundary.
Changes specific to Part 3:
Unfortunately, since old v1 logs could contain completely bogus entries as the
result of being read (at some point) from the wrong offset, we should not load
v1 logs any more (neither dirty nor clean because what looks clean now could
be based on a previously dirty and, hence, corrupted log). This forced us to
raise the swap.state format version from 1 to 2.
After this change, if a v1 swap log is detected, Squid ignores it and does a
from-directory rebuild as if no swap.state files were found.
Since we had to change swap.state format version, we also made log entry size
and composition the same across virtually all platforms; added checksums so
that a similar bug would not go unnoticed for so long (and would not result in
log corruption); and increased the size of time-related entries to avoid the
"year 2038" problem.
The swap log entries are still written to disk in host byte order.
We now also zero the [dirty] swap.state header padding to prevent random and
potentially sensitive garbage in logs.
Cache index rebuild kinds are now reported using the following three labels:
* Rebuild using a swap log created by Squid during clean shutdown: "clean log"
* Rebuild using a swap log accumulated by a running Squid: "dirty log"
* Rebuild using directory scan: "no log"
The first kind used to be reported as CLEAN and the other two as DIRTY rebuild.
Customers want Squid to detect domain mismatch as early as possible so
that Squid uses a minimal valid fake certificate and serves a
[customized] Squid error. Currently, the browser always displays the
built-in error even if CONNECT has a domain name.
This patch tells Squid to use CONNECT host name if not peeking or if it is not
an IP while peeking. It also sends SNI information if host name is not an IP.
Also on error pages use the server certificate CN as hostname only if the
CONNECT host name is not an IP address.
Add checks to assure that a cached certificate is valid for current request
Add checks in Ssl::certificateMatchesProperties to assure:
- The CN name of the cached certificate matches the requested CN
- "Not After" and "Not Before" fields of the cached certificate are valid
Ssl::CommonHostName and getOrganization functions moved to gadgets.cc to allow
use by ssl_crtd daemon
Amos Jeffries [Thu, 29 Mar 2012 09:22:41 +0000 (21:22 +1200)]
Polish: de-duplicate UDP port dialers
This create a Comm::UdpOpenDialer class which replaces the ICP, HTCP and
SNMP start-listening dialer classes. Their code was very close to
identical anyway.
ICP and HTCP can now also use the dialer Comm::Connection parameter
instead of assuming that the callback relates to the global incoming
port variable.
According the RFC 5280: "When extensions are used, as expected in this profile,
version MUST be 3 (value is 2)". This patch sets the generated certificates
version to 3 when the subjectAltName extension copied from mimicking certificate.
It is reported at least one case where squid crashed with segfault because the
signing certificate was NULL.
This patch:
- Add assertion checks inside buildSslCertGenerationParams and
Ssl::certificateMatchesProperties functions (a)to avoid segfaults
- In the case the signing certificate is not given in http_port configuration
or the given certificate filename was not valid, squid does not start.
- Creates the http_port_list::configureSslServerContext method and move
here the cache_cf.cc code which was responsible to initialize ssl contexts
and sslBump feature.
Honor the "deny" part of "foobar deny ACL" options - Temporary patch
When AuthenticateAcl() and aclMatchExternal() were converted to use extended
authentication ACL states (r11644 and r11645 dated 2011-08-14), the result of
those function calls was set as the current checklist answer. This was
incorrect because those functions do not make allow/deny decisions. They only
tell us whether the ACL part of the allow/deny rule matches. If there is a
match, the ACCESS_ALLOWED/ACCESS_DENIED answer depends on whether it is an
allow or deny rule.
For example, "http_access deny BadGuys" should deny access when the BadGuys
ACL matches, but it was allowing access instead.
Without authentication, bump-server-first CONNECT requests allow uncontrolled
SSL handhsakes with origin servers, which is not desirable if the proxy operatordoes not want to allow users to access external resources anonymously.
Authenticating CONNECT requests is troublesome because when CONNECT
authentication fails, the proxy has difficulties communicating details of the
error to the browser, due to security vulnerabilities discussed at
https://bugzilla.mozilla.org/show_bug.cgi?id=479880
This patch implements the following logic to allow for seamless authentication
of CONNECT requests in a bump-server-first setup:
- Process http_access. Authenticate CONNECT request if needed, which may
require several HTTP CONNECT exchanges. This should be already supported.
- If access is allowed, use Connect-To-Server-First (for bumped connections) or normal TCP tunneling (for regular connections). This should be already supported.
- If access is denied, check ssl_bump and delay the error (for bumped
connections) or serve the error immediately (for regular connections).
This needs work.
"Delaying the error" in this context means remembering the error, responding
with 200 Established, establishing a bumped secure connection with the client,
not connecting to the origin server at all, and serving the error to the client
when the first encapsulated request comes.
Alex Rousskov [Thu, 8 Mar 2012 01:50:04 +0000 (18:50 -0700)]
Do not assert if we fail to compose ssl_crtd request. Do blocking generation.
Users report assertions when OpenSSL fails to write a true server certificate
to to memory. Since that certificate is received from a 3rd party, we should
not assert that it is writeable. Besides, OpenSSL may have limitations/bugs
even if dealing with valid certificates.
If we fail to componse a request, we now try the good old blocking in-process
certificate generation.
Currently, it is not known what exactly causes OpenSSL to fail as we are
unable to trigger the assertion in a controlled test.
projects / thirdparty / squid.git / log
