Modify

Opened 8 years ago

Closed 10 months ago

#6788 closed enhancement (fixed)

[patch] Add a RadiusAuthStore to AccountManagerPlugin

Reported by: Chris Shenton Owned by: Steffen Hoffmann
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: needinfo radius authentication
Cc: Chris Shenton, Ryan J Ollos 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 (2)

AccountManagerPlugin.patch (6.0 KB) - added by Chris Shenton 8 years ago.
PATCH to add RADIUS auth to AccountManagerPlugin
20141229_radius-auth-store_revised.patch (12.4 KB) - added by Steffen Hoffmann 3 years ago.
updated patch proposal for optional RADIUS auth store implementation

Download all attachments as: .zip

Change History (18)

Changed 8 years ago by Chris Shenton

Attachment: AccountManagerPlugin.patch added

PATCH to add RADIUS auth to AccountManagerPlugin

comment:1 Changed 8 years ago by Chris Shenton

Owner: changed from Matt Good to anonymous
Status: newassigned
Summary: Adding RADIUS auth to AccountManagerPlugin (running code)[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 8 years ago by Odd Simon Simonsen

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 8 years ago by anonymous

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

comment:4 Changed 7 years ago by Steffen Hoffmann

Summary: [PATCH] Adding RADIUS auth to AccountManagerPlugin[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 7 years ago by Steffen Hoffmann

Owner: changed from anonymous to Steffen Hoffmann
Status: assignednew
Summary: [PATCH] Add a RadiusAuthStore to AccountManagerPlugin[patch] Add a RadiusAuthStore to AccountManagerPlugin

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

comment:6 Changed 7 years ago by 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.

comment:7 in reply to:  6 Changed 7 years ago by Steffen Hoffmann

Cc: Ryan J Ollos 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 7 years ago by 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.

comment:9 Changed 5 years ago by 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.

comment:10 Changed 3 years ago by 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

sidauthenticationnamevalue
trac_username1aliaswikid_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]

comment:12 Changed 3 years ago by 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!

Last edited 3 years ago by Steffen Hoffmann (previous) (diff)

comment:13 Changed 3 years ago by 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.

Changed 3 years ago by Steffen Hoffmann

updated patch proposal for optional RADIUS auth store implementation

comment:14 Changed 3 years ago by 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.

comment:15 Changed 2 years ago by 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

comment:16 Changed 2 years ago by 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.

comment:17 Changed 10 months ago by Ryan J Ollos

Resolution: fixed
Status: newclosed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Steffen Hoffmann.
The resolution will be deleted.

Add Comment


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

 
Note: See TracTickets for help on using tickets.