[jira] Commented: (THRIFT-876) Add SASL support

["Todd Lipcon (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12903930#action_12903930 ] 

Todd Lipcon commented on THRIFT-876:
------------------------------------

- The debug messages are good, but any of them that do any computation should be guarded by LOGGER.isDebugEnabled(). eg:
{code}
if (LOGGER.isDebugEnabled()) {
    LOGGER.debug("writing initial response: {}", new String(response));
}
{code}
(otherwise they'll slow down people even when logs are off)

- 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"
- 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.
- 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)
- In the tests, do you need to use a different port for each test? Better to just use ServerTestBase.PORT for both, I think
- In tests, just make the methods throw Exception, rather than catching exceptions and calling fail()
- For extra brownie points (perhaps in a separate JIRA) it would be great if there were an example in tutorial/java/ that used sasl

- 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.


> 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.