Skip to content
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

Feature-flagged support for socks5 proxy in Client #1311

Merged
merged 3 commits into from
Oct 29, 2023

Conversation

Razz4780
Copy link
Contributor

@Razz4780 Razz4780 commented Oct 12, 2023

Closes #1270

Motivation

kubectl supports using SOCKS5 proxy via proxy-url property in the kubeconfig. In kube-client, this property is only parsed into the Config struct and never used.

Solution

Added hyper-socks2 dependency to use a different HttpConnector when creating the ClientBuilder. Had to move most of the TryFrom<Config> for ClientBuilder implementation into a separate generic function and add generic parameters to some tls-related traits.

New logic is hidden behind the socks5 feature. If the feature is disabled, proxy-url is ignored (as was before this change).

Tested manually on a minikube cluster using this plugin.

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #1311 (954a4bf) into main (4083979) will decrease coverage by 0.2%.
The diff coverage is 61.3%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1311     +/-   ##
=======================================
- Coverage   72.4%   72.1%   -0.2%     
=======================================
  Files         75      75             
  Lines       6352    6377     +25     
=======================================
+ Hits        4595    4597      +2     
- Misses      1757    1780     +23     
Files Coverage Δ
kube-client/src/client/config_ext.rs 41.7% <100.0%> (-6.4%) ⬇️
kube-client/src/config/mod.rs 45.4% <ø> (ø)
kube-client/src/client/builder.rs 59.0% <60.7%> (-12.5%) ⬇️

... and 1 file with indirect coverage changes

@nightkr
Copy link
Member

nightkr commented Oct 12, 2023

Haven't looked into the implementation yet, but in theory this is a +1 from me.

New logic is hidden behind the socks5 feature. If the feature is disabled, proxy-url is ignored (as was before this change).

A connection URL doesn't mean the same thing if a proxy is involved, so IMO it's invalid to just ignore the proxy address if the feature is enabled. If we don't support the protocol required to access the service then that should be a hardfail, just like you wouldn't silently downgrade a HTTPS request to HTTP.

@Razz4780
Copy link
Contributor Author

Razz4780 commented Oct 12, 2023

Haven't looked into the implementation yet, but in theory this is a +1 from me.

New logic is hidden behind the socks5 feature. If the feature is disabled, proxy-url is ignored (as was before this change).

A connection URL doesn't mean the same thing if a proxy is involved, so IMO it's invalid to just ignore the proxy address if the feature is enabled. If we don't support the protocol required to access the service then that should be a hardfail, just like you wouldn't silently downgrade a HTTPS request to HTTP.

That's true, thanks. I wanted to preserve the current behavior in case the new feature is disabled, but probably this is somehting we should check

@clux clux changed the title Feature-flagged support for in kubeconfig Feature-flagged support for socks5 proxy in Client Oct 16, 2023
@clux clux added the changelog-add changelog added category for prs label Oct 16, 2023
@clux
Copy link
Member

clux commented Oct 16, 2023

Had a quick look at this. I think in general this looks sensible. While the hyper-socks2 library does not receive a lot of attention, it also is not big.

I am getting some errors actually using it with a server that has proxy though:

2023-10-16T10:51:03.908265Z ERROR kube_client::client::builder: failed with error error trying to connect: tcp connect error: Connection refused (os error 61)
Error: failed to perform initial object list: HyperError: error trying to connect: tcp connect error: Connection refused (os error 61)

Caused by:
    0: HyperError: error trying to connect: tcp connect error: Connection refused (os error 61)
    1: error trying to connect: tcp connect error: Connection refused (os error 61)
    2: tcp connect error: Connection refused (os error 61)
    3: Connection refused (os error 61)

on a config kind of like:

- cluster:
    certificate-authority-data: ...
    proxy-url: socks5://0.0.0.0:3126
    server: https://127.0.0.1:6443
  name: myproxy

which is socks5 proxied over ssh (in ~/.ssh/config):

Host myproxy
  HostName myproxyhost
  User clux
  IdentityFile ~/.ssh/main_id
  DynamicForward 3126

which works with kubectl get pods

haven't dug in too deep yet.

@nightkr
Copy link
Member

nightkr commented Oct 16, 2023

@clux

By default OpenSSH will only bind DynamicForward to localhost (see DynamicForward, GatewayPorts), so you'd need to set proxy-url: socks5://127.0.0.1:3126.

I'm more surprised that it worked with kubectl...

@clux
Copy link
Member

clux commented Oct 16, 2023

i think that's just 0.0.0.0 as the default route is just 127.0.0.1 for me. either way, the outcome is the same for both kubectl and kube.

@clux
Copy link
Member

clux commented Oct 27, 2023

Ok, apologies, I did some digging and found the reason it is not working; the socks5 feature is not enabled in examples/Cargo.toml that i was using to test... 🤦

Do you mind applying this diff:

diff --git a/examples/Cargo.toml b/examples/Cargo.toml
index 2224fe7..5e95e5c 100644
--- a/examples/Cargo.toml
+++ b/examples/Cargo.toml
@@ -14,11 +14,12 @@ license = "Apache-2.0"
 release = false
 
 [features]
-default = ["rustls-tls", "kubederive", "ws", "latest", "runtime", "refresh"]
+default = ["rustls-tls", "kubederive", "ws", "latest", "socks5", "runtime", "refresh"]
 kubederive = ["kube/derive"]
 openssl-tls = ["kube/client", "kube/openssl-tls"]
 rustls-tls = ["kube/client", "kube/rustls-tls"]
 runtime = ["kube/runtime", "kube/unstable-runtime"]
+socks5 = ["kube/socks5"]
 refresh = ["kube/oauth", "kube/oidc"]
 ws = ["kube/ws"]
 latest = ["k8s-openapi/latest"]

then it's a 👍 from me as well.

@clux clux added this to the 0.87.0 milestone Oct 27, 2023
@clux clux merged commit 12bd223 into kube-rs:main Oct 29, 2023
17 checks passed
@deepu105
Copy link

Does this impl also support the http_proxy env vars?

@clux
Copy link
Member

clux commented Oct 29, 2023

currently this only supports socks5.

the http_proxy evars will still be read and used to override the config.proxy_url though, so i imagine it could be difficult to use this feature if you want to support both https_proxy and socks proxy (because we don't check the protocol atm and just send it down to the socks lib where i imagine it will fail).

it probably makes sense long term to add another feature that handles http proxying so we could switch on the protocol scheme. i see there's https://crates.io/crates/hyper-proxy that could perhaps be used, but it's not well maintained (related issue: tafia/hyper-proxy#37).

depending on failure modes encountered for https_proxy with the socks5 feature enabled, it might make sense to add some extra error handling here between that time, or maybe just some scheme matching so we don't enable socks5 proxy in cases where people aren't asking for a socks5:// url?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-add changelog added category for prs
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Client proxy_url handling
4 participants