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

…abled (#1746) 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 <details> <summary>output</summary> ``` # 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 ``` </details> 2. Verified that if the cloud hostname is overridden, that TLS will not be disabled if `PX_DISABLE_TLS` is not set to `1` <details> <summary>output</summary> ``` # 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. ``` </details> 3. Verified that if the cloud hostname is overridden and `PX_DISABLE_TLS=1` that TLS will be disabled. <details> <summary>output</summary> ``` $ PX_DISABLE_TLS=1 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: 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 ``` </details> --------- Signed-off-by: Dom Del Nano <[email protected]>
pixie-io · Nov 2, 2023 · 18340f4 · 18340f4
1 parent 05fb849
commit 18340f4
Show file tree

Hide file tree

Showing 4 changed files with 85 additions and 6 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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)
+	}
+}