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

Migrate rex/logging into Core from Msf namespace #32

Closed

Conversation

sempervictus
Copy link

Using Rex' various gems without Msf will result in errors when the logging subsystem is undefined (as that remained in Msf during the great Rex excision). This manifests in rex-socket as noted by @zeroSteiner in rapid7/rex-socket#38.

Address the dependency problem by moving rex/logging into this gem which is already required by rex-socket and other descendants.

Testing:
None - this is a quick-n-dirty subdirectory move. If this works, someone with real git skill should migrate the relevant historyof the code; as losing that stuff results in people not knowing whom to ask when the time comes to fix some deeply-bored bug.

Using Rex' various gems without Msf will result in errors when the
logging subsystem is undefined (as that remained in Msf during the
great Rex excision). This manifests in rex-socket as noted by
@zeroSteiner in rapid7/rex-socket#38.

Address the dependency problem by moving rex/logging into this gem
which is already required by rex-socket and other descendants.

Testing:
 None - this is a quick-n-dirty subdirectory move. If this works,
someone with real git skill should migrate the relevant history
of the code; as losing that stuff results in people not knowing
whom to ask when the time comes to fix some deeply-bored bug.
@adfoster-r7
Copy link
Contributor

I'm guessing this is an issue other gems as well? i.e. this isn't a viable fix in isolation?

$ ls | grep rex | xargs egrep -ir 'elog|wlog|ilog|rlog|dlog'

rex-core/lib/rex/io/socket_abstraction.rb:              wlog('monitor_rsock: the remote socket has been closed, exiting loop')
rex-core/lib/rex/io/socket_abstraction.rb:              wlog("monitor_rsock: exception during select: #{e.class} #{e}")
rex-core/lib/rex/io/socket_abstraction.rb:                  wlog('monitor_rsock: closed remote socket due to nil read')
rex-core/lib/rex/io/socket_abstraction.rb:                dlog('monitor_rsock: EOF in rsock')
rex-core/lib/rex/io/socket_abstraction.rb:                wlog("monitor_rsock: exception during read: #{e.class} #{e}")
rex-core/lib/rex/io/socket_abstraction.rb:                    wlog('monitor_rsock: failed writing, socket must be dead')
rex-core/lib/rex/io/socket_abstraction.rb:                  wlog("monitor_rsock: exception during write: #{e.class} #{e}")
rex-core/lib/rex/io/stream_server.rb:              elog('The accept() returned nil in stream server listener monitor')
rex-core/lib/rex/io/stream_server.rb:            elog("Error in stream server server monitor: #{$!}")
rex-core/lib/rex/io/stream_server.rb:            rlog(ExceptionCallStack)
rex-core/lib/rex/io/stream_server.rb:            elog("Error in stream server client monitor: #{$!}")
rex-core/lib/rex/io/stream_server.rb:            rlog(ExceptionCallStack)
rex-core/lib/rex/io/stream_server.rb:          elog("Error in stream server client monitor: #{$!}")
rex-core/lib/rex/io/stream_server.rb:          rlog(ExceptionCallStack)
rex-socket/lib/rex/socket.rb:      elog("#{e.message} (#{e.class})#{e.backtrace * "\n"}\n", LogSource, LEV_3)
rex-socket/lib/rex/socket/parameters.rb:        elog("Failed to read cert: #{e.class}: #{e}", LogSource)
rex-socket/lib/rex/socket/parameters.rb:        elog("Failed to read client cert: #{e.class}: #{e}", LogSource)
rex-socket/lib/rex/socket/parameters.rb:        elog("Failed to read client key: #{e.class}: #{e}", LogSource)

@sempervictus
Copy link
Author

Socket needs Core, so i think that if we can make Core happy by moving this in here, Socket should "become" happy as well.

@sempervictus
Copy link
Author

ping @adfoster-r7 - any blockers on this one? I cant rip these files from the Framework side until we have this PR merged 😄

@adfoster-r7
Copy link
Contributor

No blockers; But since this will auto release as a gem bump on merge - it'd be good to have a the metasploit-framework PR prepped waiting to go so we can ship both one after the other 👀

