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

Fix failing ldap server tests #18850

Merged
merged 1 commit into from Feb 19, 2024

Conversation

adfoster-r7
Copy link
Contributor

@adfoster-r7 adfoster-r7 commented Feb 16, 2024

Fix failing ldap server tests introduced by #18678 - by no longer creating real TCP or UDP sockets

Verification

  • Ensure CI passes

Ensure tests pass locally when port 40000 is already bound:

$ ncat -lnvp 40000
Ncat: Version 7.94 ( https://nmap.org/ncat )
Ncat: Listening on [::]:40000
Ncat: Listening on 0.0.0.0:40000

Before:

$ bundle exec rspec ./spec/lib/rex/proto/ldap/server_spec.rb
...

Randomized with seed 44755
Rex::Proto::LDAP::Server ..F...FF.F...

  1) Rex::Proto::LDAP::Server#stop stops the server when running
     Failure/Error: @tcp_sock = Rex::Socket::TcpServer.create(sock_options)
     
     Rex::BindFailed:
       The address is already in use or unavailable: (0.0.0.0:40000).
 

After:

$ bundle exec rspec ./spec/lib/rex/proto/ldap/server_spec.rb
...
Finished in 0.04572 seconds (files took 6.97 seconds to load)
13 examples, 0 failures

@@ -15,7 +15,32 @@

let(:response) {}

let(:tcp_server_socket) do
double :tcp_server_socket,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not possible to use instance_double Rex::Socket::TcpServer here for an InstanceVerifyingDouble - as behind the scenes in result of Rex::Socket::TcpServer.create(...) is a ::Socket instance that's been monkey patched with modules like Rex::Socket::TcpServer etc, which makes it awkward to use instance_double here:

[5] pry(#<Rex::Proto::LDAP::Server>)> whereami

From: /Users/user/Documents/code/metasploit-framework/lib/rex/proto/ldap/server.rb:102 Rex::Proto::LDAP::Server#start:

     97:           end
     98: 
     99:           if serve_tcp
    100:             @tcp_sock = Rex::Socket::TcpServer.create(sock_options)
    101:             require 'pry-byebug'; binding.pry
 => 102:             tcp_sock.on_client_connect_proc = proc do |cli|
    103:               on_client_connect(cli)
    104:             end
    105:             tcp_sock.on_client_data_proc = proc do |cli|
    106:               on_client_data(cli)
    107:             end

[6] pry(#<Rex::Proto::LDAP::Server>)> @tcp_sock.class
=> Socket
[7] pry(#<Rex::Proto::LDAP::Server>)> @tcp_sock.singleton_class.included_modules
=> [Rex::Socket::TcpServer,
 Rex::IO::StreamServer,
 Rex::Socket,
 Net::BER::BERParser,
  ...]

Copy link
Contributor

Choose a reason for hiding this comment

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

If possible can you explain why that was happening?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For unit tests we generally want to avoid creating real servers etc. So I've gone with mocking the Rex::Socket::TcpServer.create(...) call.

The rex-socket implementation uses Ruby's API for creating a socket here:

https://github.com/rapid7/rex-socket/blob/fba62cdf31e2804b91667b34759a31da823bbeb7/lib/rex/socket/tcp_server.rb#L36-L40

And here:

https://github.com/rapid7/rex-socket/blob/fba62cdf31e2804b91667b34759a31da823bbeb7/lib/rex/socket/comm/local.rb#L170-L175

Then adds additional methods and state onto the socket instance with .extend ... here:

https://github.com/rapid7/rex-socket/blob/fba62cdf31e2804b91667b34759a31da823bbeb7/lib/rex/socket/tcp_server.rb#L54

Let me know if I haven't answered your question 😄

@adfoster-r7
Copy link
Contributor Author

cc @JustAnda7 - does this look okay to you as fix? 🤞

@JustAnda7
Copy link
Contributor

Thanks for correcting my mistakes.

@dwelch-r7 dwelch-r7 self-assigned this Feb 19, 2024
@dwelch-r7 dwelch-r7 merged commit 6db865a into rapid7:master Feb 19, 2024
35 checks passed
@dwelch-r7 dwelch-r7 added the rn-fix release notes fix label Feb 19, 2024
@dwelch-r7
Copy link
Contributor

dwelch-r7 commented Feb 19, 2024

Release Notes

Fixes failing ldap server tests

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

Successfully merging this pull request may close these issues.

None yet

3 participants