[patch] Add a RadiusAuthStore to AccountManagerPlugin

[Chris Shenton]

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

[Chris Shenton]

PATCH to add RADIUS auth to AccountManagerPlugin

[Chris Shenton]

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.

[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.

[anonymous]

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

[Steffen Hoffmann]

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?

[Steffen Hoffmann]

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

[Chris Shenton]

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.

[Steffen Hoffmann]

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.

[Chris Shenton]

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.

[Ryan J Ollos]

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.

[gilad]

This patch works great. I merged it into acct_mgr-0.4.4 in Trac 1.0.2.

I use with two factor authentication server (wikid). One problem is that sometimes the pre-existing wikid username does not match the pre-existing Trac username. Rather then force users to choose, I added a few more line to allow username aliasing.

The alias goes in session attribute for example

sid	authentication	name	value
trac_username	1	alias	wikid_user_name

The patch goes in web_ui.py at line 373 just above req.environ['REMOTE_USER'] = user

        #  Check if user is an alias
        db = self.env.get_db_cnx()
        cursor = db.cursor()
        cursor.execute("""
            SELECT  sid
            FROM    session_attribute
            WHERE   name='alias' AND value=%s
            """, (user,))
        for sid in cursor:
            user = "" + sid[0]

[Chris Shenton]

Steffenn Hoffmann wrote me in email:

we've got some recent activity on the ticket you opened years ago (#6788). Since account manager moved to BSD license I'm asking for your kind permission to put your patch​ under BSD license as well in order to be able to use it now and in the future. Your reply should ideally go to the Trac-Hack's ticket in question.

I'm happy to have my patch under the BSD license. Thanks!

[Steffen Hoffmann]

I've been reviewing the code including latest changes from GitHub and propose some further adjustments, including

• corrected auth store password check reject value FALSE instead of None
• password obfuscation for logging
• default RADIUS server port value according to RFC
• embedded configuration doc strings

Basic unit tests pending, testing of and feedback on my modifications welcomed.

[Steffen Hoffmann]

updated patch proposal for optional RADIUS auth store implementation

[Steffen Hoffmann]

I've pulled the updated patch published 4 days ago and propose this one instead, including light unit test coverage. More and stronger tests including real RADIUS challenges would be welcomed.

[Steffen Hoffmann]

In 14504:

AccountManagerPlugin: Provide optional support for Radius authentication, refs #6788.

Changes originate from a patch kindly contributed by Chris Shenton for
acct_mgr-0.2.1dev - long ago.

Thanks for taking care for this as well as for recent license change agreement.
It finally enabled merging the code including latest changes from GitHub and
further adjustments, including

• correct auth store password check reject value FALSE instead of NONE
• password and shared secret obfuscation for logging
• default RADIUS server port value according to RFC
• embedded configuration doc strings
• unit tests for RADIUS auth store

[Steffen Hoffmann]

I've not spend more time since preparing the patch proposal last year. Hope to get a bit more test feedback by pushing changes into the source - feedback welcome, but make sure to put it down here.