mimaison commented on code in PR #18519:
URL: https://github.com/apache/kafka/pull/18519#discussion_r1914977181


##########
clients/src/main/java/org/apache/kafka/common/config/internals/BrokerSecurityConfigs.java:
##########
@@ -130,6 +130,9 @@ public class BrokerSecurityConfigs {
 
     public static final String SASL_MECHANISM_INTER_BROKER_PROTOCOL_CONFIG = 
"sasl.mechanism.inter.broker.protocol";
     public static final String SASL_MECHANISM_INTER_BROKER_PROTOCOL_DOC = 
"SASL mechanism used for inter-broker communication. Default is GSSAPI.";
+
+    // The white list of the SASL OAUTHBEARER endpoints
+    public static final String ALLOWED_SASL_OAUTHBEARER_URL_CONFIG = 
"org.apache.kafka.sasl.oauthbearer.allowed.url";

Review Comment:
   If it's a list, should it be `_URLS_CONFIG` and `.urls`?



##########
clients/src/main/java/org/apache/kafka/common/config/internals/BrokerSecurityConfigs.java:
##########
@@ -130,6 +130,9 @@ public class BrokerSecurityConfigs {
 
     public static final String SASL_MECHANISM_INTER_BROKER_PROTOCOL_CONFIG = 
"sasl.mechanism.inter.broker.protocol";
     public static final String SASL_MECHANISM_INTER_BROKER_PROTOCOL_DOC = 
"SASL mechanism used for inter-broker communication. Default is GSSAPI.";
+
+    // The white list of the SASL OAUTHBEARER endpoints

Review Comment:
   Let's not use the term "whitelist", instead we can use "allowlist"



##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/ConfigurationUtils.java:
##########
@@ -228,4 +233,16 @@ public <T> T get(String name) {
         return (T) configs.get(name);
     }
 
+    // make sure the url is in the 
"org.apache.kafka.sasl.oauthbearer.allowed.url" system property
+    public void throwIfURLIsNotAllowed(String urlConfig) {
+        Set<String> allowedLoginModuleList = Arrays.stream(
+                        
System.getProperty(ALLOWED_SASL_OAUTHBEARER_URL_CONFIG, "").split(","))

Review Comment:
   If we default to empty list, is this going to break all users and force them 
to set this new system property? If so we should probably default to allow all 
paths, no?



-- 
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: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to