Verify whether mTLS works

[jpkrohling]

@tonglil commented on #2507 that mutual TLS might not be working at the moment. This task is to give it a try and document the outcome.

[tonglil]

Thanks for creating this tracking issue.

Seems like an issue with the mux matcher for gRPC.
When I comment out the HTTP matcher and change grpc to match on any, my code finally works:

opentelemetry-collector/receiver/opencensusreceiver/opencensus.go

Lines 251 to 267 in 8050a88

	// Start the gRPC and HTTP/JSON (grpc-gateway) servers on the same port.
	m := cmux.New(ocr.ln)
	grpcL := m.MatchWithWriters(
	cmux.HTTP2MatchHeaderFieldSendSettings("content-type", "application/grpc"),
	cmux.HTTP2MatchHeaderFieldSendSettings("content-type", "application/grpc+proto"))
	httpL := m.Match(cmux.Any())
	go func() {
	if errGrpc := ocr.serverGRPC.Serve(grpcL); errGrpc != nil {
	host.ReportFatalError(errGrpc)
	}
	}()
	go func() {
	if errHTTP := ocr.httpServer().Serve(httpL); errHTTP != nil {
	host.ReportFatalError(errHTTP)
	}
	}()

With:

 		// Start the gRPC and HTTP/JSON (grpc-gateway) servers on the same port.
 		m := cmux.New(ocr.ln)
-		grpcL := m.MatchWithWriters(
-			cmux.HTTP2MatchHeaderFieldSendSettings("content-type", "application/grpc"),
-			cmux.HTTP2MatchHeaderFieldSendSettings("content-type", "application/grpc+proto"))
 
-		httpL := m.Match(cmux.Any())
+		grpcL := m.Match(cmux.Any())
 		go func() {
 			if errGrpc := ocr.serverGRPC.Serve(grpcL); errGrpc != nil {
 				host.ReportFatalError(errGrpc)
 			}
 		}()
-		go func() {
-			if errHTTP := ocr.httpServer().Serve(httpL); errHTTP != nil {
-				host.ReportFatalError(errHTTP)
-			}
-		}()

I tried specifying the ocagent.WithHeaders but it didn't seem to work.

This is my sample code:

func main() {
	absPathClientCrt, err := filepath.Abs("client.crt")
	if err != nil {
		log.Fatal(err)
	}
	absPathClientKey, err := filepath.Abs("client.key")
	if err != nil {
		log.Fatal(err)
	}

	cert, err := tls.LoadX509KeyPair(absPathClientCrt, absPathClientKey)
	if err != nil {
		log.Fatal("Unable to load cert", err)
	}

	roots := x509.NewCertPool()

	absPathCACrt, err := filepath.Abs("ca.crt")
	if err != nil {
		log.Fatal(err)
	}

	caRaw, err := ioutil.ReadFile(absPathCACrt)
	if err != nil {
		log.Fatal(err)
	}

	if !roots.AppendCertsFromPEM(caRaw) {
		log.Fatal("Couldn't append from PEM file")
	}

	tlsConf := &tls.Config{
		ServerName:   "example.test",
		Certificates: []tls.Certificate{cert},
		RootCAs:      roots,
	}

	// Initiate an exporter
	oce, err := ocagent.NewExporter(
		ocagent.WithAddress("example.test:55678"),
		ocagent.WithTLSCredentials(credentials.NewTLS(tlsConf)),
		ocagent.WithGRPCDialOption(
			// Useful for testing, blocks code execution until connection is made
			// if code doesn't print "zonk", it likely has not passed this point.
			// NOTE: Comment out to show error (usually, but not this time?)
			grpc.WithBlock(),
		),
		ocagent.WithServiceName("OpenCensusExampleService"),
		// NOTE: Tried this but it didn't do anything
		// ocagent.WithHeaders(map[string]string{
		// 	"content-type": "application/grpc",
		// }),
	)
	if err != nil {
		log.Fatalf("Failed to create a new ocagent exporter: %v", err)
	}

	log.Println("zonk")

	defer func() {
		log.Println("stopping...")
		err := oce.Stop()
		if err != nil {
			log.Fatalf("shutdown error: %v", err)
		}
		log.Println("stopped")
	}()

	// Register the exporter for tracing
	trace.RegisterExporter(oce)

	// Configure tracing
	trace.ApplyConfig(trace.Config{
		DefaultSampler: trace.AlwaysSample(),
	})

	// Initiate a background context
	ctx := context.Background()

	// Do some tracing
	ctx, span := trace.StartSpan(ctx, "main")
	span.End()
}

