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.

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

add TLS testing #150

Merged
merged 29 commits into from
Feb 21, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
1148d68
add TLS support & tests
Feb 2, 2023
bb72f18
Merge branch 'main' into tls-testing
Feb 2, 2023
25bba11
add tempfile cleanup
Feb 2, 2023
c9609f0
pass tls data around instead of using a global
Feb 3, 2023
79b00e0
remove unused field, touch up key id
Feb 3, 2023
fdd3d76
ignore pem files
Feb 3, 2023
8d31291
get TLS test working for gRPC, add CLI flags
Feb 3, 2023
9a368ea
remove struct fields that don't need to be there
Feb 14, 2023
ce27cb8
reorder fields
Feb 14, 2023
8cefb51
fix spelling error in envvar name
Feb 15, 2023
bbf573a
refactor TLS config to a func to reduce duplication
Feb 15, 2023
6218120
fix help text
Feb 15, 2023
c76a5f0
remove unused fields, inappropriate RootCA, and add ServerName
Feb 15, 2023
fd98ba6
add CA to RootCAs in server TLS config
Feb 15, 2023
3ce2d52
switch from envvars for root pool to --no-tls-verify
Feb 15, 2023
06f977a
expire all certs after an hour
Feb 15, 2023
7c4c790
add a comment to tls_for_test.go with a security warning
Feb 15, 2023
c7ee1b0
clean up TLS setting names
Feb 15, 2023
18028a9
rationalize client cert testing templates
Feb 15, 2023
7c53a39
implement client cert auth tests for grpc & HTTP
Feb 15, 2023
ae46149
implement client cert authentication
Feb 15, 2023
6738562
fix up error messages
Feb 15, 2023
d014834
strip out unneeded TLS settings
Feb 15, 2023
95466a0
Merge branch 'main' into tls-testing
Feb 15, 2023
26bed91
go mod tidy
Feb 15, 2023
bf2365b
Merge branch 'main' into tls-testing
Feb 15, 2023
d8c8691
rename TLS options to have a --tls prefix, clean up help text
Feb 15, 2023
4453a51
fix --insecure help text
Feb 15, 2023
ecd80aa
Merge branch 'main' into tls-testing
Feb 21, 2023
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
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,5 @@ otel-cli
# vendor/

dist/

