Update pxapi Go client to enforce an opt in mechanism when TLS is disabled #1746

ddelnano · 2023-10-18T14:43:05Z

Summary: Update pxapi Go client to enforce an opt in mechanism when TLS is disabled

Disabling TLS is often useful for test environments, but is not expected to be relied on in any non test environments. The doc string of NewClient was updated, so that this new behavior will explained in the pxapi's Godoc documentation.

Relevant Issues: N/A

Type of change: /kind bug

Test Plan: The following scenarios were tested to ensure the opt in mechanism is functional

Verified that the default behavior does not disable TLS

output

# Set cloud.cluster.local to the IP address of work.withpixie.ai
$ grep cloud /etc/hosts
34.102.151.106 cloud.cluster.local

# Set cloud address to a cluster.local domain
$ git diff
diff --git a/src/api/go/pxapi/examples/basic_example/example.go b/src/api/go/pxapi/examples/basic_example/example.go
index 16e71f9787..cea9fd9ee0 100644
--- a/src/api/go/pxapi/examples/basic_example/example.go
+++ b/src/api/go/pxapi/examples/basic_example/example.go
@@ -74,7 +74,9 @@ func main() {
        }

        ctx := context.Background()
-       client, err := pxapi.NewClient(ctx, pxapi.WithAPIKey(apiKey))
+       cloudAddr := "cloud.cluster.local:443"
+       // client, err := pxapi.NewClient(ctx, pxapi.WithAPIKey(apiKey))
+       client, err := pxapi.NewClient(ctx, pxapi.WithAPIKey(apiKey), pxapi.WithCloudAddr(cloudAddr))
        if err != nil {
                panic(err)
        }


$ PX_CLUSTER_ID='<cluster id -- not secret>'  PX_API_KEY=$(cat .px-api-key)  bazel run src/api/go/pxapi/examples/basic_example:basic_example
INFO: Invocation ID: a7111f36-8a12-45a7-a8f4-b28eab9c841c
INFO: Streaming build results to: https://bb.corp.pixielabs.ai/invocation/a7111f36-8a12-45a7-a8f4-b28eab9c841c
INFO: Analyzed target //src/api/go/pxapi/examples/basic_example:basic_example (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //src/api/go/pxapi/examples/basic_example:basic_example up-to-date:
  bazel-bin/src/api/go/pxapi/examples/basic_example/basic_example_/basic_example
INFO: Elapsed time: 0.367s, Critical Path: 0.02s
INFO: 1 process: 1 internal.
INFO: Running command line: bazel-bin/src/api/go/pxapi/examples/basic_example/basic_example_/basic_example
INFO: Streaming build results to: https://bb.corp.pixielabs.ai/invocation/a7111f36-8a12-45a7-a8f4-b28eab9c841c
INFO: Build completed successfully, 1 total action
Running on Cluster: fbef7892-6302-4bd4-8b55-ae2a93d556fc
Running script
panic: rpc error: code = Unavailable desc = connection error: desc = "transport: authentication handshake failed: tls: failed to verify certificate: x509: certificate is valid for withpixie.ai, work.withpixie.ai, docs.withpixie.ai, segment.withpixie.ai, not cloud.cluster.local"

goroutine 1 [running]:
main.main()
        src/api/go/pxapi/examples/basic_example/example.go:95 +0x498

Verified that if the cloud hostname is overridden, that TLS will not be disabled if PX_DISABLE_TLS is not set to 1

output

# The following change was applied to main in order to test this and the next scenario
$ git diff
diff --git a/src/api/go/pxapi/examples/basic_example/example.go b/src/api/go/pxapi/examples/basic_example/example.go
index 16e71f9787..766ea39de0 100644
--- a/src/api/go/pxapi/examples/basic_example/example.go
+++ b/src/api/go/pxapi/examples/basic_example/example.go
@@ -74,7 +74,9 @@ func main() {
        }

        ctx := context.Background()
