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

Add Tls with keystore type config support #6853

Merged
merged 11 commits into from
May 8, 2020

Conversation

jiazhai
Copy link
Member

@jiazhai jiazhai commented Apr 30, 2020

Fixes #6640

Motivation

Add Tls with keystore type config.

Modifications

Add Tls with keystore type config.

Verifying this change

  • Unit test passed

(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

@rdhabalia
Copy link
Contributor

rdhabalia commented Apr 30, 2020

I haven't gone through with the entire change but it seems it touches many different files and I think supporting multiple CA trust stores can be done in an easier way and might not need this big change.
we can always bundle multiple CA trust store in one file and it can be provided to pulsar-broker. We also have similar kind of requirement and we are going to bundle multiple certs into one to support multiple ca usecase.
https://alvinalexander.com/java/java-using-keytool-import-certificate-keystore/
We can also add script in the repo for reference which can allow user to bundle multiple certs.

@merlimat can you also please review this PR.

@jiazhai jiazhai changed the title Add Tls with keystore type config to support multi CAs Add Tls with keystore type config support Apr 30, 2020
@jiazhai
Copy link
Member Author

jiazhai commented May 1, 2020

@rdhabalia Thanks for the comments. The name of the PR seems be a little misleading. Changed the name to align with the main changes. It mainly want to support keystore configs. and make multi ca easy config is one result of it.
This change tries to keep the original tls settings untouched, and add new config in needed path, so it changed number of files. Maybe we could make the supporting work aligned in the future.

@rdhabalia
Copy link
Contributor

I will try to review it soon

Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

Overall the change looks good to me. And it is protected by the flag tlsEnabledWithKeyStore. It seems to be a safe change.

@sijie sijie added this to the 2.6.0 milestone May 7, 2020
@sijie sijie added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. doc-required Your PR changes impact docs and you will update later. labels May 7, 2020
@sijie
Copy link
Member

sijie commented May 7, 2020

@jiazhai I think we need to add those settings to the configuration files and the website documentation. Can you please create an issue to follow up on a change?

@jiazhai
Copy link
Member Author

jiazhai commented May 8, 2020

Hi @rdhabalia Thanks for taking care. If this is too hurry for the review, we could handle it in the following issues and prs.

@jiazhai jiazhai merged commit 367ce78 into apache:master May 8, 2020
jiazhai added a commit that referenced this pull request May 8, 2020
Fixes #6640

Add Tls with keystore type config.

Add Tls with keystore type config.

- Unit test passed

(cherry picked from commit 367ce78)
jiazhai added a commit that referenced this pull request May 12, 2020
related with #6853
add keystore tls config doc
jiazhai added a commit that referenced this pull request May 12, 2020
related with #6853
add keystore tls config doc
(cherry picked from commit fd6f772)
sijie pushed a commit that referenced this pull request May 18, 2020
some files are left unused, in #6853. this pr is to remove them
Huanli-Meng pushed a commit to Huanli-Meng/pulsar that referenced this pull request May 27, 2020
Fixes apache#6640

### Motivation

Add Tls with keystore type config.

### Modifications

Add Tls with keystore type config.

### Verifying this change

- Unit test passed
Huanli-Meng pushed a commit to Huanli-Meng/pulsar that referenced this pull request May 27, 2020
related with apache#6853
add keystore tls config doc
Huanli-Meng pushed a commit to Huanli-Meng/pulsar that referenced this pull request May 27, 2020
some files are left unused, in apache#6853. this pr is to remove them
addisonj pushed a commit to instructure/pulsar that referenced this pull request Jun 12, 2020
Fixes apache#6640

Add Tls with keystore type config.

Add Tls with keystore type config.

- Unit test passed

(cherry picked from commit 367ce78)
addisonj pushed a commit to instructure/pulsar that referenced this pull request Jun 12, 2020
related with apache#6853
add keystore tls config doc
(cherry picked from commit fd6f772)
jiazhai pushed a commit that referenced this pull request Jul 10, 2020
Motivation
This doc PR is updated for configurations for PRs:
#6716
#6853
#6074

1: The broker configuration (for #6716) is updated by Jia Zhai.

2: Add other supported configurations to the client, standlone and proxy configuration docs based on the client.config, standlone.config and proxy.config files.

Modifications
1: Add TLS with keystore type config in standlone and proxy configuration file.
2: update reference > pulsar configuration > client for PIP-55: Refresh Authentication Credentials
Add other supported configurations to the standlone and proxy configuration files based on the standlone.config and proxy.config files.
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
Fixes apache#6640

### Motivation

Add Tls with keystore type config.

### Modifications

Add Tls with keystore type config.

### Verifying this change

- Unit test passed
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
related with apache#6853
add keystore tls config doc
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
some files are left unused, in apache#6853. this pr is to remove them
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
Motivation
This doc PR is updated for configurations for PRs:
apache#6716
apache#6853
apache#6074

1: The broker configuration (for apache#6716) is updated by Jia Zhai.

2: Add other supported configurations to the client, standlone and proxy configuration docs based on the client.config, standlone.config and proxy.config files.

Modifications
1: Add TLS with keystore type config in standlone and proxy configuration file.
2: update reference > pulsar configuration > client for PIP-55: Refresh Authentication Credentials
Add other supported configurations to the standlone and proxy configuration files based on the standlone.config and proxy.config files.
@Anonymitaet Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. doc-complete Your PR changes impact docs and the related docs have been already added. release/2.5.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broker and Proxy support multiple CA certs
5 participants