You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/12/29 04:38:21 UTC
[GitHub] [pulsar] tuteng opened a new pull request #9074: [feature][cpp-client]Expose cpp end to end encryption interface
tuteng opened a new pull request #9074:
URL: https://github.com/apache/pulsar/pull/9074
### Motivation
Currently some users want to use end-to-end encryption on other clients, such as python or node clients, and this pr is used to expose the end-to-end encryption interface.
### Modifications
* Add a default class `DefaultCryptoKeyReader` to implement reading public and private keys
* The client calls the `pulsar_consumer_configuration_set_default_crypto_key_reader` function to specify the path of the public and private keys to be passed to the cpp client
* Add `DefaultCryptoKeyReader` class to the test
### Verifying this change
* Update test
*(Please pick either of the following options)*
This change is a trivial rework / code cleanup without any test coverage.
*(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
*(or)*
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (yes / no)
- The public API: (yes / no)
- The schema: (yes / no / don't know)
- The default values of configurations: (yes / no)
- The wire protocol: (yes / no)
- The rest endpoints: (yes / no)
- The admin cli options: (yes / no)
- Anything that affects deployment: (yes / no / don't know)
### Documentation
- Does this pull request introduce a new feature? (yes / no)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
- If a feature is not applicable for documentation, explain why?
- If a feature is not documented yet in this PR, please create a followup issue for adding the documentation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng commented on pull request #9074: [feature][cpp-client]Expose cpp end to end encryption interface
Posted by GitBox <gi...@apache.org>.
tuteng commented on pull request #9074:
URL: https://github.com/apache/pulsar/pull/9074#issuecomment-752745931
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #9074: [feature][cpp-client]Expose cpp end to end encryption interface
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #9074:
URL: https://github.com/apache/pulsar/pull/9074#discussion_r549653212
##########
File path: pulsar-client-cpp/include/pulsar/c/producer_configuration.h
##########
@@ -63,8 +63,17 @@ typedef enum {
pulsar_AutoPublish = -4,
} pulsar_schema_type;
+typedef enum {
+ // This is the default option to fail send if crypto operation fails
+ pulsar_producerFail,
+ // Ignore crypto failure and proceed with sending unencrypted messages
+ pulsar_producerSend
Review comment:
The naming style should be consistent with other enum values, i.e. change `pulsar_producerFail` to `pulsar_ProducerFail`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng commented on a change in pull request #9074: [feature][cpp-client]Expose cpp end to end encryption interface
Posted by GitBox <gi...@apache.org>.
tuteng commented on a change in pull request #9074:
URL: https://github.com/apache/pulsar/pull/9074#discussion_r549712070
##########
File path: pulsar-client-cpp/tests/BasicEndToEndTest.cc
##########
@@ -1323,7 +1324,14 @@ TEST(BasicEndToEndTest, testRSAEncryption) {
std::string subName = "my-sub-name";
Producer producer;
- std::shared_ptr<EncKeyReader> keyReader = std::make_shared<EncKeyReader>();
+ std::string PUBLIC_CERT_FILE_PATH =
Review comment:
There is another test method `testEncryptionFailure` that needs to use this mock class, testEncryptionFailure mainly tests for some failure cases, it may be more appropriate to use this mock class
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng commented on a change in pull request #9074: [feature][cpp-client]Expose cpp end to end encryption interface
Posted by GitBox <gi...@apache.org>.
tuteng commented on a change in pull request #9074:
URL: https://github.com/apache/pulsar/pull/9074#discussion_r550159530
##########
File path: pulsar-client-cpp/tests/BasicEndToEndTest.cc
##########
@@ -1323,7 +1324,14 @@ TEST(BasicEndToEndTest, testRSAEncryption) {
std::string subName = "my-sub-name";
Producer producer;
- std::shared_ptr<EncKeyReader> keyReader = std::make_shared<EncKeyReader>();
+ std::string PUBLIC_CERT_FILE_PATH =
Review comment:
Thanks, I have removed this class `EncKeyReader`, PTAL @BewareMyPower
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #9074: [feature][cpp-client]Expose cpp end to end encryption interface
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #9074:
URL: https://github.com/apache/pulsar/pull/9074#issuecomment-752004288
@BewareMyPower Can you review this pull request?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie merged pull request #9074: [feature][cpp-client]Expose cpp end to end encryption interface
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #9074:
URL: https://github.com/apache/pulsar/pull/9074
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #9074: [feature][cpp-client]Expose cpp end to end encryption interface
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #9074:
URL: https://github.com/apache/pulsar/pull/9074#discussion_r549653887
##########
File path: pulsar-client-cpp/tests/BasicEndToEndTest.cc
##########
@@ -1323,7 +1324,14 @@ TEST(BasicEndToEndTest, testRSAEncryption) {
std::string subName = "my-sub-name";
Producer producer;
- std::shared_ptr<EncKeyReader> keyReader = std::make_shared<EncKeyReader>();
+ std::string PUBLIC_CERT_FILE_PATH =
Review comment:
Should `EncKeyReader` in `testEncryptionFailure` also be replaced with the default crypto key reader? Then the implementation of `EncKeyReader` could be removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #9074: [feature][cpp-client]Expose cpp end to end encryption interface
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #9074:
URL: https://github.com/apache/pulsar/pull/9074#discussion_r549908284
##########
File path: pulsar-client-cpp/tests/BasicEndToEndTest.cc
##########
@@ -1323,7 +1324,14 @@ TEST(BasicEndToEndTest, testRSAEncryption) {
std::string subName = "my-sub-name";
Producer producer;
- std::shared_ptr<EncKeyReader> keyReader = std::make_shared<EncKeyReader>();
+ std::string PUBLIC_CERT_FILE_PATH =
Review comment:
But the implementation difference between `EncKeyReader` and `DefaultCryptoKeyReader` is just that `EncKeyReader` uses hard coding path while `DefaultCryptoKeyReader` does not, right? The tests cases of `testEncryptionFailure` are mocked by `prodConf.addEncryptionKey(path)` but not the `EncKeyReader` itself.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #9074: [feature][cpp-client]Expose cpp end to end encryption interface
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #9074:
URL: https://github.com/apache/pulsar/pull/9074#issuecomment-752026774
@sijie OK
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng commented on pull request #9074: [feature][cpp-client]Expose cpp end to end encryption interface
Posted by GitBox <gi...@apache.org>.
tuteng commented on pull request #9074:
URL: https://github.com/apache/pulsar/pull/9074#issuecomment-752293545
I have updated the code PTAL @BewareMyPower
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng commented on pull request #9074: [feature][cpp-client]Expose cpp end to end encryption interface
Posted by GitBox <gi...@apache.org>.
tuteng commented on pull request #9074:
URL: https://github.com/apache/pulsar/pull/9074#issuecomment-752293343
> Why the keys configuration is not configured in the PulsarClient rather in the producer and consumer creation?
End-to-end encryption is for producers and consumers, but not for clients. Clients can connect normally even if the encryption is misconfigured, and the key is only configured when sending messages, but can only be read, not set, on the consumer side. @zymap
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org