search

cloudflare / mitmengine

A MITM (monster-in-the-middle) detection tool. Used to build MALCOLM:
https://malcolm.cloudflare.com
BSD 3-Clause "New" or "Revised" License
799 stars 68 forks source link

Add s3 functionality #2

Closed gabbifish closed 5 years ago

gabbifish commented 5 years ago

This PR adds functionality and documentation for allowing contributors to add their own loaders; that is, configurations and encapsulated functionality for reading fingerprints and mitmheaders from databases in addition to textfiles. I chose to introduce a new loader package to make it easy to extend the loader interface, implement any sort of streaming or periodic data ingestion, and have it work out of the box with mitmengine (more details in the updated README).

This PR implements a loader for reading from S3-style databases (S3 and Ceph, for example). I also added unit and integration tests for ensuring that the S3 loader worked as expected.

Would love your feedback!

gabbifish commented 5 years ago

I definitely think I could improve s3_test.go... I'm just in a hard place because these tests require a working s3 config file, and I certainly don't want to upload mine. We could provide a disclaimer that the s3 tests are exclusively for checking that your own s3 configuration works?

gabbifish commented 5 years ago

Thanks for your feedback! I've implemented all the changes you've suggested--let me know if there's anything else to look at before we merge.

On the S3 note: I would be interested in setting up a dummy S3 bucket, but I do fear it could be easily abused. I've run all the S3 tests using our internal Ceph instance, however, and it's worked just fine :)

gabbifish commented 5 years ago

Oh, a quick followup: tests _TestProcessorKnownMitmFingerprints and _TestProcessorKnownBrowserFingerprints look like they're there to just make sure fingerprints are loaded as expected, but would break if I added new fingerprints to these files. Is that correct? I've commented out any invocation of these tests in the meantime.

lukevalenta commented 5 years ago

Oh, a quick followup: tests _TestProcessorKnownMitmFingerprints and _TestProcessorKnownBrowserFingerprints look like they're there to just make sure fingerprints are loaded as expected, but would break if I added new fingerprints to these files. Is that correct? I've commented out any invocation of these tests in the meantime.

Right, it's fine for these to be commented out. It was just something I added as a sanity check while I was debugging signatures. It just makes sure that the signatures actually match the fingerprints that they were derived from.