[
https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12903995#action_12903995
]
Aaron T. Myers commented on THRIFT-876:
---------------------------------------
bq. 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.
bq. Since TSaslTransport itself isn't meant to be used by users, perhaps it
should be package-protected?
Done.
bq. 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.
bq. 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).
bq. 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.
bq. 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. :)
bq. 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.
bq. 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.
bq. 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.
> Add SASL support
> ----------------
>
> Key: THRIFT-876
> URL: https://issues.apache.org/jira/browse/THRIFT-876
> Project: Thrift
> Issue Type: New Feature
> Components: Java - Library
> Reporter: Aaron T. Myers
> Assignee: Aaron T. Myers
> Attachments: thrift-876.txt
>
>
> It'd be nice if there were some way of securing Thrift communication in a
> pluggable fashion. SASL is the implementation chosen by Hadoop for this.
> Seems like a good option for Thrift, too.
> I'll start with a Java implementation, then move on to support the other
> language bindings.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
