rlm_sql: default Acct-On/Off query for all backends is somewhat bogus
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi, the query states (in variants): ~ accounting_onoff_query = "\ ~ UPDATE ${acct_table1} \ ~ SET \ ~ acctstoptime = '%S', \ ~ acctsessiontime = unix_timestamp('%S') - \ ~ unix_timestamp(acctstarttime), \ ~ acctterminatecause = '%{Acct-Terminate-Cause}', \ ~ acctstopdelay = %{%{Acct-Delay-Time}:-0} \ ~ WHERE acctsessiontime = 0 \ ~ AND acctstoptime = NULL \ ~ AND nasipaddress = '%{NAS-IP-Address}' \ ~ AND acctstarttime <= '%S'" and the acctsessiontime = 0 is bad: acctsessiontime gets updated whenever an interim record updates the session. The job of the query should be to close all open sessions, not just all open sessions that have been opened recently enough to not have received an update yet. I suggest to change the WHERE to ~ WHERE acctstoptime = NULL \ ~ AND nasipaddress = '%{NAS-IP-Address}' \ ~ AND acctstarttime <= '%S'" (leaving out the acctsessiontime = 0 constraint) Greetings, Stefan Winter -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.4-svn0 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org iD8DBQFIFtC/+jm90f8eFWYRAi7SAJ9jFGjLwKl7yyDhSAvWukgKkd8hNQCeKjrj s4DcCitbUu0FIEx+slNwB/A= =qNWG -----END PGP SIGNATURE-----
Stefan Winter wrote: ...
The job of the query should be to close all open sessions, not just all open sessions that have been opened recently enough to not have received an update yet. I suggest to change the WHERE to ... (leaving out the acctsessiontime = 0 constraint)
Looks good to me. Comments? Alan DeKok.
Stefan Winter wrote:
... I suggest to change the WHERE to
~ WHERE acctstoptime = NULL \ ~ AND nasipaddress = '%{NAS-IP-Address}' \ ~ AND acctstarttime <= '%S'"
(leaving out the acctsessiontime = 0 constraint)
Updated. Please test. Alan DeKok.
Stefan is absolutely correct about this. This query is also broken because it does "WHERE acctstoptime = NULL" when it should do "WHERE acctstoptime IS NULL". This bug does not exist in the Postgresql or Oracle backends (which I use in favour of MySQL) so either I fixed them at some point in the past or someone else broke MySQL... Because I run heavily customised queries in production it has been a while since I took a close look at what we are shipping. I guess I should do so again. I have committed a fix for the MySQL NULL issue.. -Peter On Tue 29 Apr 2008, Stefan Winter wrote:
Hi,
the query states (in variants):
~ accounting_onoff_query = "\ ~ UPDATE ${acct_table1} \ ~ SET \ ~ acctstoptime = '%S', \ ~ acctsessiontime = unix_timestamp('%S') - \ ~ unix_timestamp(acctstarttime), \ ~ acctterminatecause = '%{Acct-Terminate-Cause}', \ ~ acctstopdelay = %{%{Acct-Delay-Time}:-0} \ ~ WHERE acctsessiontime = 0 \ ~ AND acctstoptime = NULL \ ~ AND nasipaddress = '%{NAS-IP-Address}' \ ~ AND acctstarttime <= '%S'"
and the acctsessiontime = 0 is bad: acctsessiontime gets updated whenever an interim record updates the session. The job of the query should be to close all open sessions, not just all open sessions that have been opened recently enough to not have received an update yet. I suggest to change the WHERE to
~ WHERE acctstoptime = NULL \ ~ AND nasipaddress = '%{NAS-IP-Address}' \ ~ AND acctstarttime <= '%S'"
(leaving out the acctsessiontime = 0 constraint)
Greetings,
Stefan Winter - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
-- Peter Nixon http://peternixon.net/
Hi, 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 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? alan
A.L.M.Buxey@lboro.ac.uk wrote:
Hi,
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
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?
Exactly the same problem but with buggy NAS, already complained bitterly to Alan D. His suggestion was sql { invalid = 2 } if (invalid) { ok } In the accounting detail reader instance. What you really want is if a section returns invalid, the packet is counted as processed. Arran
alan - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
Arran Cudbard-Bell wrote: ...
What you really want is if a section returns invalid, the packet is counted as processed.
Sounds good. Given the number of obvious mistakes I've made in the last few days, I'd like some other people to think over the side-effects, and comment. Alan DeKok.
A.L.M.Buxey@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?
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? Alan DeKok.
Hi,
A.L.M.Buxey@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
A.L.M.Buxey@lboro.ac.uk wrote: ...
rlm_sql.c
"stop packet with zero session length" as found
Ok... but how do you normally deal with that? In some cases, the server just doesn't respond to accounting packets. e.g. if there's a failure of the SQL database. But for the "detail" file reader, a failure of the SQL database means that it should keep re-trying. i.e. the "detail" file reader is a lot like a NAS. It will re-transmit accounting packets forever... until it's told to shut up.
1)main server sees accounting packet but simply dumps it to detail file
Exactly.
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....
Exactly.
2a) our chosen method is using postgresql to put the detail into database, so DB module is used (rlm_sql).
Yup.
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.
Because that's what a real NAS does. If the server doesn't respond to a NAS, then the NAS re-transmits until the sun runs out of fuel.
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? :-)
Yes. That's the design intent... because the detail file reader doesn't know what a "bad packet" is. *You* do. If you don't tell it "yes, this packet was dealt with", then it will keep bugging you until you tell it that the packet was dealt with. Maybe this has to be made more obvious in the configuration...
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?)
If a session has zero time, it's still a valid accounting packet. But a packet without Acct-Status-Type is non-compliant... and invalid.
- 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.
Maybe a sample policy would help: if ((!Acct-Status-Type) || (!Acct-Session-Time) || (Acct-Session-Session-Time == 0)) { ok } ' else { sql ... } Alan DeKok.
participants (5)
-
A.L.M.Buxey＠lboro.ac.uk -
Alan DeKok -
Arran Cudbard-Bell -
Peter Nixon -
Stefan Winter