From 18340f4e15da6052082b638f25d972db6e987785 Mon Sep 17 00:00:00 2001 From: Dom Del Nano Date: Thu, 2 Nov 2023 13:35:45 -0700 Subject: [PATCH] Update pxapi Go client to enforce an opt in mechanism when TLS is disabled (#1746) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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](https://pkg.go.dev/px.dev/pxapi#NewClient). Relevant Issues: N/A Type of change: /kind bug Test Plan: The following scenarios were tested to ensure the opt in mechanism is functional 1. 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='' 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 ```
2. 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='' 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. ```
3. 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 ```
--------- Signed-off-by: Dom Del Nano --- src/api/go/pxapi/BUILD.bazel | 5 ++- src/api/go/pxapi/client.go | 8 ++--- src/api/go/pxapi/opts.go | 21 +++++++++++++ src/api/go/pxapi/opts_test.go | 57 +++++++++++++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 6 deletions(-) create mode 100644 src/api/go/pxapi/opts_test.go diff --git a/src/api/go/pxapi/BUILD.bazel b/src/api/go/pxapi/BUILD.bazel index da523913435..a552d982f0f 100644 --- a/src/api/go/pxapi/BUILD.bazel +++ b/src/api/go/pxapi/BUILD.bazel @@ -58,7 +58,10 @@ filegroup( pl_go_test( name = "pxapi_test", - srcs = ["results_test.go"], + srcs = [ + "opts_test.go", + "results_test.go", + ], embed = [":pxapi"], deps = [ "//src/api/go/pxapi/errdefs", diff --git a/src/api/go/pxapi/client.go b/src/api/go/pxapi/client.go index 1d00d85aba5..3d3b59f6496 100644 --- a/src/api/go/pxapi/client.go +++ b/src/api/go/pxapi/client.go @@ -22,7 +22,6 @@ import ( "context" "crypto/tls" "fmt" - "strings" "google.golang.org/grpc" "google.golang.org/grpc/credentials" @@ -61,7 +60,8 @@ type Client struct { cloudAddr string - useEncryption bool + useEncryption bool + disableTLSVerification bool grpcConn *grpc.ClientConn cmClient cloudpb.VizierClusterInfoClient @@ -86,9 +86,7 @@ func NewClient(ctx context.Context, opts ...ClientOption) (*Client, error) { } func (c *Client) init(ctx context.Context) error { - isInternal := strings.Contains(c.cloudAddr, "cluster.local") - - tlsConfig := &tls.Config{InsecureSkipVerify: isInternal} + tlsConfig := &tls.Config{InsecureSkipVerify: c.disableTLSVerification} creds := credentials.NewTLS(tlsConfig) conn, err := grpc.Dial(c.cloudAddr, grpc.WithTransportCredentials(creds)) diff --git a/src/api/go/pxapi/opts.go b/src/api/go/pxapi/opts.go index 220bed81d39..db3f9448740 100644 --- a/src/api/go/pxapi/opts.go +++ b/src/api/go/pxapi/opts.go @@ -18,6 +18,12 @@ package pxapi +import ( + "log" + "os" + "strings" +) + // ClientOption configures options on the client. type ClientOption func(client *Client) @@ -28,6 +34,21 @@ func WithCloudAddr(cloudAddr string) ClientOption { } } +// Allows disabling TLS verification if the `PX_DISABLE_TLS` env var is set and the cloud address is a cluster domain (cluster.local). +func WithDisableTLSVerification(cloudAddr string) ClientOption { + return func(c *Client) { + isInternal := strings.Contains(cloudAddr, "cluster.local") + tlsDisabled := os.Getenv("PX_DISABLE_TLS") == "1" + insecureSkipVerify := tlsDisabled && isInternal + + if !tlsDisabled && isInternal { + log.Fatalf("The `PX_DISABLE_TLS` environment variable must be set to \"1\" when making cloud connections that do not support TLS.\n") + } else { + c.disableTLSVerification = insecureSkipVerify + } + } +} + // WithBearerAuth is the option to specify bearer auth to use. func WithBearerAuth(auth string) ClientOption { return func(c *Client) { diff --git a/src/api/go/pxapi/opts_test.go b/src/api/go/pxapi/opts_test.go new file mode 100644 index 00000000000..03972f3d041 --- /dev/null +++ b/src/api/go/pxapi/opts_test.go @@ -0,0 +1,57 @@ +/* + * Copyright 2018- The Pixie Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + */ + +package pxapi + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestWithDisableTLSVerificationOnlyAppliesForLocalConn(t *testing.T) { + if err := os.Setenv("PX_DISABLE_TLS", "1"); err != nil { + t.Fatalf("failed to set the `PX_DISABLE_TLS` environment variable: %v", err) + } + c := &Client{} + // Hostname that isn't a cluster domain (cluster.local or custom domain) + opt := WithDisableTLSVerification("work.withpixie.ai") + opt(c) + + assert.False(t, c.disableTLSVerification) + + if err := os.Unsetenv("PX_DISABLE_TLS"); err != nil { + t.Fatalf("failed to unset the `PX_DISABLE_TLS` environment variable: %v", err) + } +} + +func TestWithDisableTLSVerificationWithEnvVarSet(t *testing.T) { + if err := os.Setenv("PX_DISABLE_TLS", "1"); err != nil { + t.Fatalf("failed to set the `PX_DISABLE_TLS` environment variable: %v", err) + } + c := &Client{} + opt := WithDisableTLSVerification("cluster.local") + opt(c) + + assert.True(t, c.disableTLSVerification) + + if err := os.Unsetenv("PX_DISABLE_TLS"); err != nil { + t.Fatalf("failed to unset the `PX_DISABLE_TLS` environment variable: %v", err) + } +}