*.pem
59 changes: 58 additions & 1 deletion data_for_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ type FixtureConfig struct {
// TODO: maybe move this up to the suite?
IsLongTest bool
// either grpcProtocol or httpProtocol, defaults to grpc
ServerProtocol serverProtocol
ServerProtocol serverProtocol
ServerTLSEnabled bool
// for timeout tests we need to start the server to generate the endpoint
// but do not want it to answer when otel-cli calls, this does that
StopServerBeforeExec bool
Expand Down Expand Up @@ -135,6 +136,62 @@ var suites = []FixtureSuite{
Spans: 1,
},
},
// TLS connections
{
Name: "minimum configuration (tls, recording, grpc)",
Config: FixtureConfig{
ServerProtocol: grpcProtocol,
CliArgs: []string{"status", "--endpoint", "https://{{endpoint}}", "--protocol", "grpc"},
TestTimeoutMs: 1000,
ServerTLSEnabled: true,
Env: map[string]string{
"SSL_CERT_FILE": "{{cert}}",
},
},
Expect: Results{
// otel-cli should NOT set insecure when it auto-detects localhost
Config: otelcli.DefaultConfig().
WithEndpoint("https://{{endpoint}}").
WithProtocol("grpc"),
Diagnostics: otelcli.Diagnostics{
IsRecording: true,
NumArgs: 5,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
},
Spans: 1,
Env: map[string]string{
"SSL_CERT_FILE": "{{cert}}",
},
},
},
{
Name: "minimum configuration (tls, recording, https)",
Config: FixtureConfig{
ServerProtocol: httpProtocol,
CliArgs: []string{"status", "--endpoint", "https://{{endpoint}}"},
TestTimeoutMs: 2000,
ServerTLSEnabled: true,
Env: map[string]string{
"SSL_CERT_FILE": "{{cert}}",
},
},
Expect: Results{
// otel-cli should NOT set insecure when it auto-detects localhost
Config: otelcli.DefaultConfig().
WithEndpoint("https://{{endpoint}}"),
Diagnostics: otelcli.Diagnostics{
IsRecording: true,
NumArgs: 3,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
},
Env: map[string]string{
"SSL_CERT_FILE": "{{cert}}",
},
Spans: 1,
},
},
},
// ensure things fail when they're supposed to fail
{
Expand Down
83 changes: 53 additions & 30 deletions main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package main_test
// TODO: stop using fixture.Name to track foreground/background

import (
"crypto/tls"
"encoding/json"
"log"
"net"
Expand Down Expand Up @@ -67,6 +68,10 @@ func TestOtelCli(t *testing.T) {
t.Fatal("no test fixtures loaded!")
}

// generates a CA, client, and server certs to use in tests
tlsData := generateTLSData(t)
defer tlsData.cleanup()

for _, suite := range suites {
// a fixture can be backgrounded after starting it up for e.g. otel-cli span background
// a second fixture with the same description later in the list will "foreground" it
Expand Down Expand Up @@ -105,7 +110,7 @@ func TestOtelCli(t *testing.T) {
fixtureWait := make(chan struct{})
fixtureDone := make(chan struct{})

go runFixture(t, fixture, fixtureWait, fixtureDone)
go runFixture(t, fixture, fixtureWait, fixtureDone, tlsData)

if fixture.Config.Background {
// save off the channels for flow control
Expand All @@ -123,32 +128,32 @@ func TestOtelCli(t *testing.T) {

// runFixture runs the OTLP server & command, waits for signal, checks
// results, then signals it's done.
func runFixture(t *testing.T, fixture Fixture, wait, done chan struct{}) {
func runFixture(t *testing.T, fixture Fixture, wait, done chan struct{}, tlsData tlsHelpers) {
// sets up an OTLP server, runs otel-cli, packages data up in these return vals
endpoint, results, span, events := runOtelCli(t, fixture)
endpoint, results, span, events := runOtelCli(t, fixture, tlsData)
<-wait
checkAll(t, fixture, endpoint, results, span, events)
checkAll(t, fixture, endpoint, results, span, events, tlsData)
done <- struct{}{}
}

// checkAll gathers up all the check* funcs below into one function.
func checkAll(t *testing.T, fixture Fixture, endpoint string, results Results, span otlpserver.CliEvent, events otlpserver.CliEventList) {
func checkAll(t *testing.T, fixture Fixture, endpoint string, results Results, span otlpserver.CliEvent, events otlpserver.CliEventList, tlsData tlsHelpers) {
// check timeout and process status expectations
checkProcess(t, fixture, results)

// compare the number of recorded spans against expectations in the fixture
checkSpanCount(t, fixture, results)

// compares the data in each recorded span against expectations in the fixture
checkSpanData(t, fixture, endpoint, span, events)
checkSpanData(t, fixture, endpoint, span, events, tlsData)

// many of the basic plumbing tests use status so it has its own set of checks
// but these shouldn't run for testing the other subcommands
if len(fixture.Config.CliArgs) > 0 && fixture.Config.CliArgs[0] == "status" && !fixture.Expect.CommandFailed {
checkStatusData(t, fixture, endpoint, results)
checkStatusData(t, fixture, endpoint, results, tlsData)
} else {
// checking the text output only makes sense for non-status paths
checkOutput(t, fixture, endpoint, results)
checkOutput(t, fixture, endpoint, results, tlsData)
}
}

Expand All @@ -175,8 +180,8 @@ func checkProcess(t *testing.T, fixture Fixture, results Results) {

// checkOutput looks that otel-cli output stored in the results and compares against
// the fixture expectation (with {{endpoint}} replaced).
func checkOutput(t *testing.T, fixture Fixture, endpoint string, results Results) {
wantOutput := strings.ReplaceAll(fixture.Expect.CliOutput, "{{endpoint}}", endpoint)
func checkOutput(t *testing.T, fixture Fixture, endpoint string, results Results, tlsData tlsHelpers) {
wantOutput := injectVars(fixture.Expect.CliOutput, endpoint, tlsData)
gotOutput := results.CliOutput
if fixture.Expect.CliOutputRe != nil {
gotOutput = fixture.Expect.CliOutputRe.ReplaceAllString(gotOutput, "")
Expand All @@ -191,17 +196,17 @@ func checkOutput(t *testing.T, fixture Fixture, endpoint string, results Results

// checkStatusData compares the sections of otel-cli status output against
// fixture data, substituting {{endpoint}} into fixture data as needed.
func checkStatusData(t *testing.T, fixture Fixture, endpoint string, results Results) {
func checkStatusData(t *testing.T, fixture Fixture, endpoint string, results Results, tlsData tlsHelpers) {
// check the env
injectEndpoint(endpoint, fixture.Expect.Env)
injectMapVars(endpoint, fixture.Expect.Env, tlsData)
if diff := cmp.Diff(fixture.Expect.Env, results.Env); diff != "" {
t.Errorf("env data did not match fixture in %q (-want +got):\n%s", fixture.Name, diff)
}

// check diagnostics, use string maps so the diff output is easy to compare to json
wantDiag := fixture.Expect.Diagnostics.ToStringMap()
gotDiag := results.Diagnostics.ToStringMap()
injectEndpoint(endpoint, wantDiag)
injectMapVars(endpoint, wantDiag, tlsData)
// there's almost always going to be cli_args in practice, so if the fixture has
// an empty string, just ignore that key
if wantDiag["cli_args"] == "" {
Expand All @@ -222,7 +227,7 @@ func checkStatusData(t *testing.T, fixture Fixture, endpoint string, results Res
wantConf[k] = gotConf[k]
}
}
injectEndpoint(endpoint, wantConf)
injectMapVars(endpoint, wantConf, tlsData)
if diff := cmp.Diff(wantConf, gotConf); diff != "" {
t.Errorf("[%s] config data did not match fixture (-want +got):\n%s", fixture.Name, diff)
}
Expand All @@ -243,10 +248,10 @@ var spanRegexChecks = map[string]*regexp.Regexp{

// checkSpanData compares the data in spans received from otel-cli against the
// fixture data.
func checkSpanData(t *testing.T, fixture Fixture, endpoint string, span otlpserver.CliEvent, events otlpserver.CliEventList) {
func checkSpanData(t *testing.T, fixture Fixture, endpoint string, span otlpserver.CliEvent, events otlpserver.CliEventList, tlsData tlsHelpers) {
// check the expected span data against what was received by the OTLP server
gotSpan := span.ToStringMap()
injectEndpoint(endpoint, gotSpan)
injectMapVars(endpoint, gotSpan, tlsData)
// remove keys that aren't supported for comparison (for now)
delete(gotSpan, "is_populated")
delete(gotSpan, "library")
Expand Down Expand Up @@ -285,15 +290,15 @@ func checkSpanData(t *testing.T, fixture Fixture, endpoint string, span otlpserv
}

// do a diff on a generated map that sets values to * when the * check succeeded
injectEndpoint(endpoint, wantSpan)
injectMapVars(endpoint, wantSpan, tlsData)
if diff := cmp.Diff(wantSpan, gotSpan); diff != "" {
t.Errorf("[%s] otel span info did not match fixture (-want +got):\n%s", fixture.Name, diff)
}
}

// runOtelCli runs the a server and otel-cli together and captures their
// output as data to return for further testing.
func runOtelCli(t *testing.T, fixture Fixture) (string, Results, otlpserver.CliEvent, otlpserver.CliEventList) {
func runOtelCli(t *testing.T, fixture Fixture, tlsData tlsHelpers) (string, Results, otlpserver.CliEvent, otlpserver.CliEventList) {
started := time.Now()

results := Results{
Expand Down Expand Up @@ -346,7 +351,13 @@ func runOtelCli(t *testing.T, fixture Fixture) (string, Results, otlpserver.CliE
}()

// port :0 means randomly assigned port, which we copy into {{endpoint}}
listener, err := net.Listen("tcp", "localhost:0")
var listener net.Listener
var err error
if fixture.Config.ServerTLSEnabled {
listener, err = tls.Listen("tcp", "localhost:0", tlsData.serverTLSConf)
} else {
listener, err = net.Listen("tcp", "localhost:0")
}
endpoint := listener.Addr().String()
if err != nil {
// t.Fatalf is not allowed since we run this in a goroutine
Expand Down Expand Up @@ -374,11 +385,11 @@ func runOtelCli(t *testing.T, fixture Fixture) (string, Results, otlpserver.CliE
args := []string{}
if len(fixture.Config.CliArgs) > 0 {
for _, v := range fixture.Config.CliArgs {
args = append(args, strings.ReplaceAll(v, "{{endpoint}}", endpoint))
args = append(args, injectVars(v, endpoint, tlsData))
}
}
statusCmd := exec.Command("./otel-cli", args...)
statusCmd.Env = mkEnviron(endpoint, fixture.Config.Env)
statusCmd.Env = mkEnviron(endpoint, fixture.Config.Env, tlsData)

cancelProcessTimeout := make(chan struct{}, 1)
go func() {
Expand Down Expand Up @@ -412,7 +423,7 @@ func runOtelCli(t *testing.T, fixture Fixture) (string, Results, otlpserver.CliE

// only try to parse status json if it was a status command
// TODO: support variations on otel-cli where status isn't the first arg
if len(cliOut) > 0 && len(args) > 0 && args[0] == "status" && !fixture.Expect.CommandFailed {
if len(args) > 0 && args[0] == "status" && !fixture.Expect.CommandFailed {
err = json.Unmarshal(cliOut, &results)
if err != nil {
t.Errorf("[%s] parsing otel-cli status output failed: %s", fixture.Name, err)
Expand Down Expand Up @@ -460,11 +471,11 @@ gather:
}

// mkEnviron converts a string map to a list of k=v strings and tacks on PATH.
func mkEnviron(endpoint string, env map[string]string) []string {
func mkEnviron(endpoint string, env map[string]string, tlsData tlsHelpers) []string {
mapped := make([]string, len(env)+1)
var i int
for k, v := range env {
mapped[i] = k + "=" + strings.ReplaceAll(v, "{{endpoint}}", endpoint)
mapped[i] = k + "=" + injectVars(v, endpoint, tlsData)
i++
}

Expand All @@ -475,12 +486,24 @@ func mkEnviron(endpoint string, env map[string]string) []string {
return mapped
}

// injectEndpoint iterates over the map and updates the values, replacing all instances
// of {{endpoint}} with the provided endpoint. 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.
func injectEndpoint(endpoint string, target map[string]string) {
// injectMapVars iterates over the map and updates the values, replacing all instances
// of {{endpoint}}, {{cert}}, {{client_key}}, and {{client_key_cert}} with test values.
func injectMapVars(endpoint string, target map[string]string, tlsData tlsHelpers) {
for k, v := range target {
target[k] = strings.ReplaceAll(v, "{{endpoint}}", endpoint)
target[k] = injectVars(v, endpoint, tlsData)
}
}

// injectVars replaces all instances of {{endpoint}}, {{cert}}, {{client_key}}, and
// {{client_key_cert}} 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
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it would be slightly clearer to say:

Once generated, we need to inject the port into all the values so the test comparisons...

// 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, "{{cert}}", tlsData.caFile)
out = strings.ReplaceAll(out, "{{client_key}}", tlsData.clientPrivKeyFile)
out = strings.ReplaceAll(out, "{{client_key_cert}}", tlsData.clientFile)
return out
}
4 changes: 4 additions & 0 deletions otelcli/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ type Config struct {
Blocking bool `json:"otlp_blocking" env:"OTEL_EXPORTER_OTLP_BLOCKING"`
NoTlsVerify bool `json:"no_tls_verify" env:"OTEL_CLI_NO_TLS_VERIFY"`

Certificate string `json:"certificate_file" env:"OTEL_EXPORTER_OTLP_CERTIFICATE,OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE"`
ClientKey string `json:"client_key_file" env:"OTEL_EXPORTER_OTLP_CLIENT_KEY,OTEL_EXPORTER_OTLP_TRACES_CLIENT_KEY"`
ClientCert string `json:"client_key_certificate_file" env:"OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE,OTEL_EXPORTER_OTLP_TRACES_CLIENT_CERTFICATE"`

ServiceName string `json:"service_name" env:"OTEL_CLI_SERVICE_NAME"`
SpanName string `json:"span_name" env:"OTEL_CLI_SPAN_NAME"`
Kind string `json:"span_kind" env:"OTEL_CLI_TRACE_KIND"`
Expand Down
17 changes: 9 additions & 8 deletions otelcli/diagnostics.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,15 @@ var diagnostics Diagnostics
// diagnosing issues with otel-cli. The only user-facing feature that should be
// using these is otel-cli status.
type Diagnostics struct {
CliArgs []string `json:"cli_args"`
IsRecording bool `json:"is_recording"`
ConfigFileLoaded bool `json:"config_file_loaded"`
NumArgs int `json:"number_of_args"`
DetectedLocalhost bool `json:"detected_localhost"`
ParsedTimeoutMs int64 `json:"parsed_timeout_ms"`
OtelError string `json:"otel_error"`
ExecExitCode int `json:"exec_exit_code"`
CliArgs []string `json:"cli_args"`
IsRecording bool `json:"is_recording"`
ConfigFileLoaded bool `json:"config_file_loaded"`
NumArgs int `json:"number_of_args"`
DetectedLocalhost bool `json:"detected_localhost"`
InsecureSkipVerify bool `json:"insecure_skip_verify"`
ParsedTimeoutMs int64 `json:"parsed_timeout_ms"`
OtelError string `json:"otel_error"`
ExecExitCode int `json:"exec_exit_code"`
}

// Handle complies with the otel error handler interface to capture errors
Expand Down
24 changes: 17 additions & 7 deletions otelcli/plumbing.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ package otelcli
import (
"context"
"crypto/tls"
"crypto/x509"
"net"
"net/url"
"os"
"regexp"
"strings"

Expand Down Expand Up @@ -128,11 +130,20 @@ func grpcOptions() []otlpgrpc.Option {
if config.Insecure || (isLoopbackAddr(config.Endpoint) && !strings.HasPrefix(config.Endpoint, "https")) {
grpcOpts = append(grpcOpts, otlpgrpc.WithInsecure())
} else if !isInsecureSchema(config.Endpoint) {
var tlsConfig *tls.Config
tlsConfig := &tls.Config{}
if config.NoTlsVerify {
tlsConfig = &tls.Config{
InsecureSkipVerify: true,
diagnostics.InsecureSkipVerify = true
tlsConfig.InsecureSkipVerify = true
}
if config.Certificate != "" {
tobert marked this conversation as resolved.
Show resolved Hide resolved
data, err := os.ReadFile(config.Certificate)
if err != nil {
softFail("uanble to load certificate: %s", err)
}

certpool := x509.NewCertPool()
certpool.AppendCertsFromPEM(data)
tlsConfig.RootCAs = certpool
}
grpcOpts = append(grpcOpts, otlpgrpc.WithDialOption(grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig))))
}
Expand Down Expand Up @@ -193,11 +204,10 @@ func httpOptions() []otlphttp.Option {
if config.Insecure || (isLoopbackAddr(config.Endpoint) && !strings.HasPrefix(config.Endpoint, "https")) {
httpOpts = append(httpOpts, otlphttp.WithInsecure())
} else if !isInsecureSchema(config.Endpoint) {
var tlsConfig *tls.Config
tlsConfig := &tls.Config{}
if config.NoTlsVerify {
tlsConfig = &tls.Config{
InsecureSkipVerify: true,
}
diagnostics.InsecureSkipVerify = true
tlsConfig.InsecureSkipVerify = true
}
httpOpts = append(httpOpts, otlphttp.WithTLSClientConfig(tlsConfig))
}
Expand Down
4 changes: 4 additions & 0 deletions otelcli/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ func addClientParams(cmd *cobra.Command) {
cmd.Flags().StringToStringVar(&config.Headers, "otlp-headers", defaults.Headers, "a comma-sparated list of key=value headers to send on OTLP connection")
cmd.Flags().BoolVar(&config.Blocking, "otlp-blocking", defaults.Blocking, "block on connecting to the OTLP server before proceeding")

cmd.Flags().StringVar(&config.Certificate, "cacert", defaults.Certificate, "a file containing the server's CA certificate")
cmd.Flags().StringVar(&config.ClientCert, "client-cert", defaults.ClientCert, "a file containing the client certificate")
cmd.Flags().StringVar(&config.ClientKey, "client-key", defaults.ClientKey, "a file containing the client certificate key")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

^^ need feedback on these. Do they look ok? Should there be a set of --tls-* options?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's dealer's choice if these should have a tls- prefix. personally, I view adding a prefix now as more future-proof because over time we're likely to see more, not fewer, cli options, and we're probably not going to go back and change these, once committed, so we might as well make them explicit now. On the other hand, the flags that we add in future are unlikely to be cert related, so maybe the prefix is implied and we can skip it. Characters aren't too expensive (even in cli options) so i've talked myself into adding the prefix, but I don't see it as a major UX/comprehensibility issue either way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made the change, it looks better this way:

      --tls-ca-cert string            a file containing the certificate authority bundle
      --tls-client-cert string        a file containing the client certificate
      --tls-client-key string         a file containing the client certificate key
      --tls-no-verify                 insecure! disables verification of the server certificate and name, mostly for self-signed CAs
      --no-tls-verify                 (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")
cmd.Flags().StringVar(&config.TraceparentCarrierFile, "tp-carrier", defaults.TraceparentCarrierFile, "a file for reading and WRITING traceparent across invocations")
Expand Down
Loading