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.

Sign up for GitHub

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

Jump to bottom

[security] Allow to config web server's cipher and protocols #13354

Merged
merged 5 commits into from
Jan 7, 2022
Merged

[security] Allow to config web server's cipher and protocols #13354

merged 5 commits into from
Jan 7, 2022

Conversation

hezhangjian
Copy link
Member

Motivation

For the security, people want to config the support ciphers and protocols

Modifications

  • Allow the broker and proxy config web server's ciphers and protocols
  • By default, ciphers and protocols are null, same as before.

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • doc

    this PR contains config file change. IMHO, the doc will be auto generated.

@github-actions github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Dec 16, 2021
@hezhangjian hezhangjian force-pushed the security-web-server-config-cipher-procotols branch 2 times, most recently from 226d92c to 462aafe Compare December 16, 2021 08:59
zymap
zymap reviewed Dec 17, 2021
View reviewed changes
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.

Could you please add a test to cover this?

@hezhangjian hezhangjian force-pushed the security-web-server-config-cipher-procotols branch 2 times, most recently from 5dbef5a to 07df6e3 Compare December 18, 2021 07:34
@hezhangjian
Copy link
Member Author

Could you please add a test to cover this?

Yes, add tests to test ciphers and protocols on jetty server.

@hezhangjian hezhangjian force-pushed the security-web-server-config-cipher-procotols branch 2 times, most recently from 8fe8031 to f64ec8e Compare December 22, 2021 06:30
eolivelli
eolivelli requested changes Dec 22, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Overall it looks good to me
I left some comments about the tests

...ava/org/apache/pulsar/common/util/keystoretls/JettySslContextFactoryWithAutoRefreshTest.java Outdated Show resolved Hide resolved
...ava/org/apache/pulsar/common/util/keystoretls/JettySslContextFactoryWithAutoRefreshTest.java Outdated Show resolved Hide resolved
...ava/org/apache/pulsar/common/util/keystoretls/JettySslContextFactoryWithAutoRefreshTest.java Outdated Show resolved Hide resolved
eolivelli
eolivelli reviewed Dec 22, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

...ava/org/apache/pulsar/common/util/keystoretls/JettySslContextFactoryWithAutoRefreshTest.java Outdated Show resolved Hide resolved
...ava/org/apache/pulsar/common/util/keystoretls/JettySslContextFactoryWithAutoRefreshTest.java Outdated Show resolved Hide resolved
@hezhangjian
Copy link
Member Author

@codelipenghui @lhotari @michaeljmarshall @merlimat @315157973 @hangc0276 PTAL, thanks

@hezhangjian hezhangjian force-pushed the security-web-server-config-cipher-procotols branch from 9ee178f to c8163cd Compare December 28, 2021 12:10
@hezhangjian
Copy link
Member Author

@eolivelli PTAL, again, thanks

@hezhangjian
Copy link
Member Author

/pulsarbot run-failure-checks

@hezhangjian
Copy link
Member Author

@eolivelli PTAL, again, thanks

@hezhangjian hezhangjian requested a review from eolivelli January 7, 2022 08:42
eolivelli
eolivelli approved these changes Jan 7, 2022
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Lgtm

@eolivelli eolivelli merged commit b540523 into apache:master Jan 7, 2022
@hezhangjian hezhangjian deleted the security-web-server-config-cipher-procotols branch January 7, 2022 12:23
liudezhi2098 pushed a commit to liudezhi2098/pulsar that referenced this pull request Jan 11, 2022
@hezhangjian @liudezhi2098
[security] Allow to config web server's cipher and protocols (apache#…
2291a7b 
…13354)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Apr 5, 2022
@hezhangjian @nicoloboschi
[security] Allow to config web server's cipher and protocols (apache#…
3c0f869 
…13354)

(cherry picked from commit b540523)
@michaeljmarshall michaeljmarshall mentioned this pull request Apr 11, 2022
Merged
@BewareMyPower
Copy link
Contributor

JettySslContextFactoryWithAutoRefreshTest failed with JDK 8.

image

It looks like our tests are all based on JDK 11 currently so that this test failure was not detected. And I found some other PRs that rely on this PR.

/cc @codelipenghui @eolivelli @nodece @michaeljmarshall

@BewareMyPower
Copy link
Contributor

It looks like it's because the JKS files are generated by a higher version JDK.

IMO, committing binaries files are terrible to maintain for developers. It's necessary to describe how to generate this files if we need to do so. Or should we add the steps to CI instead of uploading these binaries?

@nodece
Copy link
Member

nodece commented Apr 26, 2022 •
edited
Loading

I found some answers:

@hezhangjian
Copy link
Member Author

@BewareMyPower what's your java8 version, I remember it needs higher java8 version.

@BewareMyPower
Copy link
Contributor

1.8.0_261

@BewareMyPower
Copy link
Contributor

I remember it needs higher java8 version.

It's right. I switched to 1.8.0_332 and it works now.

@michaeljmarshall
Copy link
Member

IMO, committing binaries files are terrible to maintain for developers. It's necessary to describe how to generate this files if we need to do so. Or should we add the steps to CI instead of uploading these binaries?

I agree. We could require adding a README.md in the same directory as the file with instructions on how to re-build the binaries. This will help PR reviewers and make maintenance easier (especially for things like certs that have expiration dates in the near future).

@BewareMyPower
Copy link
Contributor

especially for things like certs that have expiration dates in the near future

Yeah, there was once a similar issue (#9607) that broke the whole CI.

@hezhangjian
Copy link
Member Author

@michaeljmarshall @BewareMyPower Thanks for your advice, I will follow up on this.

@codelipenghui codelipenghui added this to the 2.11.0 milestone Apr 28, 2022
@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.10.0 Apr 28, 2022
@BewareMyPower BewareMyPower mentioned this pull request Apr 28, 2022
Merged
1 task
@mattisonchao
Copy link
Member

Hi @Shoothzj
There are a lot of conflicts when this PR is cherry-picked to 2.9, Could you please help push a new PR to branch-2.9?

@hezhangjian
Copy link
Member Author

@mattisonchao I am afraid I don't have time this week. But don't worry, @Lico-Tom will help open a new PR

@hezhangjian hezhangjian mentioned this pull request May 24, 2022
Closed
nicoloboschi added a commit to datastax/pulsar that referenced this pull request Jun 1, 2022
@nicoloboschi
Revert "[security] Allow to config web server's cipher and protocols (a…
146ad92 
…pache#13354)"

This reverts commit 3c0f869.
@mattisonchao
Copy link
Member

mattisonchao commented Jun 16, 2022 •
edited
Loading

@Shoothzj It looks like cherry-picked pull request a118ab9 already cover this change in branch-2.9. Could you have time to help check it?

@hezhangjian
Copy link
Member Author

@mattisonchao Yes, It's already covered this change.

@mattisonchao mattisonchao added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Jun 17, 2022
@BewareMyPower
Copy link
Contributor

@mattisonchao @Shoothzj Does it mean there is no need to cherry-pick this PR? If yes, I think it would be better to remove all release/xxx labels and cherry-picked/xxx labels instead of marking this PR as "cherry-picked".

@BewareMyPower
Copy link
Contributor

I removed these labels after confirming it from @mattisonchao.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zymap zymap zymap left review comments

@eolivelli eolivelli eolivelli approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

Assignees

@hezhangjian hezhangjian

Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files.
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.
8 participants
@hezhangjian @BewareMyPower @nodece @michaeljmarshall @mattisonchao @eolivelli @codelipenghui @zymap