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
Implement the zeitwerk autoloader within lib/msf/core #14202
Conversation
954aa46
to
5a2c4ea
Compare
Looks like the https handlers are failing:
|
b53aad8
to
5d143fe
Compare
@msjenkins-r7 test this please |
81d737a
to
333c035
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This look pretty sane.
A question about class variable
usage, and a question about possible file refactor change that as is stands would be a breaking change and mean this PR should bump version to 6.1.0
.
Mostly whitespace comments and minor code cleanup nits.
lib/msf/core/auxiliary/web.rb
Outdated
require 'msf/core/auxiliary/web/target' | ||
|
||
include Auxiliary::Report | ||
include Auxiliary::Report |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include Auxiliary::Report | |
include Auxiliary::Report |
lib/msf/core/exploit/local.rb
Outdated
@@ -10,8 +10,7 @@ | |||
# | |||
### | |||
class Msf::Exploit::Local < Msf::Exploit | |||
require 'msf/core/post_mixin' | |||
include Msf::PostMixin | |||
include Msf::PostMixin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include Msf::PostMixin | |
include Msf::PostMixin |
lib/msf/core/exploit/php_exe.rb
Outdated
require 'msf/core/payload' | ||
require 'msf/core/payload/php' | ||
include Payload::Php | ||
include Payload::Php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include Payload::Php | |
include Payload::Php |
lib/msf/core/exploit/remote.rb
Outdated
# | ||
### | ||
class Exploit::Remote < Exploit | ||
include Msf::Exploit::AutoTarget |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include Msf::Exploit::AutoTarget | |
include Msf::Exploit::AutoTarget |
require 'msf/core/exploit/kerberos/client/cache_credential' | ||
|
||
include Msf::Exploit::Remote::Kerberos::Client::Base | ||
include Msf::Exploit::Remote::Kerberos::Client::Base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include Msf::Exploit::Remote::Kerberos::Client::Base | |
include Msf::Exploit::Remote::Kerberos::Client::Base |
lib/msf/core/exploit/http/typo3.rb
Outdated
include Msf::Exploit::Remote::HttpClient | ||
include Msf::Exploit::Remote::HTTP::Typo3::Login | ||
include Msf::Exploit::Remote::HTTP::Typo3::URIs | ||
include Msf::Exploit::Remote::HttpClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include Msf::Exploit::Remote::HttpClient | |
include Msf::Exploit::Remote::HttpClient |
include Msf::Exploit::Remote::HTTP::Wordpress::Users | ||
include Msf::Exploit::Remote::HTTP::Wordpress::Version | ||
include Msf::Exploit::Remote::HTTP::Wordpress::XmlRpc | ||
include Msf::Exploit::Remote::HttpClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include Msf::Exploit::Remote::HttpClient | |
include Msf::Exploit::Remote::HttpClient |
module Exploit::Remote::SMB::Server | ||
module Share | ||
module InformationLevel | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end | |
end |
require 'msf/core/exploit/smb/server/share/command/trans2/query_path_information' | ||
|
||
# Handles an SMB_COM_TRANSACTION2 command, used to provide support for a richer set of | ||
# Handles an SMB_COM_TRANSACTION2 command, used to provide support for a richer set of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Handles an SMB_COM_TRANSACTION2 command, used to provide support for a richer set of | |
# Handles an SMB_COM_TRANSACTION2 command, used to provide support for a richer set of |
# autoload :ApiToken, 'msf/core/web_services/authentication/strategies/api_token' | ||
# autoload :AdminApiToken, 'msf/core/web_services/authentication/strategies/admin_api_token' | ||
# autoload :UserPassword, 'msf/core/web_services/authentication/strategies/user_password' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# autoload :ApiToken, 'msf/core/web_services/authentication/strategies/api_token' | |
# autoload :AdminApiToken, 'msf/core/web_services/authentication/strategies/admin_api_token' | |
# autoload :UserPassword, 'msf/core/web_services/authentication/strategies/user_password' |
This caught my eye because I tried to do something like this in the ancient port-to-rubinius (gil-free ruby) effort. What I learned during that process is that meddling with loader semantics can produce some really messed up objects due to our use of #extend on already instantiated objects (required for our interactions w/ stdlib FD-based objects of instance because the FD is marshalled from outside the runtime so we cant compose it proper). |
@sempervictus if you could find that test I'd love to see it, anything to help with this is much appreciated 😅 |
c59a833
to
487f278
Compare
Looks like this introduces a regression to the verify_datastore tool:
It errors out trying to include the mixins with the approach that it's using. I believe the tool is broken in the first place, but this was a difference in behavior I spotted |
Looks like this is failing for me if I run the http scanner title module:
Edit: Looks like this isn't specific to the scanner module:
|
Noticed another failure with:
Which gives:
Edit: Looks like you can get a stack trace with
Should be an easy |
487f278
to
7714fca
Compare
I imagine this is just completely broken like you said, it's regex for a class is:
hasn't been that in a long time 😬 |
Looks like I can't upgrade sessions:
|
Can't upgrade osx meterpreter from python either:
|
Running through the jsofu tool breaks:
Replication steps: #6511 |
I've ran through a few workflows, and any issues I've come across have been fixed now. @jmartin-r7 / @acammack-r7 Are there any critical paths that you'd like us to test through before this is landed? Either from Metasploit framework's perspective or Pro |
:wmap_generic | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nab: It might be nice to fix up missing trailing whitespace/remove the extra whitespace added to the top of the file 👍
require 'msf/core/exploit/smb/server/share/information_level' | ||
|
||
include Msf::Exploit::Remote::SMB::Server::Share::Command::Close | ||
include Msf::Exploit::Remote::SMB::Server::Share::Command::Close |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation issue
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation issue
@dwelch-r7 I was poking at the zeitwerk testing again, and I extracted all of referenced constants in framework and spot-checked a few, and and hit some failures on this branch:
I don't know if these are already a problem on master, but worth cross checking that 😄 |
84f5e34
to
b19a219
Compare
@adfoster-r7 Those super just don't exist on master by the looks of it, not just that they aren't being required properly but they aren't defined anywhere from a quick look |
0138e85
to
4cbdb3d
Compare
lib/msf/core/exception.rb
Outdated
@@ -21,7 +20,7 @@ module Exception | |||
# can be obtained through the options attribute. | |||
# | |||
### | |||
class OptionValidateError < ArgumentError | |||
class Msf::OptionValidateError < ArgumentError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just confirming if this is needed?
class Msf::OptionValidateError < ArgumentError | |
class OptionValidateError < ArgumentError |
# framework events occur, such as the loading and creation of a module. | ||
# | ||
### | ||
module GeneralEventSubscriber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nab: It looks like this could be namespaced, as it's an internal concept
lib/msf_autoload.rb
Outdated
loader.push_dir("#{__dir__}/msf/core/", namespace: Msf) | ||
loader.push_dir("#{__dir__}/../app/validators/") | ||
|
||
loader.ignore("#{__dir__}/msf/core/constants.rb", "#{__dir__}/msf/core/cert_provider.rb", "#{__dir__}/msf/core/rpc/json/error.rb", "#{__dir__}/msf/core/rpc/json/v2_0/", "#{__dir__}/msf/core/modules/external/ruby/metasploit.rb", "#{__dir__}/msf/core/rpc/v10/constants.rb") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we split this on to new lines, it'll make future reviews easier when new items are added to this list 👍
loader.ignore(
"#{__dir__}/msf/core/constants.rb",
"#{__dir__}/msf/core/cert_provider.rb",
# ...
)
lib/msf_autoload.rb
Outdated
loader.push_dir("#{__dir__}/../app/validators/") | ||
|
||
loader.ignore("#{__dir__}/msf/core/constants.rb", "#{__dir__}/msf/core/cert_provider.rb", "#{__dir__}/msf/core/rpc/json/error.rb", "#{__dir__}/msf/core/rpc/json/v2_0/", "#{__dir__}/msf/core/modules/external/ruby/metasploit.rb", "#{__dir__}/msf/core/rpc/v10/constants.rb") | ||
loader.collapse("#{__dir__}/msf/core", "#{__dir__}/msf/core/rpc/v10", "#{__dir__}/msf/core/payload/osx/x64", "#{__dir__}/msf/core/payload/windows/x64", "#{__dir__}/msf/core/payload/linux/x64", "#{__dir__}/msf/core/web_services/servlet") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above 👍
@@ -0,0 +1,167 @@ | |||
require "zeitwerk" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're treating this file as all of the things we need to fix with namespacing at some point; What are your thoughts on keeping track of the other files we want to fix later as comments? i.e. namespacing wmap/events properly. At the very least it won't be lost in a ticket
3e1181d
to
e02dda4
Compare
e02dda4
to
49a6b1b
Compare
Release NotesUpdated Metasploit to only load core library files when they are explicitly required. Previously, all core library files would be loaded eagerly on console start up. This initial work will reduce console start up time by around 0.5 seconds. Future releases will build upon this initial effort of lazily loading internal library files to further improve both the console and msfvenom's startup performance. |
Absolutely adore this (and the speedup), thanks a lot! |
Fix merge conflict from rapid7#14202, in linear history.
Awesome job! Fixed on my end to work with #14446's refactor of |
You can check out what zeitwerk is what it does and how it works here: https://github.com/fxn/zeitwerk
This is the first in a series of PRs that will have us transition to using the zeitwerk autoloader.
The main goal of this effort is to reduce boot up times of msfconsole/msfvenom but it also has added benefits of enforcing consistent organization fo the code and will allow us to hopefully transition to rails 6 and it's built in use of zeitwerk
This first PR tackles the
lib/msf/core
folder, a vast majority of requires which reference this path are being removed and the entire folder is being managed by zeitwerk so any constants referenced that live in this folder will be loaded when needed. Testing locally on Ubuntu this resulted in a 0.7s boot time reduction with more gains expected as this effort continues.Future PRs will follow a similar pattern to this one targeting a subsection of the library folders until as much as we can reasonably manage with zeitwerk is done.
More work and testing still needs done with this PR (for example deleting rather than commenting out the requires), all entry points (msfconsole, msfvenom, msfdb, etc.) all need to be tested and working too.
Opening this up as a draft to get some eyes on it and let the tests run through