THRIFT-912. java: Fix some bugs in SASL implementation, update protoc…

…ol spec slightly

git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@1001973 13f79535-47bb-0310-9956-ffa450edef68

doc/thrift-sasl-spec.txt

@@ -1,6 +1,5 @@
-A Thrift SASL message shall be a byte array of one of the following forms:
+A Thrift SASL message shall be a byte array of the following form:
 
-| 1-byte START status code | 1-byte mechanism name length | variable length mechanism name | 4-byte payload length | variable-length payload |
 | 1-byte status code | 4-byte payload length | variable-length payload |
 
 The length fields shall be interpreted as integers, with the high byte sent
@@ -24,15 +23,10 @@ underlying SASL security mechanism that it supports.
 name -> mechanism options.
 
 3. At connection time, the client will initiate communication by sending the
-server a START byte, followed by a 1-byte field indicating the length in bytes
-of the underlying security mechanism name that the client would like to use.
+server a START message. The payload of this message will be the name of the
+underlying security mechanism that the client would like to use.
 This mechanism name shall be 1-20 characters in length, and follow the
-specifications for SASL mechanism names specified in RFC 2222. This mechanism
-name shall be followed by a 4-byte, potentially zero-value message length word,
-followed by a potentially zero-length payload. The payload is determined by the
-output byte array of the underlying actual security mechanism, and will be
-empty except for those underlying security protocols which implement the
-optional SASL initial response.
+specifications for SASL mechanism names specified in RFC 2222.
 
 4. The server receives this message and, if the mechanism name provided is
 among the set of mechanisms this server transport is configured to accept,
@@ -44,18 +38,25 @@ status code or message indicating failure. No further communication may take
 place via this transport. If the mechanism name is one which the server
 supports, then proceed to step 5.
 
-5. The server then provides the byte array of the payload received to its
+5. Following the START message, the client must send another message containing
+the "initial response" of the chosen SASL implementation. The client may send
+this message piggy-backed on the "START" message of step 3. The message type
+of this message must be either "OK" or "COMPLETE", depending on whether the
+SASL implementation indicates that this side of the authentication has been
+satisfied.
+
+6. The server then provides the byte array of the payload received to its
 underlying security mechanism. A challenge is generated by the underlying
 security mechanism on the server, and this is used as the payload for a message
 sent to the client. This message shall consist of an OK byte, followed by the
 non-zero message length word, followed by the payload.
 
-6. The client receives this message from the server and passes the payload to
+7. The client receives this message from the server and passes the payload to
 its underlying security mechanism to generate a response. The client then sends
 the server an OK byte, followed by the non-zero-value length of the response,
 followed by the bytes of the response as the payload.
 
-7. Steps 5 and 6 are repeated until both security mechanisms are satisfied with
+8. Steps 6 and 7 are repeated until both security mechanisms are satisfied with
 the challenge/response exchange. When either side has completed its security
 protocol, its next message shall be the COMPLETE byte, followed by a 4-byte
 potentially zero-value length word, followed by a potentially zero-length
@@ -78,18 +79,18 @@ be passed to the protocol above the thrift transport by whatever mechanism is
 appropriate and idiomatic for the particular language these thrift bindings are
 for.
 
-If step 7 completes successfully, then the communication is considered
+If step 8 completes successfully, then the communication is considered
 authenticated and subsequent communication may commence.
 
-If step 7 fails to complete successfully, then no further communication may
+If step 8 fails to complete successfully, then no further communication may
 take place via this transport.
 
 8. All writes to the underlying transport must be prefixed by the 4-byte length
 of the payload data, followed by the payload. All reads from this transport
 should read the 4-byte length word, then read the full quantity of bytes
 specified by this length word.
 
-If no SASL QOP (quality of protection) is negotiated during steps 5 and 6, then
+If no SASL QOP (quality of protection) is negotiated during steps 6 and 7, then
 all subsequent writes to/reads from this transport are written/read unaltered,
 save for the length prefix, to the underlying transport.
 

lib/java/src/org/apache/thrift/transport/TSaslClientTransport.java

@@ -75,6 +75,12 @@ public TSaslClientTransport(String mechanism, String authorizationId, String pro
     this.mechanism = mechanism;
   }
 
