Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feature][cpp-client]Expose cpp end to end encryption interface #9074

Merged
merged 7 commits into from
Dec 31, 2020

Conversation

tuteng
Copy link
Member

@tuteng tuteng commented Dec 29, 2020

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

The end-to-end tests already exist in the cpp client, so let's go ahead and use this example

TEST(BasicEndToEndTest, testRSAEncryption) {
to test our code

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

@tuteng tuteng self-assigned this Dec 29, 2020
@tuteng tuteng added component/c++ area/client type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Dec 29, 2020
@tuteng tuteng added this to the 2.8.0 milestone Dec 29, 2020
@sijie
Copy link
Member

sijie commented Dec 29, 2020

@BewareMyPower Can you review this pull request?

@BewareMyPower
Copy link
Contributor

@sijie OK

@@ -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 =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should EncKeyReader in testEncryptionFailure also be replaced with the default crypto key reader? Then the implementation of EncKeyReader could be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I have removed this class EncKeyReader, PTAL @BewareMyPower

Copy link
Member

@zymap zymap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the keys configuration is not configured in the PulsarClient rather in the producer and consumer creation?

@tuteng
Copy link
Member Author

tuteng commented Dec 30, 2020

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

@tuteng
Copy link
Member Author

tuteng commented Dec 30, 2020

I have updated the code PTAL @BewareMyPower

@tuteng
Copy link
Member Author

tuteng commented Dec 30, 2020

/pulsarbot run-failure-checks

@sijie sijie merged commit 956328d into apache:master Dec 31, 2020
@sijie sijie deleted the fix/expose-cpp-e2e-api branch December 31, 2020 03:44
codelipenghui pushed a commit that referenced this pull request Jan 7, 2021
### 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

The end-to-end tests already exist in the cpp client, so let's go ahead and use this example https://github.com/apache/pulsar/blob/041424cf06f16bedf4ef5787c9b96b7c5daf5fce/pulsar-client-cpp/tests/BasicEndToEndTest.cc#L1320 to test our code

(cherry picked from commit 956328d)
@codelipenghui codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client area/security cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.1 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants