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 2022/04/11 12:17:25 UTC
[GitHub] [pulsar] liudezhi2098 opened a new pull request, #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
liudezhi2098 opened a new pull request, #15121:
URL: https://github.com/apache/pulsar/pull/15121
### Motivation
When use Kerberos authentication , using Pulsar Admin to query topic state, will appear HTTP 401 Unauthorized, becasue
request redirect url will use current SaslRoleToken ，but redirect broker not recognized the token, per broker secret is not same.
```
WARN org.apache.pulsar.broker.web.AuthenticationFilter - [10.3.0.4] Failed to authenticate HTTP request: Invalid signature
```
per broker secret is Random
```
protected String computeSignature(String str) {
try {
MessageDigest md = MessageDigest.getInstance("SHA-512");
md.update(str.getBytes());
md.update(secret);
byte[] digest = md.digest();
return new Base64(0).encodeToString(digest);
} catch (NoSuchAlgorithmException ex) {
throw new RuntimeException("It should not happen, " + ex.getMessage(), ex);
}
}
```
### Modifications
secret can configuration
`this.signer = new SaslRoleTokenSigner(config.getSaslJaasServerRoleTokenSignerSecret().getBytes());`
### 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: (no)
- The schema: (no )
- The default values of configurations: (yes)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: ( no)
- Anything that affects deployment: (no)
### Documentation
Need to update docs?
- [x] `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] liudezhi2098 commented on a diff in pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on code in PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#discussion_r861494402
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -1456,6 +1456,16 @@ public class ServiceConfiguration implements PulsarConfiguration {
)
private String saslJaasServerSectionName = SaslConstants.JAAS_DEFAULT_BROKER_SECTION_NAME;
+ @FieldContext(
+ category = CATEGORY_SASL_AUTH,
+ doc = "Configure the secret to be used to SaslRoleTokenSigner\n"
+ + "The secret can be specified like:\n"
+ + "saslJaasServerRoleTokenSignerSecret=file:///my/saslRoleTokenSignerSecret.key\n"
+ + "If saslJaasServerRoleTokenSignerSecret is empty, will use Default value "
+ + "`SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET`."
+ )
+ private String saslJaasServerRoleTokenSignerSecret;
Review Comment:
There won't be a default here because a file path is required.
--
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] codelipenghui commented on a diff in pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#discussion_r848990644
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -1438,6 +1438,12 @@ public class ServiceConfiguration implements PulsarConfiguration {
)
private String saslJaasServerSectionName = SaslConstants.JAAS_DEFAULT_BROKER_SECTION_NAME;
+ @FieldContext(
+ category = CATEGORY_SASL_AUTH,
+ doc = "secret for SaslRoleTokenSigner . Default value is \"PulsarSecret\"."
+ )
+ private String saslJaasServerRoleTokenSignerSecret = SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET;
+
Review Comment:
Should also add to ProxyConfiguration
##########
conf/broker.conf:
##########
@@ -767,6 +767,9 @@ saslJaasClientAllowedIds=
# Default value `SaslConstants.JAAS_DEFAULT_BROKER_SECTION_NAME`, which is "Broker".
saslJaasBrokerSectionName=
+# Secret for SaslRoleTokenSigner.
+# Default value `SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET`, which is "PulsarSecret".
+saslJaasServerRoleTokenSignerSecret=PulsarSecret
Review Comment:
Should also add to proxy.conf
--
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 diff in pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#discussion_r861453146
##########
conf/proxy.conf:
##########
@@ -277,6 +277,23 @@ tokenAudienceClaim=
# The token audience stands for this broker. The field `tokenAudienceClaim` of a valid token, need contains this.
tokenAudience=
+### --- SASL Authentication Provider --- ###
+
+# This is a regexp, which limits the range of possible ids which can connect to the Broker using SASL.
+# Default value: `SaslConstants.JAAS_CLIENT_ALLOWED_IDS_DEFAULT`, which is ".*pulsar.*",
+# so only clients whose id contains 'pulsar' are allowed to connect.
+saslJaasClientAllowedIds=
+
+# Service Principal, for login context name.
+# Default value `SaslConstants.JAAS_DEFAULT_BROKER_SECTION_NAME`, which is "Broker".
+saslJaasBrokerSectionName=
+
+# Configure the secret to be used to SaslRoleTokenSigner
+# The secret can be specified like:
+# saslJaasServerRoleTokenSignerSecret=file:///my/saslRoleTokenSignerSecret.key
+# If saslJaasServerRoleTokenSignerSecret is empty, will use Default value `SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET`.
+#saslJaasServerRoleTokenSignerSecret=
Review Comment:
Nit: I think it might make sense to end the variable with `Path` to make it clearer for users.
Also, what is the purpose for leaving the configuration commented out? Is it possible to have the config present in the file?
##########
conf/proxy.conf:
##########
@@ -277,6 +277,23 @@ tokenAudienceClaim=
# The token audience stands for this broker. The field `tokenAudienceClaim` of a valid token, need contains this.
tokenAudience=
+### --- SASL Authentication Provider --- ###
+
+# This is a regexp, which limits the range of possible ids which can connect to the Broker using SASL.
+# Default value: `SaslConstants.JAAS_CLIENT_ALLOWED_IDS_DEFAULT`, which is ".*pulsar.*",
+# so only clients whose id contains 'pulsar' are allowed to connect.
+saslJaasClientAllowedIds=
+
+# Service Principal, for login context name.
+# Default value `SaslConstants.JAAS_DEFAULT_BROKER_SECTION_NAME`, which is "Broker".
+saslJaasBrokerSectionName=
Review Comment:
If there is a default, I think we should specify it here.
```suggestion
saslJaasBrokerSectionName=Broker
```
##########
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConfiguration.java:
##########
@@ -351,6 +351,16 @@ public class ProxyConfiguration implements PulsarConfiguration {
)
private String saslJaasServerSectionName = SaslConstants.JAAS_DEFAULT_PROXY_SECTION_NAME;
+ @FieldContext(
+ category = CATEGORY_SASL_AUTH,
+ doc = "Configure the secret to be used to SaslRoleTokenSigner\n"
+ + "The secret can be specified like:\n"
+ + "saslJaasServerRoleTokenSignerSecret=file:///my/saslRoleTokenSignerSecret.key\n"
+ + "If saslJaasServerRoleTokenSignerSecret is empty, will use Default value "
+ + "`SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET`."
+ )
+ private String saslJaasServerRoleTokenSignerSecret;
Review Comment:
Same as above:
```suggestion
private String saslJaasServerRoleTokenSignerSecret = SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET;
```
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -1456,6 +1456,16 @@ public class ServiceConfiguration implements PulsarConfiguration {
)
private String saslJaasServerSectionName = SaslConstants.JAAS_DEFAULT_BROKER_SECTION_NAME;
+ @FieldContext(
+ category = CATEGORY_SASL_AUTH,
+ doc = "Configure the secret to be used to SaslRoleTokenSigner\n"
+ + "The secret can be specified like:\n"
+ + "saslJaasServerRoleTokenSignerSecret=file:///my/saslRoleTokenSignerSecret.key\n"
+ + "If saslJaasServerRoleTokenSignerSecret is empty, will use Default value "
+ + "`SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET`."
+ )
+ private String saslJaasServerRoleTokenSignerSecret;
Review Comment:
Similarly, we can have it default to the following:
```suggestion
private String saslJaasServerRoleTokenSignerSecret = SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET;
```
##########
conf/proxy.conf:
##########
@@ -277,6 +277,23 @@ tokenAudienceClaim=
# The token audience stands for this broker. The field `tokenAudienceClaim` of a valid token, need contains this.
tokenAudience=
+### --- SASL Authentication Provider --- ###
+
+# This is a regexp, which limits the range of possible ids which can connect to the Broker using SASL.
+# Default value: `SaslConstants.JAAS_CLIENT_ALLOWED_IDS_DEFAULT`, which is ".*pulsar.*",
+# so only clients whose id contains 'pulsar' are allowed to connect.
+saslJaasClientAllowedIds=
Review Comment:
If there is a default, I think we should specify it here. Will it work to do the following?
```suggestion
saslJaasClientAllowedIds=.*pulsar.*
```
##########
pulsar-broker-auth-sasl/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderSasl.java:
##########
@@ -98,8 +102,14 @@ public void initialize(ServiceConfiguration config) throws IOException {
throw new IOException(e);
}
}
-
- this.signer = new SaslRoleTokenSigner(Long.toString(new Random().nextLong()).getBytes());
+ String saslJaasServerRoleTokenSignerSecretFile = config.getSaslJaasServerRoleTokenSignerSecret();
+ byte[] secret = null;
+ if (StringUtils.isNotBlank(saslJaasServerRoleTokenSignerSecretFile)) {
+ secret = readSecretFromUrl(saslJaasServerRoleTokenSignerSecretFile);
+ } else {
+ secret = SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET.getBytes();
Review Comment:
What is the purpose of defaulting to a static value? I think we should maintain the current default (using a random Long), and then users that want to use a shared token can do so. Otherwise, users might not realize they need to update the value and the signing secret would be easy to guess, assuming I understand correctly.
--
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] lhotari commented on a diff in pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#discussion_r868824262
##########
pulsar-broker-auth-sasl/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderSasl.java:
##########
@@ -175,6 +186,24 @@ private String createAuthRoleToken(String role, String sessionId) {
return signed;
}
+ private byte[] readSecretFromUrl(String secretConfUrl) throws IOException {
+ if (secretConfUrl.startsWith("file:")) {
+ try {
+ return IOUtils.toByteArray(URL.createURL(secretConfUrl.trim()));
+ } catch (IOException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new IOException(e);
+ }
Review Comment:
I think this exception handling could be omitted. Catching all exceptions isn't really justified in this case.
--
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] liudezhi2098 commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1120166992
/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] michaeljmarshall commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1121938460
I've added the release note required label because this PR introduces a breaking change that requires users to configure a secret that they didn't have to configure before. We should document that in the release notes.
--
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] Anonymitaet commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1096216333
@momo-jun a soft reminder: here is a PR w/ doc-required label, could u pls follow up? 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] Anonymitaet commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1095866702
@liudezhi2098 I see you label this PR w/ `doc` but seems that there is no doc update in this PR?
--
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] Anonymitaet commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1096215176
> > @liudezhi2098 I see you label this PR w/ `doc` but seems that there is no doc update in this PR?
>
> Wait for this pr to approved, I will submit another pr.
so this PR should be labeled w/ `doc-required`.
--
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] liudezhi2098 commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1095741421
/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] liudezhi2098 commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1119167991
>
yes, I agree.
--
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] momo-jun commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
momo-jun commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1096598433
@liudezhi2098 feel free to ping me when you need review or any help on the follow-up doc PR.
--
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] liudezhi2098 commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1119387156
@michaeljmarshall @eolivelli Please help review this PR again.
--
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] liudezhi2098 commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1122043053
@eolivelli @lhotari thank you very much , PTAL.
--
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 merged pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
michaeljmarshall merged PR #15121:
URL: https://github.com/apache/pulsar/pull/15121
--
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 diff in pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#discussion_r853371421
##########
conf/broker.conf:
##########
@@ -764,6 +764,9 @@ saslJaasClientAllowedIds=
# Default value `SaslConstants.JAAS_DEFAULT_BROKER_SECTION_NAME`, which is "Broker".
saslJaasServerSectionName=
+# Secret for SaslRoleTokenSigner.
+# Default value `SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET`, which is "PulsarSecret".
+saslJaasServerRoleTokenSignerSecret=PulsarSecret
Review Comment:
Why don't we leave this blank and force the user to configure it? I agree with @eolivelli that a default secret seems like a security hole.
Also, can we supply the secret as a file? Finally, note that the python script that configures these `.conf` files only obfuscates configs with `password` in the name. As it is written, this would not be obfuscated.
--
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] liudezhi2098 commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1095921865
> @liudezhi2098 I see you label this PR w/ `doc` but seems that there is no doc update in this PR?
Wait for this pr to approved, I will submit another pr.
--
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 pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1111742341
> Is someone able to provide a link to where the Kerberos/SASL spec says this is a valid and safe solution to the problem of redirects?
@liudezhi2098 - I'll review this PR tomorrow. Are you able to answer this question for me? 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] liudezhi2098 commented on a diff in pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on code in PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#discussion_r859664147
##########
conf/broker.conf:
##########
@@ -764,6 +764,9 @@ saslJaasClientAllowedIds=
# Default value `SaslConstants.JAAS_DEFAULT_BROKER_SECTION_NAME`, which is "Broker".
saslJaasServerSectionName=
+# Secret for SaslRoleTokenSigner.
+# Default value `SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET`, which is "PulsarSecret".
+saslJaasServerRoleTokenSignerSecret=PulsarSecret
Review Comment:
ok, I will supply the secret as a file
--
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] liudezhi2098 commented on a diff in pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on code in PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#discussion_r861489274
##########
conf/proxy.conf:
##########
@@ -277,6 +277,23 @@ tokenAudienceClaim=
# The token audience stands for this broker. The field `tokenAudienceClaim` of a valid token, need contains this.
tokenAudience=
+### --- SASL Authentication Provider --- ###
+
+# This is a regexp, which limits the range of possible ids which can connect to the Broker using SASL.
+# Default value: `SaslConstants.JAAS_CLIENT_ALLOWED_IDS_DEFAULT`, which is ".*pulsar.*",
+# so only clients whose id contains 'pulsar' are allowed to connect.
+saslJaasClientAllowedIds=
+
+# Service Principal, for login context name.
+# Default value `SaslConstants.JAAS_DEFAULT_BROKER_SECTION_NAME`, which is "Broker".
+saslJaasBrokerSectionName=
+
+# Configure the secret to be used to SaslRoleTokenSigner
+# The secret can be specified like:
+# saslJaasServerRoleTokenSignerSecret=file:///my/saslRoleTokenSignerSecret.key
+# If saslJaasServerRoleTokenSignerSecret is empty, will use Default value `SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET`.
+#saslJaasServerRoleTokenSignerSecret=
Review Comment:
leaving the configuration commented out , compatible with existing schemas, give a 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] liudezhi2098 commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1096498772
@codelipenghui @jiazhai PTAL
--
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] liudezhi2098 commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1096703104
> @liudezhi2098 feel free to ping me when you need review or any help on the follow-up doc PR.
👌
--
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 diff in pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#discussion_r853371421
##########
conf/broker.conf:
##########
@@ -764,6 +764,9 @@ saslJaasClientAllowedIds=
# Default value `SaslConstants.JAAS_DEFAULT_BROKER_SECTION_NAME`, which is "Broker".
saslJaasServerSectionName=
+# Secret for SaslRoleTokenSigner.
+# Default value `SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET`, which is "PulsarSecret".
+saslJaasServerRoleTokenSignerSecret=PulsarSecret
Review Comment:
Why don't we leave this blank and force the user to configure it? I agree with @eolivelli that a default secret seems like a security hole.
Also, can we supply the secret as a file? Finally, note that the python script that configures these `.conf` files only obfuscates configs with `password` in the name. As it is written, this would not be obfuscated. We either need to supply the secret by file name, update this config name, or update the python script.
--
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] eolivelli commented on a diff in pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#discussion_r868861082
##########
pulsar-broker-auth-sasl/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderSasl.java:
##########
@@ -175,6 +186,24 @@ private String createAuthRoleToken(String role, String sessionId) {
return signed;
}
+ private byte[] readSecretFromUrl(String secretConfUrl) throws IOException {
+ if (secretConfUrl.startsWith("file:")) {
+ try {
+ return IOUtils.toByteArray(URL.createURL(secretConfUrl.trim()));
Review Comment:
for JWT token Authentication we use URI and not URL
https://github.com/apache/pulsar/blob/330fcb9787d9f822667f0617b22bcae66b4644e5/pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/AuthenticationToken.java#L81
I don't know the exact difference but I believe it is better to do the same way
##########
pulsar-broker-auth-sasl/src/test/java/org/apache/pulsar/broker/authentication/ProxySaslAuthenticationTest.java:
##########
@@ -182,6 +183,10 @@ protected void setup() throws Exception {
conf.setAuthenticationEnabled(true);
conf.setSaslJaasClientAllowedIds(".*" + localHostname + ".*");
conf.setSaslJaasServerSectionName("PulsarBroker");
+ File secretKeyFile = File.createTempFile("saslRoleTokenSignerSecret", ".key");
+ secretKeyFile.deleteOnExit();
Review Comment:
please do not use deleteOnExit but delete this file in the After/Teardown method or in a finally block
--
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 diff in pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#discussion_r868810934
##########
conf/broker.conf:
##########
@@ -760,11 +760,16 @@ tokenAudience=
# This is a regexp, which limits the range of possible ids which can connect to the Broker using SASL.
# Default value: `SaslConstants.JAAS_CLIENT_ALLOWED_IDS_DEFAULT`, which is ".*pulsar.*",
# so only clients whose id contains 'pulsar' are allowed to connect.
-saslJaasClientAllowedIds=
+saslJaasClientAllowedIds=.*pulsar.*
# Service Principal, for login context name.
-# Default value `SaslConstants.JAAS_DEFAULT_BROKER_SECTION_NAME`, which is "Broker".
-saslJaasServerSectionName=
+# Default value `SaslConstants.JAAS_DEFAULT_BROKER_SECTION_NAME`, which is "PulsarBroker".
+saslJaasServerSectionName=PulsarBroker
+
+# Path to file containing the secret to be used to SaslRoleTokenSigner
+# The Path can be specified like:
+# saslJaasServerRoleTokenSignerSecretPath=file:///my/saslRoleTokenSignerSecret.key
Review Comment:
Nit: we should document guidance on an appropriate minimum length for the secret. A short secret would lead to an easily brute forced discovery of the broker's secret, which would be very problematic.
--
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] liudezhi2098 commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1122349345
/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] michaeljmarshall commented on a diff in pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#discussion_r861838371
##########
pulsar-broker-auth-sasl/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderSasl.java:
##########
@@ -98,8 +102,14 @@ public void initialize(ServiceConfiguration config) throws IOException {
throw new IOException(e);
}
}
-
- this.signer = new SaslRoleTokenSigner(Long.toString(new Random().nextLong()).getBytes());
+ String saslJaasServerRoleTokenSignerSecretFile = config.getSaslJaasServerRoleTokenSignerSecret();
+ byte[] secret = null;
+ if (StringUtils.isNotBlank(saslJaasServerRoleTokenSignerSecretFile)) {
+ secret = readSecretFromUrl(saslJaasServerRoleTokenSignerSecretFile);
+ } else {
+ secret = SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET.getBytes();
Review Comment:
I see. How have users been using this feature? It seems odd to me that this redirect problem would only just be discovered now, if we assume this feature has been in use. If the current default is really problematic, then I think we should default to `null` and then fail on startup if the value is `null` while authentication is enabled for SASL.
--
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] liudezhi2098 commented on a diff in pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on code in PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#discussion_r868855183
##########
pulsar-broker-auth-sasl/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderSasl.java:
##########
@@ -175,6 +186,24 @@ private String createAuthRoleToken(String role, String sessionId) {
return signed;
}
+ private byte[] readSecretFromUrl(String secretConfUrl) throws IOException {
+ if (secretConfUrl.startsWith("file:")) {
+ try {
+ return IOUtils.toByteArray(URL.createURL(secretConfUrl.trim()));
+ } catch (IOException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new IOException(e);
+ }
Review Comment:
yes , I will remove.
--
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] github-actions[bot] commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1094991475
@liudezhi2098: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
[GitHub] [pulsar] liudezhi2098 commented on a diff in pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on code in PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#discussion_r863651392
##########
pulsar-broker-auth-sasl/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderSasl.java:
##########
@@ -98,8 +102,14 @@ public void initialize(ServiceConfiguration config) throws IOException {
throw new IOException(e);
}
}
-
- this.signer = new SaslRoleTokenSigner(Long.toString(new Random().nextLong()).getBytes());
+ String saslJaasServerRoleTokenSignerSecretFile = config.getSaslJaasServerRoleTokenSignerSecret();
+ byte[] secret = null;
+ if (StringUtils.isNotBlank(saslJaasServerRoleTokenSignerSecretFile)) {
+ secret = readSecretFromUrl(saslJaasServerRoleTokenSignerSecretFile);
+ } else {
+ secret = SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET.getBytes();
Review Comment:
I agree fail on startup if the value is null while authentication is enabled for SASL.
--
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] liudezhi2098 commented on a diff in pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on code in PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#discussion_r861487853
##########
pulsar-broker-auth-sasl/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderSasl.java:
##########
@@ -98,8 +102,14 @@ public void initialize(ServiceConfiguration config) throws IOException {
throw new IOException(e);
}
}
-
- this.signer = new SaslRoleTokenSigner(Long.toString(new Random().nextLong()).getBytes());
+ String saslJaasServerRoleTokenSignerSecretFile = config.getSaslJaasServerRoleTokenSignerSecret();
+ byte[] secret = null;
+ if (StringUtils.isNotBlank(saslJaasServerRoleTokenSignerSecretFile)) {
+ secret = readSecretFromUrl(saslJaasServerRoleTokenSignerSecretFile);
+ } else {
+ secret = SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET.getBytes();
Review Comment:
if we should maintain the current default (using a random Long)，this will cause users to be unable to use under multiple brokers.
--
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] liudezhi2098 commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1116130589
/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] liudezhi2098 commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1100014509
> Having a constant secret looks like a security hole to me.
>
> Can you please explain more?
@eolivelli becasue DefaultAsyncHttpClient will automatic request redirect url , and use current SaslRoleToken, there is currently no place to modify, so requesting to another broker, the saslRoleTokenSigner check failed.
If you use rest api, it will also cause this problem, having a constant secret , there will be certain security risks, but the key is stored on the broker side, and the client side does get.
eg :
```
DefaultAsyncHttpClientConfig.Builder confBuilder = new DefaultAsyncHttpClientConfig.Builder();
confBuilder.setFollowRedirect(true);
```
--
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] liudezhi2098 commented on pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#issuecomment-1099805348
/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] liudezhi2098 commented on a diff in pull request #15121: [Authenticate] fix Invalid signature error when use Kerberos Authentication
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on code in PR #15121:
URL: https://github.com/apache/pulsar/pull/15121#discussion_r848991282
##########
conf/broker.conf:
##########
@@ -767,6 +767,9 @@ saslJaasClientAllowedIds=
# Default value `SaslConstants.JAAS_DEFAULT_BROKER_SECTION_NAME`, which is "Broker".
saslJaasBrokerSectionName=
+# Secret for SaslRoleTokenSigner.
+# Default value `SaslConstants.JAAS_DEFAULT_ROLE_TOKEN_SIGNER_SECRET`, which is "PulsarSecret".
+saslJaasServerRoleTokenSignerSecret=PulsarSecret
Review Comment:
yes , I will add
--
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