You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Aaron T. Myers (JIRA)" <ji...@apache.org> on 2010/08/29 06:27:53 UTC
[jira] Created: (THRIFT-876) Add SASL support
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
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.
[jira] Updated: (THRIFT-876) Add SASL support
Posted by "Aaron T. Myers (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Aaron T. Myers updated THRIFT-876:
----------------------------------
Attachment: thrift-876.txt.3
Addressing Bryan's comments.
> 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, thrift-876.txt.3
>
>
> 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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Aaron T. Myers (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12906599#action_12906599 ]
Aaron T. Myers commented on THRIFT-876:
---------------------------------------
bq. Took a quick skim through the new transport factory class. One issue - the new static maps should be wrapped with Collections.synchronizedMap(...) since it's an unsynchronized data structure. I think it's OK, though, that the containsKey() and .put() are unsynchronized, since we assume that concurrent callers of getTransport() will always be passing different base transports.
Good call. I'll fix this in the next patch, after I receive some more comments.
bq. Another nit: the empty @param entries in the JavaDoc just take up space and don't say anything.
I'll remove them.
bq. One last thought: rather than making TSaslServerDefinition a private class and taking the 5 parameters everywhere else, would it make a cleaner API to make it public and force users to pass one into TSaslClientTransport, TSaslServerTransport, addServerDefinition, etc. Feel free to disagree :)
I thought about doing this, but concluded it didn't add much value, and made the interface slightly more difficult to understand. I don't feel strongly about this. Happy to change it if people think it's an improvement.
> 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, thrift-876.txt.3, thrift-876.txt.5, thrift-876.txt.6, thrift-sasl-spec.txt, thrift-sasl-spec.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.
[jira] Updated: (THRIFT-876) Add SASL support
Posted by "Aaron T. Myers (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Aaron T. Myers updated THRIFT-876:
----------------------------------
Attachment: thrift-sasl-spec.txt
Updated spec.
> 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, thrift-876.txt.3, thrift-sasl-spec.txt, thrift-sasl-spec.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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904814#action_12904814 ]
Bryan Duxbury commented on THRIFT-876:
--------------------------------------
bq. We should specify this for the same reason that we specify that the ints over the wire are big-endian.
+1
> 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, thrift-876.txt.3, thrift-sasl-spec.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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Todd Lipcon (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904805#action_12904805 ]
Todd Lipcon commented on THRIFT-876:
------------------------------------
Leaving it up to higher-level code allows there to be disagreement between different languages. The point of a specification is to avoid that confusion. If the idea is that these error messages are user readable text, then they should specify an encoding, and UTF8 is a reasonable choice. We should specify this for the same reason that we specify that the ints over the wire are big-endian.
> 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, thrift-876.txt.3, thrift-sasl-spec.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.
[jira] Updated: (THRIFT-876) Add SASL support
Posted by "Aaron T. Myers (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Aaron T. Myers updated THRIFT-876:
----------------------------------
Attachment: thrift-876.txt.7
Updated to address Bryan and Todd's comments.
> 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, thrift-876.txt.3, thrift-876.txt.5, thrift-876.txt.6, thrift-876.txt.7, thrift-sasl-spec.txt, thrift-sasl-spec.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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12906903#action_12906903 ]
Bryan Duxbury commented on THRIFT-876:
--------------------------------------
Maybe I'm just being dense today, but why would the transport factory get invoked more than once for a given underlying transport? If that is a must-have, then you might be better off always doing the get() first thing then checking if it's null rather than doing a contains() call, as in essence that will lead to two gets for every call.
Everything else looks pretty good.
> 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, thrift-876.txt.3, thrift-876.txt.5, thrift-876.txt.6, thrift-sasl-spec.txt, thrift-sasl-spec.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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904799#action_12904799 ]
David Reiss commented on THRIFT-876:
------------------------------------
I didn't say ASCII. I said bytes. Leave it up to higher level code to decide whether the bytes are UTF-8, Shift JIS, or a PNG image of a sad face.
> 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, thrift-876.txt.3, thrift-sasl-spec.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.
[jira] Updated: (THRIFT-876) Add SASL support
Posted by "Aaron T. Myers (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Aaron T. Myers updated THRIFT-876:
----------------------------------
Attachment: thrift-876.txt
Implementation of TSaslClientTransport and TSaslServerTransport. Includes one helper class in addition and one test class.
> 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.
[jira] Updated: (THRIFT-876) Add SASL support
Posted by "Aaron T. Myers (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Aaron T. Myers updated THRIFT-876:
----------------------------------
Attachment: thrift-876.txt.6
New patch attached.
bq. I think EncodingUtils.encodeInteger would be more properly named encodeBigEndian or something like that.
Done.
bq. Should TSaslClientTransport's mechanism field be final?
Done.
bq. You seem to have a no-content comment in TSaslTransport above messageHeader.
Fixed. Thanks for catching that.
bq. in receiveSaslMessage, you use the set of valid statuses to check if you should throw an exception. You should either make VALID_STATUSES a HashSet (to avoid the linear-time contains check) or make a method called isValidStatus that just checks 0 <= s <= 5.
Changed to a HashSet.
bq. When you get a BAD or ERROR response, you should probably throw the error with new String(payload, "UTF-8"), right?
Done.
bq. TSaslTransport.readLength still calls TFramedTransport.decodeFrameSize.
Whoops! Fixed.
bq. I think your test should include a test that actually uses a complete server, rather than just animating the transports yourself.
Done. Note that I had to make a slight change to ServerTestBase to make this work. ServerTestBase wasn't actually using getTransport(), even though it required subclasses to implement it.
Also, note that I had to create a TTransportFactory for TSaslServerTransport. This implementation involves a bit of a hack to work around the fact that TServers use different TTransport instances for input and output. I'm not in love with this solution, and am very much open to alternatives.
> 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, thrift-876.txt.3, thrift-876.txt.5, thrift-876.txt.6, thrift-sasl-spec.txt, thrift-sasl-spec.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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904743#action_12904743 ]
Bryan Duxbury commented on THRIFT-876:
--------------------------------------
I like the proposal.
The way it's described and implemented now, this is an unbuffered transport on write and a buffered transport on read, which I think will make it hard to use. What if, instead, we make it buffered on both the write and read side and basically be a stand-in for the existing TFramedTransport. That would mean that an entire RPC request or response would fit into a single frame and there'd be no ambiguity about when messages would be sent (it would happen on flush()).
> 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, thrift-876.txt.3, thrift-sasl-spec.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.
[jira] Updated: (THRIFT-876) Add SASL support
Posted by "Aaron T. Myers (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Aaron T. Myers updated THRIFT-876:
----------------------------------
Attachment: thrift-876.txt.5
Updated patch to include the Java implementation of the current version of the Thrift SASL protocol. Note that this patch also includes the protocol description in the doc/ directory.
> 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, thrift-876.txt.3, thrift-876.txt.5, thrift-sasl-spec.txt, thrift-sasl-spec.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.
[jira] Closed: (THRIFT-876) Add SASL support
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury closed THRIFT-876.
--------------------------------
Fix Version/s: 0.5
Resolution: Fixed
Great, I just committed this. Thanks for the monster contribution, Aaron!
> 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
> Fix For: 0.5
>
> Attachments: thrift-876.txt, thrift-876.txt.2, thrift-876.txt.3, thrift-876.txt.5, thrift-876.txt.6, thrift-876.txt.7, thrift-sasl-spec.txt, thrift-sasl-spec.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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904266#action_12904266 ]
Bryan Duxbury commented on THRIFT-876:
--------------------------------------
I'm comfortable with the idea of "optional" features. In practice, almost all new features are optional at some point until people decide to incorporate them. Take the Compact Protocol, for instance - it exists in a bunch of languages but isn't yet everywhere.
> 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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Aaron T. Myers (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904815#action_12904815 ]
Aaron T. Myers commented on THRIFT-876:
---------------------------------------
bq. If you just throw a framed transport on top, the data will be buffered twice on the read side, which seems unnecessary.
This is certainly true, and undesirable.
I think the only thing that would need to change in the protocol specification to support buffering writes would be to also require a length prefix even in the event a QOP is not negotiated/used. I was trying to limit the scope of this specification to only the features strictly required to implement SASL, but this seems like a fine addition to support potentially dramatically improved performance.
I'll go ahead and make that change, as well as the change regarding extra data with the COMPLETE message, and post an updated spec.
If we're cool with it, I'm going to call this protocol spec complete and rework the Java implementation to implement this new spec.
> 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, thrift-876.txt.3, thrift-sasl-spec.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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904813#action_12904813 ]
David Reiss commented on THRIFT-876:
------------------------------------
Woudlnt' the messages come from the SASL implementation, rather than the Thrift glue?
> 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, thrift-876.txt.3, thrift-sasl-spec.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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Aaron T. Myers (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904342#action_12904342 ]
Aaron T. Myers commented on THRIFT-876:
---------------------------------------
bq. I agree that we should address those requirements in a doc. Parts 1 (define the "service name") and 5 (define how authorization is to be interpreted) seem to be inapplicable to Thrift, since really the implementing service should define those things. But the other parts seem useful - especially part 3, which might not be addressed by the current patch - there should be some status codes preceding each part of the SASL handshake in order to pass back a useful error message for failure cases.
Sounds good. I'll work on a proposal for a (very tiny) "Thrift with SASL" protocol.
> 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, thrift-876.txt.3
>
>
> 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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Todd Lipcon (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904032#action_12904032 ]
Todd Lipcon commented on THRIFT-876:
------------------------------------
dreiss and I chatted briefly on IRC and he clarified what he meant above by "add-on component". The distinction here is one of classification - ie we add a document somewhere that enumerates (a) the "core" features that every language implementation must have to be considered "complete", and (b) the "add-on" features that some implementations choose to implement. The "core" features would be things like TBinaryProtocol and a Socket transport. The add-on features would be things like zlib, JSON protocol, HTTP transport, SASL security, etc. The two types of features would live in the same place in the code - we're just making clear to the users that if they use an "add-on" feature it may not be supported by their favorite boutique language.
> 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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Aaron T. Myers (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12906919#action_12906919 ]
Aaron T. Myers commented on THRIFT-876:
---------------------------------------
bq. Maybe I'm just being dense today, but why would the transport factory get invoked more than once for a given underlying transport?
It's because the TServer implementations support having different transports for input and output. See TThreadPoolServer.java:247-248. Even if you only pass in a single TTransportFactory, the server still calls getTransport() twice.
bq. If that is a must-have, then you might be better off always doing the get() first thing then checking if it's null rather than doing a contains() call, as in essence that will lead to two gets for every call.
Thought about doing that. This seemed cleaner. Will change it in the forthcoming patch.
> 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, thrift-876.txt.3, thrift-876.txt.5, thrift-876.txt.6, thrift-sasl-spec.txt, thrift-sasl-spec.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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Aaron T. Myers (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904724#action_12904724 ]
Aaron T. Myers commented on THRIFT-876:
---------------------------------------
bq. Overall I like it.
Glad to hear it.
bq. 1/ As usual, I don't think we should specify UTF-8. SASL is an octet-oriented system. Let's just define all of the messages as bytes.
I'll defer to Todd on this one, as it was his idea to specify this.
bq. 2/ The COMPLETE message should be allowed to contain a payload. What SASL calls "additional data with success". (Why it doesn't allow additional data with failure is beyond me.)
Very good point. I will amend and post a patch of the spec.
bq. 3/ Are you planning on using QOP soon? If not, we should just drop the last paragraph until someone is ready to implement it. If so, why is the extra framing necessary?
The patch I've posted already does indeed use the negotiated QOP, and the tests exercise it. The additional framing is necessary because, for example, if the QOP does integrity assurance, it may do some hashing of the message payload and then include this computed hash in the amended message. The entire message, including this hash, must be read by the receiving end so that it may be passed to the underlying security mechanism for integrity verification.
> 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, thrift-876.txt.3, thrift-sasl-spec.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.
[jira] Updated: (THRIFT-876) Add SASL support
Posted by "Aaron T. Myers (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Aaron T. Myers updated THRIFT-876:
----------------------------------
Attachment: thrift-876.txt.2
Updated patch incorporating Todd's comments.
> 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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Aaron T. Myers (JIRA)" <ji...@apache.org>.
[ 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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904009#action_12904009 ]
David Reiss commented on THRIFT-876:
------------------------------------
I think this is the sort of thing that belongs under contrib or outside of Thrift entirely. Thrift was designed to make it possible for things like this to work properly even when the core is completely unaware of them. I think Thrift is complicated enough already without having to define and integrate a SASL-compatible protocol. Do others agree?
> 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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904019#action_12904019 ]
David Reiss commented on THRIFT-876:
------------------------------------
> I think if this is something that at least a couple people want to use, we should put it in the project.
I actually disagree with that. Every feature we add to Thrift makes it less approachable for everyone who isn't using it. Even if "a couple people" it, we need to consider the cost for everyone else. Otherwise, Thrift will just become a massive unnavigable pile of half-implemented special-purpose features. (Mark Slee wrote a great email about this a few months ago, but I unfortunately can't find it now.)
That said, I think it is also possible bundle "add-on"-type features like this in the main codebase without considering them core components of Thrift. It has the benefit of allowing internal API changes to hit both core and non-core components at the same time, and obviously simplifies dependencies.
If we're going to go ahead with that, I'd feel more comfortable if we all informally agree to think of this as an add-on component, rather than a defining feature of Thrift. In fact, there are a lot of components already in the source tree that I think we should think of like that. Examples include the zlib transport in C++, the disklog transport in Erlang, and (IMO) all of the HTTP clients and servers.
> 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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904271#action_12904271 ]
Bryan Duxbury commented on THRIFT-876:
--------------------------------------
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?
Some comments:
* The rest of Thrift uses 2-space indentation, and you have 4-space
* 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
* Can you reuse the same empty byte[] in TSaslClientTransport.open()? Does it actually do anything?
* 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.
* 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.
* You should reuse a single 4-byte buffer in TSaslTransport for encoding and decoding the frame size.
* You don't need to close your TMemoryInputTransports, since they're memory.
> 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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Todd Lipcon (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904794#action_12904794 ]
Todd Lipcon commented on THRIFT-876:
------------------------------------
bq. As usual, I don't think we should specify UTF-8. SASL is an octet-oriented system. Let's just define all of the messages as bytes.
The reason I suggested UTF8 is that assumedly, authentication errors are meant to be human-readable. Thus, systems may respond with localized errors in non-latin text. "メールアドレスもしくはパスワードが異なります。" doesn't fit in ASCII.
> 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, thrift-876.txt.3, thrift-sasl-spec.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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Aaron T. Myers (JIRA)" <ji...@apache.org>.
[ 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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Todd Lipcon (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12906597#action_12906597 ]
Todd Lipcon commented on THRIFT-876:
------------------------------------
Took a quick skim through the new transport factory class. One issue - the new static maps should be wrapped with Collections.synchronizedMap(...) since it's an unsynchronized data structure.
I think it's OK, though, that the containsKey() and .put() are unsynchronized, since we assume that concurrent callers of getTransport() will always be passing different base transports.
Another nit: the empty @param entries in the JavaDoc just take up space and don't say anything.
One last thought: rather than making TSaslServerDefinition a private class and taking the 5 parameters everywhere else, would it make a cleaner API to make it public and force users to pass one into TSaslClientTransport, TSaslServerTransport, addServerDefinition, etc. Feel free to disagree :)
> 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, thrift-876.txt.3, thrift-876.txt.5, thrift-876.txt.6, thrift-sasl-spec.txt, thrift-sasl-spec.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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904720#action_12904720 ]
David Reiss commented on THRIFT-876:
------------------------------------
Overall I like it.
1/ As usual, I don't think we should specify UTF-8. SASL is an octet-oriented system. Let's just define all of the messages as bytes.
2/ The COMPLETE message should be allowed to contain a payload. What SASL calls "additional data with success". (Why it doesn't allow additional data with failure is beyond me.)
3/ Are you planning on using QOP soon? If not, we should just drop the last paragraph until someone is ready to implement it. If so, why is the extra framing necessary?
> 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, thrift-876.txt.3, thrift-sasl-spec.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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904752#action_12904752 ]
Bryan Duxbury commented on THRIFT-876:
--------------------------------------
If you just throw a framed transport on top, the data will be buffered twice on the read side, which seems unnecessary.
> 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, thrift-876.txt.3, thrift-sasl-spec.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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12905748#action_12905748 ]
Bryan Duxbury commented on THRIFT-876:
--------------------------------------
I think EncodingUtils.encodeInteger would be more properly named encodeBigEndian or something like that.
Should TSaslClientTransport's mechanism field be final?
You seem to have a no-content comment in TSaslTransport above messageHeader.
in receiveSaslMessage, you use the set of valid statuses to check if you should throw an exception. You should either make VALID_STATUSES a HashSet (to avoid the linear-time contains check) or make a method called isValidStatus that just checks 0 <= s <= 5.
When you get a BAD or ERROR response, you should probably throw the error with new String(payload, "UTF-8"), right?
TSaslTransport.readLength still calls TFramedTransport.decodeFrameSize.
I think your test should include a test that actually uses a complete server, rather than just animating the transports yourself.
> 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, thrift-876.txt.3, thrift-876.txt.5, thrift-sasl-spec.txt, thrift-sasl-spec.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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Todd Lipcon (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904030#action_12904030 ]
Todd Lipcon commented on THRIFT-876:
------------------------------------
I agree that we should address those requirements in a doc. Parts 1 (define the "service name") and 5 (define how authorization is to be interpreted) seem to be inapplicable to Thrift, since really the implementing service should define those things. But the other parts seem useful - especially part 3, which might not be addressed by the current patch -- there should be some status codes preceding each part of the SASL handshake in order to pass back a useful error message for failure cases.
> 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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Todd Lipcon (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904012#action_12904012 ]
Todd Lipcon commented on THRIFT-876:
------------------------------------
David: I have mixed feelings about that. It seems the topic of secure transmission has come up a lot on the lists, etc, and SASL is a very common mechanism by which you can achieve it (eg it's used in XMPP, IMAP, and lots of other protocols). Although I don't think it's good to just throw the "kitchen sink" into the project itself, I think if this is something that at least a couple people want to use, we should put it in the project.
So, poll: how many people find this useful outside Cloudera? If it's just us we'll just put it in our own project code.
> 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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "Todd Lipcon (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904021#action_12904021 ]
Todd Lipcon commented on THRIFT-876:
------------------------------------
Where do we draw the line? eg the non-blocking java servers are more complicated to use than the threadpool server, since they require the framed transport, etc. But we do include them in the core codebase.
bq. I actually disagree with that. Every feature we add to Thrift makes it less approachable for everyone who isn't using it. Even if "a couple people" it, we need to consider the cost for everyone else
How is this so? Having the extra code there (so long as it doesn't drag in extra external dependencies) doesn't seem like a real problem. How is it any different if we put it in a "contrib/" directory? In my experience, "contrib" directories usually become a very strange place - half of the components are pieces of code that are long since unmaintained (and don't really work), and others are components that everyone uses every day. This becomes very confusing for users to separate the things that really work from the things that are experiments.
> 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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904747#action_12904747 ]
David Reiss commented on THRIFT-876:
------------------------------------
I think what you would really want to do is throw a framed transport on top of this. Let SASL take care of itself on open, then do normal framed operation afterwards.
> 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, thrift-876.txt.3, thrift-sasl-spec.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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904024#action_12904024 ]
David Reiss commented on THRIFT-876:
------------------------------------
> Where do we draw the line?
That's a good question, and, unfortunately, I don't have a good answer. The design of Thrift is such that almost all of the concrete classes (servers, protocols, and transports) could be moved out of the core and into separate modules and the system would work just fine. My intuition just tells me that SASL is not a core part of RPC.
If you want to move ahead with this, I could be okay with it. If this is going to be committed to the main tree, I think it's necessary to have a definition of the "Thrift with SASL" protocol outside of the Java source, specifically the "profiling requirements" specified by the SASL RFC (http://www.ietf.org/rfc/rfc2222.txt). I think 2 and 3 are the most important. I set up http://wiki.apache.org/thrift/ThriftIntegrationConventions for documenting this sort of thing, or it could just go in a text file under doc.
> 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.
[jira] Commented: (THRIFT-876) Add SASL support
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904817#action_12904817 ]
David Reiss commented on THRIFT-876:
------------------------------------
I don't think that's the same thing. That's necessary for the number to be transmitted from Thrift to Thrift. The higher-level code doesn't have to concern itself with that level. We *don't* specify that images should be transmitted as PNG, though. We just give client code access to the binary type and let it do whatever it wants. Making a protocol SASL-compliant is all about providing a way for implementations to pass bytes back and forth. That's what we should be doing: passing bytes, not putting any interpretation on them.
> 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, thrift-876.txt.3, thrift-sasl-spec.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.
[jira] Updated: (THRIFT-876) Add SASL support
Posted by "Aaron T. Myers (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-876?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Aaron T. Myers updated THRIFT-876:
----------------------------------
Attachment: thrift-sasl-spec.txt
First draft of the "Thrift with SASL" protocol specification. Comments appreciated. My intention is to include this document in the doc/ folder of Thrift repository.
Note that the protocol described is not yet implemented by the previous patch. This is because I expanded the protocol as Todd recommended to include allowing the Thrift server transport to support a list of security mechanisms it will accept, and to require the client to specify which security mechanism it would like to use. Once we nail down this protocol spec, I'll update the patch to implement it precisely.
> 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, thrift-876.txt.3, thrift-sasl-spec.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.