Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TLS support for HTTP API of Query server #2337

Merged
merged 39 commits into from
Jan 19, 2021
Merged

TLS support for HTTP API of Query server #2337

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
92d363c
Added TLS for HTTP (consumer-query) server
rjs211 Jul 9, 2020
3bf0746
Add testcase of error in TLS HTTP server creation
rjs211 Jul 9, 2020
abe4ff5
Merge branch 'master' of github.com:jaegertracing/jaeger into dev-add…
rjs211 Jul 9, 2020
050fb62
Minor refactoring of properties and vars
rjs211 Jul 9, 2020
d02d85f
Exposing flags for HTTP and GRPC with TLS config
rjs211 Jul 9, 2020
e135064
minor refactoring of comments
rjs211 Jul 11, 2020
657e0b4
Changed TLS server to use tlsCfg instead of injection
rjs211 Jul 11, 2020
0f67d15
Create test for HTTP server with TLS and MTLS
rjs211 Jul 11, 2020
73867a5
Removing checks to avoid race condition
rjs211 Jul 11, 2020
3e000a7
Merge branch 'master' of github.com:jaegertracing/jaeger into dev-add…
rjs211 Jul 13, 2020
a763d9e
Adding testdata of certificates and keys of CA, server & client
rjs211 Jul 14, 2020
024b179
Merge branch 'master' of github.com:jaegertracing/jaeger into dev-add…
rjs211 Jul 14, 2020
0a88eb5
Changing the names of keys and certificates
rjs211 Jul 14, 2020
691ffea
Coverage increase and cleanup
rjs211 Jul 15, 2020
2278e6e
Merge branch 'master' of github.com:jaegertracing/jaeger into dev-add…
rjs211 Jul 15, 2020
5cdf944
removing redundant certif/keys set and using previously available set
rjs211 Jul 15, 2020
cca70e9
Added helper function to serve HTTP server
rjs211 Jul 15, 2020
fea7e14
Modify cmux and tests for secure HTTP and GRPC
rjs211 Jul 16, 2020
4cb348e
Merge branch 'master' of github.com:jaegertracing/jaeger into dev-add…
rjs211 Jul 17, 2020
31b31e7
Fixing testscases for safe re-use
rjs211 Jul 17, 2020
9c8cfdf
Merge branch 'master' of github.com:jaegertracing/jaeger into dev-add…
rjs211 Jul 17, 2020
affb566
Use common certificate flags for GRPC and HTTP
rjs211 Jul 17, 2020
5def2de
Use common certificate flags for GRPC and HTTP
rjs211 Jul 17, 2020
1d8133e
tempCommit
rjs211 Jul 18, 2020
3c0b23c
Using same tlsCfg structure for server
rjs211 Jul 18, 2020
7d9e18f
Removing reduntant code, added comments, using correct port for testing
rjs211 Jul 30, 2020
17bd199
Using separate ports in case of TLS
rjs211 Sep 11, 2020
e45614f
Merge branch 'master' of https://github.com/jaegertracing/jaeger into…
rjs211 Sep 11, 2020
0ceb4e5
modified test-cases for dedicated ports with TLS
rjs211 Sep 11, 2020
0119c1e
remove redundant test, created error var
rjs211 Sep 14, 2020
da5d790
Merge branch 'master' of https://github.com/jaegertracing/jaeger into…
rjs211 Sep 14, 2020
601c269
remove redundant test, created error var
rjs211 Sep 14, 2020
186184f
Split long conditional
rjs211 Oct 15, 2020
a964218
Merge branch 'master' into dev-addTLS-rjs211
rjs211 Oct 22, 2020
f4eb8f6
Merge branch 'master' of https://github.com/jaegertracing/jaeger into…
rjs211 Nov 10, 2020
f0ac83e
removed code repitition, added comment
rjs211 Nov 10, 2020
c53af21
added table-based tests for QueryOptions port allocation
rjs211 Nov 10, 2020
d3edb98
Merge branch 'dev-addTLS-rjs211' of https://github.com/rjs211/jaeger …
rjs211 Nov 10, 2020
1bee20f
Merge branch 'master' into dev-addTLS-rjs211
rjs211 Jan 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
tempCommit 
Signed-off-by: rjs211 <srivatsa211@gmail.com>
  • Loading branch information
