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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
