I've noticed what i think is a problem with the accounting part of the
sql module(s) (mysql in my case, but this applies to all of them).
The default sql-query defined for an accounting update packet is:
accounting_update_query = "UPDATE ${acct_table1} SET FramedIPAddress =
'%{Framed-IP-Address}' WHERE AcctSessionId = '%{Acct-Session-Id}' AND
UserName = '%{SQL-User-Name}' AND NASIPAddress= '%{NAS-IP-Address}'"
Note the "AND UserName = '%{SQL-User-Name}'" part.
When an accouting alive (Acct-Status-Type = Alive) packet arrives,
triggering the above query, SQL-User-Name is never set in rlm_sql.
This means that no rows will be matched in the radacct table, since the
username will be set to "", hence no update will be performed, no
counters will be updated, FramedIPAddress won't be set etc, which is a
Bad Thing (tm).
This does not affect start or stop accounting records.
The fix seems to be to modify freeradius/src/modules/rlm_sql/rlm_sql.c
rlm_sql_accounting():
Right after 'case PW_STATUS_ALIVE:'
if(sql_set_user(inst, request, sqlusername, 0) < 0)
return RLM_MODULE_FAIL;
The above is what is done for start and stop accounting packets.
But while i was looking at this i saw another problem, if sql_set_user
fails the rlm_sql module will return RLM_MODULE_FAIL (as seen above),
this can happen for start and stop accounting packets by default. This
doesn't seem to comply with the rfc's, since User-Name isn't a required
attribute in accounting packets. A RLM_MODULE_FAIL will result in no
reply being sent back to the NAS for the accounting request.
The best fix for all of this seems to be to only do a:
sql_set_user(inst, request, sqlusername, 0);
without the return on error for start, stop and alive accounting
packets. This will result in everything working fine whether the
User-Name attribute is included or not. Or, atleast better under some
circumstances and just as good in the general case.
This was tested under a vanilla 0.5, i also checked the sources from the
latest cvs snapshot, and the some seems to hold true there although i
didn't actually try compiling and running it with that.
Anyone with better knowledge of the sources care to comment on
this? I'll be happy to supply a patch against latest cvs if i'm not
completely of base.
--
Simon
-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/users.html
