On 25/09/12 17:25, Matthew Newton wrote:
I've been looking at the code recently to also see if the Post-Auth REJECT in inner-tunnel can be fixed. I can see an easy and fairly obvious of doing it, but the right way seems to involve the core event system, where I don't really want to go. That would fix up the one thing that is missing from our logs (outer reject doesn't log inner username, so it's hard to find these). I could then stop relaying outer auths to the central log entirely, as they're generally uninteresting.
[moving to -devel] Ah I remember this. It's because rad_authenticate calls rad_postauth if authentication succeeds, but relies on request_finish to call rad_postauth in the case of failure. I did think about this myself; one option is to call rad_postauth manually if rad_authenticate(fake) fails in peap.c - which I guess is the easy/obvious solution you're referring to? Certainly preferable to the current situation IMO. The "Post-Auth-Type REJECT" stuff did originally live in rad_authenticate - it was removed in commit 47a090897a. Not sure what the rationale was - something to do with reject_delay? - so I was unwilling to fiddle. Alan, can you comment?
On Tue, Sep 25, 2012 at 06:08:04PM +0100, Phil Mayers wrote:
On 25/09/12 17:25, Matthew Newton wrote:
I've been looking at the code recently to also see if the Post-Auth REJECT in inner-tunnel can be fixed. I can see an easy and fairly obvious of doing it, but the right way seems to involve
Ah I remember this. It's because rad_authenticate calls rad_postauth if authentication succeeds, but relies on request_finish to call rad_postauth in the case of failure.
That's the one.
I did think about this myself; one option is to call rad_postauth manually if rad_authenticate(fake) fails in peap.c - which I guess is the easy/obvious solution you're referring to? Certainly preferable to the current situation IMO.
Yes. I'm just testing that to see if it behaves as expected - will post a patch for discussion if it seems OK.
The "Post-Auth-Type REJECT" stuff did originally live in rad_authenticate - it was removed in commit 47a090897a. Not sure what the rationale was - something to do with reject_delay? - so I was unwilling to fiddle. Alan, can you comment?
I guess reject_delay makes sense - assuming that post_auth should be delayed until the same time as the reply packet. I'm not knowledgable enought to know why the two should happen at the same time (e.g. why post_auth can't happen a little while before the actual reject is sent back). Assuming that is the reason, anyway. However, for a virtual server, there won't be a reject delay, so that's not a problem. Looking at the code, rad_authenticate seems to have a lot of legacy stuff in it, and seems to have quite a "jumpy" code path (lots of ifs that can jump over each other). Especially calling post_auth on success, but not on failure, seems the wrong thing to do - it would be better (IMHO) for consistency to not call post_auth at all, and leave it up to the calling function to handle it. Maybe it could do with a bit of a tidy? I'll try and have a look. 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 Tue, Sep 25, 2012 at 09:49:53PM +0100, Matthew Newton wrote:
On Tue, Sep 25, 2012 at 06:08:04PM +0100, Phil Mayers wrote:
I did think about this myself; one option is to call rad_postauth manually if rad_authenticate(fake) fails in peap.c - which I guess is the easy/obvious solution you're referring to? Certainly preferable to the current situation IMO.
Yes. I'm just testing that to see if it behaves as expected - will post a patch for discussion if it seems OK.
Righto, finished hacking something together on master - pushed it to https://github.com/mcnewton/freeradius-server/commits/rad_virtual_server There are three small commits - 1. move rad_postauth (for accept) from the end of the rad_authenticate function to next to the rad_postauth called for reject in process.c. This means that rad_authenticate never calls rad_postauth - it always has to be called explicitly. 2. Add a new function, 'rad_virtual_server', which does everything needed to run a virtual server. In this case, calling rad_authenticate, and then rad_postauth. It could be easily expanded in future to also handle acct packets, although I can't quite think of a reason to at the moment (maybe for the rlm_inject idea?...) 3. Update PEAP/EAP_TLS/TTLS to call rad_virtual_server, rather than rad_authenticate. This means the inner-tunnels get a post-auth REJECT as well. I've tested it with successes and failures for PAP, EAP-TLS (check-eap-tls), PEAP and EAP-TTLS (inner-tunnel), and all work as expected. The only thing I'm slightly unsure about is (1) - it seems to behave as expected, and seems the right place to put it, but I'm not 100%. On why rad_postauth was previously removed from rad_authenticate for rejects, my guess would be that there are so many places that rad_authenticate return for a reject, it would be called from several places. It's just tidier to do an if() and call it once after rad_authenticate has returned. rad_postauth is called immediately on failure, so reject_delay doesn't seem the right reason. But that's just my guess. 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>
Matthew Newton wrote:
There are three small commits -
It looks good. Send a pull request and I'll add it in.
I've tested it with successes and failures for PAP, EAP-TLS (check-eap-tls), PEAP and EAP-TTLS (inner-tunnel), and all work as expected.
What about proxying the inner-tunnel stuff? That probably still works, too.
The only thing I'm slightly unsure about is (1) - it seems to behave as expected, and seems the right place to put it, but I'm not 100%. On why rad_postauth was previously removed from rad_authenticate for rejects, my guess would be that there are so many places that rad_authenticate return for a reject, it would be called from several places. It's just tidier to do an if() and call it once after rad_authenticate has returned. rad_postauth is called immediately on failure, so reject_delay doesn't seem the right reason. But that's just my guess.
I don't recall. The server is getting a bit complicated. Part of the design goal of the new process.c state machine was to simplify it. I think I've been partially successful, but more work is required. Alan DeKok.
On 26/09/12 00:18, Matthew Newton wrote:
On Tue, Sep 25, 2012 at 09:49:53PM +0100, Matthew Newton wrote:
On Tue, Sep 25, 2012 at 06:08:04PM +0100, Phil Mayers wrote:
I did think about this myself; one option is to call rad_postauth manually if rad_authenticate(fake) fails in peap.c - which I guess is the easy/obvious solution you're referring to? Certainly preferable to the current situation IMO.
Yes. I'm just testing that to see if it behaves as expected - will post a patch for discussion if it seems OK.
Righto, finished hacking something together on master - pushed it to
https://github.com/mcnewton/freeradius-server/commits/rad_virtual_server
There are three small commits -
1. move rad_postauth (for accept) from the end of the rad_authenticate function to next to the rad_postauth called for reject in process.c. This means that rad_authenticate never calls rad_postauth - it always has to be called explicitly.
Out of curiosity, was there any reason you took this approach rather than moving rad_postauth back into rad_authenticate? The reason I ask is that rad_authenticate already calls the "authorize {}" and "authenticate {}" blocks, so symmetry would seem to suggest it call the "post-auth {}" block too. The other thing is that this change moves the execution context for the vast majority of post-auth calls (since most auths succeed). That might not matter, although I'm not sure about which thread request_finish executes in, or if it matters.
Phil Mayers wrote:
The reason I ask is that rad_authenticate already calls the "authorize {}" and "authenticate {}" blocks, so symmetry would seem to suggest it call the "post-auth {}" block too.
That might make sense, too. There are many ways of doing things. Some I have strong opinions about. Some I don't.
The other thing is that this change moves the execution context for the vast majority of post-auth calls (since most auths succeed). That might not matter, although I'm not sure about which thread request_finish executes in, or if it matters.
request_finish() should run in the same thread as rad_authenticate() Alan DeKok.
On Wed, Sep 26, 2012 at 10:26:15AM +0100, Phil Mayers wrote:
There are three small commits -
Out of curiosity, was there any reason you took this approach rather than moving rad_postauth back into rad_authenticate?
I was trying to simplify rad_authenticate, rather than make it more bloated - it's a very long function as it is. In the end it turned out to only be removing one call to rad_postauth, though. I've just looked at https://github.com/alandekok/freeradius-server/commit/47a090897a4b200b597f18... where it was removed - looks like the reason was to try and tidy the code a bit. I guess this reverses that slightly. The other thought was just so that there is a central function to call to handle virtual servers. This could easily also handle acct packets in the future, too.
The reason I ask is that rad_authenticate already calls the "authorize {}" and "authenticate {}" blocks, so symmetry would seem to suggest it call the "post-auth {}" block too.
Good point. I keep staring at that code and thinking that something needs to be done, but can't quite figure out what at the moment. Maybe that should be split, although it might not make things simpler, so may not be worth it. Most of that code has been untouched for 10 years or so, but just added to. 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 Wed, Sep 26, 2012 at 12:18:10AM +0100, Matthew Newton wrote:
On Tue, Sep 25, 2012 at 09:49:53PM +0100, Matthew Newton wrote:
On Tue, Sep 25, 2012 at 06:08:04PM +0100, Phil Mayers wrote:
I did think about this myself; one option is to call rad_postauth manually if rad_authenticate(fake) fails in peap.c - which I guess
Just FYI, I've backported it to 2.2.0 and tested it on one of our live servers. It make reject logs much more usable. The patch is here in case anyone's interested. It's only intended as a quick hack, so all in one commit. https://github.com/mcnewton/freeradius-server/commit/97caa34a0dd3dac9e49d20f... 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>
Phil Mayers wrote:
The "Post-Auth-Type REJECT" stuff did originally live in rad_authenticate - it was removed in commit 47a090897a. Not sure what the rationale was - something to do with reject_delay? - so I was unwilling to fiddle. Alan, can you comment?
Not sure. It's probably easier to just fix the problem. Alan DeKok.
On 25/09/12 18:08, Phil Mayers wrote:
The "Post-Auth-Type REJECT" stuff did originally live in rad_authenticate - it was removed in commit 47a090897a. Not sure what the rationale was - something to do with reject_delay? - so I was
Just to note, it's not particularly important, but it can't be that - have just noticed whilst investigating another thing, post-auth runs immediately. It's the packet sending that's delayed.
participants (3)
-
Alan DeKok -
Matthew Newton -
Phil Mayers