Unify TLS configuration

[jrcamp]

There are several concurrent efforts to improve TLS configuration, some in core and some in contrib, so I think it'd help to get everybody aligned.

#288
#927
open-telemetry/opentelemetry-collector-contrib#215
open-telemetry/opentelemetry-collector-contrib#215
open-telemetry/opentelemetry-collector-contrib#198

Here is one proposal that includes all options I've seen so far:

// TLSConfig holds common TLS config options.
type TLSConfig struct {
	// Enabled is set to true if TLS should be enabled with default config.
	Enabled bool `mapstructure:"enabled"`
	// Path to the CA cert. For a client this verifies the server certificate. 
	// For a server this verifies client certificates. If empty uses system root CA.
	// (optional)
	CAFile string `mapstructure:"ca_file"`
	// Path to the client TLS cert to use for TLS required connections. (optional)
	CertFile string `mapstructure:"cert_file"`
	// Path to the client TLS key to use for TLS required connections. (optional)
	KeyFile string `mapstructure:"key_file"`
	// Whether or not to verify the exporter's TLS cert. (optional, default false)
	InsecureSkipVerify bool `mapstructure:"insecure_skip_verify"`
	// ServerName requested by client for virtual hosting. (optional)
	ServerName string `mapstructure:"server_name"`
}

@tigrannajaryan @bogdandrutu @ccaraman @pavolloffay @pjanotti

[jrcamp]

The purpose of having enabled flag is because all the config options are optional. We need a way to enable TLS with just the default options.

tls_config:

Wouldn't work because the value of tls_config would be empty so no easy way to tell it was enabled. So instead:

tls_config:
  enabled: true

Of course this is assuming TLS is off by default and it's something the user explicitly enables. Is this the case for all components?

[bogdandrutu]

Current config has a "UseSecure" property that is almost equal with tls_config.enabled

[jrcamp]

Current config has a "UseSecure" property that is almost equal with tls_config.enabled

I don't think UseSecure is a good name. It's confusing especially with InsecureSkipVerify (standard Go name).

tls_config:
   use_secure: true
   insecure_skip_verify: true

So it's both secure and insecure? :P

[jrcamp]

Other option is to separate out tls_enabled

tls_enabled: true

(tls_config not required, uses defaults)

tls_enabled: true
tls_config:
  ca_cert: /path/to/cert

I kind of prefer ^.

[ccaraman]

@jrcamp - I will take this task. I will update with a plan in a bit.

[pavolloffay]

+1 on unifying the TLS configuration.

It is a bit confusing to have /receiver/securereceiverconfig.go and config/configgrpc/configgrpc.go and probably more in the contrib.

[ccaraman]

Instead of adding 'tls_enabled as a variable - just have it that if the tls_config exists then tls is enabled. For HTTP scenarios, it would look like this

type MyConfig struct {
  configmodels.ReceiverSettings `mapstructure:",squash"`

  // Configures the receiver/exporter to use TLS.
  // The default value is nil, which will cause the receiver/exporter to not use TLS.
  TLSConfig *TLSConfig `mapstructure:"tls_config, omitempty"`
}

and the code base would check

if c.TLSConfig {
  // TLS is enabled 
}

[ccaraman]

insecure_skip_verify when set to true:

// InsecureSkipVerify controls whether a client verifies the
// server's certificate chain and host name.

use_secure is used to enable/disable WithInsecure by using the opposite value that is set in the configuration. ie when not set or explicitly set to false, this is used

// WithInsecure returns a DialOption which disables transport security for this
// ClientConn. Note that transport security is required unless WithInsecure is
// set.

They are similar but not quite the same.

There are two paths forward:

1. merge the boolean into one option that has one behavior in gRPC and one in HTTP
   or
2. we expose it as two booleans grpc_use_secure as is and in gRPC scenarios use_secure is used and in HTTP insecure_skip_verify is used.

I prefer option 1. This will result in a breaking change for existing configurations; however, it does default the behavior to use a secure connection (in line with gRPC and HTTP connections)

type TLSConfig struct {
	// Path to the CA cert. For a client this verifies the server certificate. 
	// For a server this verifies client certificates. If empty uses system root CA.
	// (optional)
	CAFile string `mapstructure:"ca_file"`
	// Path to the TLS cert to use for TLS required connections. (optional)
	CertFile string `mapstructure:"cert_file"`
	// Path to the TLS key to use for TLS required connections. (optional)
	KeyFile string `mapstructure:"key_file"`
	// In gRPC when set to true, this is used to disable the client transport security. 
        // See https://godoc.org/google.golang.org/grpc#WithInsecure.
        // In HTTP, this disables verifiying the server's certificate chain and host name 
        // (InsecureSkipVerify in the tls Config).
        // This is only used for client connections. (optional, default false)
	UseInsecure bool `mapstructure:"insecure"`
	// ServerName requested by client for virtual hosting. (optional)
	ServerName string `mapstructure:"server_name"`
}

[jrcamp]

