KAFKA-6319: Quote strings stored in JSON configs #4303

rajinisivaram · 2017-12-07T20:30:17Z

This is required for ACLs where SSL principals contain special characters (e.g. comma) that are escaped using backslash. The strings need to be quoted for JSON to ensure that the JSON stored in ZK is valid. Since ACLs may have been stored in ZK without escaping with previous versions of Kafka, a workaround has been added to parse those.
Also converted SslEndToEndAuthorizationTest to use a principal with special characters to ensure that this path is tested.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

rajinisivaram · 2017-12-07T20:30:46Z

@ijuma Can you review, please? Thank you!

ijuma · 2017-12-07T20:44:02Z

Thanks @rajinisivaram. What do we do about the invalid json strings that have already been stored?

rajinisivaram · 2017-12-08T10:47:27Z

@ijuma Have updated to handle non-escaped backslash in configs persisted by 1.0 code.

ijuma

Thanks for the PR, looks good overall, just a few minor comments.

ijuma · 2017-12-11T06:22:57Z

core/src/main/scala/kafka/utils/Json.scala

-    catch { case _: JsonProcessingException => None }
+    catch {
+      case _: JsonProcessingException =>
+        // In 1.0, Json#encode did not escape backslash or any other special characters. SSL principals


We should probably say, "Before 1.0.1, ..."

ijuma · 2017-12-11T06:24:23Z

core/src/main/scala/kafka/utils/Json.scala

+        // Note that this does not handle all special characters (e.g. non-escaped double quotes are not supported)
+        val escapedInput = input.replaceAll("\\\\", "\\\\\\\\")
+        try Option(mapper.readTree(escapedInput)).map(JsonValue(_))
+        catch { case _: JsonProcessingException => None }


Can we file a JIRA to move this so that it only applies to ACLs in trunk (for 1.1)?

https://issues.apache.org/jira/browse/KAFKA-6342

ijuma · 2017-12-11T06:24:40Z

core/src/main/scala/kafka/utils/Json.scala

@@ -19,6 +19,7 @@ package kafka.utils
 import java.nio.charset.StandardCharsets

 import com.fasterxml.jackson.core.JsonProcessingException
+import com.fasterxml.jackson.core.io.JsonStringEncoder


I think this is not used any more.

ijuma · 2017-12-11T06:27:00Z

core/src/test/scala/integration/kafka/api/SslEndToEndAuthorizationTest.scala

+  //
+  // Leading and trailing spaces in Kafka principal dont work with ACLs, but we can workaround by using
+  // a PrincipalBuilder that removes/replaces them.
+  private val clientCn = "\\#A client with special chars in CN : (\\, \\+ \\\" \\\\ \\< \\> \\; ')"


Use triple quotes to avoid the need for Java escaping?

ijuma · 2017-12-11T06:28:06Z

core/src/test/scala/integration/kafka/api/SslEndToEndAuthorizationTest.scala

  @Before
  override def setUp() {
    startSasl(jaasSections(List.empty, None, ZkSasl))
    super.setUp()
  }

+  override def clientSecurityProps(certAlias: String): Properties = {
+    val props = TestUtils.securityConfigs(Mode.CLIENT, securityProtocol, trustStoreFile, certAlias, clientCn, clientSaslProperties)
+    props.remove(SslConfigs.SSL_ENDPOINT_IDENTIFICATION_ALGORITHM_CONFIG)


Do we have endpoint identification enabled anywhere?

No, but since we may decide to make HTTPS the default value and this test would fail with endpoint validation enabled, I thought it was better to explicitly remove it.

ijuma · 2017-12-11T06:28:31Z

core/src/test/scala/unit/kafka/admin/AclCommandTest.scala

-  private val Users = Set(KafkaPrincipal.fromString("User:CN=writeuser,OU=Unknown,O=Unknown,L=Unknown,ST=Unknown,C=Unknown"), KafkaPrincipal.fromString("User:test2"))
+  private val Users = Set(KafkaPrincipal.fromString("User:CN=writeuser,OU=Unknown,O=Unknown,L=Unknown,ST=Unknown,C=Unknown"),
+    KafkaPrincipal.fromString("User:test2"),
+    KafkaPrincipal.fromString("User:CN=\\#User with special chars in CN : (\\, \\+ \\\" \\\\ \\< \\> \\; ')"))


Triple quotes to avoid Java escaping.

ijuma · 2017-12-11T06:30:37Z

core/src/test/scala/unit/kafka/utils/JsonTest.scala

+          }.mkString(",") + "}"
+        case o => Json.encode(o)
+      }
+    }


Simple this is a reasonably simple string, can we not just provide the full string without encoding instead of implementing this method?

ijuma · 2017-12-11T06:30:59Z

core/src/test/scala/unit/kafka/utils/JsonTest.scala

@@ -61,6 +85,8 @@ class JsonTest {
    assertEquals("{}", Json.encode(Map()))
    assertEquals("{\"a\":1,\"b\":2}", Json.encode(Map("a" -> 1, "b" -> 2)))
    assertEquals("{\"a\":[1,2],\"c\":[3,4]}", Json.encode(Map("a" -> Seq(1,2), "c" -> Seq(3,4))))
+    assertEquals("\"str1\\\\,str2\"", Json.encode("str1\\,str2"))
+    assertEquals("\"\\\"quoted\\\"\"", Json.encode("\"quoted\""))


Triple quotes in both lines.

…special chars

rajinisivaram · 2017-12-11T10:14:38Z

@ijuma Thank you for the review. I have made the updates.

ijuma · 2017-12-11T10:18:22Z

core/src/main/scala/kafka/utils/Json.scala

@@ -39,7 +38,7 @@ object Json {
    try Option(mapper.readTree(input)).map(JsonValue(_))
    catch {
      case _: JsonProcessingException =>
-        // In 1.0, Json#encode did not escape backslash or any other special characters. SSL principals
+        // Before 1.0.1, Json#encode did not escape backslash or any other special characters. SSL principals
        // stored in ACLs may contain backslash as an escape char, making the JSON generated in 1.0 invalid.


We probably want to replace 1.0 as well.

ijuma · 2017-12-11T10:20:48Z

core/src/test/scala/unit/kafka/utils/JsonTest.scala

-    assertEquals("\"str1\\\\,str2\"", Json.encode("str1\\,str2"))
-    assertEquals("\"\\\"quoted\\\"\"", Json.encode("\"quoted\""))
+    assertEquals(""""str1\\,str2"""", Json.encode("""str1\,str2"""))
+    assertEquals(""""\"quoted\""""", Json.encode(""""quoted""""))


Ouch, maybe my suggestion was bad in this line.

A bit unclear to be honest. Both options seem hard to read. :)

Shall I leave it as is or do you want me to change it back - I don't mind either way.

ijuma · 2017-12-12T08:54:13Z

retest this please

ijuma

Thanks for the PR, LGTM. Can you please update the PR description to remove the automatically generated text?

rajinisivaram · 2017-12-12T10:11:16Z

@ijuma Thanks for the review. Fixed the description.

ijuma · 2017-12-12T10:26:37Z

Merged to trunk and 1.0 branches.

This is required for ACLs where SSL principals contain special characters (e.g. comma) that are escaped using backslash. The strings need to be quoted for JSON to ensure that the JSON stored in ZK is valid. Also converted `SslEndToEndAuthorizationTest` to use a principal with special characters to ensure that this path is tested. Author: Rajini Sivaram <rajinisivaram@googlemail.com> Reviewers: Ismael Juma <ismael@juma.me.uk> Closes #4303 from rajinisivaram/KAFKA-6319 (cherry picked from commit 651c6e4) Signed-off-by: Ismael Juma <ismael@juma.me.uk>

rajinisivaram force-pushed the KAFKA-6319 branch from 9518f46 to dd03670 Compare December 8, 2017 10:14

ijuma reviewed Dec 11, 2017

View reviewed changes

rajinisivaram added 2 commits December 11, 2017 09:58

KAFKA-6319: Quote strings in JSON to enable ACLs for principals with …

97811d8

…special chars

Address review comments

f3d08b2

rajinisivaram force-pushed the KAFKA-6319 branch from dd03670 to f3d08b2 Compare December 11, 2017 10:00

ijuma reviewed Dec 11, 2017

View reviewed changes

Address review comments

1a405cc

ijuma approved these changes Dec 12, 2017

View reviewed changes

asfgit closed this in 651c6e4 Dec 12, 2017

umesh9794 mentioned this pull request Dec 13, 2017

KAFKA-6342 : Move workaround for JSON parsing of non-escaped strings #4321

Closed

3 tasks

viktorsomogyi mentioned this pull request Apr 30, 2020

KAFKA-6342: Move workaround for JSON parsing of non-escaped strings #8591

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-6319: Quote strings stored in JSON configs #4303

KAFKA-6319: Quote strings stored in JSON configs #4303

rajinisivaram commented Dec 7, 2017 •

edited

Loading

rajinisivaram commented Dec 7, 2017

ijuma commented Dec 7, 2017

rajinisivaram commented Dec 8, 2017

ijuma left a comment

ijuma Dec 11, 2017

ijuma Dec 11, 2017

rajinisivaram Dec 11, 2017

ijuma Dec 11, 2017

rajinisivaram Dec 11, 2017

ijuma Dec 11, 2017

ijuma Dec 11, 2017

rajinisivaram Dec 11, 2017

ijuma Dec 11, 2017

ijuma Dec 11, 2017

ijuma Dec 11, 2017

rajinisivaram commented Dec 11, 2017

ijuma Dec 11, 2017

ijuma Dec 11, 2017

ijuma Dec 11, 2017

rajinisivaram Dec 11, 2017

ijuma commented Dec 12, 2017

ijuma left a comment

rajinisivaram commented Dec 12, 2017

ijuma commented Dec 12, 2017

KAFKA-6319: Quote strings stored in JSON configs #4303

KAFKA-6319: Quote strings stored in JSON configs #4303

Conversation

rajinisivaram commented Dec 7, 2017 • edited Loading

Committer Checklist (excluded from commit message)

rajinisivaram commented Dec 7, 2017

ijuma commented Dec 7, 2017

rajinisivaram commented Dec 8, 2017

ijuma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajinisivaram commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma commented Dec 12, 2017

ijuma left a comment

Choose a reason for hiding this comment

rajinisivaram commented Dec 12, 2017

ijuma commented Dec 12, 2017

rajinisivaram commented Dec 7, 2017 •

edited

Loading