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