-       client, err := pxapi.NewClient(ctx, pxapi.WithAPIKey(apiKey))
+       cloudAddr := "cloud.cluster.local:443"
+       // client, err := pxapi.NewClient(ctx, pxapi.WithAPIKey(apiKey))
+       client, err := pxapi.NewClient(ctx, pxapi.WithAPIKey(apiKey), pxapi.WithCloudAddr(cloudAddr), pxapi.WithDisableTLSVerification(cloudAddr))

$ PX_CLUSTER_ID='<cluster id -- not secret>'  PX_API_KEY=$(cat .px-api-key)  bazel run src/api/go/pxapi/examples/basic_example:basic_example
INFO: Invocation ID: a7111f36-8a12-45a7-a8f4-b28eab9c841c
INFO: Streaming build results to: https://bb.corp.pixielabs.ai/invocation/a7111f36-8a12-45a7-a8f4-b28eab9c841c
INFO: Analyzed target //src/api/go/pxapi/examples/basic_example:basic_example (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //src/api/go/pxapi/examples/basic_example:basic_example up-to-date:
  bazel-bin/src/api/go/pxapi/examples/basic_example/basic_example_/basic_example
INFO: Elapsed time: 0.367s, Critical Path: 0.02s
INFO: 1 process: 1 internal.
INFO: Running command line: bazel-bin/src/api/go/pxapi/examples/basic_example/basic_example_/basic_example
INFO: Streaming build results to: https://bb.corp.pixielabs.ai/invocation/a7111f36-8a12-45a7-a8f4-b28eab9c841c
INFO: Build completed successfully, 1 total action
2023/11/02 18:23:15 The `PX_DISABLE_TLS` environment variable must be set to "1" when making cloud connections that do not support TLS.

Verified that if the cloud hostname is overridden and PX_DISABLE_TLS=1 that TLS will be disabled.

output

``` $ PX_DISABLE_TLS=1 PX_CLUSTER_ID='' PX_API_KEY=$(cat .px-api-key) bazel run src/api/go/pxapi/examples/basic_example:basic_example INFO: Invocation ID: 18efc480-2caf-4598-886e-7a1a5ac15fd9 INFO: Streaming build results to: https://bb.corp.pixielabs.ai/invocation/18efc480-2caf-4598-886e-7a1a5ac15fd9 INFO: Analyzed target //src/api/go/pxapi/examples/basic_example:basic_example (0 packages loaded, 0 targets configured). INFO: Found 1 target... Target //src/api/go/pxapi/examples/basic_example:basic_example up-to-date: bazel-bin/src/api/go/pxapi/examples/basic_example/basic_example_/basic_example INFO: Elapsed time: 0.287s, Critical Path: 0.02s INFO: 1 process: 1 internal. INFO: Running command line: bazel-bin/src/api/go/pxapi/examples/basic_example/basic_example_/basic_example INFO: Streaming build results to: https://bb.corp.pixielabs.ai/invocation/18efc480-2caf-4598-886e-7a1a5ac15fd9 INFO: Build completed successfully, 1 total action Running on Cluster: fbef7892-6302-4bd4-8b55-ae2a93d556fc Running on Cluster: fbef7892-6302-4bd4-8b55-ae2a93d556fc Running script 000008bf-0000-0aa8-0000-000000000742 /healthz 169.254.169.254 GET 000008bf-0000-0eff-0000-000000000a1b /readyz 10.68.192.129 GET 000008bf-0000-0eff-0000-000000000a1b /healthz 10.68.192.129 GET 000008bf-0000-0f06-0000-000000000a1b /healthz 10.68.192.129 GET 000008bf-0000-0f06-0000-000000000a1b /readyz 10.68.192.129 GET 000008bf-0000-0fb4-0000-000000000ae3 /readiness 10.68.192.129 GET 000008bf-0000-1113-0000-000000000d0a /healthcheck/dnsmasq 10.68.192.129 GET 000008bf-0000-1113-0000-000000000d0a /healthcheck/kubedns 10.68.192.129 GET 000008bf-0000-1113-0000-000000000d0a /metrics 10.68.192.129 GET 000008bf-0006-b357-0000-000003ab11bd /is-dynamic-lb-initialized 127.0.0.1 GET Execution Time: 940¬µs Bytes received: 646 ```

…abled Signed-off-by: Dom Del Nano <ddelnano@pixielabs.ai>