sempervictus pushed a commit to sempervictus/metasploit-framework that referenced this pull request Jan 18, 2023
Using Rex' various gems without Msf will result in errors when the
logging subsystem is undefined (as that remained in Msf during the
great Rex excision). This manifests in rex-socket as noted by
@zeroSteiner in rapid7/rex-socket#38.

Address the dependency problem by moving rex/logging into rex-core
which is already required by rex-socket and other descendants.

Notes:
  This PR is staged to allow github.com/rapid7/rex-core/pull/32
to be merged without creating a (seemingly harmless) redundancy.
@sempervictus
Copy link
Author

@adfoster-r7 - Msf side staged, should be good to roll. Requesting manual validation post-merge as my environment deviates significantly from upstream's such that "what works here may not work for everyone."

RageLtMan added 2 commits January 19, 2023 17:01
b95be3ed103c8 removed the internal require statements from logging
in favor of Zeitwerk autoloading. Restore the requires removed at
the time into rex-core to permit proper loading of the namespace.
@sempervictus
Copy link
Author

Is there anyone around who knows how to move or clone the git history for these files from the MSF repo into this one? We want to preserve those histories as much as possible so subsequent generations can hunt down and yell at/beg for help from the original authors 😄

@adfoster-r7
Copy link
Contributor

Good idea 👍

Should be able to look into this after 6.3 is released - unless someone gets nerds sniped on it sooner 😄

@gwillcox-r7
Copy link
Contributor

@adfoster-r7 This looks good to me, can you clarify if there is anything you would need from my side in order to get this landed? I think you wanted to make sure there was the corresponding Metasploit Framework PR and that seems to be up at the moment.

@adfoster-r7
Copy link
Contributor

@gwillcox-r7 The latest was this comment: rapid7/metasploit-framework#17506 (comment)

I'm not super opinionated on preserving the history

@adfoster-r7
Copy link
Contributor

adfoster-r7 commented May 3, 2023

This PR won't work in its current form, attempting to log raises an error:

2.7.2 :001 > require 'rex/core'
 => true 
2.7.2 :002 > elog('a')
Traceback (most recent call last):
        7: from /Users/user/.rvm/gems/ruby-2.7.2/bin/ruby_executable_hooks:22:in `<main>'
        6: from /Users/user/.rvm/gems/ruby-2.7.2/bin/ruby_executable_hooks:22:in `eval'
        5: from /Users/user/.rvm/gems/ruby-2.7.2/bin/irb:23:in `<main>'
        4: from /Users/user/.rvm/gems/ruby-2.7.2/bin/irb:23:in `load'
        3: from /Users/user/.rvm/rubies/ruby-2.7.2/lib/ruby/gems/2.7.0/gems/irb-1.2.6/exe/irb:11:in `<top (required)>'
        2: from (irb):2
        1: from /Users/user/Documents/code/rex-core/lib/rex/logging/log_dispatcher.rb:160:in `elog'
NameError (uninitialized constant LOG_ERROR)
Did you mean?  LoadError
               IOError

It turns out that msf console is mutating the global Kernel to include the rex logging constants:

[13] pry(#<Msf::Framework>)> Kernel.const_source_location(:LOG_ERROR)
=> ["/Users/user/Documents/code/metasploit-framework/lib/rex/logging.rb", 8]

Which we can see here:

https://github.com/rapid7/metasploit-framework/blob/c6547737a6f28252c6638409e61d0e86ece6fc81/lib/msf.rb#L5

So this PR would need to be modified to work outside of the context of msfconsole still; Taking a step back though - including a gem probably shouldn't globally define methods such as elog etc, so I'm not sure this PR is a good idea in isolation either.

@adfoster-r7
Copy link
Contributor

Will mark this as attic'd for now - to help keep the PR queue tidy; it looks like there's more work required in other places to actually wire this up correctly (context) - regardless of preserving Git history 👍

Just for a paper trail: Cross-referencing the other test PR rapid7/metasploit-framework#17506 (comment) which I think had more of the missing pieces than this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants