Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turning OWTF into a Python package #875

Merged
merged 66 commits into from
Aug 29, 2017
Merged

Turning OWTF into a Python package #875

merged 66 commits into from
Aug 29, 2017

Conversation

viyatb
Copy link
Member

@viyatb viyatb commented Aug 19, 2017

This PR is the first of many steps to refactoring OWTF and make it installable as a Python package.
While most of the changes are self-explanatory, here are a couple of notes on the work:

  • Including non-Python files like templates, plugin files, or any directory which is not a Python module is a PITA. I had to add the correct paths to the MANIFEST.in file to correct that.
  • The new web interface was moved to its separate directory (this was done in an earlier commit). We need to deprecate the old method of rendering templates using Tornado.
  • I completely removed Zest, PlugnHack, WafBypasser and Proxy miner support. We need addons support in OWTF so that optional features can be easily plugged in.
  • I have renamed configuration to conf to separate it from the other config folder.
  • The OWTF current install runs a post installation step in python setup.py install. I removed the virtualenv setup completely, since now it is the user's job to run python setup.py install in a separate virtualenv for maximum compatibility.
  • Added Sphinx docstrings to almost every function and module in OWTF
  • Convert all function names to snake case.
  • All code is now compatible with Python3 and Python2
  • Fix tests
  • Refactor installation method to install everything to ~/.owtf.
  • Add Debian packaging scripts
  • Better Makefile

Steps for installation

(I have not yet uploaded the package to PyPi, yet!)

  1. Create a new virtualenv, virtualenv <env> and activate the environment.
  2. Go into OWTF directory and run python setup.py install which install OWTF as a package and starts the postsetup install script.
  3. To run OWTF, make a new folder for your target engagement, and run OWTF as python -m owtf.

After this, OWTF should no longer be responsible for

  • running Postgresql on startup (user's job!)
  • virtualenv management (users should use it by default for separate projects)

Let me know if there are any questions.

@viyatb viyatb requested a review from DePierre August 19, 2017 14:32
@DePierre
Copy link
Contributor

@viyatb Very happy to see such changes :)

I won't be able to review anything until starting next week though. I hope you won't mind :s Even then it might take time to review so many changes, although most are simply moving things around.

I'm surprised regarding your comment about the PITA of including non-python files. I thought that it was fairly easy but I'm surely remembering wrong. I'll have to check next week see what I can find.

@viyatb
Copy link
Member Author

viyatb commented Aug 19, 2017

@DePierre no problem, I'll merge it after fixing the tests. This goes on develop branch, so it is not a big deal (the branch is by default unstable and should have breaking changes).
re: adding non-Python files, I had not done this before, so I had to look it up and read the docs lol.

@saganshul
Copy link
Contributor

saganshul commented Aug 19, 2017

@viyatb .gitsubmodule will also need changes.
Also one question, can we move owtf/webui folder out of owtf because I don't think python module will require this?

@viyatb
Copy link
Member Author

viyatb commented Aug 19, 2017

@saganshul re gitmodule: We discussed this on Slack, we'll have to move it to the installation part (download the templates into ~/.owtf)
re webui: IMO the web interface is the one of the main parts of the tool, so it should come bundled with the package.

@saganshul
Copy link
Contributor

saganshul commented Aug 19, 2017

@viyatb re gitmodule: yeah! we can do that.