Possibly related?

• Review TLS support for OC receiver and add tests #170
• agent -> collector TLS does not seem to work census-instrumentation/opencensus-service#596
• receiver/opencensus: Support TLS in HTTP server census-instrumentation/opencensus-service#623

[jpkrohling]

You are absolutely right, this was indeed mentioned as part of #1256. Mutual TLS isn't possible at all when cmux is being used.

@tigrannajaryan, @bogdandrutu, I believe we need to revert the change that unified gRPC and HTTP ports before GA. It's easier to unify it again in the future than to split them after GA, in terms of backward compatibility.

[jpkrohling]

I'm closing this, as @tonglil has verified that this does NOT work at the moment. This is being tracked via #1256 already.

[tonglil]

Gotcha, this is an unfortunate finding.
Should I PR a note to https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configtls/README.md to mention this so no one else spends a day trying to figure it out?

Also, I'm all for the multiple-port solution, it's easier to understand.

FWIW this bit of code in startServer was quite confusing because initially

opentelemetry-collector/receiver/opencensusreceiver/opencensus.go

Line 233 in 8050a88

opts := []grpc.DialOption{grpc.WithInsecure()}

was a red herring.

[jpkrohling]

Documenting this would be wonderful, thanks!

[tonglil]

I will say, this worked: soheilhy/cmux#60

Switch from:

opentelemetry-collector/receiver/opencensusreceiver/opencensus.go

Lines 252 to 257 in 8050a88

	m := cmux.New(ocr.ln)
	grpcL := m.MatchWithWriters(
	cmux.HTTP2MatchHeaderFieldSendSettings("content-type", "application/grpc"),
	cmux.HTTP2MatchHeaderFieldSendSettings("content-type", "application/grpc+proto"))
	httpL := m.Match(cmux.Any())

To:

 		m := cmux.New(ocr.ln)
-		grpcL := m.MatchWithWriters(
-			cmux.HTTP2MatchHeaderFieldSendSettings("content-type", "application/grpc"),
-			cmux.HTTP2MatchHeaderFieldSendSettings("content-type", "application/grpc+proto"))
+		httpL := m.Match(cmux.HTTP1Fast())
+		grpcL := m.Match(cmux.Any())
 
-		httpL := m.Match(cmux.Any())
 		go func() {

Funny enough, grpcL := m.Match(cmux.HTTP2()) does not work.

Not sure if it is an acceptable solution (didn't try sending HTTP), but would help me get things up and running.

[tigrannajaryan]

You are absolutely right, this was indeed mentioned as part of #1256. Mutual TLS isn't possible at all when cmux is being used.

@tigrannajaryan, @bogdandrutu, I believe we need to revert the change that unified gRPC and HTTP ports before GA. It's easier to unify it again in the future than to split them after GA, in terms of backward compatibility.

I think reverting or not reverting can be decided when #1256 is done (or rejected because it cannot be done). It is already part of the GA milestone.

[tonglil]

So #1256 is closed, with the conclusion to split ports.
AFAICT that issue is for the OTLP receiver.

Can we rename this ticket to "unsplit the ports for the OpenCensus (OC) receiver"?

Thanks!

[jpkrohling]

I think this ticket is still relevant, as we need to make sure mTLS works. Could you please create another ticket to track the OC receiver part?

[jpkrohling]

I'm closing this, as I verified this as part of a recent issue.