Bug in captive portal accounting with multiple radius servers
-
Hello All,
both pfSense 1.2.3 and 2.0 captive portal support authentication against multiple radius servers. We are currently running pfSense 1.2.3-RELEASE but the following argument also applies to 2.0.
under the captive portal authentication section in the webadmin admins have the option of entering 1 or 2 radius servers against which to authenticate. Admins are also given the option to authenticate every minute and also to send accounting updates. When configuring for interim accounting updates we notice that only the primary radius ever received accounting updates while authentication can happen at either radius sever. In otherwords… if the primary radius server goes down authentication continues to happen on the secondary radius sever ** BUT ** accounting updates are never sent to this second server. Once the primary server comes back on line then accounting updates resume.
Looking at the captive portal code the reason is obvious. Here is the prototype for radius authentication
function RADIUS_AUTHENTICATION($username,$password,$radiusservers,$clientip,$clientmac,$ruleno) {the function takes an array of radiusservers as an argument. Up to 10 radius servers can be configured. The radius code uses round-robin to access the servers starting with the first one. So... if the primary fails the code goes on to the next server. The code tries 3 times to access the server with a timeout of 3 seconds. So if in 9 seconds there is no response from the radius server the client moves on to the next.
Here is the prototype for the authentication function
function RADIUS_ACCOUNTING_START($ruleno,$username,$sessionid,$radiusip,$radiusport,$radiuskey,$clientip,$clientmac) {Notice that this function only takes one radius server as an argument! Unlike radius authentication the radius accounting function only supports one host.
Looking at the code in /usr/local/captiveportal/index.php and in /etc/inc/captiveportal.inc we find
index.php...
if (isset($config['captiveportal']['radacct_enable']) && isset($radiusservers[0])) {
$acct_val = RADIUS_ACCOUNTING_START($ruleno,
$username,
$sessionid,
$radiusservers[0]['ipaddr'],
$radiusservers[0]['acctport'],
$radiusservers[0]['key'],
$clientip,
$clientmac);captiveportal.inc …
/* do periodic RADIUS reauthentication? */
if (!$timedout && isset($config['captiveportal']['reauthenticate']) &&
($radiusservers !== false)) {if (isset($config['captiveportal']['radacct_enable'])) {
if ($config['captiveportal']['reauthenticateacct'] == "stopstart") {
/* stop and restart accounting */
RADIUS_ACCOUNTING_STOP($cpdb[$i][1], // ruleno
$cpdb[$i][4], // username
$cpdb[$i][5], // sessionid
$cpdb[$i][0], // start time
$radiusservers[0]['ipaddr'],
$radiusservers[0]['acctport'],
$radiusservers[0]['key'],
$cpdb[$i][2], // clientip
$cpdb[$i][3], // clientmac
10); // NAS Request
exec("/sbin/ipfw zero {$cpdb[$i][1]}");
RADIUS_ACCOUNTING_START($cpdb[$i][1], // ruleno
$cpdb[$i][4], // username
$cpdb[$i][5], // sessionid
$radiusservers[0]['ipaddr'],
$radiusservers[0]['acctport'],
$radiusservers[0]['key'],
$cpdb[$i][2], // clientip
$cpdb[$i][3]); // clientmac
} else if ($config['captiveportal']['reauthenticateacct'] == "interimupdate") {
RADIUS_ACCOUNTING_STOP($cpdb[$i][1], // ruleno
$cpdb[$i][4], // username
$cpdb[$i][5], // sessionid
$cpdb[$i][0], // start time
$radiusservers[0]['ipaddr'],
$radiusservers[0]['acctport'],
$radiusservers[0]['key'],
$cpdb[$i][2], // clientip
$cpdb[$i][3], // clientmac
10, // NAS Request
true); // Interim Updates
}
}/* check this user against RADIUS again */
$auth_list = RADIUS_AUTHENTICATION($cpdb[$i][4], // username
base64_decode($cpdb[$i][6]), // password
$radiusservers,
$cpdb[$i][2], // clientip
$cpdb[$i][3], // clientmac
$cpdb[$i][1]); // rulenonote that accounting data is only sent to $radiusservers[0]['ipaddr'] i.e. the primary RADIUS sever.
The obvious solution is to modify RADIUS_ACCOUNTING_START/STOP to accept an array of radius servers in the same way that RADIUS_AUTHENTICATION does.
Here is the code in radius_authentication.inc for RADIUS_AUTHENTICATION() supporting multiple servers
/*
Add support for more then one radiusserver.
At most 10 servers may be specified.
When multiple servers are given, they are tried in round-robin fashion until a valid response is received
*/foreach ($radiusservers as $radsrv) {
// Add a new server to our instance
$rauth->addServer($radsrv['ipaddr'], $radsrv['port'], $radsrv['key']);}
here is the code in radius_accounting.inc for RADIUS_ACCOUNTING_START()
// Create our instance
$racct = new Auth_RADIUS_Acct_Start;// Construct data package
$racct->username = $username;
$racct->addServer($radiusip, $radiusport, $radiuskey);it seems that since Anth_RADIUS_Acct_Start is derived from Auth_RADIUS that the same
addServer scheme can be used for accounting as for authentication. Here is the addServer method in Auth_RADIUS which supports multiple RADIUS servers.
/**
* Adds a RADIUS server to the list of servers for requests.
*
* At most 10 servers may be specified. When multiple servers
* are given, they are tried in round-robin fashion until a
* valid response is received
*
* @access public
* @param string $servername Servername or IP-Address
* @param integer $port Portnumber
* @param string $sharedSecret Shared secret
* @param integer $timeout Timeout for each request
* @param integer $maxtries Max. retries for each request
* @return void
*/
function addServer($servername = 'localhost', $port = 0, $sharedSecret = 'testing123', $timeout = 5, $maxtries = 3)
{
$this->_servers[] = array($servername, $port, $sharedSecret, $timeout, $maxtries);
}I will hack up the code in the next few days to see if I can get this to work…
Not sending accounting updates to the secondary/backup RADIUS server is a pretty serious omission if the the accounting info is being used as part of the authentication process.
--luis
-
The following patches fix the captive portal so that it uses multiple radius servers for accounting.
–- captiveportal.inc 2010-12-25 00:22:13.000000000 +0000
+++ captiveportal.inc.new 2011-01-02 08:28:20.000000000 +0000
@@ -540,9 +562,7 @@
$cpdb[$i][4], // username
$cpdb[$i][5], // sessionid
$cpdb[$i][0], // start time
- $radiusservers[0]['ipaddr'],
- $radiusservers[0]['acctport'],
- $radiusservers[0]['key'],
+ $radiusservers,
$cpdb[$i][2], // clientip
$cpdb[$i][3], // clientmac
10); // NAS Request
@@ -550,9 +570,7 @@
RADIUS_ACCOUNTING_START($cpdb[$i][1], // ruleno
$cpdb[$i][4], // username
$cpdb[$i][5], // sessionid
- $radiusservers[0]['ipaddr'],
- $radiusservers[0]['acctport'],
- $radiusservers[0]['key'],
+ $radiusservers,
$cpdb[$i][2], // clientip
$cpdb[$i][3]); // clientmac
} else if ($config['captiveportal']['reauthenticateacct'] == "interimupdate") {
@@ -560,9 +578,7 @@
$cpdb[$i][4], // username
$cpdb[$i][5], // sessionid
$cpdb[$i][0], // start time
- $radiusservers[0]['ipaddr'],
- $radiusservers[0]['acctport'],
- $radiusservers[0]['key'],
+ $radiusservers,
$cpdb[$i][2], // clientip
$cpdb[$i][3], // clientmac
10, // NAS Request
@@ -573,7 +589,7 @@
/* check this user against RADIUS again */
$auth_list = RADIUS_AUTHENTICATION($cpdb[$i][4], // username
base64_decode($cpdb[$i][6]), // password
- $radiusservers,
+ $radiusservers,
$cpdb[$i][2], // clientip
$cpdb[$i][3], // clientmac
$cpdb[$i][1]); // ruleno
@@ -605,9 +621,7 @@
$dbent[4], // username
$dbent[5], // sessionid
$dbent[0], // start time- $radiusservers[0]['ipaddr'],
- $radiusservers[0]['acctport'],
- $radiusservers[0]['key'],
- $radiusservers,
$dbent[2], // clientip
$dbent[3], // clientmac
$term_cause, // Acct-Terminate-Cause
@@ -678,9 +692,7 @@
$cpdb[$i][4], // username
$cpdb[$i][5], // sessionid
$cpdb[$i][0], // start time
- $radiusservers[0]['ipaddr'],
- $radiusservers[0]['acctport'],
- $radiusservers[0]['key'],
- $radiusservers,
$cpdb[$i][2], // clientip
$cpdb[$i][3], // clientmac
7); // Admin Reboot
@@ -844,7 +856,11 @@
if ($line) {
$radsrv = array();
list($radsrv['ipaddr'],$radsrv['port'],$radsrv['acctport'],$radsrv['key']) = explode(",",$line);
- $radiusservers[] = $radsrv; - // make sure server is available before adding to the list
- $stat = mwexec("ping -t 7 -oqc 3 -i 0.5 {$radsrv['ipaddr']} 2>1 ");
- if ( $stat == 0 ) {
+ $radiusservers[] = $radsrv; - }
}
}
fclose($fd);
Note that the ping is not required. However, it makes things go a little faster since the captive portal would otherwise have to timeout numerous times to get the reports to the backup radius server.
–- radius_accounting.inc 2011-01-02 07:38:16.000000000 +0000
+++ radius_accounting.inc.new 2011-01-02 07:52:15.000000000 +0000
@@ -43,7 +43,7 @@*/
-function RADIUS_ACCOUNTING_START($ruleno,$username,$sessionid,$radiusip,$radiusport,$radiuskey,$clientip,$clientmac) {
+function RADIUS_ACCOUNTING_START($ruleno,$username,$sessionid,$radiusservers,$clientip,$clientmac) {global $config;
@@ -81,7 +81,19 @@
// Construct data package
$racct->username = $username;
- $racct->addServer($radiusip, $radiusport, $radiuskey);+ /*
+ Add support for more then one radiusserver.
+ At most 10 servers may be specified.
+ When multiple servers are given, they are tried in round-robin fashion until a valid response is received
+ */
+
+ foreach ($radiusservers as $radsrv) {
+
+ // Add a new server to our instance
+ $racct->addServer($radsrv['ipaddr'], $radsrv['port'], $radsrv['key']);
+
+ }if (PEAR::isError($racct->start())) {
$retvalue['acct_val'] = 1;
@@ -146,7 +158,7 @@
–---------------------------
*/-function RADIUS_ACCOUNTING_STOP($ruleno,$username,$sessionid,$start_time,$radiusip,$radiusport,$radiuskey,$clientip,$clientmac, $term_cause = 1, $interimupdate=false,$stop_time = null) {
+function RADIUS_ACCOUNTING_STOP($ruleno,$username,$sessionid,$start_time,$radiusservers,$clientip,$clientmac, $term_cause = 1, $interimupdate=false,$stop_time = null) {global $config;
@@ -181,26 +193,25 @@
else
$racct = new Auth_RADIUS_Acct_Stop;+ // See RADIUS_ACCOUNTING_START for info
+ $racct->authentic = RADIUS_AUTH_RADIUS;+ // Construct data package
+ $racct->username = $username;/*
- * Currently disabled
- Add support for more then one radiusserver.
- At most 10 servers may be specified.
- When multiple servers are given, they are tried in round-robin fashion until a valid response is received
+ Add support for more then one radiusserver.
+ At most 10 servers may be specified.
+ When multiple servers are given, they are tried in round-robin fashion until a valid response is received
+ */foreach ($radiusservers as $radsrv) {
+
// Add a new server to our instance
$racct->addServer($radsrv['ipaddr'], $radsrv['port'], $radsrv['key']);+
}
- */- // See RADIUS_ACCOUNTING_START for info
- $racct->authentic = RADIUS_AUTH_RADIUS;- // Construct data package
- $racct->username = $username;
- $racct->addServer($radiusip, $radiusport, $radiuskey);
// Set session_time
$racct->session_time = $session_time;@@ -306,4 +317,4 @@
}
-?>
\ No newline at end of file
+?>–- index.php 2011-01-02 07:25:55.000000000 +0000
+++ index.php.new 2011-01-02 07:28:52.000000000 +0000
@@ -321,9 +321,7 @@
$acct_val = RADIUS_ACCOUNTING_START($ruleno,
$username,
$sessionid,
- $radiusservers[0]['ipaddr'],
- $radiusservers[0]['acctport'],
- $radiusservers[0]['key'],
+ $radiusservers,
$clientip,
$clientmac);
-
I belive you are looking only at 1.2.3 code!