I don't think we can differentiate between tls_config being not set and it being set to empty/nil without custom unmarshal logic. See demo code here:

https://repl.it/repls/MonthlyLowLinux

I'm a bit confused about gRPC. Does WithInsecure just mean TLS isn't enabled? If that's the case then that's controlled by the overall TLS on/off flag right? (Depends on how we resolve the first issue above). Am I missing something?

[ccaraman]

An empty TLS config doesn't do anything.

In HTTP client scenarios, tls is determined by the url passed into the HTTP client config. If the CA need to be updated, they can add it to the TLSConfig. But that isn't a hard requirement, they can add CAs to the system.

In gRPC path, the default behavior as implemented is to use WithInsecure when no TLS config option is set.

Re your WithInsecure gRPC question -
If WithInsecure is the only option set, then there is no TLS.
If CACert is set and WithInsecure - then the server connection is verified from the client. So it is still needed explicitly for gRPC.

Looking at some implementations of http receivers, the http/https scheme is passed in as a separate field to the config. It would be better for the endpoint to require http:// or https:// and that is parsed for the user. That would be more inline with HTTP Client/Server handlings.

[jrcamp]

Looking at some implementations of http receivers, the http/https scheme is passed in as a separate field to the config. It would be better for the endpoint to require http:// or https:// and that is parsed for the user. That would be more inline with HTTP Client/Server handlings.

I agree when a user is writing the endpoint explicitly it's better to have it as part of the URL. However this causes some problems with auto-discovered receivers. It's possible to put it in the endpoint but it requires a bit more trickery.

Examples where you can configure tls_enabled as a boolean:

receivers:
  receiver_creator:
    http/1:
      rule: pod.labels["app"] == "http"
      config:
        tls_enabled: true

receivers:
  receiver_creator:
    http/1:
      rule: pod.labels["app"] == "http"
      config:
        tls_enabled: `pod.labels["secure"] == "true"`

Examples where you have to configure it as part of endpoint:

receivers:
  receiver_creator:
    http/1:
      rule: pod.labels["app"] == "http"
      config:
        endpoint: `"https://" + endpoint`

receivers:
  receiver_creator:
    http/1:
      rule: pod.labels["app"] == "http"
      config:
        endpoint: `sprintf("%s://%s", pod.labels["secure"] == "true" ? "https" : "http", endpoint)`

We could perhaps support both the flag and having it as part of endpoint but would have to figure out how to handle cases where the options conflict (error out or one takes precedence?)

[pavolloffay]

Note that that receiver and exporter TLS struct might want to expose different set properties:

Exporter in addition to the cert and key should expose ServerNameOverride and perhaps SkipHostVerify.

Receiver, on the other hand, should expose clients' CA cert #963

[ccaraman]

@pavolloffay - so the real distinction comes from if the receiver/exporter is making a client or server connection. Ex. Prometheus Receiver is making a client connection.

A nice way to do this is to have the base TLSConfig (CA , cert and key for ex) and then have extra stuff in a TLSClientConfig/TLSServerConfig then have the components use the TLS*Config htat works best for them.

type TLSConfig struct {
	// Path to the CA cert. For a client this verifies the server certificate. 
	// For a server this verifies client certificates. If empty uses system root CA.
	// (optional)
	CAFile string `mapstructure:"ca_file"`
	// Path to the TLS cert to use for TLS required connections. (optional)
	CertFile string `mapstructure:"cert_file"`
	// Path to the TLS key to use for TLS required connections. (optional)
	KeyFile string `mapstructure:"key_file"`
}


type TLSClientConfig struct {
         TLSConfig `mapstructure:",squash"` // squash ensures fields are correctly decoded in embedded struct 
        
         // These are config options specific to client connections
	// In gRPC when set to true, this is used to disable the client transport security. 
        // See https://godoc.org/google.golang.org/grpc#WithInsecure.
        // In HTTP, this disables verifiying the server's certificate chain and host name 
        // (InsecureSkipVerify in the tls Config).
        // This is only used for client connections. (optional, default false)
	UseInsecure bool `mapstructure:"insecure"`
	// ServerName requested by client for virtual hosting. (optional)
	ServerName string `mapstructure:"server_name"`

}

type TLSServerConfig struct {
         TLSConfig `mapstructure:",squash"` // squash ensures fields are correctly decoded in embedded struct    

          // Underneath is anythin that is server connection specific. 
}

[ccaraman]

Synced with @jrcamp offline - to address the receiver_creator case the current suggestion is to add a scheme field to the receiver_creator config. That will handle the use cases of enabling client tls for automatically discovered endpoints.

[jrcamp]

@bogdandrutu I think this is mostly done with latest TLS changes. I think we still have the open issue of how to specify TLS is enabled in the case where tls_settings are not needed? (ie you just want default TLS config).

Do you do:

endpoint: https://localhost:1234

endpoint: unix+tls:///var/run/sock

I think we started discussing it but couldn't come to a conclusion.

[bogdandrutu]

@jrcamp do you mind closing this issue and start smaller issues focus on the problems left to be resolved?