Refactoring Net::SSH: Part 5
After nearly a month of refactoring Net::SSH to take advantage of dependency injection (via the Needle framework) visible progress has been made: the transport layer works!
(A quick review: the transport layer of the SSH2 protocol deals with the low-level workings of SSH. It manages the algorithm negotiation, key exchange, encryption/compression of packets, and so forth. All other parts of the SSH protocol are built on top of the abstraction provided by the transport layer.)
How the Session is Initialized
As some of you may recall, the last article in this series ended on a question as I debated what approach to take regarding the initialization of the Session class. After all of that, Eivind Eklund suggested that I just pass the container to the Session constructor and let the Session initialize itself a la carte:
container.register( :session, :model => :prototype ) do |c,p|
Session.new( c.logs.get(p.fullname), c )
end
At first, I was opposed to this approach, since it didn’t feel like dependency injection to me. The fact is, it isn’t. It’s the service locator pattern, which is closely related.
However, after some thought I realized that the price of being a purist in this case would mean more work for me. A good programmer should be able to use all of the tools at his disposal, understanding where they make sense and where they don’t. In this case, the service locator was a more efficient approach due to the large number of dependencies that Session had.
Further Refactoring, and Lessons Learned
The Session class itself was a beast (see the original sources). In order to tame it a bit and make it unit-testable, I moved the version and algorithm negotiation pieces into two different classes. This trimmed it down nicely, although it is still pretty big.
This most recent refactoring has finally hammered home a lesson that I feel ready to pen (though I’m certain others have already discovered and codified it elsewhere):
If you have a private method of a class that you feel needs unit testing, that method probably belongs in a class of its own.
In other words, suppose you have a private method foo that is called by a public method bar:
class Agnathostomata
def bar
...
foo
...
end
def foo
...
end
private :foo
end
When it comes time to test the class, you discover that you would like to unit test the foo method directly. Your options in this case are to (a) make foo public, or (b) circumvent the access control via send or instance_eval. Neither are very attractive options.
What I discovered is that it is probably better, in many cases, to move foo (and any related functionality) into a class of its own, making foo public in the process. Then, the first class delegates that functionality to the new class, like this:
class Spiloma
def foo
...
end
end
class Agnathostomata
def initialize
@delegate = Spiloma.new
end
def bar
...
@delegate.foo
...
end
end
This is exactly how the new VersionNegotiator and AlgorithmNegotiator classes came to be.
Results
I had been unit testing every piece this entire time, so I felt confident that there wouldn’t be any significant bugs. However, the pieces I hadn’t tested so far were the parts that dealt with the dependency injection. Naturally, when it came time to run the integration test, those were the parts that failed first. :)
The problems weren’t significant, however. Usually only a few typos or scoping issues. There was one place where I had indicated the maximum number of bits in the key should be 8196, when it should have been 8192, and that caused me a bit of grief, but I found and fixed it.
The integration test was simple: for every combination of cryptography backend, host key, kex algorithm, cipher algorithm, HMAC algorithm, and compression algorithm, run a test that opened a new connection via the transport Session, sent a message and received a response, and then closed the connection.
Voila!
backends.each do |backend|
keys.each do |key|
kexs.each do |kex|
encryptions.each do |encryption|
hmacs.each do |hmac|
compressions.each do |compression|
method_name = "test_#{backend}__#{key}__#{kex}__#{encryption}__#{hmac}__#{compression}"
method_name.gsub!( /-/, "_" )
define_method( method_name ) do
@registry.register( :crypto_backend ) { backend }
session = @registry.transport.session
assert_nothing_raised do
session.open "localhost",
:host_key => key,
:kex => kex,
:encryption => encryption,
:hmac => hmac,
:compression => compression
end
assert_equal key, session.algorithms.host_key
assert_equal kex, session.algorithms.kex
assert_equal encryption, session.algorithms.encryption_c2s
assert_equal encryption, session.algorithms.encryption_s2c
assert_equal hmac, session.algorithms.mac_c2s
assert_equal hmac, session.algorithms.mac_s2c
assert_equal compression, session.algorithms.compression_c2s
assert_equal compression, session.algorithms.compression_s2c
type = nil
assert_nothing_raised do
session.send_message "#{session.class::SERVICE_REQUEST.chr}\0\0\0\14ssh-userauth"
type, buffer = session.wait_for_message
end
assert_equal session.class::SERVICE_ACCEPT, type
session.close
end
end
end
end
end
end
end
The result?
$ ruby test_integration.rb Loaded suite test_integration Started ......................................................................... ......................................................................... .............. Finished in 21.566316 seconds. 160 tests, 1760 assertions, 0 failures, 0 errors
It’s alive!
Future Directions
Well, as I said earlier, the transport layer is just one piece (albeit a very fundamental one) of the SSH protocol stack. So, the parts that remain to be done are:
- User authentication
- Connection management (channels, etc.)
- SFTP protocol
- Various convenience interfaces
I figure I might be about half done with the entire refactoring. Regardless, once I’m done I’ll release Net::SSH 0.2.0, and will finally be able to proceed with all the parts that are still missing from it.