So basically, if we have plans to render everything using ReactJS so there are two ways of working in webpack environment. ( I will remove the gulp completely because webpack will be able to handle development things.

  • Development: That will need to install node_modules, webpack etc. These things are only required if we are developing UI. Webpack comes if integrated development environment and working with react-redux is a ++. Only requirement is to render everything using React. https://webpack.github.io/docs/webpack-dev-server.html. Hence using this things development becomes very easier, webpack automatically detects changes using socket and only build for changes.
    Also using https://webpack.github.io/docs/hot-module-replacement.html makes ReactJS development a charm.

  • Production: In production environment we actually don't need any of JS from /owtf/webui/src/. We will just need bundle.js, bundle.css to work with. If some user is not developing UI, he/she doesn't need to install any node_modules. Also if we are providing owtf as python package, I think we should just ship latest bundle.js and bundle.css(after we move to scss).

So moving webui/src/ directory actually make sense. IMO if user is installing/building python module, installer should not build any of JS module. Let me know your thoughts on the same.

@viyatb
Copy link
Member Author

viyatb commented Aug 19, 2017

@saganshul You are right. The only problem there is we would have to add a hack to get the path before the owtf package is installed and add that to the UI config so that it builds the bundle.js and adds it to the appropriate path. It would be messy.

@saganshul
Copy link
Contributor

@viyatb tell me one thing, after this PR how will user launch the UI?

@viyatb
Copy link
Member Author

viyatb commented Aug 19, 2017

@saganshul a normal run is sufficient, python -m owtf

screen shot 2017-08-19 at 12 45 47

This is the first step of many. I would like to have different commands to run OWTF as the user wishes, say for example, to only run CLI, the user would have to run python -m owtf start cli or for web interface, python -m owtf start web

@saganshul
Copy link
Contributor

@viyatb One thing we can do we can host bundle.js and bundle.css somewhere and wget them at the time of installation.

Copy link
Contributor

@DePierre DePierre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of remarks.

@@ -170,6 +170,7 @@ def get_first(self, criteria, target_id=None):
:return:
:rtype:
"""
print criteria, target_id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like debug output to me.

@@ -195,7 +196,7 @@ def get_transaction(self, trans):
:return:
:rtype:
"""
if trans:
if (trans and len(trans) > 0):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parenthesis are superfluous.

@@ -217,7 +218,9 @@ def get_transactions(self, transactions):
"""
owtf_tlist = []
for transaction_obj in transactions:
owtf_tlist.append(self.get_transaction(transaction_obj))
print self.get_transaction(transaction_obj)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like debug output to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is.

@@ -217,7 +218,9 @@ def get_transactions(self, transactions):
"""
owtf_tlist = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be possible to shorten those lines with:

return [self.get_transaction(transaction) for transaction in transactions if self.get_transaction(transaction) is not None]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1


def add_urls_end(self):
num_urls_after = self.get_num_urls()
msg = str(num_urls_after - self.num_urls_before)+" URLs have been added and classified"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space surrounding the + operator.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the function.

def add_urls_end(self):
num_urls_after = self.get_num_urls()
msg = str(num_urls_after - self.num_urls_before)+" URLs have been added and classified"
logging.info(msg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this debug output?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not, but the function is not used anyway. I am removing it.

count = self.db.session.query(models.Url(target_id=target_id)).count()
return count

def add_urls_start(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the name of the function accurate? I am not sure to understand its purpose when I read add_urls_start and I see that it stores the number of URLs to a class attribute.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ported it from a commit while back because the plugin helper had a method which was calling it. I removed it from there.

except HTTPError as Error: # page NOT found.
# Error is really a response for anything other than 200 OK in urllib2 :)
self.http_transaction.set_transaction(False, raw_request[0], Error)
except urlError as Error: # Connection refused?
except URLError as Error: # Connection refused?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error should be snake case and not camel case.

@@ -17,6 +17,7 @@
except ImportError:
from httplib import responses as response_messages

from owtf.lib.general import *
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wildcard import should be avoided at all cost to ease debugging and prevent scope override.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@@ -552,7 +552,7 @@ def process_plugins_for_target_list(self, plugin_group, status, target_list):
{'SomeAborted': False, 'SomeSuccessful': False, 'AllSkipped': True},
{target})
else:
pass
exit(0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about the context. Is the user expecting OWTF to exit in such case? Is it an actual success?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed, was just for debugging. Removing this.

@viyatb
Copy link
Member Author

viyatb commented Aug 27, 2017

@DePierre @saganshul @7a ready to merge? :)

@viyatb viyatb merged commit 520cdc0 into develop Aug 29, 2017
@viyatb viyatb deleted the viyatb/break branch September 3, 2017 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants