TLS support for HTTP API of Query server

cmd/query/app/flags.go

@@ -51,12 +51,18 @@ const (
 	queryMaxClockSkewAdjust = "query.max-clock-skew-adjustment"
 )
 
-var tlsFlagsConfig = tlscfg.ServerFlagsConfig{
+var tlsGRPCFlagsConfig = tlscfg.ServerFlagsConfig{
 	Prefix:       "query.grpc",
 	ShowEnabled:  true,
 	ShowClientCA: true,
 }
 
+var tlsHTTPFlagsConfig = tlscfg.ServerFlagsConfig{
+	Prefix:       "query.http",
+	ShowEnabled:  true,
+	ShowClientCA: true,
+}
+
 // QueryOptions holds configuration for query service
 type QueryOptions struct {
 	// HostPort is the host:port address that the query service listens on
@@ -73,8 +79,10 @@ type QueryOptions struct {
 	UIConfig string
 	// BearerTokenPropagation activate/deactivate bearer token propagation to storage
 	BearerTokenPropagation bool
-	// TLS configures secure transport
-	TLS tlscfg.Options
+	// TLSGRPC configures secure transport (Consumer to Query service GRPC API)
+	TLSGRPC tlscfg.Options
+	// TLSHTTP configures secure transport (Consumer to Query service HTTP API)
+	TLSHTTP tlscfg.Options
 	// AdditionalHeaders
 	AdditionalHeaders http.Header
 	// MaxClockSkewAdjust is the maximum duration by which jaeger-query will adjust a span
@@ -93,6 +101,8 @@ func AddFlags(flagSet *flag.FlagSet) {
 	flagSet.String(queryUIConfig, "", "The path to the UI configuration file in JSON format")
 	flagSet.Bool(queryTokenPropagation, false, "Allow propagation of bearer token to be used by storage plugins")
 	flagSet.Duration(queryMaxClockSkewAdjust, 0, "The maximum delta by which span timestamps may be adjusted in the UI due to clock skew; set to 0s to disable clock skew adjustments")
+	tlsGRPCFlagsConfig.AddFlags(flagSet)
+	tlsHTTPFlagsConfig.AddFlags(flagSet)
 }
 
 // InitPortsConfigFromViper initializes the port numbers and TLS configuration of ports
@@ -101,10 +111,16 @@ func (qOpts *QueryOptions) InitPortsConfigFromViper(v *viper.Viper, logger *zap.
 	qOpts.GRPCHostPort = v.GetString(queryGRPCHostPort)
 	qOpts.HostPort = ports.GetAddressFromCLIOptions(v.GetInt(queryPort), v.GetString(queryHostPort))
 
-	qOpts.TLS = tlsFlagsConfig.InitFromViper(v)
+	qOpts.TLSGRPC = tlsGRPCFlagsConfig.InitFromViper(v)
+	qOpts.TLSHTTP = tlsHTTPFlagsConfig.InitFromViper(v)
 
-	// query.host-port is not defined and at least one of query.grpc-server.host-port or query.http-server.host-port is defined.
-	// User intends to use separate GRPC and HTTP host:port flags
+	// If either GRPC or HTTP servers use TLS, use dedicated ports.
+	if qOpts.TLSGRPC.Enabled || qOpts.TLSHTTP.Enabled {
+		return qOpts
+	}
+
+	// --query.host-port flag is not set and either or both of --query.grpc-server.host-port or --query.http-server.host-port is set by the user with command line flags.
+	// i.e. user intends to use separate GRPC and HTTP host:port flags
 	if !(v.IsSet(queryHostPort) || v.IsSet(queryPort)) && (v.IsSet(queryHTTPHostPort) || v.IsSet(queryGRPCHostPort)) {
 		return qOpts
 	}
@@ -124,8 +140,8 @@ func (qOpts *QueryOptions) InitFromViper(v *viper.Viper, logger *zap.Logger) *Qu
 	qOpts.StaticAssets = v.GetString(queryStaticFiles)
 	qOpts.UIConfig = v.GetString(queryUIConfig)
 	qOpts.BearerTokenPropagation = v.GetBool(queryTokenPropagation)
-	qOpts.MaxClockSkewAdjust = v.GetDuration(queryMaxClockSkewAdjust)
 
+	qOpts.MaxClockSkewAdjust = v.GetDuration(queryMaxClockSkewAdjust)
 	stringSlice := v.GetStringSlice(queryAdditionalHeaders)
 	headers, err := stringSliceAsHeader(stringSlice)
 	if err != nil {

cmd/query/app/flags_test.go

@@ -61,26 +61,6 @@ func TestQueryBuilderFlags(t *testing.T) {
 	assert.Equal(t, 10*time.Second, qOpts.MaxClockSkewAdjust)
 }
 
-func TestQueryBuilderFlagsSeparatePorts(t *testing.T) {
-	v, command := config.Viperize(AddFlags)
-	command.ParseFlags([]string{
-		"--query.http-server.host-port=127.0.0.1:8080",
-	})
-	qOpts := new(QueryOptions).InitFromViper(v, zap.NewNop())
-	assert.Equal(t, "127.0.0.1:8080", qOpts.HTTPHostPort)
-	assert.Equal(t, ports.PortToHostPort(ports.QueryGRPC), qOpts.GRPCHostPort)
-}
-
-func TestQueryBuilderFlagsSeparateNoPorts(t *testing.T) {
-	v, command := config.Viperize(AddFlags)
-	command.ParseFlags([]string{})
-	qOpts := new(QueryOptions).InitFromViper(v, zap.NewNop())
-
-	assert.Equal(t, ports.PortToHostPort(ports.QueryHTTP), qOpts.HTTPHostPort)
-	assert.Equal(t, ports.PortToHostPort(ports.QueryHTTP), qOpts.GRPCHostPort)
-	assert.Equal(t, ports.PortToHostPort(ports.QueryHTTP), qOpts.HostPort)
-}
-
 func TestQueryBuilderBadHeadersFlags(t *testing.T) {
 	v, command := config.Viperize(AddFlags)
 	command.ParseFlags([]string{
@@ -145,3 +125,116 @@ func TestBuildQueryServiceOptions(t *testing.T) {
 	assert.NotNil(t, qSvcOpts.ArchiveSpanReader)
 	assert.NotNil(t, qSvcOpts.ArchiveSpanWriter)
 }
+
+func TestQueryOptionsPortAllocationFromFlags(t *testing.T) {
+	var flagPortCases = []struct {
+		name                 string
+		flagsArray           []string
+		expectedHTTPHostPort string
+		expectedGRPCHostPort string
+		verifyCommonPort     bool
+		expectedHostPort     string
+	}{
+		{
+			// Since TLS is enabled in atleast one server, the dedicated host-ports obtained from viper are used, even if common host-port is specified
+			name: "Atleast one dedicated host-port and common host-port is specified, atleast one of GRPC, HTTP TLS enabled",
+			flagsArray: []string{
+				"--query.grpc.tls.enabled=true",
+				"--query.http-server.host-port=127.0.0.1:8081",
+				"--query.host-port=127.0.0.1:8080",
+			},
+			expectedHTTPHostPort: "127.0.0.1:8081",
+			expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // fallback in viper
+			verifyCommonPort:     false,
+		},
+		{
+			// TLS is disabled in both servers, since common host-port is specified, common host-port is used
+			name: "Atleast one dedicated host-port is specified, common host-port is specified, both GRPC and HTTP TLS disabled",
+			flagsArray: []string{
+				"--query.http-server.host-port=127.0.0.1:8081",
+				"--query.host-port=127.0.0.1:8080",
+			},
+			expectedHTTPHostPort: "127.0.0.1:8080",
+			expectedGRPCHostPort: "127.0.0.1:8080",
+			verifyCommonPort:     true,
+			expectedHostPort:     "127.0.0.1:8080",
+		},
+		{
+			// Since TLS is enabled in atleast one server, the dedicated host-ports obtained from viper are used
+			name: "Atleast one dedicated host-port is specified, common host-port is not specified, atleast one of GRPC, HTTP TLS enabled",
+			flagsArray: []string{
+				"--query.grpc.tls.enabled=true",
+				"--query.http-server.host-port=127.0.0.1:8081",
+			},
+			expectedHTTPHostPort: "127.0.0.1:8081",
+			expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), //  fallback in viper
+			verifyCommonPort:     false,
+		},
+		{
+			// TLS is disabled in both servers, since common host-port is not specified but atleast one dedicated port is specified, the dedicated host-ports obtained from viper are used
+			name: "Atleast one dedicated port, common port defined, both GRPC and HTTP TLS disabled",
+			flagsArray: []string{
+				"--query.http-server.host-port=127.0.0.1:8081",
+			},
+			expectedHTTPHostPort: "127.0.0.1:8081",
+			expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // fallback in viper
+			verifyCommonPort:     false,
+		},
+		{
+			// Since TLS is enabled in atleast one server, the dedicated host-ports obtained from viper are used, even if common host-port is specified and the dedicated host-port are not specified
+			name: "No dedicated host-port is specified, common host-port is specified, atleast one of GRPC, HTTP TLS enabled",
+			flagsArray: []string{
+				"--query.grpc.tls.enabled=true",
+				"--query.host-port=127.0.0.1:8080",
+			},
+			expectedHTTPHostPort: ports.PortToHostPort(ports.QueryHTTP), // fallback in viper
+			expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // fallback in viper
+			verifyCommonPort:     false,
+		},
+		{
+			// TLS is disabled in both servers, since only common host-port is specified, common host-port is used
+			name: "No dedicated host-port is specified, common host-port is specified, both GRPC and HTTP TLS disabled",
+			flagsArray: []string{
+				"--query.host-port=127.0.0.1:8080",
+			},
+			expectedHTTPHostPort: "127.0.0.1:8080",
+			expectedGRPCHostPort: "127.0.0.1:8080",
+			verifyCommonPort:     true,
+			expectedHostPort:     "127.0.0.1:8080",
+		},
+		{
+			// Since TLS is enabled in atleast one server, the dedicated host-ports obtained from viper are used
+			name: "No dedicated host-port is specified, common host-port is  not specified, atleast one of GRPC, HTTP TLS enabled",
+			flagsArray: []string{
+				"--query.grpc.tls.enabled=true",
+			},
+			expectedHTTPHostPort: ports.PortToHostPort(ports.QueryHTTP), // fallback in viper
+			expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // fallback in viper
+			verifyCommonPort:     false,
+		},
+		{
+			// TLS is disabled in both servers, since common host-port is not specified and neither dedicated ports are specified, common host-port from viper is used
+			name:                 "No dedicated host-port is specified, common host-port is not specified, both GRPC and HTTP TLS disabled",
+			flagsArray:           []string{},
+			expectedHTTPHostPort: ports.PortToHostPort(ports.QueryHTTP),
+			expectedGRPCHostPort: ports.PortToHostPort(ports.QueryHTTP),
+			verifyCommonPort:     true,
+			expectedHostPort:     ports.PortToHostPort(ports.QueryHTTP), //  fallback in viper
+		},
+	}
+
+	for _, test := range flagPortCases {
+		t.Run(test.name, func(t *testing.T) {
+			v, command := config.Viperize(AddFlags)
+			command.ParseFlags(test.flagsArray)
+			qOpts := new(QueryOptions).InitFromViper(v, zap.NewNop())
+
+			assert.Equal(t, test.expectedHTTPHostPort, qOpts.HTTPHostPort)
+			assert.Equal(t, test.expectedGRPCHostPort, qOpts.GRPCHostPort)
+			if test.verifyCommonPort {
+				assert.Equal(t, test.expectedHostPort, qOpts.HostPort)
+			}
+
+		})
+	}
+}

cmd/query/app/server.go

@@ -15,6 +15,7 @@
 package app
 
 import (
+	"errors"
 	"net"
 	"net/http"
 	"strings"
@@ -62,18 +63,27 @@ func NewServer(logger *zap.Logger, querySvc *querysvc.QueryService, options *Que
 		return nil, err
 	}
 
+	if (options.TLSHTTP.Enabled || options.TLSGRPC.Enabled) && (grpcPort == httpPort) {
+		return nil, errors.New("server with TLS enabled can not use same host ports for gRPC and HTTP.  Use dedicated HTTP and gRPC host ports instead")
+	}
+
 	grpcServer, err := createGRPCServer(querySvc, options, logger, tracer)
 	if err != nil {
 		return nil, err
 	}
 
+	httpServer, err := createHTTPServer(querySvc, options, tracer, logger)
+	if err != nil {
+		return nil, err
+	}
+
 	return &Server{
 		logger:             logger,
 		querySvc:           querySvc,
 		queryOptions:       options,
 		tracer:             tracer,
 		grpcServer:         grpcServer,
-		httpServer:         createHTTPServer(querySvc, options, tracer, logger),
+		httpServer:         httpServer,
 		separatePorts:      grpcPort != httpPort,
 		unavailableChannel: make(chan healthcheck.Status),
 	}, nil
@@ -87,11 +97,12 @@ func (s Server) HealthCheckStatus() chan healthcheck.Status {
 func createGRPCServer(querySvc *querysvc.QueryService, options *QueryOptions, logger *zap.Logger, tracer opentracing.Tracer) (*grpc.Server, error) {
 	var grpcOpts []grpc.ServerOption
 
-	if options.TLS.Enabled {
-		tlsCfg, err := options.TLS.Config(logger)
+	if options.TLSGRPC.Enabled {
+		tlsCfg, err := options.TLSGRPC.Config(logger)
 		if err != nil {
 			return nil, err
 		}
+
 		creds := credentials.NewTLS(tlsCfg)
 
 		grpcOpts = append(grpcOpts, grpc.Creds(creds))
@@ -104,11 +115,12 @@ func createGRPCServer(querySvc *querysvc.QueryService, options *QueryOptions, lo
 	return server, nil
 }
 
-func createHTTPServer(querySvc *querysvc.QueryService, queryOpts *QueryOptions, tracer opentracing.Tracer, logger *zap.Logger) *http.Server {
+func createHTTPServer(querySvc *querysvc.QueryService, queryOpts *QueryOptions, tracer opentracing.Tracer, logger *zap.Logger) (*http.Server, error) {
 	apiHandlerOptions := []HandlerOption{
 		HandlerOptions.Logger(logger),
 		HandlerOptions.Tracer(tracer),
 	}
+
 	apiHandler := NewAPIHandler(
 		querySvc,
 		apiHandlerOptions...)
@@ -126,9 +138,20 @@ func createHTTPServer(querySvc *querysvc.QueryService, queryOpts *QueryOptions,
 	}
 	handler = handlers.CompressHandler(handler)
 	recoveryHandler := recoveryhandler.NewRecoveryHandler(logger, true)
-	return &http.Server{
+
+	server := &http.Server{
 		Handler: recoveryHandler(handler),
 	}
+
+	if queryOpts.TLSHTTP.Enabled {
+		tlsCfg, err := queryOpts.TLSHTTP.Config(logger) // This checks if the certificates are correctly provided
+		if err != nil {
+			return nil, err
+		}
+		server.TLSConfig = tlsCfg
+
+	}
+	return server, nil
 }
 
 // initListener initialises listeners of the server
@@ -153,6 +176,7 @@ func (s *Server) initListener() (cmux.CMux, error) {
 	if err != nil {
 		return nil, err
 	}
+
 	s.conn = conn
 
 	var tcpPort int
@@ -206,13 +230,19 @@ func (s *Server) Start() error {
 
 	go func() {
 		s.logger.Info("Starting HTTP server", zap.Int("port", httpPort), zap.String("addr", s.queryOptions.HTTPHostPort))
-
-		switch err := s.httpServer.Serve(s.httpConn); err {
+		var err error
+		if s.queryOptions.TLSHTTP.Enabled {
+			err = s.httpServer.ServeTLS(s.httpConn, "", "")
+		} else {
+			err = s.httpServer.Serve(s.httpConn)
+		}
+		switch err {
 		case nil, http.ErrServerClosed, cmux.ErrListenerClosed:
 			// normal exit, nothing to do
 		default:
 			s.logger.Error("Could not start HTTP server", zap.Error(err))
 		}
+
 		s.unavailableChannel <- healthcheck.Unavailable
 	}()
 
@@ -245,7 +275,8 @@ func (s *Server) Start() error {
 
 // Close stops http, GRPC servers and closes the port listener.
 func (s *Server) Close() error {
-	s.queryOptions.TLS.Close()
+	s.queryOptions.TLSGRPC.Close()
+	s.queryOptions.TLSHTTP.Close()
 	s.grpcServer.Stop()
 	s.httpServer.Close()
 	if s.separatePorts {