@rjs211
rjs211 committed Jul 18, 2020
commit 1d8133e253bb368b3836c31a8dcd2a5544c45ec0
30 changes: 16 additions & 14 deletions cmd/query/app/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const (
queryMaxClockSkewAdjust = "query.max-clock-skew-adjustment"
queryHTTPTLSEnabled = "query.http.tls.enabled"
queryGRPCTLSEnabled = "query.gprc.tls.enabled"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need these? enabled is already a standard flag in the TLS config https://github.com/jaegertracing/jaeger/blob/master/pkg/config/tlscfg/flags.go#L26

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need two one for GRPC one for HTTP. but even if either one of them are enabled, we need only one set of certificates and keys. So, I used two hardCoded flags for each and used showEnabled:false for the common certificate configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using enabled of tlsCfg would also force two sets of certificate and key. #2338 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow. If you want to share certificates and keys between two servers, then shouldn't we have only one TLS tlscfg.Options field in the options struct, as we had before? Then it would make sense to have to extra .enabled flags to allow switching TLS on each server separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'll do that.

Does the project have any convention for multi-vlaued flags? Because clientCA of GRPC and HTTP server might be different. Currently we still have this problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpkrohling Agreed. So our options are:

  1. Either we have to use one certificate where the server uses only one port for both HTTP and gRPC endpoints.
  2. Use separate port for http and another port fro gRPC. then, each will have different listeners and we can use two set of certificates generated by two diffrent CAs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tcolgate given that Jaeger 2.x will be based on OpenTelemetry Collector, you might want to bring your concerns to this issue as well: open-telemetry/opentelemetry-collector#1256.

This PR here is about the query server, which has been released already with both protocols on the same port. Splitting it now would cause current clients to break, so, I'm afraid we don't have much choice at this point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpkrohling I did consider that. I added the mutual TLS support to jaeger and thanos, and then looked at doing it for otel, but cmux made it awkward, and debugging it did my brain in. (I disliked the cmux setup before that, but this was the first time I had to actually modify a project using it, that wasn't just ripping it out).
Hopefully someone with more time an patience will do the otel work before I have to look at it again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpkrohling I've added a couple of comments to that bug. I hadn't realised they'd reverted the cmux usage. I failed to get mutual TLS working with cmux, I can't recall the exact details of why, but it looks like the bug that caused them to revert might be the reason.
If the grpc service only supports functionality that is provided by the UI, then having the same CA/certs is probably reasonable, assuming none of the other jaeger components need grpc to talk to it (other than maybe a cli query, which would be roughly equivalent to browser usage, so could just use a system cert pack).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yurishkuro can confirm, but my understanding is that the query gRPC endpoint is (or is going to be) the supported way to query the backend. The first client is the UI, but external clients are also expected to exist, so, mTLS is certainly something we'd want to support there.

queryServerClientCA = "query.server.tls.client-ca"
)

var tlsFlagsConfig = tlscfg.ServerFlagsConfig{
Expand All @@ -68,14 +69,18 @@ type QueryOptions struct {
UIConfig string
// BearerTokenPropagation activate/deactivate bearer token propagation to storage
BearerTokenPropagation bool
// 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
// HTTPTLSEnabled Enable/Disable secure HTTP connection
HTTPTLSEnabled bool
// GRPCTLSEnabled Enable/Disable secure CRPC connection
rjs211 marked this conversation as resolved.
Show resolved Hide resolved
GRPCTLSEnabled bool
// TLS configures secure transport
TLS tlscfg.Options
// AdditionalHeaders
AdditionalHeaders http.Header
// MaxClockSkewAdjust is the maximum duration by which jaeger-query will adjust a span
MaxClockSkewAdjust time.Duration

liststring []string
}

// AddFlags adds flags for QueryOptions
Expand All @@ -93,13 +98,6 @@ func AddFlags(flagSet *flag.FlagSet) {
tlsFlagsConfig.AddFlags(flagSet)
}

func getOptions(enabled bool, qTLSOptions tlscfg.Options) tlscfg.Options {
if enabled {
return tlscfg.Options{Enabled: enabled, CertPath: qTLSOptions.CertPath, KeyPath: qTLSOptions.KeyPath, ClientCAPath: qTLSOptions.ClientCAPath}
}
return tlscfg.Options{Enabled: enabled}
}

// InitFromViper initializes QueryOptions with properties from viper
func (qOpts *QueryOptions) InitFromViper(v *viper.Viper, logger *zap.Logger) *QueryOptions {
qOpts.HostPort = ports.GetAddressFromCLIOptions(v.GetInt(queryPort), v.GetString(queryHostPort))
Expand All @@ -108,9 +106,13 @@ func (qOpts *QueryOptions) InitFromViper(v *viper.Viper, logger *zap.Logger) *Qu
qOpts.UIConfig = v.GetString(queryUIConfig)
qOpts.BearerTokenPropagation = v.GetBool(queryTokenPropagation)

qTLSOptions := tlsFlagsConfig.InitFromViper(v)
qOpts.TLSGRPC = getOptions(v.GetBool(queryGRPCTLSEnabled), qTLSOptions)
qOpts.TLSHTTP = getOptions(v.GetBool(queryHTTPTLSEnabled), qTLSOptions)
qOpts.TLS = tlsFlagsConfig.InitFromViper(v)
qOpts.TLS.Enabled = false
qOpts.GRPCTLSEnabled = v.GetBool(queryGRPCTLSEnabled)
qOpts.HTTPTLSEnabled = v.GetBool(queryHTTPTLSEnabled)
if qOpts.GRPCTLSEnabled || qOpts.HTTPTLSEnabled {
qOpts.TLS.Enabled = true
}

qOpts.MaxClockSkewAdjust = v.GetDuration(queryMaxClockSkewAdjust)
stringSlice := v.GetStringSlice(queryAdditionalHeaders)
Expand Down
23 changes: 10 additions & 13 deletions cmd/query/app/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ func (s Server) HealthCheckStatus() chan healthcheck.Status {

func createGRPCServer(querySvc *querysvc.QueryService, options *QueryOptions, logger *zap.Logger, tracer opentracing.Tracer) (*grpc.Server, error) {

if options.TLSGRPC.Enabled {
_, err := options.TLSGRPC.Config()
if options.GRPCTLSEnabled {
_, err := options.TLS.Config()
if err != nil {
return nil, err
}
Expand All @@ -102,8 +102,8 @@ func createHTTPServer(querySvc *querysvc.QueryService, queryOpts *QueryOptions,
HandlerOptions.Tracer(tracer),
}

if queryOpts.TLSHTTP.Enabled {
_, err := queryOpts.TLSHTTP.Config() // This checks if the certificates are correctly provided
if queryOpts.HTTPTLSEnabled {
_, err := queryOpts.TLS.Config() // This checks if the certificates are correctly provided
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -156,44 +156,41 @@ func (s *Server) getCmux() (cmux.CMux, net.Listener, net.Listener, error) {
var grpcListener net.Listener
var cmux1 cmux.CMux
rjs211 marked this conversation as resolved.
Show resolved Hide resolved
// var cmux2 cmux.CMux
var tlsOptions tlscfg.Options
conn, err := net.Listen("tcp", s.queryOptions.HostPort)
s.conn = conn

if err != nil {
return nil, nil, nil, err
}

if s.queryOptions.TLSHTTP.Enabled != s.queryOptions.TLSGRPC.Enabled {
if s.queryOptions.HTTPTLSEnabled != s.queryOptions.GRPCTLSEnabled {
cmux1 = cmux.New(conn)

if !s.queryOptions.TLSHTTP.Enabled {
if !s.queryOptions.HTTPTLSEnabled {
httpListener = cmux1.Match(cmux.HTTP1Fast())
rjs211 marked this conversation as resolved.
Show resolved Hide resolved
tlsOptions = s.queryOptions.TLSGRPC
} else {
grpcListener = cmux1.MatchWithWriters(
cmux.HTTP2MatchHeaderFieldSendSettings("content-type", "application/grpc"),
cmux.HTTP2MatchHeaderFieldSendSettings("content-type", "application/grpc+proto"),
)
tlsOptions = s.queryOptions.TLSHTTP
}
tlsListener := cmux1.Match(cmux.Any())
tlsListener, err = getTLSListener(tlsListener, tlsOptions, "")
tlsListener, err = getTLSListener(tlsListener, s.queryOptions.TLS, "")
if err != nil {
return nil, nil, nil, err
rjs211 marked this conversation as resolved.
Show resolved Hide resolved
}

// cmux2 = cmux.New(tlsListener)
if s.queryOptions.TLSHTTP.Enabled {
if s.queryOptions.HTTPTLSEnabled {
httpListener = tlsListener
} else {
grpcListener = tlsListener
}

} else {
var muxListener net.Listener
if s.queryOptions.TLSHTTP.Enabled {
muxListener, err = getTLSListener(conn, s.queryOptions.TLSHTTP, s.queryOptions.TLSGRPC.ClientCAPath)
if s.queryOptions.HTTPTLSEnabled {
muxListener, err = getTLSListener(conn, s.queryOptions.TLS, s.queryOptions.TLSGRPC.ClientCAPath)
if err != nil {
return nil, nil, nil, err
}
Expand Down
6 changes: 4 additions & 2 deletions pkg/config/tlscfg/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,10 @@ func (c ServerFlagsConfig) AddFlags(flags *flag.FlagSet) {
}
flags.String(c.Prefix+tlsCert, "", "Path to a TLS Certificate file, used to identify this server to clients")
flags.String(c.Prefix+tlsKey, "", "Path to a TLS Private Key file, used to identify this server to clients")
flags.String(c.Prefix+tlsClientCA, "", "Path to a TLS CA (Certification Authority) file used to verify certificates presented by clients (if unset, all clients are permitted)")
flags.String(c.Prefix+tlsClientCAOld, "", "(deprecated) see --"+c.Prefix+tlsClientCA)
if c.ShowClientCA {
rjs211 marked this conversation as resolved.
Show resolved Hide resolved
flags.String(c.Prefix+tlsClientCA, "", "Path to a TLS CA (Certification Authority) file used to verify certificates presented by clients (if unset, all clients are permitted)")
flags.String(c.Prefix+tlsClientCAOld, "", "(deprecated) see --"+c.Prefix+tlsClientCA)
}
}

// InitFromViper creates tls.Config populated with values retrieved from Viper.
Expand Down