Add client TLS auth to gRPC reporter #1591

tcolgate · 2019-06-07T13:57:20Z

No description provided.

tcolgate · 2019-06-07T13:59:49Z

The option names in the agent probably aren't optimal. Need docs too I guess.

yurishkuro · 2019-06-10T22:58:16Z

Is it possible to add tests? I think we already have tests for TLS & certificates in other parts of the code

tcolgate · 2019-06-11T09:56:18Z

yep, the existing tests should make this reasonably easy I think, I'll get started. I'll need to update the docs too.

codecov · 2019-06-17T09:37:24Z

Codecov Report

Merging #1591 into master will decrease coverage by 0.22%.
The diff coverage is 61.81%.

@@            Coverage Diff             @@
##           master    #1591      +/-   ##
==========================================
- Coverage   98.71%   98.49%   -0.23%     
==========================================
  Files         193      193              
  Lines        9243     9284      +41     
==========================================
+ Hits         9124     9144      +20     
- Misses         91      111      +20     
- Partials       28       29       +1

Impacted Files	Coverage Δ
cmd/collector/app/grpcserver/grpc_server.go	`52.5% <0%> (-47.5%)`	⬇️
cmd/agent/app/reporter/grpc/flags.go	`100% <100%> (ø)`	⬆️
cmd/collector/app/builder/builder_flags.go	`100% <100%> (ø)`	⬆️
cmd/agent/app/reporter/grpc/builder.go	`96.55% <90.9%> (-3.45%)`	⬇️

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 d3ee676...33fba12. Read the comment docs.

tcolgate · 2019-06-17T09:39:15Z

@yurishkuro added tests that check the various combinations of agent/client TLS misconfiguration

tcolgate · 2019-06-17T11:02:06Z

I'm not sure why the CI tests failed there, I don't think it's related to my PR.

cmd/agent/app/reporter/grpc/builder_test.go

cmd/collector/app/builder/builder_flags.go

tcolgate · 2019-06-25T07:10:50Z

@yurishkuro any more thoughts on this PR?

freeformz · 2019-06-25T21:58:33Z

Why not take an optional tls.Config when tls configuration is required?

tcolgate · 2019-06-26T06:57:18Z

Why not take an optional tls.Config when tls configuration is required?

@freeformz I'm not sure what you are suggesting, do you mean have a tls.Config in a user configurable config object? (I don't think that could work, most of that type is not configurable by literals, and none of the fields have the appropirate annotations).

yurishkuro · 2019-07-11T12:00:36Z

cmd/agent/app/reporter/grpc/flags.go

