You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/12/21 05:01:02 UTC
[GitHub] [pulsar] Shoothzj opened a new pull request #13424: Allow to config no-password truststore
Shoothzj opened a new pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424
### Motivation
Supprot to config truststore which password is empty `""`
### Modifications
- when password is null, load as `""`
### Verifying this change
- add test method to verify tls success
### Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
- [x] `no-need-doc`
That's a little enhance, doesn't need doc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#issuecomment-1000949348
ping @michaeljmarshall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#issuecomment-1000087822
@codelipenghui @lhotari @merlimat @315157973 @hangc0276 PTAL, thanks
@michaeljmarshall PTAL agian, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on a change in pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#discussion_r774242147
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/keystoretls/KeyStoreSSLContext.java
##########
@@ -105,7 +105,11 @@ public KeyStoreSSLContext(Mode mode,
? DEFAULT_KEYSTORE_TYPE
: trustStoreTypeString;
this.trustStorePath = trustStorePath;
- this.trustStorePassword = trustStorePassword;
+ if (trustStorePassword == null) {
+ this.trustStorePassword = "";
+ } else {
+ this.trustStorePassword = trustStorePassword;
+ }
Review comment:
@michaeljmarshall we have a unit test to ensure java code default value equals to config file. The config file can't have a empty value semantic, it only has `null` or `non-empty`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#discussion_r773370518
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/keystoretls/KeyStoreSSLContext.java
##########
@@ -105,7 +105,11 @@ public KeyStoreSSLContext(Mode mode,
? DEFAULT_KEYSTORE_TYPE
: trustStoreTypeString;
this.trustStorePath = trustStorePath;
- this.trustStorePassword = trustStorePassword;
+ if (trustStorePassword == null) {
+ this.trustStorePassword = "";
+ } else {
+ this.trustStorePassword = trustStorePassword;
+ }
Review comment:
Since `null` is the default, this is effectively changing the default value. I think it'd be more transparent to update the default value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#discussion_r774335745
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/keystoretls/KeyStoreSSLContext.java
##########
@@ -105,7 +105,11 @@ public KeyStoreSSLContext(Mode mode,
? DEFAULT_KEYSTORE_TYPE
: trustStoreTypeString;
this.trustStorePath = trustStorePath;
- this.trustStorePassword = trustStorePassword;
+ if (trustStorePassword == null) {
+ this.trustStorePassword = "";
+ } else {
+ this.trustStorePassword = trustStorePassword;
+ }
Review comment:
I didn't know about those tests. Thank you for explaining. In that case, I think we should document the appropriate configuration values to indicate that `null` is converted to `""`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj removed a comment on pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
Shoothzj removed a comment on pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#issuecomment-998518201
@codelipenghui @lhotari @michaeljmarshall @merlimat @315157973 @hangc0276 PTAL, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#issuecomment-1006294409
> Hi, just a tip for new committers. When you merge a PR, it's better to copy and paste the PR description like
>
> 
>
> 
>
> Otherwise, the commit log would only contain all the single commits like [53ad936](https://github.com/apache/pulsar/commit/53ad936b28035a7ae81e80c12e924b3f03a573e9)
>
> See a correct commit like: [40ac793](https://github.com/apache/pulsar/commit/40ac7934ed567b2b9f16dbd7f937f7973dcddc21)
Thank you, I haven't known that before, sorry for my fault.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#issuecomment-1000108622
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj removed a comment on pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
Shoothzj removed a comment on pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#issuecomment-1000108622
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on a change in pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#discussion_r773500013
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/keystoretls/KeyStoreSSLContext.java
##########
@@ -105,7 +105,11 @@ public KeyStoreSSLContext(Mode mode,
? DEFAULT_KEYSTORE_TYPE
: trustStoreTypeString;
this.trustStorePath = trustStorePath;
- this.trustStorePassword = trustStorePassword;
+ if (trustStorePassword == null) {
+ this.trustStorePassword = "";
+ } else {
+ this.trustStorePassword = trustStorePassword;
+ }
Review comment:
@michaeljmarshall I can't find a way to change the default value which in the config file to `""`, this will cause the inconsistent between config file and code. Could you give me some advice? :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj merged pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
Shoothzj merged pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#discussion_r774226970
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/keystoretls/KeyStoreSSLContext.java
##########
@@ -105,7 +105,11 @@ public KeyStoreSSLContext(Mode mode,
? DEFAULT_KEYSTORE_TYPE
: trustStoreTypeString;
this.trustStorePath = trustStorePath;
- this.trustStorePassword = trustStorePassword;
+ if (trustStorePassword == null) {
+ this.trustStorePassword = "";
+ } else {
+ this.trustStorePassword = trustStorePassword;
+ }
Review comment:
I might be wrong, but I think it is set here:
https://github.com/apache/pulsar/blob/6657a6c2fb23be579e74bac7c7c07a70cabfb4b2/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java#L2390-L2394
It looks like that file has several variables that end in `TlsTrustStorePassword` though, so I'm not sure how they relate to this code block. I wonder if we should consider updating all of the defaults to make sure that every one supports an empty string default and is therefore consistent.
(I haven't looked closely at this part of the code base, so please take that into consideration when looking at these other variables.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#issuecomment-1000165309
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#issuecomment-1006292692
Hi, just a tip for new committers. When you merge a PR, it's better to copy and paste the PR description like
Otherwise, the commit log would only contain all the single commits like https://github.com/apache/pulsar/commit/53ad936b28035a7ae81e80c12e924b3f03a573e9
See a correct commit like: https://github.com/apache/pulsar/commit/40ac7934ed567b2b9f16dbd7f937f7973dcddc21
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] dave2wave commented on pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
dave2wave commented on pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#issuecomment-999170323
> I don't believe we're allowed to add binary files to our repo.
Assuming it is not possible to create this *test* file from source then to me this test case is reasonable.
It would be good to have a description of how these two keystores are created. Then we can determine if it is relatively simple to build.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#issuecomment-998518201
@codelipenghui @lhotari @michaeljmarshall @merlimat @315157973 @hangc0276 PTAL, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#issuecomment-999171615
> I don't believe we're allowed to add binary files to our repo. @dave2wave and @eolivelli - can you confirm this for me?
We do have binaries when it make sense. eg: PNG image for diagrams. I believe that if you want to test with a keystore and be able to read it, there's no other way than to include it.
Or if you want, just encode in base64.. which kind of demonstrate that bytes are bytes..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #13424: Allow to config no-password truststore
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #13424:
URL: https://github.com/apache/pulsar/pull/13424#issuecomment-1006299888
Never mind. There was a PR that adds the guideline for merging a PR: https://github.com/apache/pulsar/pull/12988. It will be included in Pulsar 2.10.0 website.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org