Update vendored thrift to 0.14.1 #584

nhatthm · 2021-05-19T11:41:55Z

Which problem is this PR solving?

Resolves #546

Short description of the changes

Update the vendor Thrift to 0.13.0 to fix the security issue.

There is a change in the Agent interface

type Agent interface {
	// Parameters:
	//  - Spans
	EmitZipkinBatch(spans []*zipkincore.Span) (err error)
	// Parameters:
	//  - Batch
	EmitBatch(batch *jaeger.Batch) (err error)
}

to

type Agent interface {
	// Parameters:
	//  - Spans
	EmitZipkinBatch(ctx context.Context, spans []*zipkincore.Span) (err error)
	// Parameters:
	//  - Batch
	EmitBatch(ctx context.Context, batch *jaeger.Batch) (err error)
}

I'm fine if we decide to put this into v3 (because of the breaking changes), then I will just wait for #583 to be merged into master and start from there. We can discuss this.

References

https://www.whitesourcesoftware.com/vulnerability-database/CVE-2019-0205

thrift/header_protocol.go

codecov · 2021-05-19T17:26:16Z

Codecov Report

Merging #584 (0e8bd74) into master (2cf72ee) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #584      +/-   ##
==========================================
+ Coverage   88.55%   88.64%   +0.09%     
==========================================
  Files          61       61              
  Lines        3328     3328              
==========================================
+ Hits         2947     2950       +3     
+ Misses        253      251       -2     
+ Partials      128      127       -1

