Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.5
    • Component/s: Java - Library
    • Labels:
      None

      Description

      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.

      1. thrift-876.txt
        25 kB
        Aaron T. Myers
      2. thrift-876.txt.2
        25 kB
        Aaron T. Myers
      3. thrift-876.txt.3
        23 kB
        Aaron T. Myers
      4. thrift-876.txt.5
        42 kB
        Aaron T. Myers
      5. thrift-876.txt.6
        50 kB
        Aaron T. Myers
      6. thrift-876.txt.7
        49 kB
        Aaron T. Myers
      7. thrift-sasl-spec.txt
        6 kB
        Aaron T. Myers
      8. thrift-sasl-spec.txt
        6 kB
        Aaron T. Myers

        Issue Links

          Activity

          Hide
          Bryan Duxbury added a comment -

          Great, I just committed this. Thanks for the monster contribution, Aaron!

          Show
          Bryan Duxbury added a comment - Great, I just committed this. Thanks for the monster contribution, Aaron!
          Hide
          Aaron T. Myers added a comment -

          Updated to address Bryan and Todd's comments.

          Show
          Aaron T. Myers added a comment - Updated to address Bryan and Todd's comments.
          Hide
          Aaron T. Myers added a comment -

          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.

          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.

          Show
          Aaron T. Myers added a comment - 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. 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.
          Hide
          Bryan Duxbury added a comment -

          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.

          Show
          Bryan Duxbury added a comment - 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.
          Hide
          Aaron T. Myers added a comment -

          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.

          Another nit: the empty @param entries in the JavaDoc just take up space and don't say anything.

          I'll remove them.

          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.

          Show
          Aaron T. Myers added a comment - 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. Another nit: the empty @param entries in the JavaDoc just take up space and don't say anything. I'll remove them. 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.
          Hide
          Todd Lipcon added a comment -

          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

          Show
          Todd Lipcon added a comment - 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
          Hide
          Aaron T. Myers added a comment -

          New patch attached.

          I think EncodingUtils.encodeInteger would be more properly named encodeBigEndian or something like that.

          Done.

          Should TSaslClientTransport's mechanism field be final?

          Done.

          You seem to have a no-content comment in TSaslTransport above messageHeader.

          Fixed. Thanks for catching that.

          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.

          When you get a BAD or ERROR response, you should probably throw the error with new String(payload, "UTF-8"), right?

          Done.

          TSaslTransport.readLength still calls TFramedTransport.decodeFrameSize.

          Whoops! Fixed.

          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.

          Show
          Aaron T. Myers added a comment - New patch attached. I think EncodingUtils.encodeInteger would be more properly named encodeBigEndian or something like that. Done. Should TSaslClientTransport's mechanism field be final? Done. You seem to have a no-content comment in TSaslTransport above messageHeader. Fixed. Thanks for catching that. 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. When you get a BAD or ERROR response, you should probably throw the error with new String(payload, "UTF-8"), right? Done. TSaslTransport.readLength still calls TFramedTransport.decodeFrameSize. Whoops! Fixed. 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.
          Hide
          Bryan Duxbury added a comment -

          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.

          Show
          Bryan Duxbury added a comment - 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.
          Hide
          Aaron T. Myers added a comment -

          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.

          Show
          Aaron T. Myers added a comment - 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.
          Hide
          Aaron T. Myers added a comment -

          Updated spec.

          Show
          Aaron T. Myers added a comment - Updated spec.
          Hide
          David Reiss added a comment -

          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.

          Show
          David Reiss added a comment - 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.
          Hide
          Aaron T. Myers added a comment -

          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.

          Show
          Aaron T. Myers added a comment - 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.
          Hide
          Bryan Duxbury added a comment -

          We should specify this for the same reason that we specify that the ints over the wire are big-endian.

          +1

          Show
          Bryan Duxbury added a comment - We should specify this for the same reason that we specify that the ints over the wire are big-endian. +1
          Hide
          David Reiss added a comment -

          Woudlnt' the messages come from the SASL implementation, rather than the Thrift glue?

          Show
          David Reiss added a comment - Woudlnt' the messages come from the SASL implementation, rather than the Thrift glue?
          Hide
          Todd Lipcon added a comment -

          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.

          Show
          Todd Lipcon added a comment - 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.
          Hide
          David Reiss added a comment -

          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.

          Show
          David Reiss added a comment - 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.
          Hide
          Todd Lipcon added a comment -

          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.

          Show
          Todd Lipcon added a comment - 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.
          Hide
          Bryan Duxbury added a comment -

          If you just throw a framed transport on top, the data will be buffered twice on the read side, which seems unnecessary.

          Show
          Bryan Duxbury added a comment - If you just throw a framed transport on top, the data will be buffered twice on the read side, which seems unnecessary.
          Hide
          David Reiss added a comment -

          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.

          Show
          David Reiss added a comment - 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.
          Hide
          Bryan Duxbury added a comment -

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

          Show
          Bryan Duxbury added a comment - 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()).
          Hide
          Aaron T. Myers added a comment -

          Overall I like it.

          Glad to hear 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.

          I'll defer to Todd on this one, as it was his idea to specify this.

          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.

          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.

          Show
          Aaron T. Myers added a comment - Overall I like it. Glad to hear 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. I'll defer to Todd on this one, as it was his idea to specify this. 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. 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.
          Hide
          David Reiss added a comment -

          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?

          Show
          David Reiss added a comment - 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?
          Hide
          Aaron T. Myers added a comment -

          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.

          Show
          Aaron T. Myers added a comment - 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.
          Hide
          Aaron T. Myers added a comment -

          Addressing Bryan's comments.

          Show
          Aaron T. Myers added a comment - Addressing Bryan's comments.
          Hide
          Aaron T. Myers added a comment -

          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.

          Show
          Aaron T. Myers added a comment - 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.
          Hide
          Aaron T. Myers added a comment -

          Thanks a lot for the review, Bryan.

          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.

          The rest of Thrift uses 2-space indentation, and you have 4-space

          My apologies. Fixed.

          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?

          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.

          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.

          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.

          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.

          You don't need to close your TMemoryInputTransports, since they're memory.

          Fixed.

          Show
          Aaron T. Myers added a comment - Thanks a lot for the review, Bryan. 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. The rest of Thrift uses 2-space indentation, and you have 4-space My apologies. Fixed. 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? 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. 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. 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. 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. You don't need to close your TMemoryInputTransports, since they're memory. Fixed.
          Hide
          Bryan Duxbury added a comment -

          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.
          Show
          Bryan Duxbury added a comment - 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.
          Hide
          Bryan Duxbury added a comment -

          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.

          Show
          Bryan Duxbury added a comment - 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.
          Hide
          Todd Lipcon added a comment -

          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.

          Show
          Todd Lipcon added a comment - 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.
          Hide
          Todd Lipcon added a comment -

          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.

          Show
          Todd Lipcon added a comment - 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.
          Hide
          David Reiss added a comment -

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

          Show
          David Reiss added a comment - > 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.
          Hide
          Todd Lipcon added a comment -

          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.

          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.

          Show
          Todd Lipcon added a comment - 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. 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.
          Hide
          David Reiss added a comment -

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

          Show
          David Reiss added a comment - > 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.
          Hide
          Todd Lipcon added a comment -

          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.

          Show
          Todd Lipcon added a comment - 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.
          Hide
          David Reiss added a comment -

          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?

          Show
          David Reiss added a comment - 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?
          Hide
          Aaron T. Myers added a comment -

          Updated patch incorporating Todd's comments.

          Show
          Aaron T. Myers added a comment - Updated patch incorporating Todd's comments.
          Hide
          Aaron T. Myers added a comment -

          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.

          Since TSaslTransport itself isn't meant to be used by users, perhaps it should be package-protected?

          Done.

          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.

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

          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.

          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.

          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.

          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.

          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.

          Show
          Aaron T. Myers added a comment - 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. Since TSaslTransport itself isn't meant to be used by users, perhaps it should be package-protected? Done. 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. 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). 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. 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. 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. 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. 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.
          Hide
          Todd Lipcon added a comment -
          • The debug messages are good, but any of them that do any computation should be guarded by LOGGER.isDebugEnabled(). eg:
            if (LOGGER.isDebugEnabled()) {
                LOGGER.debug("writing initial response: {}", new String(response));
            }
            

            (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.
          Show
          Todd Lipcon added a comment - The debug messages are good, but any of them that do any computation should be guarded by LOGGER.isDebugEnabled(). eg: if (LOGGER.isDebugEnabled()) { LOGGER.debug( "writing initial response: {}" , new String (response)); } (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.
          Hide
          Aaron T. Myers added a comment -

          Implementation of TSaslClientTransport and TSaslServerTransport. Includes one helper class in addition and one test class.

          Show
          Aaron T. Myers added a comment - Implementation of TSaslClientTransport and TSaslServerTransport. Includes one helper class in addition and one test class.

            People

            • Assignee:
              Aaron T. Myers
              Reporter:
              Aaron T. Myers
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development