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

Only include peer in prefix for single hosts #10790

Merged
merged 3 commits into from Oct 12, 2018

Conversation

Projects
None yet
4 participants
@TheNaterz
Contributor

TheNaterz commented Oct 10, 2018

Currently, when a status message gets printed for a module that has an RHOSTS defined as a file with hundreds of hosts, the prefix takes this gigantic space separated list of hosts and shoves it into a message. Every time the message is printed, you get this really ugly block of text and it makes the output hard to read.

I propose that peer information isn't necessary enough to include in these messages.

Before

  • Start msfconsole
  • use auxiliary/scanner/ssh/ssh_version (just as an example)
  • Set RHOSTS to some file with 100 hosts or something.
  • Every scanner progress message looks gross.

After

  • Do all the above steps.
  • Bask in the glory of minimal output. Look upon its simplistic face and behold its overwhelming mediocrity. But also it's cleaner and cleanliness is close to godliness.

In other words...

When did this:
selection_002
become hotter than this?:
selection_001

Thoughts?

@wvu-r7

This comment has been minimized.

Show comment
Hide comment
@wvu-r7

wvu-r7 Oct 11, 2018

Contributor

My thoughts are death to the peer gods.

Contributor

wvu-r7 commented Oct 11, 2018

My thoughts are death to the peer gods.

@busterb

This comment has been minimized.

Show comment
Hide comment
@busterb

busterb Oct 11, 2018

Contributor

🙏 📿

Contributor

busterb commented Oct 11, 2018

🙏 📿

@busterb busterb self-assigned this Oct 11, 2018

@jhart-r7

This comment has been minimized.

Show comment
Hide comment
@jhart-r7

jhart-r7 Oct 11, 2018

Contributor

How does this affect RHOSTS use-cases that are not file:// based?

Contributor

jhart-r7 commented Oct 11, 2018

How does this affect RHOSTS use-cases that are not file:// based?

@TheNaterz

This comment has been minimized.

Show comment
Hide comment
@TheNaterz

TheNaterz Oct 11, 2018

Contributor

In the case of a single RHOSTS or even using ranges, this change would also apply. Messages for modules that are subject to msf/core/exploit/tcp would not include {rhost}:{rport}.

Contributor

TheNaterz commented Oct 11, 2018

In the case of a single RHOSTS or even using ranges, this change would also apply. Messages for modules that are subject to msf/core/exploit/tcp would not include {rhost}:{rport}.

@busterb

This comment has been minimized.

Show comment
Hide comment
@busterb

busterb Oct 11, 2018

Contributor

Any other mixins we should update? udp?

Contributor

busterb commented Oct 11, 2018

Any other mixins we should update? udp?

@@ -196,11 +196,12 @@ def cleanup
end
def print_prefix

This comment has been minimized.

@busterb

busterb Oct 11, 2018

Contributor

I think deleting print_prefix entirely from this file has the same outcome as it just calling super.

@busterb

busterb Oct 11, 2018

Contributor

I think deleting print_prefix entirely from this file has the same outcome as it just calling super.

This comment has been minimized.

@TheNaterz

TheNaterz Oct 11, 2018

Contributor

Yep valid!

@TheNaterz

TheNaterz Oct 11, 2018

Contributor

Yep valid!

@TheNaterz

This comment has been minimized.

Show comment
Hide comment
@TheNaterz

TheNaterz Oct 11, 2018

Contributor

Maybe? This has just been the real nagging one that I've noticed. If this issue propagates in other areas, I would recommend the same changes, yeah.

Contributor

TheNaterz commented Oct 11, 2018

Maybe? This has just been the real nagging one that I've noticed. If this issue propagates in other areas, I would recommend the same changes, yeah.

@wvu-r7

This comment has been minimized.

Show comment
Hide comment
@wvu-r7

wvu-r7 Oct 11, 2018

Contributor

