[defect] access-accept after failed checkrad run

tarelda commented 1 year ago

What type of defect/bug is this?

Unexpected behaviour (obvious or verified by project member)

How can the issue be reproduced?

I use customized rlm_sql configuration and I have modified checkrad (it is API compatibile) to my needs to facilitate stale session delete. Tested scenario is as follows:

User credentials are used by device A.
Device B tries to use them also, but fails simultaneous check - simul_count_query.
Freeradius proceeds with simul_verify_query and it also fails.
Freeradius as expected moves to checkrad check, but connection to NAS is disturbed and checkrad fails with error code 2.
Device B session gets accepted.

I believe expected result is to issue Access-Reject and wait for another login attempt.

Log output from the FreeRADIUS daemon

FreeRADIUS Version 3.0.26
Copyright (C) 1999-2021 The FreeRADIUS server project and contributors
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE
You may redistribute copies of FreeRADIUS under the terms of the
GNU General Public License
For more information about these matters, see the file named COPYRIGHT
Starting - reading configuration files ...
<non relevant>
(3) # Executing section session from file /etc/freeradius/servers.d/default
(3)   session {
rlm_sql (sql_acct_old): Reserved connection (3)
(3) sql_acct_old: EXPAND SELECT COUNT(*) FROM radacct WHERE UserName='%{User-Name}' AND AcctStopTime = 0
(3) sql_acct_old:    --> SELECT COUNT(*) FROM radacct WHERE UserName='TEST' AND AcctStopTime = 0
(3) sql_acct_old: Executing select query: SELECT COUNT(*) FROM radacct WHERE UserName='TEST' AND AcctStopTime = 0
(3) sql_acct_old: EXPAND SELECT RadAcctId, AcctSessionId, UserName,       NASIPAddress, NASPortId, FramedIPAddress, CallingStationId, FramedProtocol       FROM radacct WHERE UserName='%{User-Name}' AND AcctStopTime = 0
(3) sql_acct_old:    --> SELECT RadAcctId, AcctSessionId, UserName,       NASIPAddress, NASPortId, FramedIPAddress, CallingStationId, FramedProtocol       FROM radacct WHERE UserName='TEST' AND AcctStopTime = 0
(3) sql_acct_old: Executing select query: SELECT RadAcctId, AcctSessionId, UserName,       NASIPAddress, NASPortId, FramedIPAddress, CallingStationId, FramedProtocol       FROM radacct WHERE UserName='TEST' AND AcctStopTime = 0
checkrad.py called
checkrad.py called
invalid user name or password (6)
(3) sql_acct_old: ERROR: Failed to check the terminal server for user 'TEST'.
rlm_sql (sql_acct_old): Released connection (3)
(3)     [sql_acct_old] = fail
(3)   } # session = reject
(3) # Executing section post-auth from file /etc/freeradius/servers.d/default
(3)   post-auth {
(3) sql_lms: EXPAND .query
(3) sql_lms:    --> .query
(3) sql_lms: Using query template 'query'
rlm_sql (sql_lms): Reserved connection (3)
(3) sql_lms: EXPAND UPDATE nodes SET lastonline = unix_timestamp() WHERE name='%{User-Name}'
(3) sql_lms:    --> UPDATE nodes SET lastonline = unix_timestamp() WHERE name='TEST'
(3) sql_lms: Executing query: UPDATE nodes SET lastonline = unix_timestamp() WHERE name='TEST'
rlm_sql_mysql: Rows matched: 1  Changed: 1  Warnings: 0
(3) sql_lms: SQL query returned: success
(3) sql_lms: 1 record(s) updated
rlm_sql (sql_lms): Released connection (3)
(3)     [sql_lms] = ok
(3)   } # post-auth = ok
(3) Sent Access-Accept Id 199 from 192.168.3.26:1812 to 192.168.3.88:35635 length 39
(3)   Mikrotik-Rate-Limit = "0k/0k"
(3)   Framed-IP-Address = <redacted>
(3) Finished request

Relevant log output from client utilities

No response

Backtrace from LLDB or GDB

No response

alandekok commented 1 year ago

Unfortunately this is how it's worked since we first implemented the checkrad function. It is very difficult to change this behavior now, as millions of sites depend on the existing behavior.

You can fix this by using a site-local policy. Something like this:

session {
   sql_acct_old {
     fail = 1
   }
   if (fail) {
     update control {
         Post-Auth-Type := Reject
     }
   }

and that should work.

tarelda commented 1 year ago

Unfortunately this is how it's worked since we first implemented the checkrad function. It is very difficult to change this behavior now, as millions of sites depend on the existing behavior.

You can fix this by using a site-local policy. Something like this:
session {
   sql_acct_old {
     fail = 1
   }
   if (fail) {
     update control {
         Post-Auth-Type := Reject
     }
   }
and that should work.

Sorry for the digging this up, but I have one follow up question regarding related behaviour. Can I ask Freeradius to restrict zapping to sessions that are on the same NAS? I mean request NAS has to be exact the same as NAS in accounting database?

tarelda commented 1 year ago

@alandekok Also your snippet produced really bizzare result:

rlm_sql (sql_acct_old): Released connection (1)
(6)     [sql_acct_old] = fail
(6)     if (fail) {
(6)     if (fail)  -> TRUE
(6)     if (fail)  {
(6)       update control {
(6)         Post-Auth-Type := Reject
(6)       } # update control = noop
(6)     } # if (fail)  = noop
(6)   } # session = noop
(6) Using Post-Auth-Type Reject
(6) # Executing group from file /etc/freeradius/servers.d/default
(6)   Post-Auth-Type REJECT {
(6) attr_filter.access_reject: EXPAND %{User-Name}
(6) attr_filter.access_reject:    --> TEST
(6) attr_filter.access_reject: Matched entry DEFAULT at line 11
(6)     [attr_filter.access_reject] = updated
(6)   } # Post-Auth-Type REJECT = updated
(6) Sent Access-Accept Id 224 from 192.168.3.26:1812 to 192.168.3.88:55446 length 20

FreeRADIUS / freeradius-server