Remove RapidJSON from Envoy

[htuch]

Now that v1 has been officially deprecated and is on track for removal in the near future, we shouldn't need RapidJSON anymore. All JSON parsing and printing should be possible via the protobuf library. This will simplify dependencies and reduce the trusted compute base.

[htuch]

@mattklein123 WDYT?

[htuch]

(CC @jmarantz who reminded me we can do this now ;) )

[mattklein123]

Sure, though I'm not sure we want to start ripping anything out until we wait a bit and see if anyone complains about v1 being gone? (Note also that RapidJSON is used for v1 schema)

[lizan]

FWIW, JWT auth filter still use it.

[htuch]

@lizan I think any users can be converted fairly mechanically to using protobuf, since they can just switch to using Struct and then doing (de)serialization.

[htuch]

Also CC @aa-stripe in the context of #4693

[qiwzhang]

In that case, I need to switch this repo: https://github.com/google/jwt_verify_lib
to use protobuf first before you can give ride of RapidJson

[htuch]

@qiwzhang ack. I think if we can at least try and avoid any new users of RapidJSON we can make this easier. @qiwzhang do you think it's reasonable to rewrite to using Struct + Protobuf<->JSON?

If this turns out to be too much of a pain, or there are other advantages of RapidJSON (better formatting, better error messages), we can stick with status quo.

[qiwzhang]

I can try, it should not be that difficult.

[ramaraochavali]

admin interface that displays stats in json format still uses it

[htuch]

@ramaraochavali yeah, but that should be possible to convert. I think everything internal to Envoy should not be a huge problem, it's more sticky with external deps though.

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[qiwzhang]

#5181

This PR will remove rapidjson from jwt_verify_lib.

[derekargueta]

Looks like the Zipkin tracer has become a user of RapidJSON as well
https://github.com/envoyproxy/envoy/blob/master/source/extensions/tracers/zipkin/zipkin_core_types.cc#L29

[htuch]

@yanavlasov any chance you can take a look at this? It's in the bucket of "reducing our external dependencies".

[mattklein123]

FYI this will be much easier when all of the v1 code/schemas are deleted, which I think can finally happen once 1.12.0 is cut.

[yanavlasov]

I'll take this one. When is 1.12.0 being released?

…

On Mon, Sep 2, 2019 at 9:15 PM Matt Klein ***@***.***> wrote: FYI this will be much easier when all of the v1 code/schemas are deleted, which I think can finally happen once 1.12.0 is cut. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4705?email_source=notifications&email_token=ABQQXW6OOEFPEBQ43PG6JO3QHW3EDA5CNFSM4F3G2SVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5WXTUY#issuecomment-527268307>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQQXWZEH3AC5ZFJLJXQT33QHW3EDANCNFSM4F3G2SVA> .

[htuch]

@yanavlasov EOQ

[htuch]

An update on this. json_fuzz_test has discovered that RapidJSON can be easily caused to stack overflow as used today. Generally this isn't a concern, as we expect that control plane handling untrusted input should be sanitizing for this or be using gRPC/proto3. RapidJSON is used on the data path in places, specifically Dynamo and MongoDB extensions, the Envoy OSS security team has audited these and made the assessment that the only uses are in extensions where we expect the other parties to be effectively trusted, so we do not need to treat this as actionable. Nonetheless, we should move ahead with this to reduce any potential future security exploit potential here (e.g. someone parsing out a header value with RapidJSON in HCM).

It's not sufficient to switch RapidJSON to iterative parsing FWIW, since the Json Objects that we build can still overflow in some situations, e.g. destructors. We should definitely switch to protobuf, which has inbuilt (and configurable) recursion depth limits when parsing.

There should be no new uses of RapidJSON, or Json::Object, please keep this in mind during reviews @envoyproxy/maintainers @envoyproxy/security-team.

Once we cut 1.12.0 and v1 is gone, we can move forward with this.

[htuch]

Now that 1.12.0 has been cut, we can move ahead with this. @derekargueta are you planning to remove the remainder of the v1 JSON sites? That's probably the first step.

[derekargueta]

@htuch done :)

[asraa]

But there seems to be a problem with the performance of protobuf, especially when doing some write operations or converting JSON.

Did rapidjson perform better than the current protobuf json parser for access logging? I would love to get rid of rapidjson as well. I think maybe improving json formatted access logging could be done orthogonally, it seems like protocol buffer json parsing is a little nitty with what is fast (e.g. ListValue's are slow).

Another high performance alternatives is the gRPC JSON parser, but I don't see any benchmarks for it online.

[lizan]

Another option would be https://github.com/nlohmann/json ? I remember @htuch said it has been tested internally?

[asraa]

Actually that would be the most ideal from our perspective!!
It has 100% test coverage, fuzzed in OSS-Fuzz, uses sanitizers for test, and is fast

[mattklein123]

Given that we no longer use JSON schema, any of our JSON needs should be very simple, so I think any supported/fast library is fine!

[aa-stripe]

I skimmed the nlohmann::json docs and that looks like it would work great. iirc the main reason we switched to using protobuf was a) to align with the rest of json handling in envoy code and b) because as long as we could fit the data into a valid proto struct, we were guaranteed to have valid data, good serialization etc.

But if we get these guarantees through another library as well, I'm all for it.

On a side note, it's lame that the qps drops so much when using json logging :(

[kyessenov]

Yes, nlohmann::json is approved for use within Google for parsing JSON. We chose it to parse config in Wasm (protobuf is too big for that purpose, and kind of slow).

[mattklein123]

Sounds like a plan! I'm guessing it will be very straightforward to swap.

[htuch]

I'm in general agreement, but could someone walk through the formalities of answering the questions in https://github.com/envoyproxy/envoy/blob/master/DEPENDENCY_POLICY.md#new-external-dependencies? Since this is an new dependency, we'd like to make sure we walk through this checklist.

[htuch]

CC @moderation

[asraa]

Sure! I'll give it a shot.

• Releases: Yes, releases happen often, there were at least three this year. https://github.com/nlohmann/json/releases
• Security vulnerability disclosure process: sort of (https://github.com/nlohmann/json/security/policy). This is linked before you create an open issue on GitHub. Contact details: mail@nlohmann.me, single owner.
• Effective governance: single maintainer/owner, I think? There seem to be other contributors and other reviewers (Fix Issue#1813: user defined input adapters nlohmann/json#2145), but I can't tell off-hand. There are references to more maintainers, so not sure.
• Code reviews: Sometimes. Pros: All PRs check for 100% test coverage in CI, there is mostly single reviewer and merge, occasionally (larger PRs?) multiple reviewers.
• 2FA: Releases are signed with PGP keys. I don't think I see 2FA for contributors: https://github.com/nlohmann/json/blob/25aab7ee411a32de38e613228962d9ff2ea10596/.github/CONTRIBUTING.md
• Coverage: 100% unit test coverage, fuzzed in OSS-Fuzz with sanitizers.

Where this was lacking:

• Generally only one maintainer and one recipient of the security vulnerabilities. Pros: seem very friendly, can probably clarify and ask.
• No (?) contributor 2FA.

[htuch]

Thanks @asraa, that's really thorough. I think this scores relatively high on our criteria, @envoyproxy/dependency-shepherds WDYT?

[mattklein123]

SGTM!

[moderation]

SGTM too