Signed-off-by: Dom Del Nano <ddelnano@pixielabs.ai>

src/api/go/pxapi/client.go

Signed-off-by: Dom Del Nano <ddelnano@pixielabs.ai>

JamesMBartlett · 2023-11-02T17:30:01Z

src/api/go/pxapi/client.go

@@ -87,8 +90,14 @@ func NewClient(ctx context.Context, opts ...ClientOption) (*Client, error) {

 func (c *Client) init(ctx context.Context) error {
 	isInternal := strings.Contains(c.cloudAddr, "cluster.local")
+	tlsDisabled := os.Getenv("PX_DISABLE_TLS") == "1"


IMO this should be a ClientOption and then downstream consumers of pxapi can set the client option if they want to when this env var is set.

I can definitely make this a ClientOption -- WithTLSVerification or something similar. However, the motivation behind this change was to ensure that TLS verification isn't implicitly disabled (such as when cloudAddr is cluster.local). So it was intentional to make the TLS verification opt out rather than silently disabling it.

Therefore, would you be ok if downstream consumers had to supply a WithDisableTLSVerification option if they needed the previous behavior?

The requirements for this came from a recent internal pentest. The goal was to document the way to opt out of TLS verification and to change the current default. My personal opinion is that the previous behavior isn't a broad security risk, but the people conducting the pentest did not share the same opinion.

Yeah I was thinking TLS verification by default, and then an explicit option to disable it.

This is updated to include that ClientOption now. I also added unit tests for it.

Signed-off-by: Dom Del Nano <ddelnano@pixielabs.ai>

src/api/go/pxapi/opts.go

ddelnano added 3 commits October 18, 2023 14:46

Update pxapi Go client to enforce an opt in mechanism when TLS is dis…

50a1398

…abled Signed-off-by: Dom Del Nano <ddelnano@pixielabs.ai>

Fix linting and format comment to multiple lines

0c4f49c

Signed-off-by: Dom Del Nano <ddelnano@pixielabs.ai>

Revert accidental change

e3c0fc0

Signed-off-by: Dom Del Nano <ddelnano@pixielabs.ai>

ddelnano force-pushed the ddelnano/require-ssl-disable-opt-in-pxapi-go-client branch from 6e537e7 to e3c0fc0 Compare October 18, 2023 14:46

etep reviewed Oct 18, 2023

View reviewed changes

src/api/go/pxapi/client.go Outdated Show resolved Hide resolved

Address PR feedback

1eefa5e

Signed-off-by: Dom Del Nano <ddelnano@pixielabs.ai>

ddelnano requested a review from a team October 23, 2023 16:11

JamesMBartlett reviewed Nov 2, 2023

View reviewed changes

Update TLS opt out to be a ClientOption and add unit tests

598c6c5

Signed-off-by: Dom Del Nano <ddelnano@pixielabs.ai>

JamesMBartlett approved these changes Nov 2, 2023

View reviewed changes

src/api/go/pxapi/opts.go Show resolved Hide resolved

JamesMBartlett merged commit 18340f4 into pixie-io:main Nov 2, 2023
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update pxapi Go client to enforce an opt in mechanism when TLS is disabled #1746

Update pxapi Go client to enforce an opt in mechanism when TLS is disabled #1746

ddelnano commented Oct 18, 2023 •

edited

Loading

JamesMBartlett Nov 2, 2023

ddelnano Nov 2, 2023 •

edited

Loading

JamesMBartlett Nov 2, 2023

ddelnano Nov 2, 2023

Update pxapi Go client to enforce an opt in mechanism when TLS is disabled #1746

Update pxapi Go client to enforce an opt in mechanism when TLS is disabled #1746

Conversation

ddelnano commented Oct 18, 2023 • edited Loading

JamesMBartlett Nov 2, 2023

Choose a reason for hiding this comment

ddelnano Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

JamesMBartlett Nov 2, 2023

Choose a reason for hiding this comment

ddelnano Nov 2, 2023

Choose a reason for hiding this comment

ddelnano commented Oct 18, 2023 •

edited

Loading

ddelnano Nov 2, 2023 •

edited

Loading