[
https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904339#action_12904339
]
Aaron T. Myers commented on THRIFT-876:
---------------------------------------
Thanks a lot for the review, Bryan.
bq. Just so I can get a sense of what you're trying to give here, should this
be like an encrypted version of the framed transport? Where should people be
plugging this in? Right above their socket, or as the last TTransport before
the client?
This is an authenticated and potentially encrypted version of the framed
transport, but with variable-length frames. Whether or not encryption or
integrity checking is actually performed depends upon the negotiated SASL
mechanism. This could theoretically be plugged in anywhere in the transport
stack, though higher is probably better, as in the event wrapping for
encryption/integrity checking is performed, doing fewer/larger reads/writes
will be preferred.
bq. The rest of Thrift uses 2-space indentation, and you have 4-space
My apologies. Fixed.
bq. Regards use of TFramedTransport.encodeFrameSize: if we're going to use this
same code in a variety of places, not just from within TFramedTransport and its
pseudo-descendant TFastFramedTransport, we should move it out into a separate
utility class
Totally agree. I didn't want to do this in the first place, as it seemed like
spaghetti code, but then I saw that TFramedTransport.(en|de)codeFrameSize is
also referenced in TAsyncMethodCall, so it seemed like this was accepted
practice.
I'm happy to move it into a utility class. Do you have thoughts as to where
this should go / what it should be named?
bq. Can you reuse the same empty byte[] in TSaslClientTransport.open()? Does it
actually do anything?
Certainly can. Fixed. It is an oddity of the signature of
SaslClient.evaluateChallenge() that this is indeed necessary.
bq. TSaslClientTransport doesn't protect itself from being open()ed twice. Not
sure if this is important, but might be a good idea to mimic other transports.
Fixed. Also amended the test.
bq. In TSaslTransport.postOpen(), there's no reason to instantiate your
TMemoryInputTransport with a new zero-length byte[]. Clearly you're going to
reset it with one of an actual size later, so don't waste the allocation.
Fixed. Thanks.
bq. You should reuse a single 4-byte buffer in TSaslTransport for encoding and
decoding the frame size.
I thought about doing this, but it seemed like just unnecessarily introducing a
non-thread safe artifact. Maybe it's implicit in the design of the transport
classes that they are not intended to be thread safe, and if so, I'm happy to
do it.
bq. You don't need to close your TMemoryInputTransports, since they're memory.
Fixed.
> 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, thrift-876.txt.2
>
>
> 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.
