add TLS testing #150

tobert · 2023-02-03T00:53:35Z

Synopsis

This PR adds TLS testing capability to otel-cli's functional test harness, and to
make sure that functionality was complete enough to merge, I also added
client certificate authentication support. These are bundled because
I fixed some problems in otel-cli's TLS handling along the way. Hopefully this
will be the last big PR for a while. While on the large side of things, the bulk
of this code is the new TLS CA and tests.

Client Authentication

otel-cli can now directly authenticate to a server using a client certificate.
While this may have worked before via opentelemetry-go environment variables, we
could not call it a supported feature while not testing it, and not making it
work consistently with other otel-cli features (config & flags).

The new flags have been added with envvars and json keys. (note: I opened issue
#164 with a follow-up to fix the json keys): --tls-ca-cert,
--tls-client-cert, and --tls-client-key.

--no-tls-verify is now deprecated and renamed to --tls-no-verify so the
prefix usage is consistent.

TLS Testing Design

Building on the existing functional tests, tls_for_test.go has been added, and
implements an ephemeral self-signed certificate authority. It generates
everything needed for the client and server side to run tests with full TLS.
It's fast enough I barely notice the difference compared to exec'ing otel-cli a
bunch. New template variables are available for test fixtures to inject
certificate and key files. When the OTLP HTTP and gRPC servers start, they will
optionally enable TLS and client cert auth as configured on the fixture.

Adds the first pass at TLS helpers and integration with tests. https works now, grpc does not and needs more work

There's still more work to do. --no-tls-verify does not seem to work as expected and that needs figuring out and tested. Also need to add tests for client cert authentication. But this all works and the certs generated seem valid. Added diagnistic for no-tls-verify. Added setting a cacert in the root bundle in otel-cli. Need to double-check this is the right behavior to ship. Cacert config needs to be added to HTTPs as well if ^^ turns up as the behavior being correct.

tobert · 2023-02-03T22:48:31Z

otelcli/root.go

@@ -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")


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

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.

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

otelcli/plumbing.go

tls_for_test.go

On the first pass of this code I wasn't sure what all I would need, so I put all the intermediate steps of the CA and certs in the struct. Now that some tests are running, it's clear some things don't need to be there and the code looks cleaner this way.

They were out of sync and it was silly to copy the code in this case. 🤔 and now it can be tested independently.

SubjectKeyId shouldn't have been there in the first place, I copied it and didn't trim it back. The IPAddresses doesn't seem to have an impact on removal so removing it too. The TLS config setups for server and client were both wrong. They shouldn't have had RootCAs set. Setting ServerName seems to have helped to get HTTP TLS working (but grpc still doesn't). Also setting ClientCAs on the server for when we start testing client cert auth.

See comment for explanation of bewilderment.

The way the tests passed before wasn't ideal but was good enough to call the approach to testing viable. Now that the TLS configs are sorted out, the tests pass under --no-tls-verify and further commits will add tests back for envvar and client auth.

Renamed config struct member to CACert. Made config file names match the opentelemetry collector yaml, for better or worse. Added TLS settings to the example config.

In the first pass I built up the CA and certs according to my outdated knowledge of building CAs with openssl and examples. Since Go doesn't really care much about a lot of the cert metadata I went through and stripped out as many superfluous fields as I could and keep tests passing. Less code is good :)

edw-eqix · 2023-02-15T21:47:47Z

main_test.go

+// injectVars replaces all instances of {{endpoint}}, {{cacert}}, {{client_cert}}, and
+// {{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


nit: it would be slightly clearer to say:

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

edw-eqix

LGTM, discussion topics are non-blocking for commit

edw-eqix · 2023-02-15T21:59:16Z

otelcli/root.go

@@ -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")


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.

edw-eqix · 2023-02-15T22:05:04Z

tls_for_test.go

+ * Do NOT copy this code for production systems. It makes a few compromises to
+ * optimize for testing and ephemeral certs that are totally inappropriate for
+ * use in settings where security matters.


is there any way to make this more forceful than a comment?
off the top of my head, perhaps some kind of output from the module that would flag the issue to an end user?
or maybe also add code comments that point out the specific compromises a cargo-cult-er needs to change?
and the last thought was adding some kind of friction here to make it hard to misuse, though I have no concrete ideas on how to go about that.

oh and one last thought was to add something akin to a comment in the output files so that anyone viewing their contents would investigate and find the comments in code

Since it's in the main_test package it can't be directly used as a dependency and doesn't ship with the otel-cli binary. Someone would have to copy the file and modify it, or copy/paste to reuse the code, so risk is pretty low. I mostly wanted to say something and leave it as an exercise to the reader to figure out those compromises.

Per code review, I'm convinced that all the tls options should start with --tls for clarity so they've been renamed. --no-tls-verify has been renamed to --tls-no-verify. The old option is still supported but will be removed before 1.0.

No idea how this was so very wrong. Fixed now :)

Amy Tobey added 3 commits February 2, 2023 18:19

add TLS support & tests

1148d68

Adds the first pass at TLS helpers and integration with tests. https works now, grpc does not and needs more work

Merge branch 'main' into tls-testing

bb72f18

add tempfile cleanup

25bba11

tobert mentioned this pull request Feb 3, 2023

add a way to test TLS parameters work as documented/expected #144

Closed

Amy Tobey added 4 commits February 3, 2023 10:22

pass tls data around instead of using a global

c9609f0

remove unused field, touch up key id

79b00e0

ignore pem files

fdd3d76

tobert commented Feb 3, 2023

View reviewed changes

otelcli/plumbing.go Outdated Show resolved Hide resolved

tobert commented Feb 3, 2023

View reviewed changes

tls_for_test.go Show resolved Hide resolved

Amy Tobey added 16 commits February 14, 2023 18:28

reorder fields

ce27cb8

fix spelling error in envvar name

8cefb51

refactor TLS config to a func to reduce duplication

bbf573a

They were out of sync and it was silly to copy the code in this case. 🤔 and now it can be tested independently.

fix help text

6218120

add CA to RootCAs in server TLS config

fd98ba6

See comment for explanation of bewilderment.

expire all certs after an hour

06f977a

add a comment to tls_for_test.go with a security warning

7c4c790

clean up TLS setting names

c7ee1b0

Renamed config struct member to CACert. Made config file names match the opentelemetry collector yaml, for better or worse. Added TLS settings to the example config.

rationalize client cert testing templates

18028a9

implement client cert auth tests for grpc & HTTP

7c53a39

implement client cert authentication

ae46149

fix up error messages

6738562

tobert marked this pull request as ready for review February 15, 2023 20:18

tobert requested a review from edw-eqix February 15, 2023 20:18

Merge branch 'main' into tls-testing

95466a0

go mod tidy

26bed91

edw-eqix reviewed Feb 15, 2023

View reviewed changes

edw-eqix previously approved these changes Feb 15, 2023

View reviewed changes

Amy Tobey added 3 commits February 15, 2023 18:18

Merge branch 'main' into tls-testing

bf2365b

fix --insecure help text

4453a51

No idea how this was so very wrong. Fixed now :)

tobert dismissed edw-eqix’s stale review via bf2365b February 16, 2023 17:12

Merge branch 'main' into tls-testing

ecd80aa

edw-eqix approved these changes Feb 21, 2023

View reviewed changes

tobert mentioned this pull request Feb 21, 2023

follow-ups to #150 #164

Closed

tobert merged commit 9a7de86 into main Feb 21, 2023

tobert deleted the tls-testing branch February 21, 2023 23:45

This was referenced Feb 21, 2023

miscellaneous documentation fixes #165

Merged

TLS naming cleanup #166

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add TLS testing #150

add TLS testing #150

tobert commented Feb 3, 2023 •

edited

Loading

tobert Feb 3, 2023

edw-eqix Feb 15, 2023

tobert Feb 16, 2023

edw-eqix Feb 15, 2023

edw-eqix left a comment

edw-eqix Feb 15, 2023

edw-eqix Feb 15, 2023

edw-eqix Feb 15, 2023

tobert Feb 16, 2023

add TLS testing #150

add TLS testing #150

Conversation

tobert commented Feb 3, 2023 • edited Loading

Synopsis

Client Authentication

TLS Testing Design

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edw-eqix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobert commented Feb 3, 2023 •

edited

Loading