rlm_sql.c in 2.0.0-pre2

[Arran Cudbard-Bell]

Phil Mayers wrote:
>> Unfortunately whoever modified rlm_sql in cvs head chose a very 
>> inefficient querying system.
>>     
>
> So change it - stored procedures maybe?
>   
Stored procedures ... but then you lose the only advantage of using the 
SQL modules in authorisation, which is that it can evaluate the check 
items when assigning users to groups.
>> First you query to pull out group membership, second you query to get 
>> each groups check items, then to get each groups reply items ... It just 
>> doesn't scale when a users a member of lots of groups.
>>
>> Previously you pulled out all the records for all the groups a user was 
>> a member of in two queries, one for check items and one for reply items..
>>     
>
> Eh? I've got to strongly disagree with that - the old code was a
> DISASTROUS scheme. If you had 2 groups with check items:
>
> RESIDENCES:
>   check: Calling-Station-Id ~ 192.168.
>   reply: Filter-Id = resnet
>
> CONFERENCES:
>   check: Calling-Station-Id ~ 10.
>   reply: Filter-Id = conferencenet
>
> ...and "johndoe" was in BOTH, NEITHER of them would *ever* match.
> Merging the groups' check items was just idiotic. The new version is
> far, far better.
>   
I agree the old way of doing things was just plain broken, but splitting 
the querying scheme into two parts, while making processing the SQL 
result far easier, is inefficient.

You can pull out all rows relating to the groups a user is a member of, 
and process them on the RADIUS server using the group name as a key. 
This could be done in one query if you were feeling adventurous, or 
probably more easily with two.

SELECT * FROM `radgroupcheck`,`radusergroup` WHERE radusergroup.UserName 
= '%{User-Name}' AND radgroupcheck.GroupName = radusergroup.GroupName

SELECT * FROM `radgroupreply`,`radusergroup` WHERE radusergroup.UserName 
= '%{User-Name}' AND radgroupcheck.GroupName = radusergroup.GroupName

This is far more scalable... Think of a user who belongs to 5 groups, we 
don't know which groups will actually be valid so all have to be pulled 
down from the SQL server ...

Thats one radcheck
one radreply
one membership query.
5 check items queries.
5 reply items queries.

If you've configured your server very efficiently you'll need to grab 
these at least twice for PEAP...
So thats 26 Select queries every time that user authenticates... as 
opposed to 8, (or 4 without radcheck,radreply).

Another possible alternative improvement using the current scheme,would 
be to bring all the rows relating to a group down, then evaluate them, 
and only continue querying if fall-through was true, or the check items 
didn't match.
>
> Not sure what you mean by that?
>   
Only do group lookups as opposed to checking for the users existence 
radcheck and radreply...
Not everyone uses SQL for storing attributes in Authentication.....

We use it because it provides a flexible means of overriding defaults in 
the users file, and allows special authorisation groups to be added, and 
memberships to be changed, without restarting the server.
>> user lookups, SQL queries really are very expensive .
>>     
>
> Expensive: we're doing ~260k authentications a day, each involving at
> least 1 SQL SELECT and 1 SQL INSERT and we've no problems. Hardware is
> nothing massively silly - dual-proc DL380 running both the SQL and
> Radius server.
>
>   
Our SQL server is running on the campus webserver, it's horribly 
overloaded and horribly slow.

Your running your SQL server on the same box as the RADIUS server which 
gives you a significant advantage in that your queries aren't affected 
by network latency.

Don't get me wrong, I appreciate you fixing the horribly broken SQL 
code, I was just suggesting some things that might improve efficiency.

Thanks,
Arran