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

Centralize networking #13750

Merged
merged 7 commits into from Jul 16, 2020
Merged

Centralize networking #13750

merged 7 commits into from Jul 16, 2020

Conversation

h00die
Copy link
Contributor

@h00die h00die commented Jun 22, 2020

As per comment in slack a few days ago, this PR moves /cisco, /juniper, /ubiquiti, /brocade all under one roof since there are minimal (usually 1) file in each folder. The new folder is /networking.

This pr does the following:

  1. redoes folder structure for networking devices
  2. adds deprecated links back to the original locations
  3. updates docs/examples to show the new locations
  4. runs rubocop -a on all the files. If it didn't auto-fix, I didn't investigate further as this isn't a clean-up PR, it's a move things to a better location PR.

This is preparation to add additional config eaters such as Arista

@gwillcox-r7 gwillcox-r7 self-assigned this Jul 13, 2020
Copy link
Contributor

@gwillcox-r7 gwillcox-r7 left a comment

Choose a reason for hiding this comment

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

@h00die Overall looks good but I do question adding in the Depreciated aspect for these modules. They aren't really depreciated, we are just moving them. Whilst I get that the Depreciated module has a nice alias to indicate the file has moved, the side effect of this PR will be that all these modules are marked as depreciated and that they may not work as intended, which I don't think was what you were going after in some cases.

Additionally, there are also a few cases of odd prompts in some of the documentation which need fixing up before I'd be satisfied with merging this, and a few cases of missing info which we should ideally fix before merging.

Overall looks good though, and although GitHub did have some issues with diffing, I didn't see anything too odd, with the exception of those aforementioned issues.

@h00die
Copy link
Contributor Author

h00die commented Jul 14, 2020

The deprecated mixing should be transparent to the user, it's only used for the moved_from functionality so any old scripts or typing memory will still work. I don't believe there is any other functionality gained/lost, I also asked about it in slack.

@h00die
Copy link
Contributor Author

h00die commented Jul 14, 2020

For the resource files, I always use them when creating/testing a module so I have a consistent, repeatable behavior. All of my module docs have them (and some I didnt write aws_ec2_instance_metadata.md): https://github.com/rapid7/metasploit-framework/search?l=Markdown&q=%22resource+%28%22

If this isn't appropriate, it would most likely be best to have one PR to fix every module doc at once.

@gwillcox-r7
Copy link
Contributor

gwillcox-r7 commented Jul 14, 2020

The deprecated mixing should be transparent to the user, it's only used for the moved_from functionality so any old scripts or typing memory will still work. I don't believe there is any other functionality gained/lost, I also asked about it in slack.

Ah okay, I think there were two things going on here. First I assumed that behavior would be invoked by default, which you are correct, it is not. Secondly, part of my concern where the affect of the accessors declared at

attr_accessor :deprecation_date
and the fact that we haven't initialized their data (which may have unintended negative side effects).

Edit: Yep I'm wrong again. Those accessors aren't used outside of that module. So looks like your all good, my apologies.

@gwillcox-r7
Copy link
Contributor

For the resource files, I always use them when creating/testing a module so I have a consistent, repeatable behavior. All of my module docs have them (and some I didnt write aws_ec2_instance_metadata.md): https://github.com/rapid7/metasploit-framework/search?l=Markdown&q=%22resource+%28%22

If this isn't appropriate, it would most likely be best to have one PR to fix every module doc at once.

If your loading a resource and then running a module, you should be showing how that resource is loaded as part of a complete overview of how to run the module. The steps shown currently lead me to believe that you had tried to load a resource previously that was unrelated to this current module and then just decided to run the current module after that was done.

Also, and more importantly, there are several prompts that should clearly be starting with msf5 but which are instead starting with resource, which look very suspicious to me. I do agree that perhaps the last comment of the depreciated module is incorrect on my part, as I thought that functionality was invoked by default, but I'm fairly confident some of these prompts are incorrect. If you'd like to do that in a separate PR that is fine, I just thought I'd raise this point as some of them clearly need fixing and I don't think it should be too complex to fix these issues.

@gwillcox-r7
Copy link
Contributor

All of these resource prompts are part of the normal running of the resource file, as when running a file with a resource it echos out every command that is to be run and prepends it with resource (*name of file*)> for the prompt. This was something I was not aware of and which @smcintyre-r7 later cleared up to me. In light of this its probably best to leave this as is rather than fixing every instance. Going to mark all the other cases of this issue as resolved.

Will merge this in shortly.

@h00die
Copy link
Contributor Author

