[GitHub] merlimat commented on a change in pull request #1129: Added end to end encryption in C++ client
merlimat commented on a change in pull request #1129: Added end to end
encryption in C++ client
URL: https://github.com/apache/incubator-pulsar/pull/1129#discussion_r164924605
##
File path: pulsar-client-cpp/lib/ConsumerConfiguration.cc
##
@@ -74,4 +74,25 @@ void
ConsumerConfiguration::setUnAckedMessagesTimeoutMs(const uint64_t milliSeco
}
impl_->unAckedMessagesTimeoutMs = milliSeconds;
}
+
+bool ConsumerConfiguration::isEncryptionEnabled() const {
+return (impl_->cryptoKeyReader != NULL);
+}
+
+const CryptoKeyReader& ConsumerConfiguration::getCryptoKeyReader() const {
return *(impl_->cryptoKeyReader); }
+
+ConsumerConfiguration&
ConsumerConfiguration::setCryptoKeyReader(CryptoKeyReader& cryptoKeyReader) {
Review comment:
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.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #1129: Added end to end encryption in C++ client
merlimat commented on a change in pull request #1129: Added end to end
encryption in C++ client
URL: https://github.com/apache/incubator-pulsar/pull/1129#discussion_r164523814
##
File path: pulsar-client-cpp/lib/MessageCrypto.h
##
@@ -0,0 +1,95 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#ifndef LIB_MESSAGECRYPTO_H_
+#define LIB_MESSAGECRYPTO_H_
+
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "SharedBuffer.h"
+#include "ExecutorService.h"
+#include "pulsar/CryptoKeyReader.h"
+#include "PulsarApi.pb.h"
+
+namespace pulsar {
+
+class MessageCrypto {
+ public:
+typedef std::map StringMap;
+typedef std::map> DataKeyCacheMap;
+
+MessageCrypto(std::string& logCtx, bool keyGenNeeded);
+~MessageCrypto();
+
+Result addPublicKeyCipher(std::set& keyNames, const
CryptoKeyReader& keyReader);
+bool removeKeyCipher(std::string& keyName);
+
+bool encrypt(std::set& encKeys, const CryptoKeyReader&
keyReader,
+proto::MessageMetadata& msgMetadata, SharedBuffer& payload,
SharedBuffer& encryptedPayload);
+bool decrypt(const proto::MessageMetadata& msgMetadata, SharedBuffer&
payload,
+const CryptoKeyReader& keyReader, SharedBuffer& decryptedPayload);
+
+ private:
+
+typedef boost::unique_lock Lock;
+boost::mutex mutex_;
+
+int dataKeyLen_;
+std::unique_ptr dataKey_;
+
+int tagLen_;
+int ivLen_;
+std::unique_ptr iv_;
Review comment:
same here, `boost::scoped_ptr`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #1129: Added end to end encryption in C++ client
merlimat commented on a change in pull request #1129: Added end to end
encryption in C++ client
URL: https://github.com/apache/incubator-pulsar/pull/1129#discussion_r164524553
##
File path: 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;
if (!batchMessageContainer) {
// If batching is enabled we compress all the payloads together before
sending the batch
payload =
CompressionCodecProvider::getCodec(conf_.getCompressionType()).encode(payload);
+compressedSize = payload.readableBytes();
+
+// Encrypt the payload if enabled
+SharedBuffer encryptedPayload;
Review comment:
Check whether it's required before creating the `SharedBuffer`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #1129: Added end to end encryption in C++ client
merlimat commented on a change in pull request #1129: Added end to end
encryption in C++ client
URL: https://github.com/apache/incubator-pulsar/pull/1129#discussion_r164522672
##
File path: pulsar-client-cpp/include/pulsar/CryptoKeyReader.h
##
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#ifndef LIB_CRYPTOKEYREADER_H_
+#define LIB_CRYPTOKEYREADER_H_
+
+#include
+#include
+
+#pragma GCC visibility push(default)
+
+using namespace pulsar;
+
+namespace pulsar {
+
+class CryptoKeyReader {
+
+ public:
Review comment:
Formatting will need to be adjusted, using the `make format` command
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #1129: Added end to end encryption in C++ client
merlimat commented on a change in pull request #1129: Added end to end
encryption in C++ client
URL: https://github.com/apache/incubator-pulsar/pull/1129#discussion_r164524311
##
File path: 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;
Review comment:
The ways it sounds, it's a bit confusing ;)
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #1129: Added end to end encryption in C++ client
merlimat commented on a change in pull request #1129: Added end to end encryption in C++ client URL: https://github.com/apache/incubator-pulsar/pull/1129#discussion_r164524955 ## File path: 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 Review comment: The expectation here is that there will be a single tgz left after the build This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat commented on a change in pull request #1129: Added end to end encryption in C++ client
merlimat commented on a change in pull request #1129: Added end to end
encryption in C++ client
URL: https://github.com/apache/incubator-pulsar/pull/1129#discussion_r164523212
##
File path: 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;
Review comment:
Creating the `SharedBuffer` will need to malloc some stuff, can we just
doing when encryption is enabled?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #1129: Added end to end encryption in C++ client
merlimat commented on a change in pull request #1129: Added end to end
encryption in C++ client
URL: https://github.com/apache/incubator-pulsar/pull/1129#discussion_r164523753
##
File path: pulsar-client-cpp/lib/MessageCrypto.h
##
@@ -0,0 +1,95 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#ifndef LIB_MESSAGECRYPTO_H_
+#define LIB_MESSAGECRYPTO_H_
+
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "SharedBuffer.h"
+#include "ExecutorService.h"
+#include "pulsar/CryptoKeyReader.h"
+#include "PulsarApi.pb.h"
+
+namespace pulsar {
+
+class MessageCrypto {
+ public:
+typedef std::map StringMap;
+typedef std::map> DataKeyCacheMap;
+
+MessageCrypto(std::string& logCtx, bool keyGenNeeded);
+~MessageCrypto();
+
+Result addPublicKeyCipher(std::set& keyNames, const
CryptoKeyReader& keyReader);
+bool removeKeyCipher(std::string& keyName);
+
+bool encrypt(std::set& encKeys, const CryptoKeyReader&
keyReader,
+proto::MessageMetadata& msgMetadata, SharedBuffer& payload,
SharedBuffer& encryptedPayload);
+bool decrypt(const proto::MessageMetadata& msgMetadata, SharedBuffer&
payload,
+const CryptoKeyReader& keyReader, SharedBuffer& decryptedPayload);
+
+ private:
+
+typedef boost::unique_lock Lock;
+boost::mutex mutex_;
+
+int dataKeyLen_;
+std::unique_ptr dataKey_;
Review comment:
I think we should stick with `boost::scoped_ptr` instead of
`std::unique_ptr` which depends on C++11
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
