You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/10/24 08:40:18 UTC
[GitHub] [kafka] dongjinleekr opened a new pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
dongjinleekr opened a new pull request #11430:
URL: https://github.com/apache/kafka/pull/11430
As I left in the comments, the `StreamTokenizer` used by `org.apache.kafka.common.security.JaasConfig` recognizes alphabetical characters, dash, underscore, and dollar sign as a 'word' only. Because of that, a string that contains a number or a symbol like '^' (which are so common in the password) raises an error.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade 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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r741090237
##########
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.
+ // Tokenizer rules:
+ // 1. All bytes from 0 to 32 ({@code ' '}) are considered to be whitespace.
+ // 2. {@code '/'} (47) is a comment character. '//', '/*', '*/' are also allowed.
+ // 3. Single quote ({@code '\u005C''}, 39) and double quote ({@code '"'}, 34) are considered to be quote.
+ // 4. Ends of lines are treated as white space, not as separate tokens.
StreamTokenizer tokenizer = new StreamTokenizer(new StringReader(jaasConfigParams));
- tokenizer.slashSlashComments(true);
- tokenizer.slashStarComments(true);
- tokenizer.wordChars('-', '-');
- tokenizer.wordChars('_', '_');
- tokenizer.wordChars('$', '$');
+ tokenizer.resetSyntax(); // Reset the default configuration.
+ tokenizer.wordChars(32, 128); // All characters in [32, 128] are allowed.
+ tokenizer.wordChars(128 + 32, 255); // All characters in [160, 255] are allowed.
+ tokenizer.ordinaryChar(';'); // ';' is treated as a reserved word.
+ tokenizer.ordinaryChar('='); // '=' is treated as a reserved word.
+ tokenizer.whitespaceChars(0, ' '); // All characters in [0, 32] (including ' ') are treated as space character.
+ tokenizer.commentChar('/'); // '/' is treated as a comment character.
+ tokenizer.quoteChar('"'); // '"' is treated as a quote.
+ tokenizer.quoteChar('\''); // ''' is treated as a quote.
+ tokenizer.slashSlashComments(true); // Allow '//' comments.
+ tokenizer.slashStarComments(true); // Allow '/*', '*/' comments.
Review comment:
Hi @rajinisivaram,
I reviewed the implementation of `sun.security.provider.ConfigFile` [here](https://github.com/openjdk/jdk/blob/jdk-9%2B181/jdk/src/java.base/share/classes/sun/security/provider/ConfigFile.java#L413) and found the following:
- `ConfigFile` treats only '\*', '\_', '-', '$' as word characters; that is, the numbers or other symbols like '%', '^' are not allowed by default - Instead, we need to add '*' as a word character. (Currently, it is not.) Obviously, it is a bug.
- It seems like we don't need to define the ordinary characters (like ';', '='), spaces, comment characters, and quote characters explicitly, like `ConfigFile` does.
- It would be better to improve `JaasContextTest` by not only checking it works like the documentation, but also checking it works like the original, `ConfigFile` implementation.
I greatly appreciate you for guiding the right direction. I am now updating the PR now. Thanks again and stay tuned! :smiley:
--
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
[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
Posted by 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
[GitHub] [kafka] dongjinleekr commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r741090237
##########
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.
+ // Tokenizer rules:
+ // 1. All bytes from 0 to 32 ({@code ' '}) are considered to be whitespace.
+ // 2. {@code '/'} (47) is a comment character. '//', '/*', '*/' are also allowed.
+ // 3. Single quote ({@code '\u005C''}, 39) and double quote ({@code '"'}, 34) are considered to be quote.
+ // 4. Ends of lines are treated as white space, not as separate tokens.
StreamTokenizer tokenizer = new StreamTokenizer(new StringReader(jaasConfigParams));
- tokenizer.slashSlashComments(true);
- tokenizer.slashStarComments(true);
- tokenizer.wordChars('-', '-');
- tokenizer.wordChars('_', '_');
- tokenizer.wordChars('$', '$');
+ tokenizer.resetSyntax(); // Reset the default configuration.
+ tokenizer.wordChars(32, 128); // All characters in [32, 128] are allowed.
+ tokenizer.wordChars(128 + 32, 255); // All characters in [160, 255] are allowed.
+ tokenizer.ordinaryChar(';'); // ';' is treated as a reserved word.
+ tokenizer.ordinaryChar('='); // '=' is treated as a reserved word.
+ tokenizer.whitespaceChars(0, ' '); // All characters in [0, 32] (including ' ') are treated as space character.
+ tokenizer.commentChar('/'); // '/' is treated as a comment character.
+ tokenizer.quoteChar('"'); // '"' is treated as a quote.
+ tokenizer.quoteChar('\''); // ''' is treated as a quote.
+ tokenizer.slashSlashComments(true); // Allow '//' comments.
+ tokenizer.slashStarComments(true); // Allow '/*', '*/' comments.
Review comment:
Hi @rajinisivaram,
I reviewed the implementation of `sun.security.provider.ConfigFile` [here](https://github.com/openjdk/jdk/blob/jdk-9%2B181/jdk/src/java.base/share/classes/sun/security/provider/ConfigFile.java#L413) and found the following:
- `ConfigFile` treats only '\*', '\_', '-', '$' as word characters; that is, the numbers or other symbols like '%', '^' are not allowed by default - if the user needs to use these symbols, they should use quotes. So, **the problem described in the Jira issue is actually not a problem, but rather a documentation issue.** Instead, we need to add '*' as a word character. (Currently, it is not.) Obviously, it is a bug.
- It seems like we don't need to define the ordinary characters (like ';', '='), spaces, comment characters, and quote characters explicitly, like `ConfigFile` does.
- It would be better to improve `JaasContextTest` by not only checking it works like the documentation, but also checking it works like the original, `ConfigFile` implementation.
I greatly appreciate you for guiding the right direction. I am now updating the PR now. Thanks again and stay tuned! :smiley:
##########
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.
+ // Tokenizer rules:
+ // 1. All bytes from 0 to 32 ({@code ' '}) are considered to be whitespace.
+ // 2. {@code '/'} (47) is a comment character. '//', '/*', '*/' are also allowed.
+ // 3. Single quote ({@code '\u005C''}, 39) and double quote ({@code '"'}, 34) are considered to be quote.
+ // 4. Ends of lines are treated as white space, not as separate tokens.
StreamTokenizer tokenizer = new StreamTokenizer(new StringReader(jaasConfigParams));
- tokenizer.slashSlashComments(true);
- tokenizer.slashStarComments(true);
- tokenizer.wordChars('-', '-');
- tokenizer.wordChars('_', '_');
- tokenizer.wordChars('$', '$');
+ tokenizer.resetSyntax(); // Reset the default configuration.
+ tokenizer.wordChars(32, 128); // All characters in [32, 128] are allowed.
+ tokenizer.wordChars(128 + 32, 255); // All characters in [160, 255] are allowed.
+ tokenizer.ordinaryChar(';'); // ';' is treated as a reserved word.
+ tokenizer.ordinaryChar('='); // '=' is treated as a reserved word.
+ tokenizer.whitespaceChars(0, ' '); // All characters in [0, 32] (including ' ') are treated as space character.
+ tokenizer.commentChar('/'); // '/' is treated as a comment character.
+ tokenizer.quoteChar('"'); // '"' is treated as a quote.
+ tokenizer.quoteChar('\''); // ''' is treated as a quote.
+ tokenizer.slashSlashComments(true); // Allow '//' comments.
+ tokenizer.slashStarComments(true); // Allow '/*', '*/' comments.
Review comment:
Hi @rajinisivaram,
I reviewed the implementation of `sun.security.provider.ConfigFile` [here](https://github.com/openjdk/jdk/blob/jdk-9%2B181/jdk/src/java.base/share/classes/sun/security/provider/ConfigFile.java#L413) and found the following:
- `ConfigFile` treats only '\*', '\_', '-', '$' as word characters; that is, the numbers or other symbols like '%', '^' are not allowed by default - Instead, we need to add '*' as a word character. (Currently, it is not.) Obviously, it is a bug.
- It seems like we don't need to define the ordinary characters (like ';', '='), spaces, comment characters, and quote characters explicitly, like `ConfigFile` does.
- It would be better to improve `JaasContextTest` by not only checking it works like the documentation, but also checking it works like the original, `ConfigFile` implementation.
I greatly appreciate you for guiding the right direction. I am now updating the PR now. Thanks again and stay tuned! :smiley:
##########
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.
+ // Tokenizer rules:
+ // 1. All bytes from 0 to 32 ({@code ' '}) are considered to be whitespace.
+ // 2. {@code '/'} (47) is a comment character. '//', '/*', '*/' are also allowed.
+ // 3. Single quote ({@code '\u005C''}, 39) and double quote ({@code '"'}, 34) are considered to be quote.
+ // 4. Ends of lines are treated as white space, not as separate tokens.
StreamTokenizer tokenizer = new StreamTokenizer(new StringReader(jaasConfigParams));
- tokenizer.slashSlashComments(true);
- tokenizer.slashStarComments(true);
- tokenizer.wordChars('-', '-');
- tokenizer.wordChars('_', '_');
- tokenizer.wordChars('$', '$');
+ tokenizer.resetSyntax(); // Reset the default configuration.
+ tokenizer.wordChars(32, 128); // All characters in [32, 128] are allowed.
+ tokenizer.wordChars(128 + 32, 255); // All characters in [160, 255] are allowed.
+ tokenizer.ordinaryChar(';'); // ';' is treated as a reserved word.
+ tokenizer.ordinaryChar('='); // '=' is treated as a reserved word.
+ tokenizer.whitespaceChars(0, ' '); // All characters in [0, 32] (including ' ') are treated as space character.
+ tokenizer.commentChar('/'); // '/' is treated as a comment character.
+ tokenizer.quoteChar('"'); // '"' is treated as a quote.
+ tokenizer.quoteChar('\''); // ''' is treated as a quote.
+ tokenizer.slashSlashComments(true); // Allow '//' comments.
+ tokenizer.slashStarComments(true); // Allow '/*', '*/' comments.
Review comment:
Hi @rajinisivaram,
I reviewed the implementation of `sun.security.provider.ConfigFile` [here](https://github.com/openjdk/jdk/blob/jdk-9%2B181/jdk/src/java.base/share/classes/sun/security/provider/ConfigFile.java#L413) and found the following:
- `ConfigFile` treats only `*`, `_`, `-`, `$` as word characters; that is, the numbers or other symbols like `%`, `^` are not allowed by default - if the user needs to use these symbols, they should use quotes. So, the problem described in the Jira issue is actually not a problem, but rather a documentation issue. Instead, we need to add `*` as a word character. Currently, it is not and obviously, it is a bug.
- It seems like we don't need to define the ordinary characters (like `;`, `=`), spaces, comment characters, and quote characters explicitly, like `ConfigFile` does.
- It would be better to improve `JaasContextTest` by not only checking it works like the documentation, but also checking it works like the original, `ConfigFile` implementation. (I am trying to, but can't certain yet.)
I greatly appreciate you for guiding the right direction. I am now updating the PR now. Thanks again and stay tuned! :smiley:
--
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
[GitHub] [kafka] dongjinleekr commented on pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#issuecomment-955643910
Here is the update. I added some line-by-line comments on what each line does.
--
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
[GitHub] [kafka] rajinisivaram commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r740102409
##########
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.
+ // Tokenizer rules:
+ // 1. All bytes from 0 to 32 ({@code ' '}) are considered to be whitespace.
+ // 2. {@code '/'} (47) is a comment character. '//', '/*', '*/' are also allowed.
+ // 3. Single quote ({@code '\u005C''}, 39) and double quote ({@code '"'}, 34) are considered to be quote.
+ // 4. Ends of lines are treated as white space, not as separate tokens.
StreamTokenizer tokenizer = new StreamTokenizer(new StringReader(jaasConfigParams));
- tokenizer.slashSlashComments(true);
- tokenizer.slashStarComments(true);
- tokenizer.wordChars('-', '-');
- tokenizer.wordChars('_', '_');
- tokenizer.wordChars('$', '$');
+ tokenizer.resetSyntax(); // Reset the default configuration.
+ tokenizer.wordChars(32, 128); // All characters in [32, 128] are allowed.
+ tokenizer.wordChars(128 + 32, 255); // All characters in [160, 255] are allowed.
+ tokenizer.ordinaryChar(';'); // ';' is treated as a reserved word.
+ tokenizer.ordinaryChar('='); // '=' is treated as a reserved word.
+ tokenizer.whitespaceChars(0, ' '); // All characters in [0, 32] (including ' ') are treated as space character.
+ tokenizer.commentChar('/'); // '/' is treated as a comment character.
+ tokenizer.quoteChar('"'); // '"' is treated as a quote.
+ tokenizer.quoteChar('\''); // ''' is treated as a quote.
+ tokenizer.slashSlashComments(true); // Allow '//' comments.
+ tokenizer.slashStarComments(true); // Allow '/*', '*/' comments.
Review comment:
@dongjinleekr Thanks for the PR, can we add some tests to ensure that the syntax is identical to that of Java's standard file-based JAAS `Configuration` parsing?
--
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
[GitHub] [kafka] dongjinleekr commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r741104074
##########
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.
+ // Tokenizer rules:
+ // 1. All bytes from 0 to 32 ({@code ' '}) are considered to be whitespace.
+ // 2. {@code '/'} (47) is a comment character. '//', '/*', '*/' are also allowed.
+ // 3. Single quote ({@code '\u005C''}, 39) and double quote ({@code '"'}, 34) are considered to be quote.
+ // 4. Ends of lines are treated as white space, not as separate tokens.
StreamTokenizer tokenizer = new StreamTokenizer(new StringReader(jaasConfigParams));
- tokenizer.slashSlashComments(true);
- tokenizer.slashStarComments(true);
- tokenizer.wordChars('-', '-');
- tokenizer.wordChars('_', '_');
- tokenizer.wordChars('$', '$');
+ tokenizer.resetSyntax(); // Reset the default configuration.
+ tokenizer.wordChars(32, 128); // All characters in [32, 128] are allowed.
+ tokenizer.wordChars(128 + 32, 255); // All characters in [160, 255] are allowed.
+ tokenizer.ordinaryChar(';'); // ';' is treated as a reserved word.
+ tokenizer.ordinaryChar('='); // '=' is treated as a reserved word.
+ tokenizer.whitespaceChars(0, ' '); // All characters in [0, 32] (including ' ') are treated as space character.
+ tokenizer.commentChar('/'); // '/' is treated as a comment character.
+ tokenizer.quoteChar('"'); // '"' is treated as a quote.
+ tokenizer.quoteChar('\''); // ''' is treated as a quote.
+ tokenizer.slashSlashComments(true); // Allow '//' comments.
+ tokenizer.slashStarComments(true); // Allow '/*', '*/' comments.
Review comment:
Hi @rajinisivaram,
I reviewed the implementation of `sun.security.provider.ConfigFile` [here](https://github.com/openjdk/jdk/blob/jdk-9%2B181/jdk/src/java.base/share/classes/sun/security/provider/ConfigFile.java#L413) and found the following:
- `ConfigFile` treats only `*`, `_`, `-`, `$` as word characters; that is, the numbers or other symbols like `%`, `^` are not allowed by default - if the user needs to use these symbols, they should use quotes. So, the problem described in the Jira issue is actually not a problem, but rather a documentation issue. Instead, we need to add `*` as a word character. Currently, it is not and obviously, it is a bug.
- It seems like we don't need to define the ordinary characters (like `;`, `=`), spaces, comment characters, and quote characters explicitly, like `ConfigFile` does.
- It would be better to improve `JaasContextTest` by not only checking it works like the documentation, but also checking it works like the original, `ConfigFile` implementation. (I am trying to, but can't certain yet.)
I greatly appreciate you for guiding the right direction. I am now updating the PR now. Thanks again and stay tuned! :smiley:
--
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
[GitHub] [kafka] dongjinleekr commented on pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#issuecomment-961934705
@rajinisivaram Here is the fix; I allowed the asterisks following the base implementation and commented on how to mix numbers & the other symbols in the string. (oh, updating the Jira would also be needed. Isn't it?)
I also tried to find a way to assert it works identically with the base implementation, but failed. Instead, I just added additional tests only.
--
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
[GitHub] [kafka] dongjinleekr commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r736269591
##########
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.
Review comment:
oh yes, I was a little bit confused; what I meant was ascci code 32 and 47, not `{@code ' '}` or `{@code '/'}`.
--
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
[GitHub] [kafka] dongjinleekr commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r736515669
##########
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. It seems better to add additional comments per line what it does, along with the overall parsing policy on top.
2. Of course, the latter `tokenizer.whitespaceChars(0, ' ');` has higher priority; i just meant to exclude all special characters in [0, 31] range with ` tokenizer.wordChars(32, 128);`.
--
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
[GitHub] [kafka] dongjinleekr commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r741090237
##########
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.
+ // Tokenizer rules:
+ // 1. All bytes from 0 to 32 ({@code ' '}) are considered to be whitespace.
+ // 2. {@code '/'} (47) is a comment character. '//', '/*', '*/' are also allowed.
+ // 3. Single quote ({@code '\u005C''}, 39) and double quote ({@code '"'}, 34) are considered to be quote.
+ // 4. Ends of lines are treated as white space, not as separate tokens.
StreamTokenizer tokenizer = new StreamTokenizer(new StringReader(jaasConfigParams));
- tokenizer.slashSlashComments(true);
- tokenizer.slashStarComments(true);
- tokenizer.wordChars('-', '-');
- tokenizer.wordChars('_', '_');
- tokenizer.wordChars('$', '$');
+ tokenizer.resetSyntax(); // Reset the default configuration.
+ tokenizer.wordChars(32, 128); // All characters in [32, 128] are allowed.
+ tokenizer.wordChars(128 + 32, 255); // All characters in [160, 255] are allowed.
+ tokenizer.ordinaryChar(';'); // ';' is treated as a reserved word.
+ tokenizer.ordinaryChar('='); // '=' is treated as a reserved word.
+ tokenizer.whitespaceChars(0, ' '); // All characters in [0, 32] (including ' ') are treated as space character.
+ tokenizer.commentChar('/'); // '/' is treated as a comment character.
+ tokenizer.quoteChar('"'); // '"' is treated as a quote.
+ tokenizer.quoteChar('\''); // ''' is treated as a quote.
+ tokenizer.slashSlashComments(true); // Allow '//' comments.
+ tokenizer.slashStarComments(true); // Allow '/*', '*/' comments.
Review comment:
Hi @rajinisivaram,
I reviewed the implementation of `sun.security.provider.ConfigFile` [here](https://github.com/openjdk/jdk/blob/jdk-9%2B181/jdk/src/java.base/share/classes/sun/security/provider/ConfigFile.java#L413) and found the following:
- `ConfigFile` treats only '\*', '\_', '-', '$' as word characters; that is, the numbers or other symbols like '%', '^' are not allowed by default - if the user needs to use these symbols, they should use quotes. So, **the problem described in the Jira issue is actually not a problem, but rather a documentation issue.** Instead, we need to add '*' as a word character. (Currently, it is not.) Obviously, it is a bug.
- It seems like we don't need to define the ordinary characters (like ';', '='), spaces, comment characters, and quote characters explicitly, like `ConfigFile` does.
- It would be better to improve `JaasContextTest` by not only checking it works like the documentation, but also checking it works like the original, `ConfigFile` implementation.
I greatly appreciate you for guiding the right direction. I am now updating the PR now. Thanks again and stay tuned! :smiley:
--
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
[GitHub] [kafka] dongjinleekr commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r740150160
##########
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.
+ // Tokenizer rules:
+ // 1. All bytes from 0 to 32 ({@code ' '}) are considered to be whitespace.
+ // 2. {@code '/'} (47) is a comment character. '//', '/*', '*/' are also allowed.
+ // 3. Single quote ({@code '\u005C''}, 39) and double quote ({@code '"'}, 34) are considered to be quote.
+ // 4. Ends of lines are treated as white space, not as separate tokens.
StreamTokenizer tokenizer = new StreamTokenizer(new StringReader(jaasConfigParams));
- tokenizer.slashSlashComments(true);
- tokenizer.slashStarComments(true);
- tokenizer.wordChars('-', '-');
- tokenizer.wordChars('_', '_');
- tokenizer.wordChars('$', '$');
+ tokenizer.resetSyntax(); // Reset the default configuration.
+ tokenizer.wordChars(32, 128); // All characters in [32, 128] are allowed.
+ tokenizer.wordChars(128 + 32, 255); // All characters in [160, 255] are allowed.
+ tokenizer.ordinaryChar(';'); // ';' is treated as a reserved word.
+ tokenizer.ordinaryChar('='); // '=' is treated as a reserved word.
+ tokenizer.whitespaceChars(0, ' '); // All characters in [0, 32] (including ' ') are treated as space character.
+ tokenizer.commentChar('/'); // '/' is treated as a comment character.
+ tokenizer.quoteChar('"'); // '"' is treated as a quote.
+ tokenizer.quoteChar('\''); // ''' is treated as a quote.
+ tokenizer.slashSlashComments(true); // Allow '//' comments.
+ tokenizer.slashStarComments(true); // Allow '/*', '*/' comments.
Review comment:
Hi @rajinisivaram,
I searched the definition of Java's standard file-based JAAS `Configuration` but could not find one. But, after reviewing [the related `LoginModule` implementations](https://docs.oracle.com/javase/10/security/appendix-b-jaas-login-configuration-file.htm) ([#1](https://github.com/openjdk/jdk/blob/jdk-9%2B181/jdk/src/jdk.security.auth/share/classes/com/sun/security/auth/module/KeyStoreLoginModule.java), [#2](https://github.com/openjdk/jdk/blob/jdk-9%2B181/jdk/src/jdk.security.auth/share/classes/com/sun/security/auth/module/Krb5LoginModule.java)) and [example](https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.4/security-reference/content/kerberos_nonambari_create_jaas_configuration_files.html), what we need to test seems almost clear:
- It should be able to parse a string (including non-reserved symbols), number, boolean, asterisk, canonical java class name, and URL; regardless it is quoted or not.
- **We should remove the extended ASCII codes (i.e., [128, 256)) from the PR.** That is, we should allow 7-bit alphabets and symbols only.
Is this okay? Then, I will proceed to update the tests accordingly.
--
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
[GitHub] [kafka] dongjinleekr commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r736245197
##########
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:
No, the numbers are now regarded alphabetical characters like the other symbols for ` tokenizer.wordChars(32, 128);`. `;`, `=`, characters with code 0 to ` ` are the exceptions.
--
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
[GitHub] [kafka] dongjinleekr commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r738930580
##########
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:
Now tests 5 cases - starts with numeric, starts with symbolic, has numeric in the middle, has symbolic in the middle, has both of numeric and symbolic in the middle.
--
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
[GitHub] [kafka] dongjinleekr commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r736271082
##########
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.
Review comment:
Totally agree with your way. 👍
--
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
[GitHub] [kafka] showuon commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r739650609
##########
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:
@dongjinleekr
Could you please also add additional comments per line what it does as you mentioned. 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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr commented on pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#issuecomment-954434181
Here is the update. I improved the parsing rule documentation as @showuon pointed out and also subdivided the test cases like @bvn13 suggested.
--
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
[GitHub] [kafka] showuon commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r736150581
##########
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); //
Review comment:
redundant `//`
##########
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:
So, we make the numbers as "whitespace". Does that mean, the password "abc12" will equals to "abc23"?
##########
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.
Review comment:
This line is not required.
##########
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.
Review comment:
Could we put the java doc like this:
```
// All characters except space, comment, quote, equal and semicolon are considered to be alphabetic.
// tokenizer rules:
// 1. All bytes from 0 to ' ' {@code ' '} are considered to be whitespace.
// 2. '/' {@code '/'} is a comment character. '//', '/*', '*/' are also allowed.
// 3. ....
```
What do you think?
##########
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.
Review comment:
Why do we need 2 identical chars in java doc?
ex: 0 to `' ' {@code ' '}` are considered to be whitespace.
`'/' {@code '/'}` is...
--
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
[GitHub] [kafka] dongjinleekr commented on pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#issuecomment-1069811650
Rebased onto the latest trunk. cc/ @rajinisivaram
--
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
[GitHub] [kafka] dongjinleekr commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r736487363
##########
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:
No Problem.
--
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
[GitHub] [kafka] dongjinleekr commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r741090237
##########
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.
+ // Tokenizer rules:
+ // 1. All bytes from 0 to 32 ({@code ' '}) are considered to be whitespace.
+ // 2. {@code '/'} (47) is a comment character. '//', '/*', '*/' are also allowed.
+ // 3. Single quote ({@code '\u005C''}, 39) and double quote ({@code '"'}, 34) are considered to be quote.
+ // 4. Ends of lines are treated as white space, not as separate tokens.
StreamTokenizer tokenizer = new StreamTokenizer(new StringReader(jaasConfigParams));
- tokenizer.slashSlashComments(true);
- tokenizer.slashStarComments(true);
- tokenizer.wordChars('-', '-');
- tokenizer.wordChars('_', '_');
- tokenizer.wordChars('$', '$');
+ tokenizer.resetSyntax(); // Reset the default configuration.
+ tokenizer.wordChars(32, 128); // All characters in [32, 128] are allowed.
+ tokenizer.wordChars(128 + 32, 255); // All characters in [160, 255] are allowed.
+ tokenizer.ordinaryChar(';'); // ';' is treated as a reserved word.
+ tokenizer.ordinaryChar('='); // '=' is treated as a reserved word.
+ tokenizer.whitespaceChars(0, ' '); // All characters in [0, 32] (including ' ') are treated as space character.
+ tokenizer.commentChar('/'); // '/' is treated as a comment character.
+ tokenizer.quoteChar('"'); // '"' is treated as a quote.
+ tokenizer.quoteChar('\''); // ''' is treated as a quote.
+ tokenizer.slashSlashComments(true); // Allow '//' comments.
+ tokenizer.slashStarComments(true); // Allow '/*', '*/' comments.
Review comment:
Hi @rajinisivaram,
I reviewed the implementation of `sun.security.provider.ConfigFile` [here](https://github.com/openjdk/jdk/blob/jdk-9%2B181/jdk/src/java.base/share/classes/sun/security/provider/ConfigFile.java#L413) and found the following:
- `ConfigFile` treats only '\*', '\_', '-', '$' as word characters; that is, the numbers or other symbols like '%', '^' are not allowed by default - if the user needs to use these symbols, they should use quotes. So, **the problem described in the Jira issue is actually not a problem, but rather a documentation issue.** Instead, we need to add '*' as a word character. (Currently, it is not.) Obviously, it is a bug.
- It seems like we don't need to define the ordinary characters (like ';', '='), spaces, comment characters, and quote characters explicitly, like `ConfigFile` does.
- It would be better to improve `JaasContextTest` by not only checking it works like the documentation, but also checking it works like the original, `ConfigFile` implementation.
I greatly appreciate you for guiding the right direction. I am now updating the PR now. Thanks again and stay tuned! :smiley:
##########
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.
+ // Tokenizer rules:
+ // 1. All bytes from 0 to 32 ({@code ' '}) are considered to be whitespace.
+ // 2. {@code '/'} (47) is a comment character. '//', '/*', '*/' are also allowed.
+ // 3. Single quote ({@code '\u005C''}, 39) and double quote ({@code '"'}, 34) are considered to be quote.
+ // 4. Ends of lines are treated as white space, not as separate tokens.
StreamTokenizer tokenizer = new StreamTokenizer(new StringReader(jaasConfigParams));
- tokenizer.slashSlashComments(true);
- tokenizer.slashStarComments(true);
- tokenizer.wordChars('-', '-');
- tokenizer.wordChars('_', '_');
- tokenizer.wordChars('$', '$');
+ tokenizer.resetSyntax(); // Reset the default configuration.
+ tokenizer.wordChars(32, 128); // All characters in [32, 128] are allowed.
+ tokenizer.wordChars(128 + 32, 255); // All characters in [160, 255] are allowed.
+ tokenizer.ordinaryChar(';'); // ';' is treated as a reserved word.
+ tokenizer.ordinaryChar('='); // '=' is treated as a reserved word.
+ tokenizer.whitespaceChars(0, ' '); // All characters in [0, 32] (including ' ') are treated as space character.
+ tokenizer.commentChar('/'); // '/' is treated as a comment character.
+ tokenizer.quoteChar('"'); // '"' is treated as a quote.
+ tokenizer.quoteChar('\''); // ''' is treated as a quote.
+ tokenizer.slashSlashComments(true); // Allow '//' comments.
+ tokenizer.slashStarComments(true); // Allow '/*', '*/' comments.
Review comment:
Hi @rajinisivaram,
I reviewed the implementation of `sun.security.provider.ConfigFile` [here](https://github.com/openjdk/jdk/blob/jdk-9%2B181/jdk/src/java.base/share/classes/sun/security/provider/ConfigFile.java#L413) and found the following:
- `ConfigFile` treats only '\*', '\_', '-', '$' as word characters; that is, the numbers or other symbols like '%', '^' are not allowed by default - Instead, we need to add '*' as a word character. (Currently, it is not.) Obviously, it is a bug.
- It seems like we don't need to define the ordinary characters (like ';', '='), spaces, comment characters, and quote characters explicitly, like `ConfigFile` does.
- It would be better to improve `JaasContextTest` by not only checking it works like the documentation, but also checking it works like the original, `ConfigFile` implementation.
I greatly appreciate you for guiding the right direction. I am now updating the PR now. Thanks again and stay tuned! :smiley:
##########
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.
+ // Tokenizer rules:
+ // 1. All bytes from 0 to 32 ({@code ' '}) are considered to be whitespace.
+ // 2. {@code '/'} (47) is a comment character. '//', '/*', '*/' are also allowed.
+ // 3. Single quote ({@code '\u005C''}, 39) and double quote ({@code '"'}, 34) are considered to be quote.
+ // 4. Ends of lines are treated as white space, not as separate tokens.
StreamTokenizer tokenizer = new StreamTokenizer(new StringReader(jaasConfigParams));
- tokenizer.slashSlashComments(true);
- tokenizer.slashStarComments(true);
- tokenizer.wordChars('-', '-');
- tokenizer.wordChars('_', '_');
- tokenizer.wordChars('$', '$');
+ tokenizer.resetSyntax(); // Reset the default configuration.
+ tokenizer.wordChars(32, 128); // All characters in [32, 128] are allowed.
+ tokenizer.wordChars(128 + 32, 255); // All characters in [160, 255] are allowed.
+ tokenizer.ordinaryChar(';'); // ';' is treated as a reserved word.
+ tokenizer.ordinaryChar('='); // '=' is treated as a reserved word.
+ tokenizer.whitespaceChars(0, ' '); // All characters in [0, 32] (including ' ') are treated as space character.
+ tokenizer.commentChar('/'); // '/' is treated as a comment character.
+ tokenizer.quoteChar('"'); // '"' is treated as a quote.
+ tokenizer.quoteChar('\''); // ''' is treated as a quote.
+ tokenizer.slashSlashComments(true); // Allow '//' comments.
+ tokenizer.slashStarComments(true); // Allow '/*', '*/' comments.
Review comment:
Hi @rajinisivaram,
I reviewed the implementation of `sun.security.provider.ConfigFile` [here](https://github.com/openjdk/jdk/blob/jdk-9%2B181/jdk/src/java.base/share/classes/sun/security/provider/ConfigFile.java#L413) and found the following:
- `ConfigFile` treats only `*`, `_`, `-`, `$` as word characters; that is, the numbers or other symbols like `%`, `^` are not allowed by default - if the user needs to use these symbols, they should use quotes. So, the problem described in the Jira issue is actually not a problem, but rather a documentation issue. Instead, we need to add `*` as a word character. Currently, it is not and obviously, it is a bug.
- It seems like we don't need to define the ordinary characters (like `;`, `=`), spaces, comment characters, and quote characters explicitly, like `ConfigFile` does.
- It would be better to improve `JaasContextTest` by not only checking it works like the documentation, but also checking it works like the original, `ConfigFile` implementation. (I am trying to, but can't certain yet.)
I greatly appreciate you for guiding the right direction. I am now updating the PR now. Thanks again and stay tuned! :smiley:
--
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
[GitHub] [kafka] dongjinleekr commented on pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#issuecomment-950285065
@rajinisivaram Could you have a look? You worked this feature in [commit 69d7671](https://github.com/apache/kafka/commit/9eb665c39af1950548f903ad959449f2e69d7671).
--
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
[GitHub] [kafka] rajinisivaram commented on a change in pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on a change in pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#discussion_r740207330
##########
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.
+ // Tokenizer rules:
+ // 1. All bytes from 0 to 32 ({@code ' '}) are considered to be whitespace.
+ // 2. {@code '/'} (47) is a comment character. '//', '/*', '*/' are also allowed.
+ // 3. Single quote ({@code '\u005C''}, 39) and double quote ({@code '"'}, 34) are considered to be quote.
+ // 4. Ends of lines are treated as white space, not as separate tokens.
StreamTokenizer tokenizer = new StreamTokenizer(new StringReader(jaasConfigParams));
- tokenizer.slashSlashComments(true);
- tokenizer.slashStarComments(true);
- tokenizer.wordChars('-', '-');
- tokenizer.wordChars('_', '_');
- tokenizer.wordChars('$', '$');
+ tokenizer.resetSyntax(); // Reset the default configuration.
+ tokenizer.wordChars(32, 128); // All characters in [32, 128] are allowed.
+ tokenizer.wordChars(128 + 32, 255); // All characters in [160, 255] are allowed.
+ tokenizer.ordinaryChar(';'); // ';' is treated as a reserved word.
+ tokenizer.ordinaryChar('='); // '=' is treated as a reserved word.
+ tokenizer.whitespaceChars(0, ' '); // All characters in [0, 32] (including ' ') are treated as space character.
+ tokenizer.commentChar('/'); // '/' is treated as a comment character.
+ tokenizer.quoteChar('"'); // '"' is treated as a quote.
+ tokenizer.quoteChar('\''); // ''' is treated as a quote.
+ tokenizer.slashSlashComments(true); // Allow '//' comments.
+ tokenizer.slashStarComments(true); // Allow '/*', '*/' comments.
Review comment:
@dongjinleekr I think the base implementation in Java comes from `sun.security.provider.ConfigFile`, so maybe worth checking that. For both the positive and negative cases, we can create an example for test, parse it using Kafka's `JaasConfig` and also write to a temp file and parse by loading standard `Configuration`, verify that both have the same behaviour. Obviously we can't add exhaustive tests, the various cases discussed in this PR would be good enough.
--
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
[GitHub] [kafka] dongjinleekr commented on pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#issuecomment-961934705
@rajinisivaram Here is the fix; I allowed the asterisks following the base implementation and commented on how to mix numbers & the other symbols in the string. (oh, updating the Jira would also be needed. Isn't it?)
I also tried to find a way to assert it works identically with the base implementation, but failed. Instead, I just added additional tests only.
--
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
[GitHub] [kafka] dongjinleekr commented on pull request #11430: KAFKA-13352: Kafka Client does not support passwords starting with number in jaas config
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #11430:
URL: https://github.com/apache/kafka/pull/11430#issuecomment-961934705
@rajinisivaram Here is the fix; I allowed the asterisks following the base implementation and commented on how to mix numbers & the other symbols in the string. (oh, updating the Jira would also be needed. Isn't it?)
I also tried to find a way to assert it works identically with the base implementation, but failed. Instead, I just added additional tests only.
--
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