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/03/23 03:37:20 UTC
[GitHub] [pulsar] xiaotongwang1 opened a new pull request #10012: [issue][improve] add SCRAM-SHA256 support for SASL
xiaotongwang1 opened a new pull request #10012:
URL: https://github.com/apache/pulsar/pull/10012
Fixes part of #8670 #9367 i feedback
### Motivation
1、SASL-SCRAM-SHA256 auth is more lightweight than SASL-Kerberos ,not need depend on a third part modules like KDC ,and can use use without TLS in Intranet．
2、we can save scram user info (salt、serverkey、storekeyiterations) to meta center (zookeeoer now )
3、in admin scenario ，we can use hmacsha256 sign value as a token , than we can use curl、postman httpclient to send a admin request ,it is more easier and safer than get a sha token after SASL-kerberos auth and it is also suggest use https to protected your hmacValue
http header : HmacAuthRoleToken like SaslAuthRoleToken
value format ：u=pulsarAdmin&i=scram&e=1616416871193&s=C4681422F1C98DB5C02F7E5812D54E61AB5219841CC1BA3ECEF89E31FA60105
### Modifications
1、extend a new sasl SCRAM-Sha256 ,will effect after user open it
2、add a interface Decryption ,it is better store your password 、scram info with encrypted ，and we will invoke your Decryption impl ,when use load data to mem
### Verifying this change
This change is already covered by existing tests, such as SaslSCRAMAuthenticateTest,SaslScramDataTest
like the sasl-kerboer testcase SaslAuthenticateTest and SaslServerTokenSignerTest
### 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): (no)
- The public API: ( no)
- The schema: (no)
- The default values of configurations: ( no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
- Does this pull request introduce a new feature? (yes)
- If yes, how is the feature documented? (not documented now ,and if this feature can be merge ,will update the doc ,now it can be check the useage by testcase)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #10012: [issue][improve] add SCRAM-SHA256 support for SASL
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #10012:
URL: https://github.com/apache/pulsar/pull/10012#discussion_r600317909
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -305,6 +305,13 @@
)
private long topicLoadTimeoutSeconds = 60;
+ @FieldContext(
+ category = CATEGORY_SERVER,
+ required = false,
+ doc = "defined your own decryption interface"
+ )
+ private String decryptionInterface;
Review comment:
Users need to provide a Decryption implementation themself, But I did not find any jar load implementation. In my opinion, users need to private a location that where to find the jar? Otherwise, users need to copy jar under the classpath.
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -305,6 +305,13 @@
)
private long topicLoadTimeoutSeconds = 60;
+ @FieldContext(
+ category = CATEGORY_SERVER,
Review comment:
Should be `CATEGORY_AUTHENTICATION` ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] xiaotongwang1 commented on a change in pull request #10012: [issue][improve] add SCRAM-SHA256 support for SASL
Posted by GitBox <gi...@apache.org>.
xiaotongwang1 commented on a change in pull request #10012:
URL: https://github.com/apache/pulsar/pull/10012#discussion_r600400106
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -305,6 +305,13 @@
)
private long topicLoadTimeoutSeconds = 60;
+ @FieldContext(
+ category = CATEGORY_SERVER,
+ required = false,
+ doc = "defined your own decryption interface"
+ )
+ private String decryptionInterface;
Review comment:
It will not do encryotion and decryption by default, and i just make a interface that user can package their decrption implementation jar to pulsar libs dir ,then they can config the password、secret with encryted.
There also exist some sensitive config like OAuth2.0 client_secret、zookeeper SASL DIGEST-MD5 passworld 、TLS keystore password， suggest to be encryted when we use it online
So i think maybe pulsar can provider a common encryption and decryption method in future , such as a common AES-GCM implementation
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] xiaotongwang1 commented on pull request #10012: [issue][improve] add SCRAM-SHA256 support for SASL
Posted by GitBox <gi...@apache.org>.
xiaotongwang1 commented on pull request #10012:
URL: https://github.com/apache/pulsar/pull/10012#issuecomment-804595489
Hi @sijie @codelipenghui @jiazhai
I am a new developer for pulsar , I am not clear who can review these code ,can you please take a look?
This feature will effect only after user open it ,and i have make some test like SASL-kerberos(testProducerAndConsumerPassed、testSaslServerAndClientAuth)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] xiaotongwang1 commented on a change in pull request #10012: [issue][improve] add SCRAM-SHA256 support for SASL
Posted by GitBox <gi...@apache.org>.
xiaotongwang1 commented on a change in pull request #10012:
URL: https://github.com/apache/pulsar/pull/10012#discussion_r600400106
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -305,6 +305,13 @@
)
private long topicLoadTimeoutSeconds = 60;
+ @FieldContext(
+ category = CATEGORY_SERVER,
+ required = false,
+ doc = "defined your own decryption interface"
+ )
+ private String decryptionInterface;
Review comment:
It will not do encryotion and decryption by default, and i just make a interface that user can package their decrption implementation jar to pulsar libs dir ,then they can config the password、secret with encryted.
There also exist some sensitive config like OAuth2.0 client_secret、zookeeper SASL DIGEST-MD5 passworld 、TLS keystore password， suggest to be encryted when we use it online
So i think maybe pulsar can provider a command encryption and decryption method in future , such as a common AES-GCM implementation
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] xiaotongwang1 commented on a change in pull request #10012: [issue][improve] add SCRAM-SHA256 support for SASL
Posted by GitBox <gi...@apache.org>.
xiaotongwang1 commented on a change in pull request #10012:
URL: https://github.com/apache/pulsar/pull/10012#discussion_r603320084
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -305,6 +305,13 @@
)
private long topicLoadTimeoutSeconds = 60;
+ @FieldContext(
+ category = CATEGORY_SERVER,
Review comment:
@codelipenghui thanks for you review , do you have any other suggestions? and I also commit a pr to fix zookeeper promethus connetion data #9778 , can you please take a look? thanks.
I will try to commit some bug fix 、security Improvements and bookkeeper SASL-SCRAM later
Hope can be merge to pulsar ,so we can upgrade new pulsar version directly ,no need to rebuild after custom-made
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] xiaotongwang1 commented on a change in pull request #10012: [issue][improve] add SCRAM-SHA256 support for SASL
Posted by GitBox <gi...@apache.org>.
xiaotongwang1 commented on a change in pull request #10012:
URL: https://github.com/apache/pulsar/pull/10012#discussion_r603320084
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -305,6 +305,13 @@
)
private long topicLoadTimeoutSeconds = 60;
+ @FieldContext(
+ category = CATEGORY_SERVER,
Review comment:
@codelipenghui thanks for you review , do you have any other suggestions? and I also commit a pr to fix zookeeper promethus connetion data #9777 , can you please take a look? thanks.
I will try to commit some bug fix 、security Improvements and bookkeeper SASL-SCRAM later
Hope can be merge to pulsar ,so we can upgrade new pulsar version directly ,no need to rebuild after custom-made
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #10012: [issue][improve] add SCRAM-SHA256 support for SASL
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #10012:
URL: https://github.com/apache/pulsar/pull/10012#issuecomment-1058892100
The pr had no activity for 30 days, mark with Stale label.
--
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] xiaotongwang1 commented on a change in pull request #10012: [issue][improve] add SCRAM-SHA256 support for SASL
Posted by GitBox <gi...@apache.org>.
xiaotongwang1 commented on a change in pull request #10012:
URL: https://github.com/apache/pulsar/pull/10012#discussion_r600400106
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -305,6 +305,13 @@
)
private long topicLoadTimeoutSeconds = 60;
+ @FieldContext(
+ category = CATEGORY_SERVER,
+ required = false,
+ doc = "defined your own decryption interface"
+ )
+ private String decryptionInterface;
Review comment:
It will not do encryotion and decryption by default, and i just make a interface that user can package their decrption implementation jar to pulsar libs dir ,then they can config the password、secret with encrypted.
There also exist some sensitive config like OAuth2.0 client_secret、zookeeper SASL DIGEST-MD5 passworld 、TLS keystore password， suggest to be encryted when we use it online
So i think maybe pulsar can provider a common encryption and decryption method in future , such as a common AES-GCM implementation
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #10012: [issue][improve] add SCRAM-SHA256 support for SASL
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #10012:
URL: https://github.com/apache/pulsar/pull/10012#discussion_r600422521
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -305,6 +305,13 @@
)
private long topicLoadTimeoutSeconds = 60;
+ @FieldContext(
+ category = CATEGORY_SERVER,
Review comment:
Ok, make sense.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] xiaotongwang1 commented on a change in pull request #10012: [issue][improve] add SCRAM-SHA256 support for SASL
Posted by GitBox <gi...@apache.org>.
xiaotongwang1 commented on a change in pull request #10012:
URL: https://github.com/apache/pulsar/pull/10012#discussion_r600409231
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -305,6 +305,13 @@
)
private long topicLoadTimeoutSeconds = 60;
+ @FieldContext(
+ category = CATEGORY_SERVER,
Review comment:
I thought about it, and I thought the decryption class should be a generic configuration.It is required for sensitive configurations, not only for authorization.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #10012: [issue][improve] add SCRAM-SHA256 support for SASL
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10012:
URL: https://github.com/apache/pulsar/pull/10012#issuecomment-1058892343
@xiaotongwang1:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? 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