[GitHub] [kafka] bvn13 commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config

[GitBox <gi...@apache.org>]

bvn13 commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r736438767



##########
File path: clients/src/test/java/org/apache/kafka/common/security/JaasContextTest.java
##########
@@ -172,14 +172,42 @@ public void testMissingSemicolon() throws Exception {
 
     @Test
     public void testNumericOptionWithoutQuotes() throws Exception {
-        checkInvalidConfiguration("test.testNumericOptionWithoutQuotes required option1=3;");
+        try {
+            Map<String, Object> options = new HashMap<>();
+            options.put("option", "3");
+            checkConfiguration("test.testNumericOptionWithoutQuotes required option=3;", "test.testNumericOptionWithoutQuotes", LoginModuleControlFlag.REQUIRED, options);
+            fail("Given Jaas config is parsed properly but sun.security.provider.ConfigFile$Spi.<init> throws a IOException wrapped with a SecurityException.");
+        } catch (SecurityException e) {
+            assertEquals(IOException.class, e.getCause().getClass());
+        }
     }
 
     @Test
     public void testInvalidControlFlag() throws Exception {
         checkInvalidConfiguration("test.testInvalidControlFlag { option1=3;");
     }
 
+    @Test
+    public void testNumericWord() throws Exception {
+        Map<String, Object> options = new HashMap<>();
+        options.put("password", "k3fka");

Review comment:
       Could you please add another test for checking case when password starts with digit? 

##########
File path: clients/src/main/java/org/apache/kafka/common/security/JaasConfig.java
##########
@@ -50,12 +50,24 @@
     private final List<AppConfigurationEntry> configEntries;
 
     public JaasConfig(String loginContextName, String jaasConfigParams) {
+        // All characters except space, comment, quote, equal and semicolon are considered to be alphabetic.
+        // That is, numbers or symbols like '@' now can be a part of a word.
+        // All bytes from 0 to ' ' {@code ' '} are considered to be whitespace.
+        // '/' {@code '/'} is a comment character. '//', '/*', '*/' are also allowed.
+        // Single quote {@code '\u005C''} and double quote {@code '"'} are considered to be quote.
+        // Ends of lines are treated as white space, not as separate tokens.
         StreamTokenizer tokenizer = new StreamTokenizer(new StringReader(jaasConfigParams));
+        tokenizer.resetSyntax();
+        tokenizer.wordChars(32, 128); //
+        tokenizer.wordChars(128 + 32, 255);
+        tokenizer.ordinaryChar(';');
+        tokenizer.ordinaryChar('=');
+        tokenizer.whitespaceChars(0, ' ');

Review comment:
       1. I am confused about a character passing into 'hi' param. I mean you're using ' ' instead of ASCII code. I've checked: it is 32, it is a space. Could it be replaced with ASCII code instead of character? It is like magic number :)
   2. You are using two settings for space character:
   ```
       tokenizer.wordChars(32, 128);
       tokenizer.whitespaceChars(0, ' ');
   ```    
   which one has higher priority?




-- 
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-unsubscribe@kafka.apache.org

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