Added end to end encryption in C++ client #1129

saandrews · 2018-01-29T06:22:58Z

Motivation

C++ client implementation to support end to end encryption based on PIP4

It supports RSA encryption at the moment.
Added unit test and added option to the perf test to send and receive encrypted messages.
Tested compatibility for encrypted messages between Java and C++ client

merlimat

Change LGTM, just few minor comment/suggestions

merlimat · 2018-01-29T18:35:40Z

pulsar-client-cpp/include/pulsar/CryptoKeyReader.h

+
+class CryptoKeyReader {
+
+ public:


Formatting will need to be adjusted, using the make format command

merlimat · 2018-01-29T18:37:47Z

pulsar-client-cpp/lib/BatchMessageContainer.cc

@@ -93,6 +93,10 @@ void BatchMessageContainer::sendMessage() {
    impl_->metadata.set_num_messages_in_batch(messagesContainerListPtr_->size());
    compressPayLoad();

+    SharedBuffer encryptedPayload;


Creating the SharedBuffer will need to malloc some stuff, can we just doing when encryption is enabled?

You are right. I have allocated a byte to have valid object. Will try to refactor it before merge.

Checking again, it seems we are ok to without allocating anything as well.
https://github.com/apache/incubator-pulsar/blob/master/pulsar-client-cpp/lib/SharedBuffer.h#L46
The ptr will be null and will be an issue if we use the buffer at this point. We will error out if encryption fails anyway, so guaranteed to have non null if success. Do you see any issues @merlimat ?

merlimat · 2018-01-29T18:39:47Z

pulsar-client-cpp/lib/MessageCrypto.h

+    boost::mutex mutex_;
+
+    int dataKeyLen_;
+    std::unique_ptr<unsigned char []>  dataKey_;


I think we should stick with boost::scoped_ptr instead of std::unique_ptr which depends on C++11

Switched to scoped_array

merlimat · 2018-01-29T18:40:01Z

pulsar-client-cpp/lib/MessageCrypto.h

+
+    int tagLen_;
+    int ivLen_;
+    std::unique_ptr<unsigned char []> iv_;


same here, boost::scoped_ptr

Switched to scoped_array

merlimat · 2018-01-29T18:41:51Z

pulsar-client-cpp/lib/ProducerImpl.cc

@@ -262,12 +295,21 @@ void ProducerImpl::sendAsync(const Message& msg, SendCallback callback) {
    SharedBuffer& payload = msg.impl_->payload;

    uint32_t uncompressedSize = payload.readableBytes();
+    uint32_t compressedSize = uncompressedSize;


The ways it sounds, it's a bit confusing ;)

:) Agree. Changed to generic name.

merlimat · 2018-01-29T18:42:37Z

pulsar-client-cpp/lib/ProducerImpl.cc

+        compressedSize = payload.readableBytes();
+
+        // Encrypt the payload if enabled
+        SharedBuffer encryptedPayload;


Check whether it's required before creating the SharedBuffer

merlimat · 2018-01-29T18:43:52Z

pulsar-client-cpp/run-unit-tests.sh

@@ -23,7 +23,9 @@

 rm -rf ./pulsar-dist
 mkdir pulsar-dist
-tar xfz ../all/target/apache-pulsar*bin.tar.gz  -C pulsar-dist --strip-components 1
+for i in `ls ../all/target/apache-pulsar*bin.tar.gz`; do


The expectation here is that there will be a single tgz left after the build

Thats right. I ended up with multiple bin.tar.gz files and tar failed in Mac.

jai1 · 2018-01-29T22:39:29Z

pulsar-client-cpp/include/pulsar/ConsumerCryptoFailureAction.h

+#ifndef LIB_CONSUMERCRYPTOFAILUREACTION_H_
+#define LIB_CONSUMERCRYPTOFAILUREACTION_H_
+
+using namespace pulsar;


Remove this line

jai1 · 2018-01-29T22:41:59Z

pulsar-client-cpp/lib/BatchMessageContainer.h

@@ -40,6 +40,7 @@
 #include "ExecutorService.h"
 #include <boost/asio.hpp>
 #include "ProducerImpl.h"
+#include "MessageCrypto.h"


Is this required?

jai1 · 2018-01-29T22:42:23Z

pulsar-client-cpp/lib/ConsumerConfiguration.cc

@@ -74,4 +74,30 @@ void ConsumerConfiguration::setUnAckedMessagesTimeoutMs(const uint64_t milliSeco
    }
    impl_->unAckedMessagesTimeoutMs = milliSeconds;
 }
+
+bool ConsumerConfiguration::isEncryptionEnabled() const {
+    if (impl_->cryptoKeyReader != NULL) {


return impl_->cryptoKeyReader != NULL

jai1 · 2018-01-29T22:45:48Z

pulsar-client-cpp/lib/ConsumerConfigurationImpl.h

@@ -31,13 +31,18 @@ struct ConsumerConfigurationImpl {
    int receiverQueueSize;
    std::string consumerName;
    long brokerConsumerStatsCacheTimeInMs;
+    CryptoKeyReader* cryptoKeyReader;


Can we use shared pointer instead like we do for MessageRouter

Slightly different use case here. It has a reference to the application allocated implementation of CryptoKeyReader

jai1 · 2018-01-29T22:56:54Z

pulsar-client-cpp/lib/ProducerImpl.cc

-      sendTimer_() {
+      sendTimer_(),
+      msgCrypto_(),
+      dataKeyGenInterval_(4 * 60 * 60) {


dataKeyGenIntervalInMs_ (time unit missing)

merlimat · 2018-01-31T00:42:54Z

pulsar-client-cpp/lib/ConsumerConfiguration.cc

+
+const CryptoKeyReader& ConsumerConfiguration::getCryptoKeyReader() const { return *(impl_->cryptoKeyReader); }
+
+ConsumerConfiguration& ConsumerConfiguration::setCryptoKeyReader(CryptoKeyReader& cryptoKeyReader) {


Instead of getting a reference and storing a pointer, we should take either a copy or a shared_ptr from the user. That would avoid any problem on the life cycle of the crypto reader. Otherwise the use needs to know it cannot be destructed and that will lead to unexpected segfaults.

Switched to shared_ptr.

merlimat · 2018-01-31T00:45:07Z

@saandrews Change looks good. Just the comment on using shared_ptr instead of relying on user to keep the object alive.

merlimat

👍

…_capture_response Capture response for broker interceptor.

saandrews added the component/c++ label Jan 29, 2018

saandrews added this to the 1.22.0-incubating milestone Jan 29, 2018

saandrews requested review from merlimat, rdhabalia and jai1 January 29, 2018 18:19

merlimat added the type/feature The PR added a new feature or issue requested a new feature label Jan 29, 2018

merlimat assigned saandrews Jan 29, 2018

merlimat reviewed Jan 29, 2018

View reviewed changes

saandrews force-pushed the cppencryption branch from e894175 to ac32356 Compare January 29, 2018 19:45

jai1 reviewed Jan 29, 2018

View reviewed changes

saandrews force-pushed the cppencryption branch 2 times, most recently from d104d05 to dfac0da Compare January 30, 2018 22:24

merlimat reviewed Jan 31, 2018

View reviewed changes

saandrews force-pushed the cppencryption branch from dfac0da to 7ad8797 Compare January 31, 2018 18:44

merlimat approved these changes Jan 31, 2018

View reviewed changes

saandrews force-pushed the cppencryption branch 5 times, most recently from 660e99c to 3b70624 Compare February 2, 2018 00:05

Added end to end encryption in C++ client

5896b55

saandrews force-pushed the cppencryption branch from 3b70624 to 5896b55 Compare February 2, 2018 00:08

merlimat merged commit 9a081ad into apache:master Feb 2, 2018

sijie added a commit to sijie/pulsar that referenced this pull request Aug 8, 2020

Merge pull request apache#1129 from streamnative/penghui/intercetptor…

3e78577

…_capture_response Capture response for broker interceptor.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added end to end encryption in C++ client #1129

Added end to end encryption in C++ client #1129

saandrews commented Jan 29, 2018

merlimat left a comment

merlimat Jan 29, 2018

saandrews Jan 29, 2018

merlimat Jan 29, 2018

saandrews Jan 30, 2018

saandrews Jan 30, 2018

merlimat Jan 29, 2018

saandrews Jan 29, 2018

merlimat Jan 29, 2018

saandrews Jan 29, 2018

merlimat Jan 29, 2018

saandrews Jan 29, 2018

merlimat Jan 29, 2018

merlimat Jan 29, 2018

saandrews Jan 29, 2018

jai1 Jan 29, 2018

jai1 Jan 29, 2018

saandrews Jan 30, 2018

jai1 Jan 29, 2018

saandrews Jan 30, 2018

jai1 Jan 29, 2018

saandrews Jan 30, 2018

jai1 Jan 29, 2018

saandrews Jan 30, 2018

merlimat Jan 31, 2018

saandrews Jan 31, 2018

merlimat commented Jan 31, 2018

merlimat left a comment


		const CryptoKeyReader& ConsumerConfiguration::getCryptoKeyReader() const { return *(impl_->cryptoKeyReader); }

		ConsumerConfiguration& ConsumerConfiguration::setCryptoKeyReader(CryptoKeyReader& cryptoKeyReader) {

Added end to end encryption in C++ client #1129

Added end to end encryption in C++ client #1129

Conversation

saandrews commented Jan 29, 2018

Motivation

merlimat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat commented Jan 31, 2018

merlimat left a comment

Choose a reason for hiding this comment