605733 - code audit for key exchange server code

Status: RESOLVED FIXED

(Cloud Services :: Server: Other, defect)

Description

[Mike Connor [:mconnor]]

This is a straightforward web app that is a standalone server.  Looking for issues around scaling, performance, and resistance to DoS.

Not sure who the best person is to do this.  Maybe one of the WebDev guys? cc-ing morgamic to see who might have cycles to do a pass.

Comment 1

[Tarek Ziadé (:tarek)]

The app is based on Ian Bicking WebOb and Paste, so he would be a good reviewer I guess.

The CI server is here: https://hudson.mozilla.org/job/KeyExchange/

There's a Makefile to create the app in a virtualenv:

  $ make build

The tests are run via nose:

  $ make test

The stress test uses Funkload. Run the app and make sure the IP filtering middleware is correctly set before running it:

  $ make bench

You can then get a report in html/ :

Comment 2

[Tarek Ziadé (:tarek)]

The app is based on Ian Bicking WebOb and Paste, so he would be a good reviewer I guess.

The CI server is here: https://hudson.mozilla.org/job/KeyExchange/

There's a Makefile to create the app in a virtualenv:

  $ make build

The tests are run via nose:

  $ make test

The stress test uses Funkload. Run the app and make sure the IP filtering middleware is correctly set before running it:

  $ make bench

You can then get a report in html/ :

  $ make bench_report

Comment 3

[Ian Bicking (:ianbicking)]

Should this review be specific to scaling/performance/DoS, or more general?

Comment 4

[Tarek Ziadé (:tarek)]

Scaling/Perf/DoS is our main focus for this service, but if you find other things to improve they're welcome of course.

Comment 5

[Ian Bicking (:ianbicking)]

I've completed the review.  Upon reflection, if there is a potential DoS issue I probably wouldn't notice it, it's not something I've had much experience with.

I put notes in comments (marked with "## ianb:") in a fork of the repo:

http://hg.mozilla.org/users/ibicking_mozilla.com/server-key-exchange-review/rev/641421526324

of course this shouldn't be pulled from, just read.

My one concern is with the IP blacklisting middleware -- it does work on every request, including contacting the memcached server.  I think it would be much better if the middleware itself only pushed IPs into the (in-memory) queue, and checked an in-memory set for blocked IPs.  This shouldn't impact the performance of the application itself.  Then a worker thread would actually look for abusing IPs and sync its data with memcached (pushing and periodically polling for updates).  If it's easy to just spawn a thread with uWSGI this should be fairly easy; in an environment like mod_wsgi it is sometimes not possible to safely start your own background thread.

Comment 6

[Tarek Ziadé (:tarek)]

Thanks for the review, Ian !

I agree that syncing w/ the memcached IP blacklist in the background will be more efficient. I'll work on this and see how this can be done so it works w/ mod_wsgi and uwsgi. We want to use NGinx but we need to make sure we can fallback on Apache in case there's any issue.

I have commented some of your comments in your clone. When not commented it means that I'll do the change.