-
Notifications
You must be signed in to change notification settings - Fork 441
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
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
…abled Signed-off-by: Dom Del Nano <ddelnano@pixielabs.ai>
Signed-off-by: Dom Del Nano <ddelnano@pixielabs.ai>
Signed-off-by: Dom Del Nano <ddelnano@pixielabs.ai>
6e537e7
to
e3c0fc0
Compare
Signed-off-by: Dom Del Nano <ddelnano@pixielabs.ai>
src/api/go/pxapi/client.go
Outdated
@@ -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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was thinking TLS verification by default, and then an explicit option to disable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is updated to include that ClientOption
now. I also added unit tests for it.
Signed-off-by: Dom Del Nano <ddelnano@pixielabs.ai>
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
output
PX_DISABLE_TLS
is not set to1
output
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 ```