Page MenuHomePhabricator

Security review of Tool Labs console application
Closed, ResolvedPublic

Description

Project Information

Description of the tool/project

Django-based web interface for management of Tool Labs related workflows.

The initial deployable application will:

  • Authenticate to LDAP using auth bind
  • Authenticate to SUL wikis using OAuth
  • Communicate with Phabricator using conduit APIs
  • Store association of LDAP, SUL and Phabricator accounts in local database
  • Create Diffusion hosted git repositories associated with Tool Labs tools

This will eventually be the single point of management for:

  • Becoming a member of the Tool Labs project
  • Creating new tool accounts
  • Managing tool account membership
  • Creating git repos related to tools
  • Managing metadata related to tools

See also

Description of how the tool will be used at WMF

See tool description.

Because this system will authenticate with LDAP and eventually edit some LDAP data it must be deployed in the WMF production network where it is safe to collect LDAP passwords and the LDAP editing credentials can be secured.

Dependencies

Various Python libraries (full list in requirements.txt):

  • Django
  • django-auth-ldap
  • django-ldapdb
  • mwoauth
  • requests

Has this project been reviewed before?

No

Working test environment

Post-deployment

The Community-Tech-Tool-Labs and Toolforge teams will be responsible for the application in WMF production with @bd808 being the initial maintainer and primary point of contact.

Event Timeline

bd808 created this task.May 19 2016, 11:13 PM
Restricted Application added a project: Cloud-Services. · View Herald TranscriptMay 19 2016, 11:13 PM
Restricted Application added subscribers: Zppix, Aklapper. · View Herald Transcript
bd808 updated the task description. (Show Details)May 25 2016, 10:58 PM
chasemp triaged this task as Normal priority.May 31 2016, 3:40 PM

@dpatrick and @Bawolff Can I get a guesstimate on when you might get to this review? The core code is complete for the initial needs and I'm trying to decide if I should tidy things up and start planning a deployment plan or continue on to add more features prior to review and deployment.

dpatrick added a comment.EditedJun 7 2016, 7:58 PM

@bd808 How does the week of July 4th and into part of the week of the 11th work for you? That's the earliest we can schedule this.

bd808 added a comment.Jun 7 2016, 8:10 PM

@bd808 How does the week of July 4th and into part of the week of the 11th work for you? That's the earliest we can schedule this.

Sounds great! I'll start planning for other follow on events accordingly.

bd808 moved this task from Backlog to Ready on the Community-Tech-Tool-Labs board.Jun 23 2016, 9:21 PM
bd808 added a comment.Jul 18 2016, 5:52 PM

@dpatrick any status update on this review?

Overall, really nice job on entire application. This is a great example of solid Django development. I'm really into striker/settings.py and the use of an ini file. The app's handling of trusted proxies, CSP, CSRF protection by default, rate-limiting connections to LDAP--all great.

Non-security issues:

  • Vagrant tools cannot find Python.h; needs to depends on -dev package
  • Database calls fails if roles is enabled prior to first "vagrant up"

Non-security questions:

  • Are tools in Tool Labs now required to only use Diffusion? In other words, do tool repos already hosted elsewhere (Github) have to be imported to Diffusion?

striker/tools/models.py, line 78

  • fix TODO for field lengths

striker/labsauth/views.py, line 48

  • Should session expiry be an ini-based config parameter?
bd808 added a comment.Jul 18 2016, 8:51 PM

Overall, really nice job on entire application. This is a great example of solid Django development. I'm really into striker/settings.py and the use of an ini file. The app's handling of trusted proxies, CSP, CSRF protection by default, rate-limiting connections to LDAP--all great.

Thanks. :)

I tried several different config management ideas and the ini-file one seemed like it would be easiest to manage via Puppet, plus I like the added bonus of non-executable config. That's one aspect of Django that has always bugged me a little.

Non-security issues:

  • Vagrant tools cannot find Python.h; needs to depends on -dev package
  • Database calls fails if roles is enabled prior to first "vagrant up"

Good catches. I'll touch up the mw-vagrant role and try to get these fixed.

Non-security questions:

  • Are tools in Tool Labs now required to only use Diffusion? In other words, do tool repos already hosted elsewhere (Github) have to be imported to Diffusion?

No. The intent here is just to make it really simple to setup version control for a tool. Using some other VCS (github, bitbucket, whatever) is fine.

I would like to add support for mirroring externally hosted git VCS into Diffusion to make discovery of source code easier for folks who want to help fix up an existing tool. It would also be nice to add mirroring out to github for folks who like to keep track of their contributions there. Neither of these seemed necessary for the initial release functionality however.

striker/tools/models.py, line 78

  • fix TODO for field lengths

Thanks for reminding me. I punted on that one during dev but should be able to at least make a lower limit for the phid I think.

striker/labsauth/views.py, line 48

  • Should session expiry be an ini-based config parameter?

That sounds like a good idea. That will make tuning things easier if we decide 2 week sessions are too long/short.

bd808 added a comment.Jul 19 2016, 4:47 AM
  • Vagrant tools cannot find Python.h; needs to depends on -dev package
  • Database calls fails if roles is enabled prior to first "vagrant up"

https://gerrit.wikimedia.org/r/#/c/299708/

bd808 added a comment.Jul 19 2016, 5:07 AM

striker/labsauth/views.py, line 48

  • Should session expiry be an ini-based config parameter?

https://github.com/bd808/striker/commit/ba766d19919ef4edaeb9dd7da4d33580cea43d2a

bd808 moved this task from Backlog to Doing on the Striker board.
bd808 added a comment.Jul 22 2016, 5:12 PM

@dpatrick Can we call this closed now, or are there other issues that you would like to see addressed?

bd808 added a comment.Aug 9 2016, 11:06 PM

@dpatrick Can we call this closed now, or are there other issues that you would like to see addressed?

ping?

dpatrick closed this task as Resolved.Aug 9 2016, 11:30 PM

Yep, you can consider it resolved. Sorry for the delay!

bd808 moved this task from Doing to Done on the Striker board.Mar 4 2017, 3:57 AM