+
+  @Override
+  protected SaslRole getRole() {
+    return SaslRole.CLIENT;
+  }
+
   /**
    * Performs the client side of the initial portion of the Thrift SASL
    * protocol. Generates and sends the initial response to the server, including
@@ -88,21 +94,15 @@ protected void handleSaslStartMessage() throws TTransportException, SaslExceptio
     if (saslClient.hasInitialResponse())
       initialResponse = saslClient.evaluateChallenge(initialResponse);
 
-    byte[] mechanismBytes = mechanism.getBytes();
-    byte[] messageHeader = new byte[STATUS_BYTES + MECHANISM_NAME_BYTES + mechanismBytes.length
-        + PAYLOAD_LENGTH_BYTES];
-
-    messageHeader[0] = START;
-    messageHeader[1] = (byte) (0xff & mechanismBytes.length);
-    System.arraycopy(mechanismBytes, 0, messageHeader, STATUS_BYTES + MECHANISM_NAME_BYTES,
-        mechanismBytes.length);
-    EncodingUtils.encodeBigEndian(initialResponse.length, messageHeader, STATUS_BYTES
-        + MECHANISM_NAME_BYTES + mechanismBytes.length);
-
     LOGGER.debug("Sending mechanism name {} and initial response of length {}", mechanism,
         initialResponse.length);
-    underlyingTransport.write(messageHeader);
-    underlyingTransport.write(initialResponse);
+
+    byte[] mechanismBytes = mechanism.getBytes();
+    sendSaslMessage(NegotiationStatus.START,
+                    mechanismBytes);
+    // Send initial response
+    sendSaslMessage(saslClient.isComplete() ? NegotiationStatus.COMPLETE : NegotiationStatus.OK,
+                    initialResponse);
     underlyingTransport.flush();
   }
 }

lib/java/src/org/apache/thrift/transport/TSaslServerTransport.java

@@ -108,6 +108,11 @@ public void addServerDefinition(String mechanism, String protocol, String server
         props, cbh));
   }
 
+  @Override
+  protected SaslRole getRole() {
+    return SaslRole.SERVER;
+  }
+
   /**
    * Performs the server side of the initial portion of the Thrift SASL protocol.
    * Receives the initial response from the client, creates a SASL server using
@@ -116,35 +121,24 @@ public void addServerDefinition(String mechanism, String protocol, String server
    */
   @Override
   protected void handleSaslStartMessage() throws TTransportException, SaslException {
-    // Get the status byte and length of the mechanism name.
-    byte[] messageHeader = new byte[STATUS_BYTES + MECHANISM_NAME_BYTES];
-    underlyingTransport.readAll(messageHeader, 0, messageHeader.length);
-    LOGGER.debug("Received status {} and mechanism name length {}", messageHeader[0],
-        messageHeader[1]);
-    if (messageHeader[0] != START) {
-      sendAndThrowMessage(ERROR, "Expecting START status, received " + messageHeader[0]);
+    SaslResponse message = receiveSaslMessage();
+
+    LOGGER.debug("Received start message with status {}", message.status);
+    if (message.status != NegotiationStatus.START) {
+      sendAndThrowMessage(NegotiationStatus.ERROR, "Expecting START status, received " + message.status);
     }
 
     // Get the mechanism name.
-    byte[] mechanismBytes = new byte[messageHeader[1]];
-    underlyingTransport.readAll(mechanismBytes, 0, mechanismBytes.length);
-
-    String mechanismName = new String(mechanismBytes);
-    TSaslServerDefinition serverDefinition = serverDefinitionMap.get(new String(mechanismBytes));
+    String mechanismName = new String(message.payload);
+    TSaslServerDefinition serverDefinition = serverDefinitionMap.get(mechanismName);
     LOGGER.debug("Received mechanism name '{}'", mechanismName);
 
     if (serverDefinition == null) {
-      sendAndThrowMessage(BAD, "Unsupported mechanism type " + mechanismName);
+      sendAndThrowMessage(NegotiationStatus.BAD, "Unsupported mechanism type " + mechanismName);
     }
     SaslServer saslServer = Sasl.createSaslServer(serverDefinition.mechanism,
         serverDefinition.protocol, serverDefinition.serverName, serverDefinition.props,
         serverDefinition.cbh);
-
-    // Evaluate the initial response and send the first challenge.
-    byte[] initialResponse = new byte[readLength()];
-    sendSaslMessage(saslServer.isComplete() ? COMPLETE : OK, saslServer
-        .evaluateResponse(initialResponse));
-
     setSaslServer(saslServer);
   }
 
@@ -221,7 +215,7 @@ public TTransport getTransport(TTransport base) {
           ret.open();
         } catch (TTransportException e) {
           LOGGER.debug("failed to open server transport", e);
-          return null;
+          throw new RuntimeException(e);
         }
         transportMap.put(base, ret);
       } else {