Modify

Opened 4 years ago

Last modified 20 months ago

#6788 new enhancement

[patch] Add a RadiusAuthStore to AccountManagerPlugin

Reported by: chris@… Owned by: hasienda
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: needinfo radius authentication
Cc: chris@…, rjollos Trac Release: 0.11

Description

We use Trac in an enterprisey environment at NASA HQ that uses RSA two-factor token authentication. We'd like Trac to be able to authenticate against it, over it's RADIUS protocol interface. RADIUS is frequently used by ISP and network access systems (e.g., WiFi routers) so is likely to be available in larger shops.

I've tried mod_auth_radius in Apache, and that works, except that:

  • Logout doesn't work (without the JavaScript hacks to clear the browser auth cache)
  • Sessions never timeout despite the setting of the expiration value in mod_auth_radius, unless we protect the entire site so the RADIUS cookie is 'visible'
  • we can't support sites with anonymous and authenticated users with session timeouts since auth protects only the /login URL which is never returned to once authenticated.

So I've written an addition to AccountManagerPlugin (trunk) which allows you to authenticate from within Trac to a RADIUS server. I'm still testing but it seems to work.

It relies on the 'pyrad' library which is available on PyPi, so I've included that in the setup.py install_requires setting. I'm unaware of a less-intrusive way to do this.

Do you want this code, and if so, how should I integrate it with yours?

Right now I'm developing it on GitHub:

http://github.com/shentonfreude/AccountManagerPlugin_radius

Attachments (1)

AccountManagerPlugin.patch (6.0 KB) - added by chris@… 4 years ago.
PATCH to add RADIUS auth to AccountManagerPlugin

Download all attachments as: .zip

Change History (10)

Changed 4 years ago by chris@…

PATCH to add RADIUS auth to AccountManagerPlugin

comment:1 Changed 4 years ago by chris@…

  • Owner changed from mgood to anonymous
  • Status changed from new to assigned
  • Summary changed from Adding RADIUS auth to AccountManagerPlugin (running code) to [PATCH] Adding RADIUS auth to AccountManagerPlugin

I've attached a patch that adds RADIUS functionality to AccountManagerPlugin. We're now using this in production against an RSA token authentication service. The only gotcha is that the setup.py now requires the 'pyrad' library to be pulled in.

comment:2 Changed 4 years ago by osimons

I'm neither an author nor a user of Account Manager, and just looked at this ticket and patch by pure chance...

My question is really why you don't package this as a separate plugin? It is a rather special use case - much like the similar LDAP password store and some other alternative stores that are packaged as separate plugins. Your plugin can then require account manager and pyrad to be installed. That way you can maintain the code yourself and update it as you need to, and don't need to touch the account manager code at all. Looking at your patch you don't really touch the source other than integrate it in common setup to make it installable?

See LdapAuthStorePlugin for a similar situation.

comment:3 Changed 4 years ago by anonymous

That's a reasonable approach I hadn't considered, osimons. Thanks.

comment:4 Changed 4 years ago by hasienda

  • Summary changed from [PATCH] Adding RADIUS auth to AccountManagerPlugin to [PATCH] Add a RadiusAuthStore to AccountManagerPlugin

I see this one related to ticket #173.

Despite the last comment and osimons statement, that there's already enough independent code for a separate plugin, by now there is still no such thing like a RadiusAuthStorePlugin at t-h.o. Therefor I'm unsure how to proceed with this ticket. Comments?

comment:5 Changed 4 years ago by hasienda

  • Owner changed from anonymous to hasienda
  • Status changed from assigned to new
  • Summary changed from [PATCH] Add a RadiusAuthStore to AccountManagerPlugin to [patch] Add a RadiusAuthStore to AccountManagerPlugin

Would you like to push me into looking into this more closely? Please respond.

comment:6 follow-up: Changed 4 years ago by shentonfreude

I should package it as a plugin but simply haven't had the time to do so. If anyone else would like to pull my RSA-oriented code out of the patch and turn it into a plugin, that would be fine. I'm just not sure when I can get to it.

comment:7 in reply to: ↑ 6 Changed 4 years ago by hasienda

  • Cc rjollos added
  • Keywords needinfo added

Replying to shentonfreude:

I should package it as a plugin but simply haven't had the time to do so.

I understand partially, that it's largly time-constraint, but are you really determined by now to go with a plugin instead of integration into AccountManagerPlugin?

If anyone else would like to pull my RSA-oriented code out of the patch and turn it into a plugin, that would be fine. I'm just not sure when I can get to it.

If Q1 is clear to me and positive, then even I could do it for you to resolve this ticket here. But I've seen regular discussion of granularity vs. maintainability:

From a technical point of view there's nothing wrong with patches extending patches. Even the contrary, as this could help to spread the burden imposed on an one-for-all maintainer.

OTOH selecting, tracking changes and updating a bunch of plugins tends to become a nightmare for Trac admins. Regarding functionality/plugin integration we already have #1061 and #1600 asking to rather go towards the one-fits-all solution with AccountManagerPlugin.

comment:8 Changed 4 years ago by shentonfreude

No, not at all determined. As you say, it is easier for trac admins to not worry about downloading a dozen plugins. Since there already are a number of different auth backends in Trac, I don't see any reason not to include one for RADIUS.

On the other hand, if it's a rare requirement only required by lunatic fringe in big enterprises, then a plugin may make more sense to prevent code bloat. And to allow independent evolution of the backend plugin. But I don't have any plans to extend it at this point.

So either approach is fine, and my pref would be for for Trac folks to integrate it.

comment:9 Changed 20 months ago by rjollos

I'm not sure where this ticket is headed, but I noticed that there is a BeerWare license header in the patch. AccountManager now uses the 3-Clause BSD license, so it would be good to get confirmation from the original author that they are okay to license their contribution under 3-Clause BSD. Updating the patch with a new license header would be even better, but a confirmation on the ticket should be enough.

Add Comment

Modify Ticket

Action
as new .
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.