zipkin-reporter-java icon indicating copy to clipboard operation
zipkin-reporter-java copied to clipboard

Upgrade libthrift to 0.14.1

Open zhfeng opened this issue 2 years ago • 1 comments

Feature

upgarde thrift to 0.14.x which contais CVE-2020-13949 libthrift: potential DoS when processing untrusted payloads

Rationale

it also help camel-zipkin which depends on io.zipkin.reporter2:zipkin-sender-libthrift

Example Scenario

Prior Art

  • it should be related to https://github.com/openzipkin/zipkin/pull/3368
  • camel upgarde thrift PR https://github.com/apache/camel/pull/5976

zhfeng avatar Aug 23 '21 14:08 zhfeng

I will raise a PR when https://github.com/openzipkin/zipkin/pull/3368 get merged and waiting for the next release of zipkin.

zhfeng avatar Aug 23 '21 14:08 zhfeng

zipkin server can upgrade its thrift independently as the important thing is that the serialization is compatible.

codefromthecrypt avatar Dec 11 '23 01:12 codefromthecrypt

so the versions don't need to be interlocked as long as newer versions of libthrift don't change the serialization format. However, versions after 0.13.0 break classpath compatibility, which is an issue.

If we change the version here above 0.13.0, it will break people who cannot change code that depends on libthrift.

So, the main way out we usually use is when there's a hard break, we make another reporter that is forwards compatible. We could just break and upgrade (on next minor) considering scribe is a deprecated transport anyway.

@shakuzen @jeqo @anuraaga any feelings on this one?

codefromthecrypt avatar Dec 11 '23 08:12 codefromthecrypt

We could just break and upgrade (on next minor) considering scribe is a deprecated transport anyway.

Would bumping major be an option?

anuraaga avatar Dec 11 '23 08:12 anuraaga

Would bumping major be an option?

yep sure

codefromthecrypt avatar Dec 11 '23 09:12 codefromthecrypt

@zhfeng @anuraaga another option is to remove the thrift/scribe transport from camel. Zipkin has deprecated this for ages and the only main consumer is finagle, which doesn't use this code in Twitter's repo, nor the finagle integration https://github.com/openzipkin/zipkin-finagle/blob/master/scribe/src/main/java/zipkin2/finagle/scribe/ScribeSender.java

codefromthecrypt avatar Dec 11 '23 09:12 codefromthecrypt

so add 👍 to just remove this transport on the next major release (stop publishing it)

codefromthecrypt avatar Dec 11 '23 09:12 codefromthecrypt

It seems reasonable to not get too deep in tricky compatibility for this old transport. Either remove or bump on a major preferably or otherwise even minor all seem fine to me.

anuraaga avatar Dec 11 '23 09:12 anuraaga

Yeah ,camel-zipkin has been removed in Camel 4.0

zhfeng avatar Dec 11 '23 09:12 zhfeng

after sleeping on it, I think in the special case of libthrift which I think the community is used to revlock forcing.. bump on next minor and cite in RATIONALE.md is how I'll handle it.

@zhfeng sorry this led to axing of camel-zipkin, but understand as this issue was not looked at in over two years. One goal I have is to find sites who are active and can help with maintenance to avoid this happening again. In any case, I apologize on behalf of the maintainers group for the time you lost.

codefromthecrypt avatar Dec 11 '23 22:12 codefromthecrypt