Group TLV refactors

[OJ]

After the group packet functionality was added I felt it was a good idea to go through the existing code and clean up some of the bits that were adding groups of tlvs as chunks. At the same time I abused the new support for arbitrarily nested TLVs. The code is a lot safer as well due to the fact that we're making more use of "api" calls and avoiding custom code which mangles values into fixed sized buffers.

This PR contains a number of these adjustments. Functionality should remain very much the same as before except that in a few cases the structure that is sent across the wire is a little more intuitive and contains a bit more information. MSF needs to be updated as well (there is an associated PR).

Verification of this PR should be done in conjunction with the MSF PR, and hence instructions for what to check can be found over there.

[metasploit-public-bot]

Test PASSED. Refer to this link for build results: https://ci.metasploit.com/job/GPR-MeterpreterWin/96/

[metasploit-public-bot]

Test FAILED. Refer to this link for build results: https://ci.metasploit.com/job/GPR-MeterpreterWin/131/

[OJ]

Ping @cdoughty-r7 -- not sure what's going on here mate, but the CI build system looks poked.

[bcook-r7]

Great work, worked well for me! I'm working through landing the rest of the dependency chain now.

[OJ]

Closed? Not merged? :)

[bcook-r7]

Merged here at least :) Is it tradition to leave both sides open and close at the same time?

I'm working through the rest of the supply chain so I can do these quicker later. Somehow I ended up at the 'I need a wordpress account, so I can connect a gravitar icon to a rubygems account' stage.

[mbuck-r7]

You actually just needed to use the word "Close" or any from this page on 4295c86 @bcook-r7 , rather than "Land". That will automatically close out this PR and mark it as merged.

[bcook-r7]

I think what happened here is I pulled your PR into a merge branch, then rebased on master before testing. That shuffled the commit hashes for the PR, so github lost track of them. I should have asked you to do it instead and push the rebased version to the PR first, so github could keep track of the updated hashes properly. Sorry about that.

[OJ]

The keyword used was fine. You don't actually have to use any particular one for github to pick it up.

I'm guessing it's because it hasn't been pushed to master yet because of the rest of the dance you are having to go through.

Cheers for landing this mate. PRs with both MSF and meterpreter changes are a bit painful ;)

[OJ]

No problems mate. As long as we get there in the end I don't mind at all.

Let me know if you need anything else.

[mbuck-r7]

In order for Github to automatically mark a PR as closed from a commit message, you in fact do have to use one of the particular keywords they provide in that list, as is explained in that article. Otherwise it just appears as a reference in the timeline.

[OJ]

That doesn't explain the dozens of times that PRs have been merged and closed without any of those keywords making an appearance in every project I've contributed to on github.

Anyway, moving on.

Brent, sing out of there is anything else you need mate. Good luck with the meterpreter_bins. I think you need to poke @cdoughty-r7 to kick off the build and publish mechanism for that gem.

[mbuck-r7]

Right, because you can close them from the interface. The keywords are needed to close them from a commit message, automatically.

[todb-r7]

The subtlety here, I think, is that @OJ was surprised by the close rather than a "clean" merge, where the commit hashes are all the same.

You /can/ close pulls from a commit message, but you generally oughtn't /have/ to.

[limhoff-r7]

You have to close issues explicitly, but not PRs even though PRs are modelled as issues on the backend

[trevrosen]

Prize for the use of “oughtn’t” here.

[jlee-r7]

@bcook-r7 rebasing is almost always a bad idea. Is there a particular reason it was necessary here?