rlm_sql: default Acct-On/Off query for all backends is somewhat bogus

[A.L.M.Buxey at lboro.ac.uk]

Hi,

> A.L.M.Buxey at lboro.ac.uk wrote:
> > I got bit by this issue - and the code still expects that acctsessiontime
> > not NULL when a stop packet etc arrives.....have a work around
> > in my sites-enable/default which only fires up the detail module
> > if acct-session-time != 0      bad muju
> 
>   Err... which code?

rlm_sql.c 

"stop packet with zero session length" as found

1248-                                           if (acctsessiontime <= 0) {
1249:                                                   radius_xlat(logstr, sizeof(logstr), "stop packet with zero session length. [user '%{User-Name}', nas '%{NAS-IP-Address}']", request, NULL);
1250-                                                   radlog(L_ERR, "rlm_sql (%s) in sql_accounting: %s", inst->config->xlat_name, logstr);
1251-                                                   sql_release_socket(inst, sqlsocket);
1252-                                                   ret = RLM_MODULE_NOOP;
1253-                                           }
1254-#endif


> > anyway, found another issue whilst away.  we are getting
> > accounting packets thrown through the detail module
> > thta dont actually have any accouting status type etc. 
> > looks like they've been stripped out by the remote
> > RADIUS server..... so the accouting packet doesnt have
> > anything useful inside.  unfortunately, when the
> > detail module hits these it just complains bitterly
> > all the time. rather than taking them for the duff
> > packet that they are.  any chance that the code can
> > see the naff packet for what it is and just ditch it?
> 
>   The detail module, or the accounting subsystem?

rlm_sql.c  has the following - which looks like what is being hit...

1075-  * Find the Acct Status Type
1076-  */
1077- if ((pair = pairfind(request->packet->vps, PW_ACCT_STATUS_TYPE)) != NULL) {
1078-         acctstatustype = pair->vp_integer;
1079- } else {
1080:         radius_xlat(logstr, sizeof(logstr), "packet has no accounting status type. [user '%{User-Name}', nas '%{NAS-IP-Address}']", request, NULL);
1081-         radlog(L_ERR, "rlm_sql (%s) in sql_accounting: %s",
1082-                inst->config->xlat_name, logstr);
1083-         return RLM_MODULE_INVALID;
1084- }


the logic-flow as a see it is.... 

1)main server sees accounting packet but simply dumps it to detail file

2)the second virtual server sucks in the detail file when the server
is not busy (working much better with 2.0.4 now delay is changed) 
and then uses the chosen method to handle it....

2a) our chosen method is using postgresql to put the detail into
database, so DB module is used (rlm_sql). 

however, i fear that the problem is that the detail code reads in
the detail file in possibly a too basic way - and when rlm_sql
detects a problem instead of just ditching the packet like
the real live server would, the detail module keeps just reading
the same one in...over and over and over. 10Gb error log file
and a very busy thread for no purpose. 

so, is my understanding of the logic correct and from a quick
parse/view of the detail/listener code would my theory that
a bad packet just gets continually munged rather than being
dropped away hold any water? :-)

as far as code and any side effects...... the main server would
have just ditched the dodgy packet (or am I wrong?) - 

ret = RLM_MODULE_NOOP  (for session length)
return RLM_MODULE_INVALID;  (for accounting status)

(ps why different return type structures?)

- so therefore if the 'out of band' method using a detail
file was to do the same, then its the same behaviour.
the only problem is that the out of band was designed
for times when the database was down, slow or problematic..
so therefore you'd have to distinguish between fail codes
due to database issues - ie keep the packet and retry.... - 
and fail codes due to bad packets in the detail file - which
need to be ditched.

alan