@@ -39,6 +41,8 @@ func AddFlags(flags *flag.FlagSet) {
 	flags.Bool(collectorTLS, false, "Enable TLS.")
 	flags.String(collectorTLSCA, "", "Path to a TLS CA file. (default use the systems truststore)")
 	flags.String(collectorTLSServerName, "", "Override the TLS server name.")
+	flags.String(agentCert, "", "Path to a TLS client certificate file.")
+	flags.String(agentKey, "", "Path to a TLS client key file.")


Could you please expand the flag descriptions (starting from --tls) to explain in more details what they enable? For example, what collectorTLS means after this PR, etc. Pretend someone is reading https://www.jaegertracing.io/docs/1.13/cli/#jaeger-collector-cassandra, is there enough information to understand what's going to happen with various combinations of the flags?

cmd/collector/app/builder/builder_flags.go

yurishkuro · 2019-07-11T12:09:20Z

cmd/collector/main.go

-			opts.CollectorGRPCKey,
-		)
+
+		tlsCfg := &tls.Config{


this code should not be in main, but somewhere in the server factory, maybe similar to grpcserver.StartGRPCCollector() function. Right now it appears that similar code is repeated in the unit tests. Can you move it to grpcserver and reuse in unit tests so that it's also tested? We can't really test main()

tcolgate · 2019-07-16T07:38:37Z

@yurishkuro rebased, squashed, updated args, and moved the TLS setup.
(Also, I removed the full-stop on the flags to make them consistant with other flag descriptions)

yurishkuro

Thanks for updating flag descriptions, they read much better now!

Just a few minor corrections, lgtm otherwise.

cmd/collector/app/builder/builder_flags.go

Signed-off-by: Tristan Colgate <[email protected]>

tcolgate · 2019-07-17T07:04:44Z

@yurishkuro pushed fixes

tcolgate requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners June 7, 2019 13:57

tcolgate mentioned this pull request Jun 7, 2019

Allow secure communication between components #458

Closed

7 tasks

tcolgate force-pushed the tlsclientauth branch from 8a2889d to 1cdefd7 Compare June 7, 2019 14:00

tcolgate force-pushed the tlsclientauth branch 3 times, most recently from 09ff05f to 6afc5d4 Compare June 17, 2019 10:16

yurishkuro reviewed Jun 18, 2019

View reviewed changes

cmd/agent/app/reporter/grpc/builder_test.go Show resolved Hide resolved

cmd/collector/app/builder/builder_flags.go Show resolved Hide resolved

tcolgate force-pushed the tlsclientauth branch from c4e4f94 to 0b863bd Compare June 18, 2019 13:34

yurishkuro reviewed Jul 11, 2019

View reviewed changes

tcolgate force-pushed the tlsclientauth branch from ffd30f7 to 0dc46b1 Compare July 16, 2019 07:38

tcolgate force-pushed the tlsclientauth branch 2 times, most recently from f73750e to cfc2a8e Compare July 16, 2019 08:43

yurishkuro reviewed Jul 16, 2019

View reviewed changes

cmd/collector/app/builder/builder_flags.go Outdated Show resolved Hide resolved

cmd/collector/app/builder/builder_flags.go Outdated Show resolved Hide resolved

cmd/collector/app/builder/builder_flags.go Outdated Show resolved Hide resolved

Add optional client TLS auth

4e49fcf

Signed-off-by: Tristan Colgate <[email protected]>

tcolgate force-pushed the tlsclientauth branch from cfc2a8e to 4e49fcf Compare July 17, 2019 07:04

yurishkuro approved these changes Jul 19, 2019

View reviewed changes

Merge branch 'master' into tlsclientauth

33fba12

yurishkuro merged commit 428cc1a into jaegertracing:master Jul 19, 2019

pavolloffay changed the title ~~Add optional client TLS auth~~ Add optional client TLS auth to gRPC reporter Sep 2, 2019

pavolloffay changed the title ~~Add optional client TLS auth to gRPC reporter~~ Add client TLS auth to gRPC reporter Sep 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add client TLS auth to gRPC reporter #1591

Add client TLS auth to gRPC reporter #1591

tcolgate commented Jun 7, 2019

tcolgate commented Jun 7, 2019 •

edited

Loading

yurishkuro commented Jun 10, 2019

tcolgate commented Jun 11, 2019

codecov bot commented Jun 17, 2019 •

edited

Loading

tcolgate commented Jun 17, 2019

tcolgate commented Jun 17, 2019

tcolgate commented Jun 25, 2019

freeformz commented Jun 25, 2019

tcolgate commented Jun 26, 2019

yurishkuro Jul 11, 2019

yurishkuro Jul 11, 2019

tcolgate commented Jul 16, 2019 •

edited

Loading

yurishkuro left a comment

tcolgate commented Jul 17, 2019 •

edited

Loading

Add client TLS auth to gRPC reporter #1591

Add client TLS auth to gRPC reporter #1591

Conversation

tcolgate commented Jun 7, 2019

tcolgate commented Jun 7, 2019 • edited Loading

yurishkuro commented Jun 10, 2019

tcolgate commented Jun 11, 2019

codecov bot commented Jun 17, 2019 • edited Loading

Codecov Report

tcolgate commented Jun 17, 2019

tcolgate commented Jun 17, 2019

tcolgate commented Jun 25, 2019

freeformz commented Jun 25, 2019

tcolgate commented Jun 26, 2019

yurishkuro Jul 11, 2019

Choose a reason for hiding this comment

yurishkuro Jul 11, 2019

Choose a reason for hiding this comment

tcolgate commented Jul 16, 2019 • edited Loading

yurishkuro left a comment

Choose a reason for hiding this comment

tcolgate commented Jul 17, 2019 • edited Loading

tcolgate commented Jun 7, 2019 •

edited

Loading

codecov bot commented Jun 17, 2019 •

edited

Loading

tcolgate commented Jul 16, 2019 •

edited

Loading

tcolgate commented Jul 17, 2019 •

edited

Loading