h00die commented Jul 14, 2020

@gwillcox-r7 hold on the merge, I'll fire up that juniper or brocade to address the one doc issue where I don't clearly show getting the initial prompt/shrll

@h00die
Copy link
Contributor Author

h00die commented Jul 15, 2020

i went ahead and did msftidy_docs on everything and fixed as many things as I could. Not all, rhosts makes sense on some modules since its not a typical usage. A bunch of issues fixed though!

gwillcox-r7
gwillcox-r7 previously approved these changes Jul 15, 2020
Copy link
Contributor

@gwillcox-r7 gwillcox-r7 left a comment

Choose a reason for hiding this comment

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

Looks good, I'll run a quick check over this to see whether or not there are any remaining issues but I do agree that whilst the RHOST explanation may raise complains by msftidy_docs.rb, in this case they are needed to help explain how to use the module properly.

Should everything pass the compliance tests, I'm happy to merge this in.

@gwillcox-r7
Copy link
Contributor

gwillcox-r7 commented Jul 15, 2020

Okay so looks like this passed the msftidy_docs.rb checks but I did see a few module corrections from rubocop that were odd, In particular these ones stood out:

Mistyping:

modules/post/networking/gather/enum_juniper.rb:72:7: W: Lint/UselessAssignment: Useless assignment to variable - os_tye. Did you mean os_type?
      os_tye = 'junos'
      ^^^^^^

Some cases of potential simplifications that could be made with if/else:

modules/post/networking/gather/enum_juniper.rb:46:7: C: Style/IfInsideElse: Convert if nested inside else to elsif.
      if session.shell_command('?') =~ /\?: No match\./ # confirmed ssh shell
      ^^
modules/post/networking/gather/enum_juniper.rb:127:12: W: Lint/ElseLayout: Odd else layout detected. Did you mean to use elsif?
      else os_type == 'junos'
           ^^^^^^^^^^^^^^^^^^

Typo leading to potential side effects due to single = vs double = :

modules/post/networking/gather/enum_juniper.rb:127:20: W: Lint/Void: Operator == used in void context.
      else os_type == 'junos'

This command seems to be unnecessary given the following line, or at the very least the output from it doesn't need to be saved into en_out:

modules/post/networking/gather/enum_cisco.rb:113:7: W: Lint/UselessAssignment: Useless assignment to variable - en_out.
      en_out = session.shell_command('enable').to_s.strip

@gwillcox-r7 gwillcox-r7 dismissed their stale review July 15, 2020 16:34

New updates needed upon further review

@h00die
Copy link
Contributor Author

h00die commented Jul 16, 2020

a few more rubocop cleanups done, should address the issues

Copy link
Contributor

@gwillcox-r7 gwillcox-r7 left a comment

Choose a reason for hiding this comment

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

Changes look good, will merge this in shortly. There will be a few extra minor RuboCop and style improvements I made when I land this, but nothing too major, just about 3 areas where I saw small fixes that could be quickly implemented. There will still be some other RuboCop errors but I didn't feel they were worth worrying about for now in this PR.

gwillcox-r7 added a commit that referenced this pull request Jul 16, 2020
@gwillcox-r7 gwillcox-r7 merged commit 8133933 into rapid7:master Jul 16, 2020
@gwillcox-r7
Copy link
Contributor

gwillcox-r7 commented Jul 16, 2020

Original Release Notes

This PR moves the auxiliary modules that were previously under /cisco, /juniper, /ubiquiti, and /brocade to a new folder, /networking, in the respective directory. This PR also updates the respective documentation for these modules to reflect their new location. Links to the old locations are maintained via the use of the moved_from to ensure that users dependent on the old file paths will be redirected to the new file paths for these modules. Finally, some RuboCop and msftidy_docs.rb changes have been applied to the modules and their associated documentation to ensure that they are more standardized and so the modules use more efficient code.

@h00die h00die deleted the centralize_networking branch July 16, 2020 20:58
@pbarry-r7 pbarry-r7 added the rn-enhancement release notes enhancement label Jul 21, 2020
@pbarry-r7
Copy link
Contributor

pbarry-r7 commented Jul 22, 2020

Release Notes

A number of auxiliary modules (includes ones in /cisco, /juniper, /ubiquiti, and /brocade locations) have been reorganized under a new folder: /networking. Related documentation was also updated, and the previous locations treated as 'deprecated' so that users attempting to use modules in the old location will be redirected to the new location.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Improving code quality docs enhancement rn-enhancement release notes enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants