zeroSteiner/metasploit-framework — pull request #2 by jvazquez-r7 (2014-02-10 01:24:17+00:00)
Title: Unify CMD Stagers

Hi @zeroSteiner,

I didn't forget about https://github.com/rapid7/metasploit-framework/pull/2484 :) But after being discussing with @jlee-r7 the CMDStager design, we had several ideas about how the mixin should work. After reviewing your pull request, I really like your approach, sooooo I've tried to mix the discussion from @jlee-r7 with your pull request in order to give a new chance to the CMD Stager unification thing. But seriously, very good work on https://github.com/rapid7/metasploit-framework/pull/2484, I really like it.

So.... what this pull request includes? Just a summary:

* Resolve conflicts on https://github.com/rapid7/metasploit-framework/pull/2484
* Makes CMDStager more universal, since https://github.com/rapid7/metasploit-framework/pull/2484 was forgetting the TFTPStager.
* It modifies the CMDStager to allow to the stager to make any needed initialization on the framework. The bad news: the Stager class (Rex) needs to get a reference to the module instance, so it can take care of things to register into the framework (like server sockets). I'm really not in love with this approach (giving to a Rex class the responsibility of init things on the framework) but really after dig into other solutions, it looked not so dirty to me so... giving a chance and opening discussion :)
* As https://github.com/rapid7/metasploit-framework/pull/2484 proposes, it gives to the CMDStager mixin the new responsibility to select the correct stager. This pull request just tries to improve this new thing a little bit :), allowing to select an stager via function options, the datastore or guessing. And allowing to specify the new 'CmdStagerFlavor' option on targets and modules metadata, in order to define compatible encoders.
* It also adds YARD documentation to the new CMDStager mixin
* It also adds specs for the CMDStager mixin. **IMPORTANT WARNING**: there are only specs for the new methods, because I assume the old ones were working good enough. On the other hand I suck writting rspecs, so read carefully, don't be very hard with me XD The specs were useful to fix some errors on the new CMDStager mixin code soooo I guess can be still useful.
* I've modified modules to have into account the new mixin, mainly defining the CmdStagerFlavor to be sure only tested stagers are marked as compatible on a per module basis atm.
* I've only tested the modules/exploits/multi/ssh/sshexec, which is working okey with the new mixin, more test is needed, but atm I'm more worried about the CMDStager design, once it's good enough we can check modules aren't broken :)

@zeroSteiner if you would like to review changes and provide feedback would be super awesome. You can just go to the last commits starting on Feb 07, 2014, since prior changes are mainly to solve conflicts, updating against the upstream master branch.

Pinging also @jduck since he did the original https://github.com/rapid7/metasploit-framework/pull/2430 and maybe would like to review these changes and provide feedback / opinion.

**IMPORTANT NOTE 2** Please @zeroSteiner, even if you feel comfortable with changes at any point, I would like to ask you to not land these changes into your repo still! :) Why? Because I'd like to give a chance to @jlee-r7 to review this work! We were discussing CMDStager design last week and obviously he rocks the framework more than me, so I would love him to review and provide feedback!

Hopefully we're closer to an unified CMDStager mixin! which would be super awesome! Thanks very much for your effort and an awesome PR! And sorry for the delay handling it, huge change! Hopefully we'll be able to put something ready soon! :)