It propagates so hard we considered reverting the entire changeset!

Contributor

wvu-r7 commented Oct 11, 2018

It propagates so hard we considered reverting the entire changeset!

@jhart-r7

This comment has been minimized.

Show comment
Hide comment
@jhart-r7

jhart-r7 Oct 11, 2018

Contributor

IMO, peer information is VERY useful, if not critical, to any RHOSTS use-case involving more than one host. How are you supposed to know what the line is about otherwise? Leaving it up to the individual module other to include or not include peer is also problematic.

Can you clarify that this will actually change? If it just changes the RHOSTS file:// use-case, 👍. If this will impact any RHOSTS use-case such that the peer information will not be printed in statusy messages by default, 👎 .

Contributor

jhart-r7 commented Oct 11, 2018

IMO, peer information is VERY useful, if not critical, to any RHOSTS use-case involving more than one host. How are you supposed to know what the line is about otherwise? Leaving it up to the individual module other to include or not include peer is also problematic.

Can you clarify that this will actually change? If it just changes the RHOSTS file:// use-case, 👍. If this will impact any RHOSTS use-case such that the peer information will not be printed in statusy messages by default, 👎 .

@wvu-r7

This comment has been minimized.

Show comment
Hide comment
@wvu-r7

wvu-r7 Oct 11, 2018

Contributor

@jhart-r7: I don't think any of us will disagree that it's useful. It's just been one of our most problematic PRs due to the regressions. It was a very aggressive changeset that we have to live with now. We must tread lightly.

Contributor

wvu-r7 commented Oct 11, 2018

@jhart-r7: I don't think any of us will disagree that it's useful. It's just been one of our most problematic PRs due to the regressions. It was a very aggressive changeset that we have to live with now. We must tread lightly.

@TheNaterz

This comment has been minimized.

Show comment
Hide comment
@TheNaterz

TheNaterz Oct 11, 2018

Contributor

Sure, so specifically, this change ONLY removes print_prefix, it does not remove peer. If a module uses peer for any specific message, those are unaffected.

A tcp peer is the entire scope that you set in RHOSTS for that module (single, comma-separated, ranges, file) and placing this peer in a print causes an unnecessary readability issue.

My guess is that other uses of peer deal with single hosts, which is fine because it's not an issue at that point.

Contributor

TheNaterz commented Oct 11, 2018

Sure, so specifically, this change ONLY removes print_prefix, it does not remove peer. If a module uses peer for any specific message, those are unaffected.

A tcp peer is the entire scope that you set in RHOSTS for that module (single, comma-separated, ranges, file) and placing this peer in a print causes an unnecessary readability issue.

My guess is that other uses of peer deal with single hosts, which is fine because it's not an issue at that point.

@busterb

This comment has been minimized.

Show comment
Hide comment
@busterb

busterb Oct 11, 2018

Contributor

How about this:

  def print_prefix
    if rhost && rhost.split(' ').length == 1
      super + peer + ' - '
    else
      super
    end
  end

The point of the 'rhost' check was to see if we are in the context of a host or not. Sometimes 'rhost' ends up mapping to 'rhosts' which ends up being too long. This figures it out and does the right thing that you would expect.

Contributor

busterb commented Oct 11, 2018

How about this:

  def print_prefix
    if rhost && rhost.split(' ').length == 1
      super + peer + ' - '
    else
      super
    end
  end

The point of the 'rhost' check was to see if we are in the context of a host or not. Sometimes 'rhost' ends up mapping to 'rhosts' which ends up being too long. This figures it out and does the right thing that you would expect.

@TheNaterz

This comment has been minimized.

Show comment
Hide comment
@TheNaterz

TheNaterz Oct 11, 2018

Contributor

Seems OK to me!

Contributor

TheNaterz commented Oct 11, 2018

Seems OK to me!

@busterb

This comment has been minimized.

Show comment
Hide comment
@busterb

