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

Test move logging to core #17506

Closed

Conversation

adfoster-r7
Copy link
Contributor

Extension of #17495
Updating the Gemfile to pull in the gem to see if CI will pass

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
Contributor

Already looking better than the bloodbath in mine, thank you.

@adfoster-r7
Copy link
Contributor Author

adfoster-r7 commented Jan 19, 2023

Looks like it would cause breakages if we merged the rex-core/framework PR combo

  1) modules/payloads windows/meterpreter/reverse_https_proxy it should behave like payload cached size is consistent windows/meterpreter/reverse_https_proxy can load 'payload/stagers/windows/reverse_https_proxy'
     Failure/Error:
       log_sink ||= Rex::Logging::LogSinkFactory.new(
         log_sink_name,
         Msf::Config.log_directory + File::SEPARATOR + "framework.log"
       )

     NameError:
       uninitialized constant Rex::Logging::LogSinkFactory
       Did you mean?  LoginDataProxy

@sempervictus
Copy link
Contributor

I updated the other side to explicitly require 'rex/logging' from core.rb to avoid reliance on auto-loading of dependencies. Lets see what it says after that change.

@adfoster-r7
Copy link
Contributor Author

Looks like it hits the same issue; Probably worth checking out locally and verifying - but I think it'll require a few more requires/autoload definitions in rex-core/framework

@sempervictus
Copy link
Contributor

Zeitwerk related - all the require statements were removed. Restored those.

@adfoster-r7
Copy link
Contributor Author

@sempervictus Looks like that went green now; I'd prefer to land this after the 6.3 release though just to be sure there's no gremlins hiding 😄

Should be the next week or so for that

@sempervictus
Copy link
Contributor

@adfoster-r7 - ping for liveness now that 6.3's been out a bit.

@adfoster-r7
Copy link
Contributor Author

ping for liveness now that 6.3's been out a bit.

It's on the TODO list to check if there's an easy way to preserve history in a way that we'd expect, but it's still pretty far down on the TODO list

Is this migration impacting anything on your end? 👀

@sempervictus
Copy link
Contributor

Doesn't impact my end, just keeps folks from using rex-socket stag without Msf :).
Can we have the code live in both repos until the git thing's figured out? That way users can grab Rex gems for their use while Msf keeps git history on this. Even if Zeitwerk reloads it for some reason... its identical code :)

@adfoster-r7
Copy link
Contributor Author

Will close this off to 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 👍

@adfoster-r7 adfoster-r7 closed this Jun 9, 2023
@adfoster-r7 adfoster-r7 added the attic Older submissions that we still want to work on again label Jun 9, 2023
@github-actions
Copy link

github-actions bot commented Jun 9, 2023

Thanks for your contribution to Metasploit Framework! We've looked at this pull request, and we agree that it seems like a good addition to Metasploit, but it looks like it is not quite ready to land. We've labeled it attic and closed it for now.

What does this generally mean? It could be one or more of several things:

  • It doesn't look like there has been any activity on this pull request in a while
  • We may not have the proper access or equipment to test this pull request, or the contributor doesn't have time to work on it right now.
  • Sometimes the implementation isn't quite right and a different approach is necessary.

We would love to land this pull request when it's ready. If you have a chance to address all comments, we would be happy to reopen and discuss how to merge this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attic Older submissions that we still want to work on again
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants