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

[improve][misc] Improve AES-GCM cipher performance #23122

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

ocadaruma
Copy link
Contributor

@ocadaruma ocadaruma commented Aug 4, 2024

Fixes #23121

Motivation

Modifications

  • Modify MessageCryptoBc to use SunJCE provider which comes with hardware-instruction optimization instead of BouncyCastle provider.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as org.apache.pulsar.tests.integration.SimpleProducerConsumerTest

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: Pulsar CI workflow run result

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 4, 2024
@ocadaruma ocadaruma force-pushed the improve-encrypt-perf branch from d86759f to 0021f17 Compare August 4, 2024 02:14
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @ocadaruma!
The only concern I have about the change is the users of Pulsar that require FIPS compliant encryption. Pulsar has docs about enabling BouncyCastle's FIPS library in https://pulsar.apache.org/docs/3.3.x/security-bouncy-castle/. It's seems when the FIPS library is enabled, the SunJCE provider shouldn't be made the default. I'll add a suggestion about how to achieve this.

UPDATE: The previous solution has already ignored the presence of BouncyCastleFipsProvider, so I think that the changes in this PR are fine. The issue in FIPS compliance is a separate issue.

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@ocadaruma
Copy link
Contributor Author

@lhotari Agree.
Currently MessageCryptoBc anyways uses hardcoded BC so we should ifx it to load bc fips provider as necessary and this PR isn't harmful.

@lhotari lhotari merged commit e9deb40 into apache:master Aug 5, 2024
59 of 61 checks passed
@ocadaruma ocadaruma deleted the improve-encrypt-perf branch August 5, 2024 10:11
lhotari pushed a commit that referenced this pull request Aug 6, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 8, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 12, 2024
Denovo1998 pushed a commit to Denovo1998/pulsar that referenced this pull request Aug 17, 2024
grssam pushed a commit to grssam/pulsar that referenced this pull request Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encryption performance improvement on Java client
4 participants