Matthew Newton wrote:
Been hacking on (mainly) rad_check_password in auth.c. It's got sections marked as FIXME, and lots of it is duplicated in rlm_pap and rlm_chap.
Yeah. I meant to clean that up before 3.0. The code is pretty horrific.
There are a series of patches here:
https://github.com/mcnewton/freeradius-server/commits/auth_tidy
In order, they are:
5843221b3 - remove the large chunk of code that finds Crypt-Password, or User-Password or Cleartext-Password, sets Auth-Type to 'Crypt' or 'Local', and checks auth for these. It's completely duplicated in rlm_pap and rlm_chap, so unnecessary here. Still throws up warnings telling the user to fix their config to use pap or chap if Auth-Type has not been set (but Cleartext-Password or User-Password are set) before failing auth.
Sounds good.
e12867d57 - no longer copy User-Password to Cleartext-Password if the admin configured it wrong. a) it's unnecessary because the login checks here have been removed, and b) rlm_pap will use both anyway, so no need to copy. Still throws up the big !!! warning !!! about using Cleartext-Password instead.
Sounds good.
0cb1cb3cd - move the !!! warning !!! about User-Password from auth.c into rlm_pap.c, which is where it is checked. There's no reason for it to clutter up rad_check_password any more really.
Which means people don't see it if they edit the configs to remove rlm_pap. Oh well. I'm fine with that. I've been gradually trying to remove RADIUS functionality from the core for years.
c5350ba22 - tidy up comments in rad_check_password
OK.
fdd53ce23 - updates to rlm_pap.c - the (undocumented) encryption_scheme option was completely broken, so change 'scheme' to 'inst->sch' to fix that, and update it so that any forced encryption type with this option must compare with Cleartext-Password, not User-Password. Warn and fail if this is not the case.
Hmm... the encryption_scheme is a holdover from 1.1.x, I think.
The next step would be either to document the PAP encryption_scheme option correctly, or to remove it entirely - I'm not sure of its history, whether it has ever worked, and if it is wanted or not. It seems a potentially useful option, but maybe {type} has superseeded it. If it is removed, the rlm_pap.c could potentially be tidied up quite a lot.
It should be removed. The {crypt} etc. method is superior, and widely used. Send a pull request && I'll get it in. Alan DeKok.