On 06/11/2013 02:14 PM, Amos Jeffries wrote:
> On 11/06/2013 10:16 p.m., Tsantilas Christos wrote:
>> The log_icap and log_access are not really needed.
>> The users have acls control for access and icap logging using the
>> access_log and icap_log configuration directives.
>>
>> This patch removes these options from configuration file.
>>
>>
>> This is a Measurement Factory project
>
> Overall I like it. Removing code and a frequent source of user confusion
> in one patch.
>
> Please also note in the descriptive message that this alters the
> documented behaviour of "Requests denied for logging will also not be
> accounted for in performance counters.", and now makes all traffic get
> performane counters accounting.
Are we OK with this?
>
> Also, this is not exactly backward-compatible. We *do* have a number of
> installations using log_access to restrict their logging for limited
> storage spaces which will suddenly get much more log data and face DoS
> effects sometime after an upgrade. This will need to have a small code
> snippet in src/parse_cf.cc function parse_obsolete() which detects at
> minimum log_access, maybe log_icap, and uses self_destruct() to
> highlight the change.
OK.
> in src/adaptation/icap/icap_log.cc:
> * including HttpReply.h pulls in HttpMsg.h. No need to #include both.
fixed
> * FilledChecklist can be created as local variables. Please remove the
> unnecessary new/delete in icapLogLog()
done
> + this also goes for the src/client_side.cc checklist.
well this is not possible here...
>
> - is it really necessary to send dash_str as the IDENT username? doing
> so completely breaks 'ident' ACLs on icap_log.
Yes. This is true. My sense is that using the dash_str as IDENT
username, just used to speedup the fast acl checking.
In client_side.cc code inside clientAclChecklistCreate function it is
used as follows:
- If it is already exist in ConnStateData::clientConnection::rfc931
then use it else do not spend time to find it (from cache or lookup).
Probably this logic should be moved inside ident acl, or let ident acl
to do its job. I had this dilemma when started this patch, but at the
end I let it untouched.
In this version I just removed the dash_str from icap_log...
> This does appear to have been a problem inherited from maybeLog(), but
> now is a good time to fix it.
>
> in src/cf.data.pre:
> * directives of type "obsolete" have their DOC text turned into debugs() .
> Please replace the description with a short sentence informing the
> admin what to do to their config file. (one-liner if possible)
OK.
> * please remove the DEFAULT_DOC, DEFAULT, COMMENT, IFDEF and LOC lines
> which are now useless.
OK.
>
>
> Amos
>
=== modified file 'src/SquidConfig.h'
--- src/SquidConfig.h 2013-05-28 14:31:59 +0000
+++ src/SquidConfig.h 2013-06-10 13:49:37 +0000
@@ -356,69 +356,64 @@
int hostStrictVerify;
int client_dst_passthru;
} onoff;
int pipeline_max_prefetch;
int forward_max_tries;
int connect_retries;
class ACL *aclList;
struct {
acl_access *http;
acl_access *adapted_http;
acl_access *icp;
acl_access *miss;
acl_access *NeverDirect;
acl_access *AlwaysDirect;
acl_access *ASlists;
acl_access *noCache;
- acl_access *log;
#if SQUID_SNMP
acl_access *snmp;
#endif
#if USE_HTTP_VIOLATIONS
acl_access *brokenPosts;
#endif
acl_access *redirector;
acl_access *store_id;
acl_access *reply;
AclAddress *outgoing_address;
#if USE_HTCP
acl_access *htcp;
acl_access *htcp_clr;
#endif
#if USE_SSL
acl_access *ssl_bump;
#endif
#if FOLLOW_X_FORWARDED_FOR
acl_access *followXFF;
#endif /* FOLLOW_X_FORWARDED_FOR */
-#if ICAP_CLIENT
- acl_access* icap;
-#endif
-
/// spoof_client_ip squid.conf acl.
/// nil unless configured
acl_access* spoof_client_ip;
} accessList;
AclDenyInfoList *denyInfoList;
struct {
size_t list_width;
int list_wrap;
char *anon_user;
int passive;
int epsv_all;
int epsv;
int eprt;
int sanitycheck;
int telnet;
} Ftp;
RefreshPattern *Refresh;
struct _cacheSwap {
=== modified file 'src/adaptation/icap/Xaction.cc'
--- src/adaptation/icap/Xaction.cc 2013-06-11 10:04:48 +0000
+++ src/adaptation/icap/Xaction.cc 2013-06-11 10:05:58 +0000
@@ -1,39 +1,37 @@
/*
* DEBUG: section 93 ICAP (RFC 3507) Client
*/
#include "squid.h"
-#include "acl/FilledChecklist.h"
#include "adaptation/icap/Config.h"
#include "adaptation/icap/Launcher.h"
#include "adaptation/icap/Xaction.h"
#include "base/TextException.h"
#include "comm.h"
#include "comm/Connection.h"
#include "comm/ConnOpener.h"
#include "comm/Write.h"
#include "CommCalls.h"
#include "err_detail_type.h"
#include "fde.h"
#include "FwdState.h"
-#include "globals.h"
#include "HttpMsg.h"
#include "HttpReply.h"
#include "HttpRequest.h"
#include "icap_log.h"
#include "ipcache.h"
#include "Mem.h"
#include "pconn.h"
#include "SquidConfig.h"
#include "SquidTime.h"
//CBDATA_NAMESPACED_CLASS_INIT(Adaptation::Icap, Xaction);
Adaptation::Icap::Xaction::Xaction(const char *aTypeName, Adaptation::Icap::ServiceRep::Pointer &aService):
AsyncJob(aTypeName),
Adaptation::Initiate(aTypeName),
icapRequest(NULL),
icapReply(NULL),
attempts(0),
connection(NULL),
theService(aService),
@@ -533,50 +531,42 @@
Adaptation::Initiate::swanSong();
}
void Adaptation::Icap::Xaction::tellQueryAborted()
{
if (theInitiator.set()) {
Adaptation::Icap::XactAbortInfo abortInfo(icapRequest, icapReply.getRaw(),
retriable(), repeatable());
Launcher *launcher = dynamic_cast<Launcher*>(theInitiator.get());
// launcher may be nil if initiator is invalid
CallJobHere1(91,5, CbcPointer<Launcher>(launcher),
Launcher, noteXactAbort, abortInfo);
clearInitiator();
}
}
void Adaptation::Icap::Xaction::maybeLog()
{
if (IcapLogfileStatus == LOG_ENABLE) {
- ACLFilledChecklist *checklist = new ACLFilledChecklist(::Config.accessList.icap, al.request, dash_str);
- if (al.reply) {
- checklist->reply = al.reply;
- HTTPMSGLOCK(checklist->reply);
- }
- if (!::Config.accessList.icap || checklist->fastCheck() == ACCESS_ALLOWED) {
- finalizeLogInfo();
- icapLogLog(alep, checklist);
- }
- delete checklist;
+ finalizeLogInfo();
+ icapLogLog(alep);
}
}
void Adaptation::Icap::Xaction::finalizeLogInfo()
{
//prepare log data
al.icp.opcode = ICP_INVALID;
const Adaptation::Icap::ServiceRep &s = service();
al.icap.hostAddr = s.cfg().host.termedBuf();
al.icap.serviceName = s.cfg().key;
al.icap.reqUri = s.cfg().uri;
al.icap.ioTime = tvSubMsec(icap_tio_start, icap_tio_finish);
al.icap.trTime = tvSubMsec(icap_tr_start, current_time);
al.icap.request = icapRequest;
HTTPMSGLOCK(al.icap.request);
if (icapReply != NULL) {
al.icap.reply = icapReply.getRaw();
=== modified file 'src/adaptation/icap/icap_log.cc'
--- src/adaptation/icap/icap_log.cc 2013-05-11 20:59:44 +0000
+++ src/adaptation/icap/icap_log.cc 2013-06-11 13:01:28 +0000
@@ -1,23 +1,26 @@
#include "squid.h"
#include "icap_log.h"
#include "AccessLogEntry.h"
+#include "acl/FilledChecklist.h"
+#include "HttpReply.h"
+#include "globals.h"
#include "log/CustomLog.h"
#include "log/File.h"
#include "log/Formats.h"
#include "SquidConfig.h"
int IcapLogfileStatus = LOG_DISABLE;
void
icapLogOpen()
{
CustomLog *log;
for (log = Config.Log.icaplogs; log; log = log->next) {
if (log->type == Log::Format::CLF_NONE)
continue;
log->logfile = logfileOpen(log->filename, log->bufferSize, log->fatal);
IcapLogfileStatus = LOG_ENABLE;
}
@@ -29,25 +32,31 @@
CustomLog *log;
for (log = Config.Log.icaplogs; log; log = log->next) {
if (log->logfile) {
logfileClose(log->logfile);
log->logfile = NULL;
}
}
}
void
icapLogRotate()
{
for (CustomLog* log = Config.Log.icaplogs; log; log = log->next) {
if (log->logfile) {
logfileRotate(log->logfile);
}
}
}
-void icapLogLog(AccessLogEntry::Pointer &al, ACLChecklist * checklist)
+void icapLogLog(AccessLogEntry::Pointer &al)
{
- if (IcapLogfileStatus == LOG_ENABLE)
- accessLogLogTo(Config.Log.icaplogs, al, checklist);
+ if (IcapLogfileStatus == LOG_ENABLE) {
+ ACLFilledChecklist checklist(NULL, al->adapted_request, NULL);
+ if (al->reply) {
+ checklist.reply = al->reply;
+ HTTPMSGLOCK(checklist.reply);
+ }
+ accessLogLogTo(Config.Log.icaplogs, al, &checklist);
+ }
}
=== modified file 'src/adaptation/icap/icap_log.h'
--- src/adaptation/icap/icap_log.h 2012-10-29 04:59:58 +0000
+++ src/adaptation/icap/icap_log.h 2013-06-10 15:22:20 +0000
@@ -1,18 +1,18 @@
#ifndef ICAP_LOG_H_
#define ICAP_LOG_H_
#include "AccessLogEntry.h"
#include "base/RefCount.h"
typedef RefCount<AccessLogEntry> AccessLogEntryPointer;
class AccessLogEntry;
class ACLChecklist;
void icapLogClose();
void icapLogOpen();
void icapLogRotate();
-void icapLogLog(AccessLogEntryPointer &al, ACLChecklist * checklist);
+void icapLogLog(AccessLogEntryPointer &al);
extern int IcapLogfileStatus;
#endif /*ICAP_LOG_H_*/
=== modified file 'src/cache_cf.cc'
--- src/cache_cf.cc 2013-06-05 15:02:11 +0000
+++ src/cache_cf.cc 2013-06-11 13:21:10 +0000
@@ -994,40 +994,46 @@
}
#endif
}
/** Parse a line containing an obsolete directive.
* To upgrade it where possible instead of just "Bungled config" for
* directives which cannot be marked as simply aliases of the some name.
* For example if the parameter order and content has changed.
* Or if the directive has been completely removed.
*/
void
parse_obsolete(const char *name)
{
// Directives which have been radically changed rather than removed
if (!strcmp(name, "url_rewrite_concurrency")) {
int cval;
parse_int(&cval);
debugs(3, DBG_CRITICAL, "WARNING: url_rewrite_concurrency upgrade overriding url_rewrite_children settings.");
Config.redirectChildren.concurrency = cval;
}
+
+ if (!strcmp(name, "log_access"))
+ self_destruct();
+
+ if (!strcmp(name, "log_icap"))
+ self_destruct();
}
/* Parse a time specification from the config file. Store the
* result in 'tptr', after converting it to 'units' */
static void
parseTimeLine(time_msec_t * tptr, const char *units, bool allowMsec)
{
char *token;
double d;
time_msec_t m;
time_msec_t u;
if ((u = parseTimeUnits(units, allowMsec)) == 0)
self_destruct();
if ((token = strtok(NULL, w_space)) == NULL)
self_destruct();
d = xatof(token);
=== modified file 'src/cf.data.pre'
--- src/cf.data.pre 2013-06-06 14:21:25 +0000
+++ src/cf.data.pre 2013-06-11 13:06:37 +0000
@@ -4029,63 +4029,49 @@
TYPE: string
DEFAULT: @DEFAULT_LOGFILED@
LOC: Log::TheConfig.logfile_daemon
DOC_START
Specify the path to the logfile-writing daemon. This daemon is
used to write the access and store logs, if configured.
Squid sends a number of commands to the log daemon:
L<data>\n - logfile data
R\n - rotate file
T\n - truncate file
O\n - reopen file
F\n - flush file
r<n>\n - set rotate count to <n>
b<n>\n - 1 = buffer output, 0 = don't buffer output
No responses is expected.
DOC_END
NAME: log_access
-TYPE: acl_access
-LOC: Config.accessList.log
-DEFAULT: none
-DEFAULT_DOC: Allow logging for all transactions.
-COMMENT: allow|deny acl acl...
+TYPE: obsolete
DOC_START
- This options allows you to control which requests gets logged
- to access.log (see access_log directive). Requests denied for
- logging will also not be accounted for in performance counters.
-
- This clause only supports fast acl types.
- See http://wiki.squid-cache.org/SquidFaq/SquidAcl for details.
+ Remove this line. Use acls with access_log directives to control access logging
DOC_END
NAME: log_icap
-TYPE: acl_access
-IFDEF: ICAP_CLIENT
-LOC: Config.accessList.icap
-DEFAULT: none
-DEFAULT_DOC: Allow logging for all ICAP transactions.
+TYPE: obsolete
DOC_START
- This options allows you to control which requests get logged
- to icap.log. See the icap_log directive for ICAP log details.
+ Remove this line. Use acls with icap_log directives to control icap logging
DOC_END
NAME: cache_store_log
TYPE: string
DEFAULT: none
LOC: Config.Log.store
DOC_START
Logs the activities of the storage manager. Shows which
objects are ejected from the cache, and which objects are
saved and for how long.
There are not really utilities to analyze this data, so you can safely
disable it (the default).
Store log uses modular logging outputs. See access_log for the list
of modules supported.
Example:
cache_store_log stdio:@DEFAULT_STORE_LOG@
cache_store_log daemon:@DEFAULT_STORE_LOG@
DOC_END
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2013-06-07 08:49:36 +0000
+++ src/client_side.cc 2013-06-10 15:02:03 +0000
@@ -681,59 +681,57 @@
* to snarf the ssl details some place earlier..
*/
if (getConn() != NULL)
al->cache.ssluser = sslGetUserEmail(fd_table[getConn()->fd].ssl);
#endif
/*Add notes*/
// The al->notes and request->notes must point to the same object.
// Enable the following assertion to check for possible bugs.
// assert(request->notes == al->notes);
typedef Notes::iterator ACAMLI;
for (ACAMLI i = Config.notes.begin(); i != Config.notes.end(); ++i) {
if (const char *value = (*i)->match(request, al->reply)) {
NotePairs ¬es = SyncNotes(*al, *request);
notes.add((*i)->key.termedBuf(), value);
debugs(33, 3, HERE << (*i)->key.termedBuf() << " " << value);
}
}
- ACLFilledChecklist *checklist = clientAclChecklistCreate(Config.accessList.log, this);
+ ACLFilledChecklist *checklist = clientAclChecklistCreate(NULL, this);
if (al->reply) {
checklist->reply = al->reply;
HTTPMSGLOCK(checklist->reply);
}
- if (!Config.accessList.log || checklist->fastCheck() == ACCESS_ALLOWED) {
- if (request) {
- al->adapted_request = request;
- HTTPMSGLOCK(al->adapted_request);
- }
- accessLogLog(al, checklist);
- if (request)
- updateCounters();
-
- if (getConn() != NULL && getConn()->clientConnection != NULL)
- clientdbUpdate(getConn()->clientConnection->remote, logType, AnyP::PROTO_HTTP, out.size);
+ if (request) {
+ al->adapted_request = request;
+ HTTPMSGLOCK(al->adapted_request);
}
+ accessLogLog(al, checklist);
+ if (request)
+ updateCounters();
+
+ if (getConn() != NULL && getConn()->clientConnection != NULL)
+ clientdbUpdate(getConn()->clientConnection->remote, logType, AnyP::PROTO_HTTP, out.size);
delete checklist;
}
void
ClientHttpRequest::freeResources()
{
safe_free(uri);
safe_free(log_uri);
safe_free(redirect.location);
range_iter.boundary.clean();
HTTPMSGUNLOCK(request);
if (client_stream.tail)
clientStreamAbort((clientStreamNode *)client_stream.tail->data, this);
}
void
httpRequestFree(void *data)
{