busterb Oct 11, 2018

Contributor

Output of the above change:

msf5 auxiliary(scanner/ssh/ssh_version) > run

[+] 192.168.56.1:22       - SSH server version: SSH-2.0-OpenSSH_7.6 ( service.version=7.6 service.vendor=OpenBSD service.family=OpenSSH service.product=OpenSSH service.cpe23=cpe:/a:openbsd:openssh:7.6 service.protocol=ssh fingerprint_db=ssh.banner )
[+] 192.168.56.1:22       - SSH server version: SSH-2.0-OpenSSH_7.6 ( service.version=7.6 service.vendor=OpenBSD service.family=OpenSSH service.product=OpenSSH service.cpe23=cpe:/a:openbsd:openssh:7.6 service.protocol=ssh fingerprint_db=ssh.banner )
^C[*] Caught interrupt from the console...
[*] Auxiliary module execution completed

Before any of these changes, MSF would print all of the hosts on the 'completed' line (annoying). Simply removing print_prefix here causes it to have the effect of no host info. This meets in the middle and probably does what was intended all along.

Contributor

busterb commented Oct 11, 2018

Output of the above change:

msf5 auxiliary(scanner/ssh/ssh_version) > run

[+] 192.168.56.1:22       - SSH server version: SSH-2.0-OpenSSH_7.6 ( service.version=7.6 service.vendor=OpenBSD service.family=OpenSSH service.product=OpenSSH service.cpe23=cpe:/a:openbsd:openssh:7.6 service.protocol=ssh fingerprint_db=ssh.banner )
[+] 192.168.56.1:22       - SSH server version: SSH-2.0-OpenSSH_7.6 ( service.version=7.6 service.vendor=OpenBSD service.family=OpenSSH service.product=OpenSSH service.cpe23=cpe:/a:openbsd:openssh:7.6 service.protocol=ssh fingerprint_db=ssh.banner )
^C[*] Caught interrupt from the console...
[*] Auxiliary module execution completed

Before any of these changes, MSF would print all of the hosts on the 'completed' line (annoying). Simply removing print_prefix here causes it to have the effect of no host info. This meets in the middle and probably does what was intended all along.

@wvu-r7

This comment has been minimized.

Show comment
Hide comment
@wvu-r7

wvu-r7 Oct 11, 2018

Contributor

Haven't tested it yet, but that looks like intended behavior!

Contributor

wvu-r7 commented Oct 11, 2018

Haven't tested it yet, but that looks like intended behavior!

@TheNaterz TheNaterz changed the title from Remove peer information from the prefix to Only include peer in prefix for single hosts Oct 11, 2018

@busterb

This comment has been minimized.

Show comment
Hide comment
@busterb

busterb Oct 12, 2018

Contributor

Added 4ae45cb to explain what we're doing to future generations. Thanks!

Contributor

busterb commented Oct 12, 2018

Added 4ae45cb to explain what we're doing to future generations. Thanks!

@busterb busterb merged commit 14e87bf into rapid7:master Oct 12, 2018

3 checks passed

Metasploit Automation - Sanity Test Execution Successfully completed all tests.
Details
Metasploit Automation - Test Execution Successfully completed all tests.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

busterb added a commit that referenced this pull request Oct 12, 2018

msjenkins-r7 added a commit that referenced this pull request Oct 12, 2018

@busterb

This comment has been minimized.

Show comment
Hide comment
@busterb

busterb Oct 12, 2018

Contributor

Release Notes

This restricts logging of target host/port prefixes to representing a single target only. This improves usability when the list of targets for a module large, while retaining logging for runs against individual targets.

Contributor

busterb commented Oct 12, 2018

Release Notes

This restricts logging of target host/port prefixes to representing a single target only. This improves usability when the list of targets for a module large, while retaining logging for runs against individual targets.

@TheNaterz TheNaterz deleted the TheNaterz:patch-2 branch Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment