admin: Json nlohmann sanitizer #20637

jmarantz · 2022-04-02T03:27:28Z

Commit Message: Creates a JSON sanitizer that delegates to the nlohmann library when there's any escapes required, or characters with the 8th bit set, but otherwise just returns its input.

This PR supersedes #20428 which has a faster hand-rolled sanitizer, but the speed penalty for nlohmann is not too bad, compared with the protobuf sanitizer, and will generally be rarely needed anyway.

--------------------------------------------------------------------
Benchmark                          Time             CPU   Iterations
--------------------------------------------------------------------
BM_ProtoEncoderNoEscape         1445 ns         1444 ns       455727
BM_NlohmannNoEscape             9.79 ns         9.79 ns     71449511
BM_ProtoEncoderWithEscape       1521 ns         1521 ns       462697
BM_NlohmannWithEscape            215 ns          215 ns      3264218

Additional Description: This is intended to be adapted into the JSON renderer introduced in #19693
Risk Level: low -- this is layered on top of an already well-trusted library.
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…lasting with memset. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…ze one char at a time. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…n tests. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…hat protobufs reject. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz

thanks for the review!

source/common/common/utility.h

jmarantz · 2022-04-04T04:09:48Z

source/common/json/json_sanitizer.cc

+    }
+  }
+  if (buffer.size() >= 2 && buffer[0] == '"' && buffer[buffer.size() - 1] == '"') {
+    return absl::string_view(buffer).substr(1, buffer.size() - 2);


actually I was able to just remove that block.

jmarantz · 2022-04-04T04:10:22Z

source/common/json/json_sanitizer.cc

+  if (str.size() >= 2 && str[0] == '"' && str[str.size() - 1] == '"') {
+    str = str.substr(1, str.size() - 2);
+  } else {
+    IS_ENVOY_BUG(absl::StrCat("stripDoubleQuotes called on a str that lacks double-quotes: ", str));


i'm still not sure when to use ENVOY_BUG. I'll switch to ASSERT.

jmarantz · 2022-04-04T04:11:44Z

test/common/json/BUILD

+# genrule(
+#     name = "protobuf_incompatible_unicodes",
+#     testonly = 1,
+#     outs = ["protobuf_incompatible_unicodes.h"],


not really. I thought it would be nicer to generate the invalidations all the time so no one has to think about this again, but I could not get it to work.

jmarantz · 2022-04-04T12:21:34Z

test/common/json/gen_excluded_unicodes.cc

+namespace {
+
+// Collects unicode values that cannot be handled by the protobuf json encoder.
+// This is not needed for correct operation of the json sanitizer, but it is


done -- in comments and messages; did not change variable/function/namespace names.

test/common/json/gen_excluded_unicodes.cc

test/common/json/json_sanitizer_fuzz_test.cc

test/common/json/json_sanitizer_test_util.cc

source/common/json/json_sanitizer.cc

repokitteh-read-only · 2022-04-04T13:01:31Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @htuch

🐱

Caused by: #20637 was synchronize by jmarantz.

see: more, trace.

moderation · 2022-04-04T17:18:01Z

This is a comment only change to the dependencies

/lgtm deps

source/common/common/utility.h

source/common/json/json_internal.h

htuch · 2022-04-05T04:00:15Z

source/common/json/json_sanitizer.h

+absl::string_view sanitize(std::string& buffer, absl::string_view str);
+
+/**
+ * Strips double-quotes on first and last characters of str. It's an ENVOY_BUG


s/ENVOY_BUG/precondition ../

htuch · 2022-04-05T04:05:25Z

test/common/json/gen_excluded_unicodes.cc

+// "-c opt" is recommended because otherwise it takes almost a minute to run
+// through all 4-byte UTF-8 sequences.
+//
+// Running in this mode causes two tests to fail, but prints two initialization


Can we avoid the failure if possible? What I'm worried about is when the person upgrading protobuf library sees 3 tests fail or something and is uncertain on how to proceed. I think it's fine to have a rerun on each upgrade as needed, but it should be pretty bullet proof, right now it feels a bit wing-and-a-prayer.

sorry that "test to fail" note is obsolete and I removed it. I think it's reasonably OK given the minefield of correctness issues from the protobuf library.

I am not fully convinced it's worth fuzzing against protobuf serialization forever. That library should not be considered the golden truth. But for now it makes sense.

If eventually we decide we need the performance of the handrolled version I think it would make sense to fuzz against nlohmann.

htuch · 2022-04-05T04:07:42Z

test/common/json/gen_excluded_unicodes.cc

+};
+
+bool isInvalidProtobufSerialization(const std::string& str) {
+  return (str.size() == 2 && str[0] == '"' && str[1] == '"') || str.size() > 8;


Not sure I follow these constraints, comments?

added comments:

// To work around https://github.com/protocolbuffers/protobuf/issues/9729 // we must examine its output for each unicode. The protobuf library's // invalid outputs take two forms: // "" (empty qutoed string, which happens on 3-byte utf8) // \u1234\u5678 (two consecutive unicode escapes for a 4-byte utf8)

test/common/json/json_sanitizer_fuzz_test.cc

htuch · 2022-04-05T04:12:25Z

test/common/json/json_sanitizer_test_util.cc

+    auto [unicode, consumed] = Utf8::decode(utf8);
+    if (consumed != 0 && unicode == escaped_unicode) {
+      utf8 = utf8.substr(consumed, utf8.size() - consumed);
+      escaped = escaped.substr(6, escaped.size() - 6);


made constants.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2022-04-05T16:57:33Z

/retest

repokitteh-read-only · 2022-04-05T16:57:37Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20637 (comment) was created by @jmarantz.

see: more, trace.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2022-04-07T12:46:34Z

/retest

repokitteh-read-only · 2022-04-07T12:46:37Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20637 (comment) was created by @jmarantz.

see: more, trace.

htuch

LGTM, thanks! Very thorough fuzzing/testing, which I think provides the confidence to make this switch.

@jmarantz before merging, can I check that this only impacts admin and we don't need runtime protection?

jmarantz · 2022-04-08T13:34:06Z

@htuch by "Can I check" did you mean you or me :) ?

This PR does not call Json::sanitize() from anywhere except tests. However I'll write another small PR to switch the admin stats serializer to call it.

htuch

/lgtm deps

…eadouts (#20745) Commit Message: Uses the new Json::sanitizer from #20637 to generate counters, gauges, and text-readouts in response to admin endpoint `/stats?format=json`, resulting in a 5.6x speedup for AllCountersJson, and making JSON output only around 10% slower than text output. Additional Description: Before: ``` ------------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------------ BM_AllCountersText 454 ms 453 ms 2 BM_UsedCountersText 35.8 ms 35.8 ms 20 BM_FilteredCountersText 1806 ms 1805 ms 1 BM_AllCountersJson 2824 ms 2823 ms 1 BM_UsedCountersJson 61.8 ms 61.8 ms 11 BM_FilteredCountersJson 1798 ms 1798 ms 1 ``` after ``` ------------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------------ BM_AllCountersText 458 ms 458 ms 2 BM_UsedCountersText 36.1 ms 36.1 ms 19 BM_FilteredCountersText 1790 ms 1790 ms 1 BM_AllCountersJson 496 ms 496 ms 2 BM_UsedCountersJson 36.4 ms 36.4 ms 19 BM_FilteredCountersJson 1789 ms 1789 ms 1 ``` Risk Level: low -- Json::sanitizer is new, but based on nlohmann, and is differentially fuzzed vs protobuf serialization Testing: //test/server/admin/... Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com> ogle.com>

…eadouts (envoyproxy#20745) Commit Message: Uses the new Json::sanitizer from envoyproxy#20637 to generate counters, gauges, and text-readouts in response to admin endpoint `/stats?format=json`, resulting in a 5.6x speedup for AllCountersJson, and making JSON output only around 10% slower than text output. Additional Description: Before: ``` ------------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------------ BM_AllCountersText 454 ms 453 ms 2 BM_UsedCountersText 35.8 ms 35.8 ms 20 BM_FilteredCountersText 1806 ms 1805 ms 1 BM_AllCountersJson 2824 ms 2823 ms 1 BM_UsedCountersJson 61.8 ms 61.8 ms 11 BM_FilteredCountersJson 1798 ms 1798 ms 1 ``` after ``` ------------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------------ BM_AllCountersText 458 ms 458 ms 2 BM_UsedCountersText 36.1 ms 36.1 ms 19 BM_FilteredCountersText 1790 ms 1790 ms 1 BM_AllCountersJson 496 ms 496 ms 2 BM_UsedCountersJson 36.4 ms 36.4 ms 19 BM_FilteredCountersJson 1789 ms 1789 ms 1 ``` Risk Level: low -- Json::sanitizer is new, but based on nlohmann, and is differentially fuzzed vs protobuf serialization Testing: //test/server/admin/... Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com> ogle.com> Signed-off-by: Andre Vehreschild <vehre@x41-dsec.de>

Creates a JSON sanitizer that delegates to the nlohmann library when there's any escapes required, or characters with the 8th bit set, but otherwise just returns its input. This PR supersedes envoyproxy#20428 which has a faster hand-rolled sanitizer, but the speed penalty for nlohmann is not too bad, compared with the protobuf sanitizer, and will generally be rarely needed anyway. -------------------------------------------------------------------- Benchmark Time CPU Iterations -------------------------------------------------------------------- BM_ProtoEncoderNoEscape 1445 ns 1444 ns 455727 BM_NlohmannNoEscape 9.79 ns 9.79 ns 71449511 BM_ProtoEncoderWithEscape 1521 ns 1521 ns 462697 BM_NlohmannWithEscape 215 ns 215 ns 3264218 Additional Description: This is intended to be adapted into the JSON renderer introduced in envoyproxy#19693 Risk Level: low -- this is layered on top of an already well-trusted library. Testing: //test/... Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com>

…eadouts (envoyproxy#20745) Commit Message: Uses the new Json::sanitizer from envoyproxy#20637 to generate counters, gauges, and text-readouts in response to admin endpoint `/stats?format=json`, resulting in a 5.6x speedup for AllCountersJson, and making JSON output only around 10% slower than text output. Additional Description: Before: ``` ------------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------------ BM_AllCountersText 454 ms 453 ms 2 BM_UsedCountersText 35.8 ms 35.8 ms 20 BM_FilteredCountersText 1806 ms 1805 ms 1 BM_AllCountersJson 2824 ms 2823 ms 1 BM_UsedCountersJson 61.8 ms 61.8 ms 11 BM_FilteredCountersJson 1798 ms 1798 ms 1 ``` after ``` ------------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------------ BM_AllCountersText 458 ms 458 ms 2 BM_UsedCountersText 36.1 ms 36.1 ms 19 BM_FilteredCountersText 1790 ms 1790 ms 1 BM_AllCountersJson 496 ms 496 ms 2 BM_UsedCountersJson 36.4 ms 36.4 ms 19 BM_FilteredCountersJson 1789 ms 1789 ms 1 ``` Risk Level: low -- Json::sanitizer is new, but based on nlohmann, and is differentially fuzzed vs protobuf serialization Testing: //test/server/admin/... Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com> ogle.com>

jmarantz added 30 commits March 20, 2022 00:01

add hand-rolled json sanitizer

5639cf6

Signed-off-by: Joshua Marantz <jmarantz@google.com>

fix corner case

1233b00

Signed-off-by: Joshua Marantz <jmarantz@google.com>

inline the char_esacpes_ values rather than unique_ptr<std::string>

0509422

Signed-off-by: Joshua Marantz <jmarantz@google.com>

improve internal names and add comments

a8d4daa

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Fix comment typo and explicitly initialize Escape.size_ rather than b…

99a381f

…lasting with memset. Signed-off-by: Joshua Marantz <jmarantz@google.com>

cleanup comments

02216fa

Signed-off-by: Joshua Marantz <jmarantz@google.com>

provide link to the utf-8 spec which shows us we can correctly saniti…

319e09c

…ze one char at a time. Signed-off-by: Joshua Marantz <jmarantz@google.com>

add fuzz test and corpus of cases pulled from the unit test.

696cc04

Signed-off-by: Joshua Marantz <jmarantz@google.com>

checkpoint

6251863

Signed-off-by: Joshua Marantz <jmarantz@google.com>

multi-byte utf8 decoder

b11e6de

Signed-off-by: Joshua Marantz <jmarantz@google.com>

utf8 decoding works

ed5616c

Signed-off-by: Joshua Marantz <jmarantz@google.com>

cleanup

b651e77

Signed-off-by: Joshua Marantz <jmarantz@google.com>

cleanup & partial format

03a33b1

Signed-off-by: Joshua Marantz <jmarantz@google.com>

typo

cc95823

Signed-off-by: Joshua Marantz <jmarantz@google.com>

minor speed tweak

f6d49a3

Signed-off-by: Joshua Marantz <jmarantz@google.com>

remove unused constants and update benchmark data

747fccc

Signed-off-by: Joshua Marantz <jmarantz@google.com>

add comment, fix tests.

412a1c4

Signed-off-by: Joshua Marantz <jmarantz@google.com>

tidy

6bc7e43

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Move isValidUtf8 out of prod code, as it's not needed in prod; only i…

974220e

…n tests. Signed-off-by: Joshua Marantz <jmarantz@google.com>

add an assert and some more comments.

2faf91c

Signed-off-by: Joshua Marantz <jmarantz@google.com>

re-sort constants so the bits line up better.

7db4d85

Signed-off-by: Joshua Marantz <jmarantz@google.com>

add some invalid-utf8 tests

318f05e

Signed-off-by: Joshua Marantz <jmarantz@google.com>

test & fix behavior for invalid input.

63906bd

Signed-off-by: Joshua Marantz <jmarantz@google.com>

speed hacks.

d3d1d03

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Add more invalid-utf8 tests.

6a3ef94

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Use StrFormat rather than snprintf even though it's likely slower.

11b9e47

Signed-off-by: Joshua Marantz <jmarantz@google.com>

added one more test surveying all 8-bit inputs.

918d9d1

Signed-off-by: Joshua Marantz <jmarantz@google.com>

refine the invalid-utf8 test to include special case unicode ranges t…

0cec2cd

…hat protobufs reject. Signed-off-by: Joshua Marantz <jmarantz@google.com>

cleanup and fix corner cases.

d1cec3b

Signed-off-by: Joshua Marantz <jmarantz@google.com>

add fuzzing utility.cc impl.

e2bceaa

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Addresss Harvey's comments

54cf298

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz commented Apr 4, 2022

View reviewed changes

repokitteh-read-only bot added deps Approval required for changes to Envoy's external dependencies and removed waiting labels Apr 4, 2022

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Apr 4, 2022

htuch suggested changes Apr 5, 2022

View reviewed changes

jmarantz added 2 commits April 5, 2022 09:18

review comments round 2.a

032d4c5

Signed-off-by: Joshua Marantz <jmarantz@google.com>

fix typo.

b172e06

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'main' into json-nlohmann-sanitizer

a78e5ec

Signed-off-by: Joshua Marantz <jmarantz@google.com>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Apr 6, 2022

htuch reviewed Apr 8, 2022

View reviewed changes

htuch approved these changes Apr 8, 2022

View reviewed changes

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Apr 8, 2022

htuch merged commit 1c66d57 into envoyproxy:main Apr 8, 2022

jmarantz deleted the json-nlohmann-sanitizer branch April 8, 2022 14:51

jmarantz mentioned this pull request Apr 9, 2022

switch to using json sanitizer for admin counters, gauges, and text-readouts #20745

Merged

admin: Json nlohmann sanitizer #20637

admin: Json nlohmann sanitizer #20637

Uh oh!

Conversation

jmarantz commented Apr 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

repokitteh-read-only bot commented Apr 4, 2022

Uh oh!

moderation commented Apr 4, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Apr 5, 2022

Uh oh!

repokitteh-read-only bot commented Apr 5, 2022

Uh oh!

jmarantz commented Apr 7, 2022

Uh oh!

repokitteh-read-only bot commented Apr 7, 2022

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Apr 8, 2022

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmarantz commented Apr 2, 2022 •

edited

Loading