Skip to content

Commit

Permalink
TLS naming cleanup (#166)
Browse files Browse the repository at this point in the history 
Make all TLS settings use the same words & order. Over the course of developing the functionality I created a few variations. There's little functional change in this PR, mostly making the code easier to follow.

* rename some TLS keys and variables to be consistent

Renames NoTlsVerify to TlsNoVerify, adds Tls prefix to client cert
vars & config keys.

Adds missing config methods.

Update tests to match new keys & methods.

Fills in some gaps from first adding the TLS support.

Adds comments indicating --no-tls-verify and its related envvar are
deprecated.

* rename test template variables to match config keys & flags

* add TLS settings to README.md
  • Loading branch information
@tobert
tobert authored Feb 23, 2023
1 parent 759fbef commit 6018f76
Show file tree
Hide file tree
Showing 8 changed files with 107 additions and 54 deletions.
5 changes: 4 additions & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,10 @@ then config file, then environment variables.
| --tp-ignore-env | OTEL_CLI_IGNORE_ENV | traceparent_ignore_env | false |
| --tp-print | OTEL_CLI_PRINT_TRACEPARENT | traceparent_print | false |
| --tp-export | OTEL_CLI_EXPORT_TRACEPARENT | traceparent_print_export | false |
| --no-tls-verify | OTEL_CLI_NO_TLS_VERIFY | no_tls_verify | false |
| --tls-no-verify | OTEL_CLI_TLS_NO_VERIFY | tls_no_verify | false |
| --tls-ca-cert | OTEL_EXPORTER_OTLP_CERTIFICATE | tls_ca_cert | |
| --tls-client-key | OTEL_EXPORTER_OTLP_CLIENT_KEY | tls_client_key | |
| --tls-client-cert | OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE | tls_client_cert | |

[Valid timeout units](https://pkg.go.dev/time#ParseDuration) are "ns", "us"/"µs", "ms", "s", "m", "h".

Expand Down
28 changes: 19 additions & 9 deletions data_for_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ var suites = []FixtureSuite{
"status",
"--endpoint", "https://{{endpoint}}",
"--protocol", "grpc",
// TODO: switch to --tls-no-verify before 1.0, for now keep testing it
"--verbose", "--fail", "--no-tls-verify",
},
TestTimeoutMs: 1000,
Expand All @@ -163,7 +164,7 @@ var suites = []FixtureSuite{
WithEndpoint("https://{{endpoint}}").
WithProtocol("grpc").
WithVerbose(true).
WithNoTlsVerify(true),
WithTlsNoVerify(true),
Diagnostics: otelcli.Diagnostics{
IsRecording: true,
NumArgs: 8,
Expand All @@ -185,7 +186,7 @@ var suites = []FixtureSuite{
Expect: Results{
// otel-cli should NOT set insecure when it auto-detects localhost
Config: otelcli.DefaultConfig().
WithNoTlsVerify(true).
WithTlsNoVerify(true).
WithEndpoint("https://{{endpoint}}"),
Diagnostics: otelcli.Diagnostics{
IsRecording: true,
Expand All @@ -205,9 +206,9 @@ var suites = []FixtureSuite{
"--endpoint", "https://{{endpoint}}",
"--protocol", "grpc",
"--verbose", "--fail",
"--tls-ca-cert", "{{cacert}}",
"--tls-client-cert", "{{client_cert}}",
"--tls-client-key", "{{client_key}}",
"--tls-ca-cert", "{{tls_ca_cert}}",
"--tls-client-cert", "{{tls_client_cert}}",
"--tls-client-key", "{{tls_client_key}}",
},
TestTimeoutMs: 1000,
ServerTLSEnabled: true,
Expand All @@ -217,6 +218,9 @@ var suites = []FixtureSuite{
Config: otelcli.DefaultConfig().
WithEndpoint("https://{{endpoint}}").
WithProtocol("grpc").
WithTlsCACert("{{tls_ca_cert}}").
WithTlsClientKey("{{tls_client_key}}").
WithTlsClientCert("{{tls_client_cert}}").
WithVerbose(true),
Diagnostics: otelcli.Diagnostics{
IsRecording: true,
Expand All @@ -236,9 +240,9 @@ var suites = []FixtureSuite{
"status",
"--endpoint", "https://{{endpoint}}",
"--verbose", "--fail",
"--tls-ca-cert", "{{cacert}}",
"--tls-client-cert", "{{client_cert}}",
"--tls-client-key", "{{client_key}}",
"--tls-ca-cert", "{{tls_ca_cert}}",
"--tls-client-cert", "{{tls_client_cert}}",
"--tls-client-key", "{{tls_client_key}}",
},
TestTimeoutMs: 2000,
ServerTLSEnabled: true,
Expand All @@ -247,6 +251,9 @@ var suites = []FixtureSuite{
Expect: Results{
Config: otelcli.DefaultConfig().
WithEndpoint("https://{{endpoint}}").
WithTlsCACert("{{tls_ca_cert}}").
WithTlsClientKey("{{tls_client_key}}").
WithTlsClientCert("{{tls_client_cert}}").
WithVerbose(true),
Diagnostics: otelcli.Diagnostics{
IsRecording: true,
Expand Down Expand Up @@ -354,7 +361,10 @@ var suites = []FixtureSuite{
WithHeaders(map[string]string{"header1": "header1-value"}).
WithInsecure(true).
WithBlocking(false).
WithNoTlsVerify(true).
WithTlsNoVerify(true).
WithTlsCACert("/dev/null").
WithTlsClientKey("/dev/null").
WithTlsClientCert("/dev/null").
WithServiceName("configured_in_config_file").
WithSpanName("config_file_span").
WithKind("server").
Expand Down
8 changes: 4 additions & 4 deletions example-config.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@
"otlp_blocking" : false,

"insecure" : true,
"no_tls_verify" : true,
"ca_file": "/dev/null",
"cert_file": "/dev/null",
"key_file": "/dev/null",
"tls_no_verify" : true,
"tls_ca_cert": "/dev/null",
"tls_client_key": "/dev/null",
"tls_client_cert": "/dev/null",

"service_name" : "configured_in_config_file",

Expand Down
13 changes: 7 additions & 6 deletions main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -494,23 +494,24 @@ func mkEnviron(endpoint string, env map[string]string, tlsData tlsHelpers) []str
}

// injectMapVars iterates over the map and updates the values, replacing all instances
// of {{endpoint}}, {{cacert}}, {{client_cert}}, and {{client_key}} with test values.
// of {{endpoint}}, {{tls_ca_cert}}, {{tls_client_cert}}, and {{tls_client_key}} with
// test values.
func injectMapVars(endpoint string, target map[string]string, tlsData tlsHelpers) {
for k, v := range target {
target[k] = injectVars(v, endpoint, tlsData)
}
}

// injectVars replaces all instances of {{endpoint}}, {{cacert}}, {{client_cert}}, and
// {{client_key}} with test values.
// injectVars replaces all instances of {{endpoint}}, {{tls_ca_cert}},
// {{tls_client_cert}}, and {{tls_client_key}} with test values.
// This is needed because the otlpserver is configured to listen on :0 which has it
// grab a random port. Once that's generated we need to inject it into all the values
// so the test comparisons work as expected. Similarly for TLS testing, a temp CA and
// certs are created and need to be injected.
func injectVars(in, endpoint string, tlsData tlsHelpers) string {
out := strings.ReplaceAll(in, "{{endpoint}}", endpoint)
out = strings.ReplaceAll(out, "{{cacert}}", tlsData.caFile)
out = strings.ReplaceAll(out, "{{client_cert}}", tlsData.clientFile)
out = strings.ReplaceAll(out, "{{client_key}}", tlsData.clientPrivKeyFile)
out = strings.ReplaceAll(out, "{{tls_ca_cert}}", tlsData.caFile)
out = strings.ReplaceAll(out, "{{tls_client_cert}}", tlsData.clientFile)
out = strings.ReplaceAll(out, "{{tls_client_key}}", tlsData.clientPrivKeyFile)
return out
}
58 changes: 41 additions & 17 deletions otelcli/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ func DefaultConfig() Config {
Headers: map[string]string{},
Insecure: false,
Blocking: false,
NoTlsVerify: false,
TlsNoVerify: false,
TlsCACert: "",
TlsClientKey: "",
TlsClientCert: "",
ServiceName: "otel-cli",
SpanName: "todo-generate-default-span-names",
Kind: "client",
Expand Down Expand Up @@ -59,18 +62,18 @@ func DefaultConfig() Config {
// This is used as a singleton as "config" and accessed from many other files.
// Data structure is public so that it can serialize to json easily.
type Config struct {
Endpoint string `json:"endpoint" env:"OTEL_EXPORTER_OTLP_ENDPOINT,OTEL_EXPORTER_OTLP_TRACES_ENDPOINT"`
Protocol string `json:"protocol" env:"OTEL_EXPORTER_OTLP_PROTOCOL,OTEL_EXPORTER_OTLP_TRACES_PROTOCOL"`
Timeout string `json:"timeout" env:"OTEL_EXPORTER_OTLP_TIMEOUT,OTEL_EXPORTER_OTLP_TRACES_TIMEOUT"`
Headers map[string]string `json:"otlp_headers" env:"OTEL_EXPORTER_OTLP_HEADERS"` // TODO: needs json marshaler hook to mask tokens
Insecure bool `json:"insecure" env:"OTEL_EXPORTER_OTLP_INSECURE"`
Blocking bool `json:"otlp_blocking" env:"OTEL_EXPORTER_OTLP_BLOCKING"`
NoTlsVerify bool `json:"no_tls_verify" env:"OTEL_CLI_NO_TLS_VERIFY"`

// json keys match the otel collector yaml
CACert string `json:"ca_file" env:"OTEL_EXPORTER_OTLP_CERTIFICATE,OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE"`
ClientKey string `json:"cert_file" env:"OTEL_EXPORTER_OTLP_CLIENT_KEY,OTEL_EXPORTER_OTLP_TRACES_CLIENT_KEY"`
ClientCert string `json:"key_file" env:"OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE,OTEL_EXPORTER_OTLP_TRACES_CLIENT_CERTIFICATE"`
Endpoint string `json:"endpoint" env:"OTEL_EXPORTER_OTLP_ENDPOINT,OTEL_EXPORTER_OTLP_TRACES_ENDPOINT"`
Protocol string `json:"protocol" env:"OTEL_EXPORTER_OTLP_PROTOCOL,OTEL_EXPORTER_OTLP_TRACES_PROTOCOL"`
Timeout string `json:"timeout" env:"OTEL_EXPORTER_OTLP_TIMEOUT,OTEL_EXPORTER_OTLP_TRACES_TIMEOUT"`
Headers map[string]string `json:"otlp_headers" env:"OTEL_EXPORTER_OTLP_HEADERS"` // TODO: needs json marshaler hook to mask tokens
Insecure bool `json:"insecure" env:"OTEL_EXPORTER_OTLP_INSECURE"`
Blocking bool `json:"otlp_blocking" env:"OTEL_EXPORTER_OTLP_BLOCKING"`

TlsCACert string `json:"tls_ca_cert" env:"OTEL_EXPORTER_OTLP_CERTIFICATE,OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE"`
TlsClientKey string `json:"tls_client_key" env:"OTEL_EXPORTER_OTLP_CLIENT_KEY,OTEL_EXPORTER_OTLP_TRACES_CLIENT_KEY"`
TlsClientCert string `json:"tls_client_cert" env:"OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE,OTEL_EXPORTER_OTLP_TRACES_CLIENT_CERTIFICATE"`
// OTEL_CLI_NO_TLS_VERIFY is deprecated and will be removed for 1.0
TlsNoVerify bool `json:"tls_no_verify" env:"OTEL_CLI_TLS_NO_VERIFY,OTEL_CLI_NO_TLS_VERIFY"`

ServiceName string `json:"service_name" env:"OTEL_CLI_SERVICE_NAME,OTEL_SERVICE_NAME"`
SpanName string `json:"span_name" env:"OTEL_CLI_SPAN_NAME"`
Expand Down Expand Up @@ -188,7 +191,10 @@ func (c Config) ToStringMap() map[string]string {
"headers": flattenStringMap(c.Headers, "{}"),
"insecure": strconv.FormatBool(c.Insecure),
"blocking": strconv.FormatBool(c.Blocking),
"no_tls_verify": strconv.FormatBool(c.NoTlsVerify),
"tls_no_verify": strconv.FormatBool(c.TlsNoVerify),
"tls_ca_cert": c.TlsCACert,
"tls_client_key": c.TlsClientKey,
"tls_client_cert": c.TlsClientCert,
"service_name": c.ServiceName,
"span_name": c.SpanName,
"span_kind": c.Kind,
Expand Down Expand Up @@ -249,9 +255,27 @@ func (c Config) WithBlocking(with bool) Config {
return c
}

// WithNoTlsVerify returns the config with NoTlsVerify set to the provided value.
func (c Config) WithNoTlsVerify(with bool) Config {
c.NoTlsVerify = with
// WithTlsNoVerify returns the config with NoTlsVerify set to the provided value.
func (c Config) WithTlsNoVerify(with bool) Config {
c.TlsNoVerify = with
return c
}

// WithTlsCACert returns the config with TlsCACert set to the provided value.
func (c Config) WithTlsCACert(with string) Config {
c.TlsCACert = with
return c
}

// WithTlsClientKey returns the config with NoTlsClientKey set to the provided value.
func (c Config) WithTlsClientKey(with string) Config {
c.TlsClientKey = with
return c
}

// WithTlsClientCert returns the config with NoTlsClientCert set to the provided value.
func (c Config) WithTlsClientCert(with string) Config {
c.TlsClientCert = with
return c
}

Expand Down
19 changes: 17 additions & 2 deletions otelcli/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,23 @@ func TestWithBlocking(t *testing.T) {
t.Fail()
}
}
func TestWithNoTlsVerify(t *testing.T) {
if DefaultConfig().WithNoTlsVerify(true).NoTlsVerify != true {
func TestWithTlsNoVerify(t *testing.T) {
if DefaultConfig().WithTlsNoVerify(true).TlsNoVerify != true {
t.Fail()
}
}
func TestWithTlsCACert(t *testing.T) {
if DefaultConfig().WithTlsCACert("/a/b/c").TlsCACert != "/a/b/c" {
t.Fail()
}
}
func TestWithTlsClientKey(t *testing.T) {
if DefaultConfig().WithTlsClientKey("/c/b/a").TlsClientKey != "/c/b/a" {
t.Fail()
}
}
func TestWithTlsClientCert(t *testing.T) {
if DefaultConfig().WithTlsClientCert("/b/c/a").TlsClientCert != "/b/c/a" {
t.Fail()
}
}
Expand Down
20 changes: 10 additions & 10 deletions otelcli/plumbing.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,15 @@ func initTracer() (context.Context, func()) {
func tlsConfig() *tls.Config {
tlsConfig := &tls.Config{}

if config.NoTlsVerify {
if config.TlsNoVerify {
diagnostics.InsecureSkipVerify = true
tlsConfig.InsecureSkipVerify = true
}

// puts the provided CA certificate into the root pool
// when not provided, Go TLS will automatically load the system CA pool
if config.CACert != "" {
data, err := os.ReadFile(config.CACert)
if config.TlsCACert != "" {
data, err := os.ReadFile(config.TlsCACert)
if err != nil {
softFail("failed to load CA certificate: %s", err)
}
Expand All @@ -124,23 +124,23 @@ func tlsConfig() *tls.Config {
}

// client certificate authentication
if config.ClientCert != "" && config.ClientKey != "" {
clientPEM, err := os.ReadFile(config.ClientCert)
if config.TlsClientCert != "" && config.TlsClientKey != "" {
clientPEM, err := os.ReadFile(config.TlsClientCert)
if err != nil {
softFail("failed to read client certificate file %s: %s", config.ClientCert, err)
softFail("failed to read client certificate file %s: %s", config.TlsClientCert, err)
}
clientKeyPEM, err := os.ReadFile(config.ClientKey)
clientKeyPEM, err := os.ReadFile(config.TlsClientKey)
if err != nil {
softFail("failed to read client key file %s: %s", config.ClientKey, err)
softFail("failed to read client key file %s: %s", config.TlsClientKey, err)
}
certPair, err := tls.X509KeyPair(clientPEM, clientKeyPEM)
if err != nil {
softFail("failed to parse client cert pair: %s", err)
}
tlsConfig.Certificates = []tls.Certificate{certPair}
} else if config.ClientCert != "" {
} else if config.TlsClientCert != "" {
softFail("client cert and key must be specified together")
} else if config.ClientKey != "" {
} else if config.TlsClientKey != "" {
softFail("client cert and key must be specified together")
}

Expand Down
10 changes: 5 additions & 5 deletions otelcli/root.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,12 @@ func addClientParams(cmd *cobra.Command) {
cmd.Flags().BoolVar(&config.Blocking, "otlp-blocking", defaults.Blocking, "block on connecting to the OTLP server before proceeding")

cmd.Flags().BoolVar(&config.Insecure, "insecure", defaults.Insecure, "allow connecting to cleartext endpoints")
cmd.Flags().StringVar(&config.CACert, "tls-ca-cert", defaults.CACert, "a file containing the certificate authority bundle")
cmd.Flags().StringVar(&config.ClientCert, "tls-client-cert", defaults.ClientCert, "a file containing the client certificate")
cmd.Flags().StringVar(&config.ClientKey, "tls-client-key", defaults.ClientKey, "a file containing the client certificate key")
cmd.Flags().BoolVar(&config.NoTlsVerify, "tls-no-verify", defaults.NoTlsVerify, "insecure! disables verification of the server certificate and name, mostly for self-signed CAs")
cmd.Flags().StringVar(&config.TlsCACert, "tls-ca-cert", defaults.TlsCACert, "a file containing the certificate authority bundle")
cmd.Flags().StringVar(&config.TlsClientCert, "tls-client-cert", defaults.TlsClientCert, "a file containing the client certificate")
cmd.Flags().StringVar(&config.TlsClientKey, "tls-client-key", defaults.TlsClientKey, "a file containing the client certificate key")
cmd.Flags().BoolVar(&config.TlsNoVerify, "tls-no-verify", defaults.TlsNoVerify, "insecure! disables verification of the server certificate and name, mostly for self-signed CAs")
// --no-tls-verify is deprecated, will remove before 1.0
cmd.Flags().BoolVar(&config.NoTlsVerify, "no-tls-verify", defaults.NoTlsVerify, "(deprecated) same as --tls-no-verify")
cmd.Flags().BoolVar(&config.TlsNoVerify, "no-tls-verify", defaults.TlsNoVerify, "(deprecated) same as --tls-no-verify")

// OTEL_CLI trace propagation options
cmd.Flags().BoolVar(&config.TraceparentRequired, "tp-required", defaults.TraceparentRequired, "when set to true, fail and log if a traceparent can't be picked up from TRACEPARENT ennvar or a carrier file")
Expand Down

0 comments on commit 6018f76

Please sign in to comment.