Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-6608

Relax Kerberos login contexts parsing by trimming whitespaces in contexts list

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.0, 1.4.0
    • Component/s: Configuration, Security
    • Labels:
      None

      Description

      The Kerberos login contexts list parsing right now isn't quite user-friendly.
      The list must be provided as: security.kerberos.login.contexts: Client,KafkaClient, without any whitespace in between.

      We can relax this to be more user-friendly by trimming any whitespaces in the list.

      A user actually stumbled across this: http://apache-flink-user-mailing-list-archive.2336050.n4.nabble.com/Problems-with-Kerberos-Kafka-connection-in-version-1-2-0-td12580.html#a12589

        Issue Links

          Activity

          Hide
          eronwright Eron Wright added a comment -

          Sorry about this, thanks for fixing.

          Show
          eronwright Eron Wright added a comment - Sorry about this, thanks for fixing.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user tzulitai opened a pull request:

          https://github.com/apache/flink/pull/3928

          FLINK-6608 [security, config] Relax Kerberos login contexts parsing

          This PR allows whitespaces in the list of configured Kerberos login contexts.
          It generally covers some over scenarios as well (covered in the additional parsing test in `SecurityUtilsTest`).

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/tzulitai/flink FLINK-6608

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/3928.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #3928


          commit c7fd98d2fde681cef2943c5772a93e20d162fc88
          Author: Tzu-Li (Gordon) Tai <tzulitai@apache.org>
          Date: 2017-05-17T08:00:37Z

          FLINK-6608 [security, config] Relax Kerberos login contexts parsing


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user tzulitai opened a pull request: https://github.com/apache/flink/pull/3928 FLINK-6608 [security, config] Relax Kerberos login contexts parsing This PR allows whitespaces in the list of configured Kerberos login contexts. It generally covers some over scenarios as well (covered in the additional parsing test in `SecurityUtilsTest`). You can merge this pull request into a Git repository by running: $ git pull https://github.com/tzulitai/flink FLINK-6608 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3928.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3928 commit c7fd98d2fde681cef2943c5772a93e20d162fc88 Author: Tzu-Li (Gordon) Tai <tzulitai@apache.org> Date: 2017-05-17T08:00:37Z FLINK-6608 [security, config] Relax Kerberos login contexts parsing
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sunjincheng121 commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3928#discussion_r117001581

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
          @@ -230,10 +231,21 @@ private void validate() {
          }

          private static List<String> parseList(String value) {

          • if(value == null) {
            + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
          • return Arrays.asList(value.split(","));
              • End diff –

          I think can we using regular expression: `return Arrays.asList(value.replaceAll("\\s*,
          s*", ",").split(","));`

          Show
          githubbot ASF GitHub Bot added a comment - Github user sunjincheng121 commented on a diff in the pull request: https://github.com/apache/flink/pull/3928#discussion_r117001581 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java — @@ -230,10 +231,21 @@ private void validate() { } private static List<String> parseList(String value) { if(value == null) { + if(value == null || value.isEmpty()) { return Collections.emptyList(); } return Arrays.asList(value.split(",")); End diff – I think can we using regular expression: `return Arrays.asList(value.replaceAll("\\s*, s*", ",").split(","));`
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tzulitai commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3928#discussion_r117003024

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
          @@ -230,10 +231,21 @@ private void validate() {
          }

          private static List<String> parseList(String value) {

          • if(value == null) {
            + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
          • return Arrays.asList(value.split(","));
              • End diff –

          Ah, right, thanks for the review and suggestion Will use regex instead.

          Show
          githubbot ASF GitHub Bot added a comment - Github user tzulitai commented on a diff in the pull request: https://github.com/apache/flink/pull/3928#discussion_r117003024 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java — @@ -230,10 +231,21 @@ private void validate() { } private static List<String> parseList(String value) { if(value == null) { + if(value == null || value.isEmpty()) { return Collections.emptyList(); } return Arrays.asList(value.split(",")); End diff – Ah, right, thanks for the review and suggestion Will use regex instead.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tzulitai commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3928#discussion_r117009648

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
          @@ -230,10 +231,21 @@ private void validate() {
          }

          private static List<String> parseList(String value) {

          • if(value == null) {
            + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
          • return Arrays.asList(value.split(","));
              • End diff –

          Addressed!

          Show
          githubbot ASF GitHub Bot added a comment - Github user tzulitai commented on a diff in the pull request: https://github.com/apache/flink/pull/3928#discussion_r117009648 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java — @@ -230,10 +231,21 @@ private void validate() { } private static List<String> parseList(String value) { if(value == null) { + if(value == null || value.isEmpty()) { return Collections.emptyList(); } return Arrays.asList(value.split(",")); End diff – Addressed!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user EronWright commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3928#discussion_r117068349

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
          @@ -230,10 +231,21 @@ private void validate() {
          }

          private static List<String> parseList(String value) {

          • if(value == null) {
            + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
          • return Arrays.asList(value.split(","));
              • End diff –

          An alternative to consider:
          `Collections.list(new StringTokenizer(",X, ,,Y , Z,", ", "))` produces `[X, Y, Z]`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user EronWright commented on a diff in the pull request: https://github.com/apache/flink/pull/3928#discussion_r117068349 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java — @@ -230,10 +231,21 @@ private void validate() { } private static List<String> parseList(String value) { if(value == null) { + if(value == null || value.isEmpty()) { return Collections.emptyList(); } return Arrays.asList(value.split(",")); End diff – An alternative to consider: `Collections.list(new StringTokenizer(",X, ,,Y , Z,", ", "))` produces ` [X, Y, Z] `.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sunjincheng121 commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3928#discussion_r117142993

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
          @@ -230,10 +231,21 @@ private void validate() {
          }

          private static List<String> parseList(String value) {

          • if(value == null) {
            + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
          • return Arrays.asList(value.split(","));
              • End diff –

          @EronWright your suggestion will be work. But I'd like using regular expression. The JDK DOC also has the same recommend:
          `StringTokenizer is a legacy class that is retained for compatibility reasons although its use is discouraged in new code. It is recommended that anyone seeking this functionality use the split method of String or the java.util.regex package instead.`
          Please see: http://docs.oracle.com/javase/7/docs/api/java/util/StringTokenizer.html
          What do you think ? @EronWright @tzulitai
          Best,
          SunJincheng

          Show
          githubbot ASF GitHub Bot added a comment - Github user sunjincheng121 commented on a diff in the pull request: https://github.com/apache/flink/pull/3928#discussion_r117142993 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java — @@ -230,10 +231,21 @@ private void validate() { } private static List<String> parseList(String value) { if(value == null) { + if(value == null || value.isEmpty()) { return Collections.emptyList(); } return Arrays.asList(value.split(",")); End diff – @EronWright your suggestion will be work. But I'd like using regular expression. The JDK DOC also has the same recommend: `StringTokenizer is a legacy class that is retained for compatibility reasons although its use is discouraged in new code. It is recommended that anyone seeking this functionality use the split method of String or the java.util.regex package instead.` Please see: http://docs.oracle.com/javase/7/docs/api/java/util/StringTokenizer.html What do you think ? @EronWright @tzulitai Best, SunJincheng
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user EronWright commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3928#discussion_r117144044

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
          @@ -230,10 +231,21 @@ private void validate() {
          }

          private static List<String> parseList(String value) {

          • if(value == null) {
            + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
          • return Arrays.asList(value.split(","));
              • End diff –

          The wisdom of that paragraph has been debated extensively on stackoverflow. I'll just say that the `StringTokenizer` is not deprecated and, in my opinion, fits this scenario uniquely well. Go ahead either way.

          Show
          githubbot ASF GitHub Bot added a comment - Github user EronWright commented on a diff in the pull request: https://github.com/apache/flink/pull/3928#discussion_r117144044 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java — @@ -230,10 +231,21 @@ private void validate() { } private static List<String> parseList(String value) { if(value == null) { + if(value == null || value.isEmpty()) { return Collections.emptyList(); } return Arrays.asList(value.split(",")); End diff – The wisdom of that paragraph has been debated extensively on stackoverflow. I'll just say that the `StringTokenizer` is not deprecated and, in my opinion, fits this scenario uniquely well. Go ahead either way.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sunjincheng121 commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3928#discussion_r117146767

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
          @@ -230,10 +230,15 @@ private void validate() {
          }

          private static List<String> parseList(String value) {

          • if(value == null) {
            + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
          • return Arrays.asList(value.split(","));
            +
            + return Arrays.asList(value
            + .replaceAll("\\s*,
            s*", ",") // remove whitespaces surrounding commas
            + .replaceAll(",,", ",") // remove empty entries
              • End diff –

          Hi, @tzulitai `.replaceAll("\\s*,
          s*", ",").replaceAll(",,", ",") ` can not deal with `" a, b,,, c d, e "`
          I think we can using the expression as follows:
          `str.trim().replaceAll("(\\s*,
          s*)
          ", ",")` OR `str.replace("/^\\s+|\\s+$/g","").replaceAll("(\\s*,
          s*)+", ",");`

          Show
          githubbot ASF GitHub Bot added a comment - Github user sunjincheng121 commented on a diff in the pull request: https://github.com/apache/flink/pull/3928#discussion_r117146767 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java — @@ -230,10 +230,15 @@ private void validate() { } private static List<String> parseList(String value) { if(value == null) { + if(value == null || value.isEmpty()) { return Collections.emptyList(); } return Arrays.asList(value.split(",")); + + return Arrays.asList(value + .replaceAll("\\s*, s*", ",") // remove whitespaces surrounding commas + .replaceAll(",,", ",") // remove empty entries End diff – Hi, @tzulitai `.replaceAll("\\s*, s*", ",").replaceAll(",,", ",") ` can not deal with `" a, b,,, c d, e "` I think we can using the expression as follows: `str.trim().replaceAll("(\\s*, s*) ", ",")` OR `str.replace("/^\\s+|\\s+$/g","").replaceAll("(\\s*, s*)+", ",");`
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tzulitai commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3928#discussion_r117205738

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java —
          @@ -230,10 +230,15 @@ private void validate() {
          }

          private static List<String> parseList(String value) {

          • if(value == null) {
            + if(value == null || value.isEmpty()) { return Collections.emptyList(); }
          • return Arrays.asList(value.split(","));
            +
            + return Arrays.asList(value
            + .replaceAll("\\s*,
            s*", ",") // remove whitespaces surrounding commas
            + .replaceAll(",,", ",") // remove empty entries
              • End diff –

          Thanks again, that makes sense

          Show
          githubbot ASF GitHub Bot added a comment - Github user tzulitai commented on a diff in the pull request: https://github.com/apache/flink/pull/3928#discussion_r117205738 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/security/SecurityUtils.java — @@ -230,10 +230,15 @@ private void validate() { } private static List<String> parseList(String value) { if(value == null) { + if(value == null || value.isEmpty()) { return Collections.emptyList(); } return Arrays.asList(value.split(",")); + + return Arrays.asList(value + .replaceAll("\\s*, s*", ",") // remove whitespaces surrounding commas + .replaceAll(",,", ",") // remove empty entries End diff – Thanks again, that makes sense
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tzulitai commented on the issue:

          https://github.com/apache/flink/pull/3928

          Thanks for the reviews @sunjincheng121 @EronWright.
          I personally prefer to stick with the regex approach. But really either way is fine

          Don't really expect any test errors on this, but doing one final run just to be safe, and then merging.

          Show
          githubbot ASF GitHub Bot added a comment - Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/3928 Thanks for the reviews @sunjincheng121 @EronWright. I personally prefer to stick with the regex approach. But really either way is fine Don't really expect any test errors on this, but doing one final run just to be safe, and then merging.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sunjincheng121 commented on the issue:

          https://github.com/apache/flink/pull/3928

          +1 to merged.

          Show
          githubbot ASF GitHub Bot added a comment - Github user sunjincheng121 commented on the issue: https://github.com/apache/flink/pull/3928 +1 to merged.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tzulitai commented on the issue:

          https://github.com/apache/flink/pull/3928

          Merging to 1.3 and master

          Show
          githubbot ASF GitHub Bot added a comment - Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/3928 Merging to 1.3 and master
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/3928

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3928
          Hide
          tzulitai Tzu-Li (Gordon) Tai added a comment -

          Fixed for 1.3 via 1a43badcd2e7893819f6aba9e7cd7688176efc58.
          Fixed for master via ffa9aa0ee08abeb2e167fc8d343b4522ba3d2ce2.

          Show
          tzulitai Tzu-Li (Gordon) Tai added a comment - Fixed for 1.3 via 1a43badcd2e7893819f6aba9e7cd7688176efc58. Fixed for master via ffa9aa0ee08abeb2e167fc8d343b4522ba3d2ce2.

            People

            • Assignee:
              tzulitai Tzu-Li (Gordon) Tai
              Reporter:
              tzulitai Tzu-Li (Gordon) Tai
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development