Hi, More tidying - this time in rlm_pap. In chunks, it's at https://github.com/mcnewton/freeradius-server/commits/rlm_pap_tidy I've submitted a pull request; details for any comments below: faa17a14 - removed the 'encryption_scheme' option entirely, together with the worthless switch() at the bottom. a9d0b282 - the first update in getting rid of the spaghetti gotos. moves the crypt auth check into a separate function. patch looks messier than it is, as there's a lot of indentation changes. d71b7e3b - the main update - move each auth test into a separate small function, to drastically reduce the size of pap_authenticate() and get rid of all the gotos. (I'm not against gotos, but when they jump in and out of ifs and switch()s, it's really hard to follow the code path!) df48421a - previous introduced an few if()s inside the main attribute check loop, and returned within the loop. this tidies that up and moves that outside. also removes the reliance on RLM_MODULE_INVALID as a loop end test, which I wasn't keen on. 52b68318 - moves header-like stuff into rlm_pap.h It now uses a function pointers to select the right auth function depending on the config attribute found. I was slightly worried that calling functions on this might slow things down a bit. However, running 1000 auths in a tight loop (non-threaded server, debug off) shows essentially no difference on my system - over three runs, old code avg 23.516s, new code avg 23.464s. Cheers, Matthew -- Matthew Newton, Ph.D. <mcn4@le.ac.uk> Systems Architect (UNIX and Networks), Network Services, I.T. Services, University of Leicester, Leicester LE1 7RH, United Kingdom For IT help contact helpdesk extn. 2253, <ithelp@le.ac.uk>
On Fri, Sep 28, 2012 at 11:57:05PM +0100, Matthew Newton wrote:
More tidying - this time in rlm_pap. In chunks, it's at
As another thought, maybe rlm_pap should now also refuse to auth against a password in User-Password? I moved the warning over from auth.c, but pap still allows it to work. 3.0 would seem to be a good place to finally break this. I guess it's a balance between forcing the Right Thing, and the number of questions on freeradius-users... although I guess they will be many when 3.0 is released anyway. Matthew -- Matthew Newton, Ph.D. <mcn4@le.ac.uk> Systems Architect (UNIX and Networks), Network Services, I.T. Services, University of Leicester, Leicester LE1 7RH, United Kingdom For IT help contact helpdesk extn. 2253, <ithelp@le.ac.uk>
Matthew Newton wrote:
As another thought, maybe rlm_pap should now also refuse to auth against a password in User-Password? I moved the warning over from auth.c, but pap still allows it to work. 3.0 would seem to be a good place to finally break this.
It's probably a good idea. Doing the User-Password thing breaks many authentication types. It's time to NOT be backwards compatible with 10+ years of stupidity. Cleartext-Password has been around since 1.1.3. It's time people used it.
I guess it's a balance between forcing the Right Thing, and the number of questions on freeradius-users... although I guess they will be many when 3.0 is released anyway.
Hopefully the documentation will be clear on the subject. See raddb/README.md. It explains the upgrade process. Alan DeKok.
Hi, On Sat, Sep 29, 2012 at 06:58:00AM +0200, Alan DeKok wrote:
Matthew Newton wrote:
As another thought, maybe rlm_pap should now also refuse to auth against a password in User-Password? I moved the warning over from auth.c, but pap still allows it to work. 3.0 would seem to be a good place to finally break this.
It's probably a good idea. Doing the User-Password thing breaks many authentication types. It's time to NOT be backwards compatible with 10+ years of stupidity.
Cleartext-Password has been around since 1.1.3. It's time people used it.
On this basis, I've made a new patch which removes User-Password checking from rlm_pap auth entirely, and moves the warning from auth back to the autz (but then fails auth).
I guess it's a balance between forcing the Right Thing, and the number of questions on freeradius-users... although I guess they will be many when 3.0 is released anyway.
Hopefully the documentation will be clear on the subject. See raddb/README.md. It explains the upgrade process.
There's also a patch which adds a warning to the documentation that User-Password should be updated to Cleartext-Password. Both at https://github.com/mcnewton/freeradius-server/commits/rlm_pap_tidy Cheers, Matthew -- Matthew Newton, Ph.D. <mcn4@le.ac.uk> Systems Architect (UNIX and Networks), Network Services, I.T. Services, University of Leicester, Leicester LE1 7RH, United Kingdom For IT help contact helpdesk extn. 2253, <ithelp@le.ac.uk>
On 3 Oct 2012, at 11:53, Matthew Newton <mcn4@LEICESTER.AC.UK> wrote:
Hi,
On Sat, Sep 29, 2012 at 06:58:00AM +0200, Alan DeKok wrote:
Matthew Newton wrote:
As another thought, maybe rlm_pap should now also refuse to auth against a password in User-Password? I moved the warning over from auth.c, but pap still allows it to work. 3.0 would seem to be a good place to finally break this.
It's probably a good idea. Doing the User-Password thing breaks many authentication types. It's time to NOT be backwards compatible with 10+ years of stupidity.
Cleartext-Password has been around since 1.1.3. It's time people used it.
On this basis, I've made a new patch which removes User-Password checking from rlm_pap auth entirely, and moves the warning from auth back to the autz (but then fails auth).
I guess it's a balance between forcing the Right Thing, and the number of questions on freeradius-users... although I guess they will be many when 3.0 is released anyway.
Hopefully the documentation will be clear on the subject. See raddb/README.md. It explains the upgrade process.
There's also a patch which adds a warning to the documentation that User-Password should be updated to Cleartext-Password.
Both at https://github.com/mcnewton/freeradius-server/commits/rlm_pap_tidy
Patch looks good, pull request? -Arran
Matthew Newton wrote:
52b68318 - moves header-like stuff into rlm_pap.h
That's my only concern. Moving C structs into header files is bad form.
It now uses a function pointers to select the right auth function depending on the config attribute found. I was slightly worried that calling functions on this might slow things down a bit. However, running 1000 auths in a tight loop (non-threaded server, debug off) shows essentially no difference on my system - over three runs, old code avg 23.516s, new code avg 23.464s.
Yeah. It's better that way. The old code was... interesting. Alan DeKok.
participants (3)
-
Alan DeKok -
Arran Cudbard-Bell -
Matthew Newton