-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
communicator/ssh: Add support SSH over HTTP Proxy #30274
Conversation
a744455
to
1715ce1
Compare
Thanks for this contribution! |
@crw |
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.
Thanks for working on this, and especially for the test docker-compose environment! I was able to verify it locally.
I have some comments inline on the implementation which I'd like to be addressed before merge. Please let me know if you have any questions on those.
Optional: true, | ||
}, | ||
"proxy_port": { | ||
Type: cty.String, |
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 think this should be a cty.Number
, not a string.
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.
Fix it.
) | ||
|
||
// Dialer implements for SSH over HTTP Proxy. | ||
type Dialer struct { |
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 don't think this type (and the new functions in this file) should be exported from this package. I'd also suggest including "proxy" in the name of this type, e.g. proxyDialer
, to clarify that it's not a generic SSH dialer.
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.
Fixed
base = base + p.username + ":" + p.password + "@" | ||
} | ||
|
||
return url.Parse(base + p.host) |
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 seems like a fragile way to construct a url.URL
—what if some of the arguments require URL quoting?
I think instead we likely want to instantiate the URL struct directly, using a url.Userinfo
to add the username and password if specified.
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.
Fixed
p.auth = true | ||
p.username = username | ||
p.password = password | ||
} |
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.
Is it required that both username and password are set? I believe it's valid to specify just username and no password, or a blank username with a password.
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.
Fixed
c.Close() | ||
return nil, err | ||
} | ||
reqUrl.Scheme = "" |
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.
Again I'm surprised by using url.Parse
here, especially since we erase the schema. Could we build a url.URL
value directly in this case, setting only the host?
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.
Fixed
|
||
if connInfo.ProxyHost != "" { | ||
p = newProxyInfo( | ||
connInfo.ProxyHost+":"+strconv.FormatUint(uint64(connInfo.ProxyPort), 10), |
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 think fmt.Sprintf
might be clearer here.
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.
Fixed
password string | ||
// Whether the HTTP Proxy requires authentication | ||
auth bool | ||
} |
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.
Upon further reading, I think we could omit this new type, and use a *url.URL
representing the proxy connection instead. While I haven't tried it, so I might be missing something, I think this would be simpler.
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.
Fixed
// If http proxy requires authentication, configure settings for basic authentication. | ||
if p.proxy.auth { | ||
req.SetBasicAuth(p.proxy.username, p.proxy.password) | ||
req.Header.Add("Proxy-Authorization", "Basic "+base64.StdEncoding.EncodeToString([]byte(p.proxy.username+":"+p.proxy.password))) |
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.
Instead of building this value ourselves, could we reuse the authorization header value?
req.Header.Add("Proxy-Authorization", "Basic "+base64.StdEncoding.EncodeToString([]byte(p.proxy.username+":"+p.proxy.password))) | |
req.Header.Add("Proxy-Authorization", req.Header.Get("Authorization")) |
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.
Fixed
Optional: true, | ||
}, | ||
"proxy_port": { | ||
Type: cty.String, |
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.
Same note as above—I think this should be a cty.Number
.
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.
Fixed
|
||
* `proxy_user_name` - The username to use connect to the private proxy host. | ||
|
||
* `proxy_user_password` - The password to use connect to the private proxy host. |
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.
We should clarify which of these fields are optional for the HTTP proxy to be used.
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.
Fixed
One other note: before pushing any changes, please rebase against the latest main branch to ensure you pick up the latest CI configuration. Thanks! |
Terraform's remote-exec provision hangs out when it execs on HTTP Proxy bacause it dosen't support SSH over HTTP Proxy. This commits enables Terraform's remote-exec to support SSH over HTTP Proxy. * adds `proxy_*` fields to `connection` which add configuration for a proxy host * if `proxy_host` set, connect to that proxy host via CONNECT method, then make the SSH connection to `host` or `bastion_host`
1715ce1
to
4fcfd08
Compare
4fcfd08
to
84f2547
Compare
@alisdair |
84f2547
to
4345d38
Compare
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 looks great! Thanks again for working on this 🙌
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
Terraform's remote-exec provision hangs out when it execs on HTTP Proxy bacause it dosen't support SSH over HTTP Proxy. This commits enables Terraform's remote-exec to support SSH over HTTP Proxy. * adds `proxy_*` fields to `connection` which add configuration for a proxy host * if `proxy_host` set, connect to that proxy host via CONNECT method, then make the SSH connection to `host` or `bastion_host`
This change is targeted for the upcoming 1.2 release. |
Thanks for working on this feature. I was doing some testing last days and I found some issues during the provisioning of files. I was not sure if opening a new bug or continuing with the conversation here, so let me know if I need to move to a new one. Terraform version
Debug Output
Expected Behavior The file provisioner should copy the file to the remote system during the deployment. Actual Behavior If the proxy is configured on the connection level, the provisioning of the file is crashing the terraform binary. If the proxy configuration is removed from the connection block the file is copied successfully to the remote system. If the provisioner block is removed the execution finish without crashing using the proxy as intended. Steps to Reproduce
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Description
These commits support SSH over HTTP Proxy in the SSH provisioner without significantly changing the existing Pure Go SSH implementation.
I found that in #4523 #1709, the lack of support for SSH on HTTP Proxy has inconvenienced many people. I myself have to switch networks whenever I use terraform in a company network.
Therefore, I proposed this implementation.
Changes
proxy_*
fields toconnection
which add configuration for a proxy hostproxy_host
: Setting this enables the SSH over HTTP connection. This hostwill be connected to first, and then the
host
orbastion_host
connection will be made from there.proxy_port
- The port to use connect to the proxy host.proxy_scheme
- http or httpsproxy_user_name
- The username to use connect to the private proxy host.proxy_user_password
- The password to use connect to the private proxy host.Others
You can create a test environment that connects to the test node via SSH through HTTP PROXY in the following repository.
https://github.com/htamakos/terraform-communicator-ssh-test-env
Result Example