The debug messages are good, but any of them that do any computation should be guarded by LOGGER.isDebugEnabled(). eg:
I just removed the 2 places where said debug messages were doing computation. Those messages were only really appropriate for SASL mechanisms with text-based protocols (e.g. not Kerberos), and those mechanisms should have their own logging in place to show the contents of the messages they're passing.
Since TSaslTransport itself isn't meant to be used by users, perhaps it should be package-protected?
SaslParticipant should be marked static. Also I think it would be better to move it down to the bottom of the containing class, since it's not "interesting"
Done and done.
readLength(): I think there's currently a bug since you don't mask the bytes by 0xff before upcasting to int. I bet if you made your test message 130 characters or so you'd see an interesting failure.
Done. I just made readLength() and writeLength() use TFramedTransport's decodeFrameSize() and encodeFrameSize() methods, much as TAsyncMethodCall does. Kind of unfortunate that this exact same i32 bit-shifting is implemented in at least two places in the project (TFramedTransport and TBinaryProtocol).
Efficiency-wise, in both readLength() and read() you can use the buffer peeking capability in TTransport - see TBinaryProtocol.java:read* functions for example. For read() you can pass the underlying buffer right on through to sasl.unwrap() to avoid a copy. If you want to do these improvements in a followup JIRA, that seems reasonable. (also, if you do these, you should make sure that the tests exercise the functionality - you'll probably need two more tests where there's a TFramedTransport underlying transport)
I'd like to hold off on this. Certainly good to do, but seems worthy of another JIRA to me.
In the tests, do you need to use a different port for each test? Better to just use ServerTestBase.PORT for both, I think
Done. Though I was closing the socket returned by the accept() call, I wasn't closing the server socket on which the accept() call was made. I fixed this, and now made it use ServerTestBase.PORT. In short, necessary because I had a bug.
In tests, just make the methods throw Exception, rather than catching exceptions and calling fail()
Done where possible. Couldn't do this for the exceptions thrown in the server's run() method, as I can't change the method's signature.
For extra brownie points (perhaps in a separate JIRA) it would be great if there were an example in tutorial/java/ that used sasl
I started to do this, but the the existing tutorials need some work. I'll make a new JIRA to clean those up and add a SASL example in there.
Lastly, is there any thought of adding a "mechanism negotiation" phase? Most SASL implementations handshake to pick a mechanism before they get going - I guess you could do this with another class like TSaslNegotiator(Map<Integer, SaslClient>) or something? Again fine to push this to a later JIRA.
Interesting thought. This would serve to shift the responsibility for defining security requirements to the client. Not obvious to me that this is necessarily desirable, as we'd either have to design a secure mechanism negotiation protocol, or be susceptible to attackers rather trivially fooling both the client and the server into using the weakest mechanism from among the list of available mechanisms. This would seem to defeat the purpose of offering stronger mechanisms in the first place. I suppose the way this could be helpful would be if there were an AuthorizeCallback which made different decisions based on the strength of the actual security mechanism being used (i.e. you can do operation X if we're using Kerberos, but not if we're using CRAM-MD5.)
I guess what I'm trying to say is: let's hold off on that until it's clear that library users want it.