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

[fix][client] Fixed an issue where a cert chain could not be used in TLS authentication #23644

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

equanz
Copy link
Contributor

@equanz equanz commented Nov 27, 2024

Motivation

From v2.8.0( #8831 ) , SecurityUtility#createAutoRefreshSslContextForClient was introduced. This method loads a single certification rather than a certification chain from cert file.

Therefore, the PulsarAdmin does not send cert chain in TLS authentication.

(From v4.0.0 ( #23110 ), this issue was fixed because the PulsarAdmin does not use this method by default.)

Modifications

  • Fix to add certs to KeyStore in KeyManagerProxy

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Add simple unit tests to check if KeyManagerProxy can load a cert chain

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: equanz#11

@lhotari
Copy link
Member

lhotari commented Nov 27, 2024

@equanz Do we need tests for validating the behavior?

@equanz equanz force-pushed the fix_to_add_certs_to_key_store branch from adc5eab to 3492aab Compare November 27, 2024 09:54
@equanz
Copy link
Contributor Author

equanz commented Nov 27, 2024

@lhotari I have added a simple unit tests to check if KeyManagerProxy can load a cert chain.

@equanz equanz force-pushed the fix_to_add_certs_to_key_store branch from 3492aab to 8a8a373 Compare November 27, 2024 10:01
@equanz equanz force-pushed the fix_to_add_certs_to_key_store branch from 8a8a373 to 7591041 Compare November 27, 2024 10:15
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

@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.34%. Comparing base (bbc6224) to head (7591041).
Report is 756 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23644      +/-   ##
============================================
+ Coverage     73.57%   74.34%   +0.77%     
- Complexity    32624    35003    +2379     
============================================
  Files          1877     1944      +67     
  Lines        139502   147201    +7699     
  Branches      15299    16240     +941     
============================================
+ Hits         102638   109439    +6801     
- Misses        28908    29307     +399     
- Partials       7956     8455     +499     
Flag Coverage Δ
inttests 27.29% <0.00%> (+2.70%) ⬆️
systests 24.40% <0.00%> (+0.07%) ⬆️
unittests 73.72% <100.00%> (+0.88%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...org/apache/pulsar/common/util/KeyManagerProxy.java 47.05% <100.00%> (-12.56%) ⬇️

... and 666 files with indirect coverage changes

@lhotari lhotari merged commit e236d61 into apache:master Nov 27, 2024
60 checks passed
lhotari pushed a commit that referenced this pull request Nov 27, 2024
lhotari pushed a commit that referenced this pull request Nov 27, 2024
lhotari pushed a commit that referenced this pull request Nov 27, 2024
@equanz equanz deleted the fix_to_add_certs_to_key_store branch November 28, 2024 01:08
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 28, 2024
…TLS authentication (apache#23644)

(cherry picked from commit e236d61)
(cherry picked from commit e0e1956)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 28, 2024
…TLS authentication (apache#23644)

(cherry picked from commit e236d61)
(cherry picked from commit e0e1956)
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.

3 participants