Drill-4335: C++ client changes for supporting encryption using SASL by sohami · Pull Request #809 · apache/drill

sohami · 2017-04-07T05:15:26Z

No description provided.

sohami · 2017-04-25T21:46:00Z

Adding @laurentgo, @sudheeshkatkam, @BitBlender to help review the changes.

karthik-man · 2017-05-02T22:41:25Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+    // Split the encoded message into the rawWrapSendSize and then encrypt each chunk. Each encrypted chunk along with
+    // its encrypted length in network order (added by Cyrus-SASL plugin) is sent over wire.
+    const int wrapChunkSize = m_encryptionCtxt.getRawWrapSendSize();
+    int numChunks = ceil((double)m_wbuf.size() / wrapChunkSize);


'chunksRemaining' would be better a better name than 'numChunks' because the value is decremented per chunk

karthik-man · 2017-05-02T22:42:20Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+    int numChunks = ceil((double)m_wbuf.size() / wrapChunkSize);
+    int lengthToEncrypt = m_wbuf.size();
+    int currentChunkLen = std::min(wrapChunkSize, lengthToEncrypt);
+    uint32_t startIndex = 0, wrappedLen = 0;


"currentChunkOffset" would be a better name than 'startIndex'.

karthik-man · 2017-05-02T22:44:17Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+            errorMsg << "Sasl wrap failed while encrypting chunk of length: " << currentChunkLen << " , EncodeError: "
+                     << wrapResult;
+            DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "DrillClientImpl::sendSyncEncrypted - " << errorMsg.str()
+                                              << " ,StartIndex: " << startIndex << ". ChunkNum: " << numChunks


Did you want to print the chunk number or chunks remaining? ChunkNum should be Total chunks - Chunks Remaining.

karthik-man · 2017-05-02T22:45:45Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+
+        if(ec || s==0){
+            errorMsg << "Failure while sending encrypted chunk. Error: " << ec.message().c_str();
+            DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "DrillClientImpl::sendSyncEncrypted - " << errorMsg.str() << " Chunk:"


Did you want to print the chunk number or chunks remaining? ChunkNum should be Total chunks - Chunks Remaining.

karthik-man · 2017-05-02T22:55:36Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+            DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "DrillClientImpl::sendSyncEncrypted - " << errorMsg.str() << " Chunk:"
+                                              << numChunks << ", Original Length: " << currentChunkLen
+                                              << ", StartIndex:" << startIndex << ", Closing connection.";)
+            return handleConnError(CONN_FAILURE, getMessage(ERR_CONN_WFAIL, errorMsg.str().c_str()));


What happens to the memory allocated for wrappedChunk when this path is taken? Does handleConnError() deal with it ?

karthik-man · 2017-05-08T22:31:43Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+        return handleQryError(QRY_INTERNAL_ERROR, getMessage(ERR_QRY_INVREADLEN), NULL);
+    }
+
+    return error ? handleQryError(QRY_COMM_ERROR, getMessage(ERR_QRY_COMMERR, error.message().c_str()), NULL)


what happens to memory allocated for bufferWithLenBytes on error?

Memory for bufferWithLenBytes is deallocated by caller. Added a comment in the function header.

