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

Use JDK default security provider when Conscrypt isn't available #12938

Merged
merged 1 commit into from Nov 25, 2021

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Nov 23, 2021

Fixes #12907

Motivation

  • fixes issue with ARM64 platform where Conscrypt isn't available.

Modifications

Call org.conscrypt.Conscrypt.checkAvailability() method using reflection to verify that Conscrypt can be loaded successfully. In a failure case, log the error and fallback to JDK default security provider.

- fixes issue with ARM64 platform where Conscrypt isn't available
@lhotari lhotari added type/bug The PR fixed a bug or issue reported a bug release/2.9.1 release/2.8.3 labels Nov 23, 2021
@lhotari lhotari added this to the 2.10.0 milestone Nov 23, 2021
@lhotari lhotari self-assigned this Nov 23, 2021
@github-actions
Copy link

@lhotari:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@lhotari lhotari added the doc-not-needed Your PR changes do not impact docs label Nov 23, 2021
@github-actions
Copy link

@lhotari:Thanks for providing doc info!

@lhotari lhotari merged commit 4f2d52e into apache:master Nov 25, 2021
codelipenghui pushed a commit that referenced this pull request Nov 26, 2021
)

- fixes issue with ARM64 platform where Conscrypt isn't available

(cherry picked from commit 4f2d52e)
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Nov 26, 2021
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
…che#12938)

- fixes issue with ARM64 platform where Conscrypt isn't available
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
…che#12938)

- fixes issue with ARM64 platform where Conscrypt isn't available
codelipenghui pushed a commit that referenced this pull request Dec 21, 2021
)

- fixes issue with ARM64 platform where Conscrypt isn't available

(cherry picked from commit 4f2d52e)
@codelipenghui codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 21, 2021
@ladhadeepak
Copy link

@lhotari Hi, is it expected to log stack trace for the warning message when Conscrypt class is not found? Thanks.

@lhotari
Copy link
Member Author

lhotari commented May 9, 2022

@lhotari Hi, is it expected to log stack trace for the warning message when Conscrypt class is not found? Thanks.

@ladhadeepak yes

@ladhadeepak
Copy link

@lhotari Hi, is it expected to log stack trace for the warning message when Conscrypt class is not found? Thanks.

@ladhadeepak yes

@lhotari Can you please explain why a warning should accompany a stack trace? Logging such an exception misleads into believing that something is wrong in the deployment but then it is not the case else it would not have been just WARNING. Isn't it ?

@lhotari
Copy link
Member Author

lhotari commented May 9, 2022

@lhotari Can you please explain why a warning should accompany a stack trace? Logging such an exception misleads into believing that something is wrong in the deployment but then it is not the case else it would not have been just WARNING. Isn't it ?

@ladhadeepak The log message is pretty clear about the warning: "Conscrypt isn't available. Using JDK default security provider." Please explain the scenario where you think that it's misleading to log the stack trace. Who is being mislead? :) What is the impact?

@lhotari
Copy link
Member Author

lhotari commented May 9, 2022

@ladhadeepak Are you running on ARM64 platform?

@ladhadeepak
Copy link

ladhadeepak commented May 9, 2022

@lhotari Can you please explain why a warning should accompany a stack trace? Logging such an exception misleads into believing that something is wrong in the deployment but then it is not the case else it would not have been just WARNING. Isn't it ?

@ladhadeepak The log message is pretty clear about the warning: "Conscrypt isn't available. Using JDK default security provider." Please explain the scenario where you think that it's misleading to log the stack trace. Who is being mislead? :) What is the impact?

@lhotari My QA engineer is misled into believing that there is an error in the system and hence he has raised a ticket/defect :(
It doesn't help either that even Java docs associate stack traces with the error (error streams to be specific) and hence the QA engineer sees this stack trace as an error in the system.
It's a different matter though that we do not support Conscrypt as a security provider.

@ladhadeepak
Copy link

@ladhadeepak Are you running on ARM64 platform?

It is x86_64

@lhotari
Copy link
Member Author

lhotari commented May 9, 2022

It's a different matter though that we do not support Conscrypt as a security provider.

Why do you want to disable Conscrypt?

@ladhadeepak
Copy link

It's a different matter though that we do not support Conscrypt as a security provider.

Why do you want to disable Conscrypt?

Oh well, we are in enterprise application development space and not in the mobile app or android space.

@lhotari
Copy link
Member Author

lhotari commented May 9, 2022

Why do you want to disable Conscrypt?

Oh well, we are in enterprise application development space and not in the mobile app or android space.

Unfortunately Conscrypt isn't related to mobile apps or android space in Pulsar. Conscrypt is used in Pulsar to improve the performance of Jetty TLS layer. #10372 explains the rationale. The official Jetty documentation recommends to use Conscrypt to achieve good performance with TLS. Embedded Jetty is used as the http server for the Pulsar broker and Pulsar proxy.

@ladhadeepak
Copy link

Why do you want to disable Conscrypt?

Oh well, we are in enterprise application development space and not in the mobile app or android space.

Unfortunately Conscrypt isn't related to mobile apps or android space in Pulsar. Conscrypt is used in Pulsar to improve the performance of Jetty TLS layer. #10372 explains the rationale. The official Jetty documentation recommends to use Conscrypt to achieve good performance with TLS. Embedded Jetty is used as the http server for the Pulsar broker and Pulsar proxy.

Sure, we can evaluate this in our context. Thanks for the detailed info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.8.3 release/2.9.2 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pulsar in AWS ARM processor group unable to start cluster
5 participants