Impacted Files	Coverage Δ
transport/http.go	`79.62% <ø> (ø)`
transport/zipkin/http.go	`70.90% <ø> (+0.53%)`	⬆️
transport_udp.go	`97.26% <ø> (ø)`
utils/udp_client.go	`65.21% <ø> (+1.38%)`	⬆️
internal/baggage/remote/restriction_manager.go	`95.00% <100.00%> (ø)`
testutils/mock_agent.go	`80.88% <100.00%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cf72ee...0e8bd74. Read the comment docs.

yurishkuro · 2021-05-19T17:32:57Z

please make sure all commits are signed (see the DCO check)

Signed-off-by: nhatthm <nt@hellofresh.com>

nhatthm · 2021-05-19T17:45:14Z

please make sure all commits are signed (see the DCO check)

It's fixed

yurishkuro · 2021-05-19T17:51:36Z

There is a change in the Agent interface

That change is only in thrift-gen, correct? Technically we should've placed it in internal/thrift-gen, since those types are not meant to be exposed.

Is there a reason you picked 0.13 thrift? There is already https://github.com/apache/thrift/releases/tag/v0.14.1, which also fixed another CVE that we ran into in Jaeger backend.

nhatthm · 2021-05-19T18:07:51Z

That change is only in thrift-gen, correct? Technically we should've placed it in internal/thrift-gen, since those types are not meant to be exposed.

Yes, exactly. The change was made by running make thrift

Is there a reason you picked 0.13 thrift? There is already apache/thrift@v0.14.1 (release), which also fixed another CVE that we ran into in Jaeger backend.

Well, I was afraid of making big changes, 0.13.0 is the recommended version so I took it. Do you want to bump to 0.14.1 in this PR too?

yurishkuro · 2021-05-19T19:38:11Z

I would certainly prefer to go to the latest, otherwise we'd still have an open CVE, and Thrift is known to make breaking changes (like adding Context argument incrementally across releases instead of all at once), so best to minimize those.

Signed-off-by: nhatthm <nt@hellofresh.com>

nhatthm · 2021-05-20T06:58:07Z

I would certainly prefer to go to the latest, otherwise we'd still have an open CVE, and Thrift is known to make breaking changes (like adding Context argument incrementally across releases instead of all at once), so best to minimize those.

I bumped to 0.14. There are new files from Thrift, see 1065ec2

It's because of

# github.com/uber/jaeger-client-go/thrift-gen/zipkincore
thrift-gen/zipkincore/zipkincore.go:1562:6: undefined: thrift.ServerConnectivityCheckInterval
thrift-gen/zipkincore/zipkincore.go:1570:32: undefined: thrift.ServerConnectivityCheckInterval
thrift-gen/zipkincore/zipkincore.go:1590:16: undefined: thrift.ErrAbandonRequest
# github.com/uber/jaeger-client-go/thrift-gen/sampling
thrift-gen/sampling/sampling.go:1070:6: undefined: thrift.ServerConnectivityCheckInterval
thrift-gen/sampling/sampling.go:1078:32: undefined: thrift.ServerConnectivityCheckInterval
thrift-gen/sampling/sampling.go:1098:16: undefined: thrift.ErrAbandonRequest
# github.com/uber/jaeger-client-go/thrift-gen/baggage
thrift-gen/baggage/baggage.go:294:6: undefined: thrift.ServerConnectivityCheckInterval
thrift-gen/baggage/baggage.go:302:32: undefined: thrift.ServerConnectivityCheckInterval
thrift-gen/baggage/baggage.go:322:16: undefined: thrift.ErrAbandonRequest
# github.com/uber/jaeger-client-go/thrift-gen/jaeger
thrift-gen/jaeger/jaeger.go:2407:6: undefined: thrift.ServerConnectivityCheckInterval
thrift-gen/jaeger/jaeger.go:2415:32: undefined: thrift.ServerConnectivityCheckInterval
thrift-gen/jaeger/jaeger.go:2435:16: undefined: thrift.ErrAbandonRequest
# github.com/uber/jaeger-client-go/crossdock/thrift/tracetest
crossdock/thrift/tracetest/tracetest.go:1320:6: undefined: thrift.ServerConnectivityCheckInterval
crossdock/thrift/tracetest/tracetest.go:1328:32: undefined: thrift.ServerConnectivityCheckInterval
crossdock/thrift/tracetest/tracetest.go:1348:16: undefined: thrift.ErrAbandonRequest
crossdock/thrift/tracetest/tracetest.go:1399:6: undefined: thrift.ServerConnectivityCheckInterval
crossdock/thrift/tracetest/tracetest.go:1407:32: undefined: thrift.ServerConnectivityCheckInterval
crossdock/thrift/tracetest/tracetest.go:1427:16: undefined: thrift.ErrAbandonRequest

Look like they split the files

Anyway, I ran make test, everything is fine

nhatthm · 2021-05-20T07:01:54Z

thrift/configuration.go

+//     protoFactory := thrift.NewTBinaryProtocolFactoryConf(conf)
+//
+// [1]: https://github.com/apache/thrift/blob/master/doc/specs/thrift-tconfiguration.md
+type TConfiguration struct {


required by

thrift/binary_protocol.go:35:17: undefined: TConfiguration thrift/binary_protocol.go:40:7: undefined: TConfiguration thrift/binary_protocol.go:60:49: undefined: TConfiguration thrift/binary_protocol.go:91:42: undefined: TConfiguration thrift/binary_protocol.go:101:58: undefined: TConfiguration thrift/binary_protocol.go:532:51: undefined: TConfiguration thrift/compact_protocol.go:79:7: undefined: TConfiguration thrift/compact_protocol.go:89:43: undefined: TConfiguration thrift/compact_protocol.go:99:59: undefined: TConfiguration thrift/compact_protocol.go:107:7: undefined: TConfiguration

I believe that @rubenvp8510 is working on cherry-picking some commits from Thrift into our fork so that we have the security fixes in a backward-compatible manner.

yurishkuro · 2021-05-20T15:44:10Z

Did you use 0.14.0 or 0.14.1? There was another CVE fixed in 0.14.1

nhatthm · 2021-05-20T16:12:31Z

Did you use 0.14.0 or 0.14.1? There was another CVE fixed in 0.14.1

Sorry, i corrected the title, it is 0.14.1

yurishkuro · 2021-05-20T16:33:45Z

gh actions are acting up again (outage in progress).

Please update thrift/README to reflect this change.

Signed-off-by: nhatthm <nt@hellofresh.com>

nhatthm · 2021-05-20T20:09:33Z

gh actions are acting up again (outage in progress).

Please update thrift/README to reflect this change.

It's done

Updated: Everything is green now

yurishkuro · 2021-05-20T21:45:28Z

Thanks!

Link to #584 Signed-off-by: Yuri Shkuro <github@ysh.us>

nhatthm · 2021-05-20T21:51:15Z

Thanks!

@yurishkuro thanks for accepting the PR. May I know when you tag a new release? And will it be v2 or v3?

yurishkuro · 2021-05-20T22:36:04Z

https://github.com/jaegertracing/jaeger-client-go/releases/tag/v2.29.0

nhatthm · 2021-05-21T06:48:47Z

v2.29.0 (release)

Thanks!

Sorry for asking again, are you in touch with the contributors of https://github.com/census-ecosystem/opencensus-go-exporter-jaeger? Because we need to update there as well

census-ecosystem/opencensus-go-exporter-jaeger#17

yurishkuro · 2021-05-21T06:52:33Z

opencensus is being sunset, not sure if it's maintained, but cc @bogdandrutu

nhatthm · 2021-05-21T07:07:18Z

opencensus is being sunset, not sure if it's maintained, but cc @bogdandrutu

Yeah I know, I'm migrating to otel as well. The library had many breaking changes before v0.19, it was very very painful to update so I think better to wait for a stable release.

nhatthm requested a review from a team as a code owner May 19, 2021 11:41

nhatthm requested review from yurishkuro and removed request for a team May 19, 2021 11:41

yurishkuro reviewed May 19, 2021

View reviewed changes

thrift/header_protocol.go Show resolved Hide resolved

nhatthm added 4 commits May 19, 2021 19:42

Update vendored thrift to 0.12.0

e97709a

Signed-off-by: nhatthm <nt@hellofresh.com>

Regenerate files

1b4c3c3

Signed-off-by: nhatthm <nt@hellofresh.com>

Add context to codebase

eadf4c0

Signed-off-by: nhatthm <nt@hellofresh.com>

Bump to thrift 0.13.0

47ceba0

Signed-off-by: nhatthm <nt@hellofresh.com>

nhatthm added 4 commits May 20, 2021 08:43

Bump Thrift to 0.14.1

16249da

Signed-off-by: nhatthm <nt@hellofresh.com>

Add missing files in vendored Thrift 0.14

1065ec2

Signed-off-by: nhatthm <nt@hellofresh.com>

Generate files with Thrift 0.14.1

a7e6898

Signed-off-by: nhatthm <nt@hellofresh.com>

Add context to read and write ops

6d18b91

Signed-off-by: nhatthm <nt@hellofresh.com>

nhatthm changed the title ~~Update vendored thrift to 0.13.0~~ Update vendored thrift to 0.14.0 May 20, 2021

nhatthm commented May 20, 2021

View reviewed changes

nhatthm requested a review from yurishkuro May 20, 2021 07:03

nhatthm changed the title ~~Update vendored thrift to 0.14.0~~ Update vendored thrift to 0.14.1 May 20, 2021

Update README.md

0e8bd74

Signed-off-by: nhatthm <nt@hellofresh.com>

yurishkuro approved these changes May 20, 2021

View reviewed changes

yurishkuro merged commit 1e98249 into jaegertracing:master May 20, 2021

nhatthm deleted the up-thrift-0.13.0 branch May 20, 2021 21:46

yurishkuro added a commit that referenced this pull request May 20, 2021

Update thrift/README

b42b2ca

Link to #584 Signed-off-by: Yuri Shkuro <github@ysh.us>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update vendored thrift to 0.14.1 #584

Update vendored thrift to 0.14.1 #584

nhatthm commented May 19, 2021 •

edited

Loading

codecov bot commented May 19, 2021 •

edited

Loading

yurishkuro commented May 19, 2021

nhatthm commented May 19, 2021

yurishkuro commented May 19, 2021

nhatthm commented May 19, 2021 •

edited

Loading

yurishkuro commented May 19, 2021

nhatthm commented May 20, 2021 •

edited

Loading

nhatthm May 20, 2021

jpkrohling May 20, 2021

yurishkuro commented May 20, 2021

nhatthm commented May 20, 2021

yurishkuro commented May 20, 2021

nhatthm commented May 20, 2021 •

edited

Loading

yurishkuro commented May 20, 2021

nhatthm commented May 20, 2021

yurishkuro commented May 20, 2021

nhatthm commented May 21, 2021

yurishkuro commented May 21, 2021

nhatthm commented May 21, 2021

Update vendored thrift to 0.14.1 #584

Update vendored thrift to 0.14.1 #584

Conversation

nhatthm commented May 19, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

References

codecov bot commented May 19, 2021 • edited Loading

Codecov Report

yurishkuro commented May 19, 2021

nhatthm commented May 19, 2021

yurishkuro commented May 19, 2021

nhatthm commented May 19, 2021 • edited Loading

yurishkuro commented May 19, 2021

nhatthm commented May 20, 2021 • edited Loading

nhatthm May 20, 2021

Choose a reason for hiding this comment

jpkrohling May 20, 2021

Choose a reason for hiding this comment

yurishkuro commented May 20, 2021

nhatthm commented May 20, 2021

yurishkuro commented May 20, 2021

nhatthm commented May 20, 2021 • edited Loading

yurishkuro commented May 20, 2021

nhatthm commented May 20, 2021

yurishkuro commented May 20, 2021

nhatthm commented May 21, 2021

yurishkuro commented May 21, 2021

nhatthm commented May 21, 2021

nhatthm commented May 19, 2021 •

edited

Loading

codecov bot commented May 19, 2021 •

edited

Loading

nhatthm commented May 19, 2021 •

edited

Loading

nhatthm commented May 20, 2021 •

edited

Loading

nhatthm commented May 20, 2021 •

edited

Loading