karthik-man · 2017-05-08T22:40:13Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+ *      status_t    - QRY_SUCCESS   - In case of success.
+ *                  - QRY_COMM_ERROR/QRY_INTERNAL_ERROR/QRY_CLIENT_OUTOFMEM - In cases of error.
+ */
+status_t DrillClientImpl::readMsg(ByteBuf_t _buf, AllocatedBufferPtr* allocatedBuffer,


For the sake of consistency, I would recommend not using parameter names with a '' prefix. AFAIK '' prefix variables are used for class member names. Also 'inBuf' would be a better name than 'buf'

Was kept as is based on previous implementation. Will change it to inBuf.

karthik-man · 2017-05-08T22:45:02Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

    {
        // We need to protect the readLength and read buffer, and the pending requests counter,
        // but we don't have to keep the lock while we decode the rest of the buffer.
        boost::lock_guard<boost::mutex> lock(this->m_dcMutex);


lock usage seems to be inconsistent with the comment. the lock remains taken when decode() is called.

preserved the old behavior.

karthik-man · 2017-05-08T22:47:07Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+ *  Read all the encrypted chunk needed to form a complete RPC message. Read an entire chunk from network, decrypt it
+ *  and put in a buffer. The same process is repeated until the entire buffer to form a completed RPC message is read.
+ *  Parameters:
+ *      _buf            - in param  - ByteBuf_t containing atleast the length bytes.


For the sake of consistency, I would recommend not using parameter names with a '' prefix. AFAIK '' prefix variables are used for class member names.

Same as above.

karthik-man · 2017-05-08T22:48:30Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+    {
+        // We need to protect the readLength and read buffer, and the pending requests counter,
+        // but we don't have to keep the lock while we decode the rest of the buffer.
+        boost::lock_guard<boost::mutex> lock(this->m_dcMutex);


lock is still held when unwrap is called.

Same as above. the decode referenced here is not unwrap but decode function below which creates a valid RpcMsg.

sohami

Need to look more on below comment, it looks like on ConnError/QryError we are just closing the socket. I am not sure if socket is dead then do we re-use DrillClientImpl instance or not. If not then in DrillClientImpl destructor, m_saslAuthenticator instance is deleted which will take care of freeing memory for decode/encode. If we re-use the DrillClientImpl instance then there can be leaks for m_saslAuthenticator

What happens to the memory allocated for wrappedChunk when this path is taken? Does handleConnError() deal with it ?

sohami · 2017-05-09T15:54:47Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+    }
+
+    // Loop through the property to find USERPROP_ENCRYPTION and it's value
+    for (size_t i = 0; i < userProperties->size(); i++) {


DrillUserProperties has a map and a vector. The vector stores the actual prop key/value pair and in map we store the prop key and corresponding bits indicating USERPROP_FLAGS_SERVERPROP|USERPROP_FLAGS_STRING. This is later used in handshake message to filter out the properties passed by client and the one which is needed for server side to send along with handshake msg.

sohami · 2017-05-09T16:04:04Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

        }
    }

+    std::stringstream errorMsg;


Fixed name and a bug where in success case with auth only it was printing wrong error message.

sohami · 2017-05-09T16:11:02Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+ *      bufWithLen          - in param  - buffer containing the bytes which has length of the RPC message/encrypted chunk
+ *      bufferWithLenBytes  - out param - buffer pointer which points to memory allocated in this function and has the
+ *                                        entire one RPC message / encrypted chunk along with the length of the message
+ *      lengthBytesLength   - out param - bytes of bufWithLen which contains the length of the entire RPC message or


Changed to "lengthFieldLength" as on Java side which is borrowed from LengthFieldBasedFrameDecoder

sohami · 2017-05-09T17:47:19Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+ *  Decode the length of the message from bufWithLen and then read entire message from the socket.
+ *  Parameters:
+ *      bufWithLen          - in param  - buffer containing the bytes which has length of the RPC message/encrypted chunk
+ *      bufferWithLenBytes  - out param - buffer pointer which points to memory allocated in this function and has the


bufferWithLenBytes may not be a complete RPC Msg. Changed to bufWithLen --> bufWithLenField and bufferWithLenBytes --> bufWithDataAndLenBytes

sohami · 2017-05-09T17:55:31Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+    *bufferWithLenBytes = NULL;
+    size_t bufferWithLenBytesSize = 0;
+
+    bytes_read = (this->*lengthDecodeHandler)(bufWithLen, rmsgLen);


lengthDecoder is a function pointer signature that accepts function of DrillClientImpl class with that signature.

sohami · 2017-05-09T22:08:42Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

    {
        // We need to protect the readLength and read buffer, and the pending requests counter,
        // but we don't have to keep the lock while we decode the rest of the buffer.
        boost::lock_guard<boost::mutex> lock(this->m_dcMutex);


preserved the old behavior.

sohami · 2017-05-09T22:10:54Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+    {
+        // We need to protect the readLength and read buffer, and the pending requests counter,
+        // but we don't have to keep the lock while we decode the rest of the buffer.
+        boost::lock_guard<boost::mutex> lock(this->m_dcMutex);


Same as above. the decode referenced here is not unwrap but decode function below which creates a valid RpcMsg.

sohami · 2017-05-10T21:44:19Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+ *  Read all the encrypted chunk needed to form a complete RPC message. Read an entire chunk from network, decrypt it
+ *  and put in a buffer. The same process is repeated until the entire buffer to form a completed RPC message is read.
+ *  Parameters:
+ *      _buf            - in param  - ByteBuf_t containing atleast the length bytes.


Same as above.

sohami · 2017-05-10T21:47:13Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+ *  Return:
+ *      size_t	- length bytes read to decode
+ */
+size_t DrillClientImpl::rpcLengthDecode(const ByteBuf_t _buf, uint32_t& rmsgLen) {


lengthDecode or rpcLengthDecode is passed as function pointer to readlenBytesFromSocket based on encryption is enabled or not. The function pointer signature accepting these functions is:
typedef size_t (DrillClientImpl::*lengthDecoder)(const ByteBuf_t, uint32_t&); which means a fynction of this signature and belonging to class DrillClientImpl. Hence the wrapper is created.

sohami · 2017-05-10T23:38:55Z

contrib/native/client/src/clientlib/utils.cpp

+EncryptionContext::EncryptionContext() {
+    this->m_bEncryptionReqd = false;
+    // SASL Framework only allows 3 octet length field during negotiation so maximum wrap message
+    // length can be 16MB i.e. 0XFFFFFF


Nice catch!. I am changing it to be 65536 now based on recent findings.

karthik-man

Have some questions. Otherwise, LGTM. +1

karthik-man · 2017-05-19T06:42:20Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+ *      errorCode    -  out param - Error code set by boost.
+ */
+void DrillClientImpl::doWriteToSocket(const char* dataPtr, size_t bytesToWrite,
+                                        boost::system::error_code& errorCode) {


Should you check for a NULL dataPtr ?

Not really. Since write_some will set the proper error code in that case. The handing for bytesToWrite == 0 was done since that's a success case and didn't want to call write_some on it.

karthik-man · 2017-05-19T06:45:37Z

contrib/native/client/src/clientlib/drillClientImpl.cpp

+                                       boost::system::error_code& errorCode) {
+
+    // Check if bytesToRead is zero
+    if(0 == bytesToRead) {


Does a NULL inBuf have to be handled ?

Same as above.

sohami · 2017-05-20T03:10:36Z

Squashed commits.

NOTE: This pull request provides support for on-wire encryption using SASL framework. Communication channel covered is: 1) C++ Drill Client and Drillbit channel.

amansinha100 · 2017-05-20T23:51:47Z

+1

sohami force-pushed the DRILL-4335-C++-04_06_2017 branch from 8b711ba to ee915e1 Compare April 7, 2017 22:34

sohami force-pushed the DRILL-4335-C++-04_06_2017 branch 2 times, most recently from 328b228 to 2997bbf Compare April 20, 2017 06:05

sohami force-pushed the DRILL-4335-C++-04_06_2017 branch from 2997bbf to 293fc2d Compare May 4, 2017 21:43

karthik-man suggested changes May 8, 2017

View reviewed changes

sohami force-pushed the DRILL-4335-C++-04_06_2017 branch from 293fc2d to 2ade1d4 Compare May 11, 2017 23:33

sohami commented May 12, 2017

View reviewed changes

sohami force-pushed the DRILL-4335-C++-04_06_2017 branch from 2ade1d4 to 695d836 Compare May 12, 2017 21:13

karthik-man approved these changes May 19, 2017

View reviewed changes

sohami force-pushed the DRILL-4335-C++-04_06_2017 branch from 695d836 to 2af2d63 Compare May 20, 2017 03:10

DRILL-4335: Apache Drill should support network encryption.

8f39be2

NOTE: This pull request provides support for on-wire encryption using SASL framework. Communication channel covered is: 1) C++ Drill Client and Drillbit channel.

sohami force-pushed the DRILL-4335-C++-04_06_2017 branch from 2af2d63 to 8f39be2 Compare May 20, 2017 17:32

asfgit closed this in d11aba2 May 20, 2017

Conversation

sohami commented Apr 7, 2017

Uh oh!

sohami commented Apr 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sohami left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karthik-man left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sohami commented May 20, 2017

Uh oh!

amansinha100 commented May 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!