golang
ssh: export a transport interface
This change exports a new interface from the x/crypto/ssh library allowing an application to use this library's implementation of the SSH Connection Protocol (RFC 4254), but provide a separate implementation of the SSH Transport Protocol (RFC 4253).
Fixes golang/go#32958
This PR (HEAD: 4f34f865ce2886313504e0149d1520096ee4113b) has been imported to Gerrit for code review.
Please visit https://go-review.googlesource.com/c/crypto/+/193117 to see it.
Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info
This PR (HEAD: 525bfca837394a4886d30f640756bf0533fb4bf7) has been imported to Gerrit for code review.
Please visit https://go-review.googlesource.com/c/crypto/+/193117 to see it.
Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info
This PR (HEAD: 797f6aada2120dc7a0f8c78175d0db9e19f02233) has been imported to Gerrit for code review.
Please visit https://go-review.googlesource.com/c/crypto/+/193117 to see it.
Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info
Message from Andrew Bonventre:
Patch Set 4:
(1 comment)
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Han-Wen Nienhuys:
Patch Set 5: Code-Review-1
https://github.com/golang/go/issues/31874#issuecomment-491214121 explains my position on this, but I defer to Filippo and the crypto committee for a final decision.
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Luke Young:
Patch Set 5:
@Han-Wen respectfully, I think you should reconsider your stance on this. I agree that a ControlMaster implementation itself shouldn't be part of this library, however exposing Transport seems like a very reasonable compromise here. If not, this isn't even something that can be solved via external packages as there is no way for an external package to satisfy the *ssh.Client interface that other external packages may expect (ex: sftp) because it's a struct within internal fields not an interface.
Since this request was spawned off a discussion on ControlMaster, I also wanted to give a real-world example of ControlMaster usage where mixing OpenSSH with go could be reasonable:
The use of ControlMaster to enforce MFA but still provide a low friction developer experience is very common in the enterprise world. A typical setup may look like this (configured via an OpenSSH ssh_config file):
workstation -> bastion (MFA) -> production host
ControlMaster is enabled for the bastion host so that the user establishes an active connection once (completing MFA) to the bastion host and then re-uses it for every ongoing request to production hosts allowing them too seamlessly "ssh foo.prod.enterprise.com" without re-prompting for MFA.
Now imagine a go CLI has been written that takes a hostname and runs a command on the host. Using this package as-is, that would mean MFA must be entered on every invocation of the CLI. Alternatively, and entire server/RPC mechanism could be written and forked, but that seems like unnecessary work (and potentially is re-implemented for every CLI), and will still require an extra MFA on first connection. It would be great to be able to specifically use an already established ControlMaster via this Transport method.
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Thanks for the support Luke. This is one use case we are hoping to address with this change.
I'm currently maintaining a fork of the entire golang/crypto library just to export this interface, and I'd rather not be.
@hanwen @FiloSottile are there any additional steps I need to take for this this review to be addressed by the crypto committee?
@y3llowcake for what it's worth, I'm also doing that (maintaining a fork) but I've extracted just the ssh subpackage (refactoring imports and internal/) so you can use golang.org/x/crypto throughout your code but just import the fork for ssh: bored-engineer/ssh, you're welcome to use it if you want
Thanks @bored-engineer. We are also maintaining a fork at my employer, but would prefer not to. I wonder how to get this issue accepted or closed out, maybe the go team isn't watching github? I will try commenting on golang.org/cl/193117
Message from Cyrus Katrak:
Patch Set 5:
Has the crypto committee or Filippo come to a final decision?
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Luke Young:
Patch Set 5:
Patch Set 5:
Has the crypto committee or Filippo come to a final decision?
@Filippo any updates on this pull request?
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
@FiloSottile Any updates on this? This has been open without update for 6+ months now
Message from David Cowden:
Patch Set 5: Code-Review+1
I'm also in favor of this change. I've been working with the x/crypto/ssh package recently and would appreciate the ability to use the transport independent of the server implementation.
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Cyrus Katrak:
Patch Set 5:
David, out of curiosity, do you have a desire to be able to swap out the transport on the server interface as well? Not trying to expand the scope of an already contentious change just yet, but given the focus of your employer I figured I would ask.
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from David Cowden:
Patch Set 5:
Cyrus: yes. In fact, it's possible I'd end up more interested in that angle and I guess I should mention I'm also interested the reverse: use the package implementation of the transport protocol with custom connection logic. In any case, I simply stumbled upon this patch set and support decomposing the x/crypto/ssh package with the goal of increased flexibility. I don't see any obvious downsides (save maybe increased API surface) and have gained a recent empathy for creative applications of the ssh protocol.
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Cyrus Katrak:
Patch Set 5:
(1 comment)
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Luke Young:
Patch Set 5:
@Han-Wen respectfully, I think you should reconsider your stance on this. I agree that a ControlMaster implementation itself shouldn't be part of this library, however exposing Transport seems like a very reasonable compromise here. If not, this isn't even something that can be solved via external packages as there is no way for an external package to satisfy the *ssh.Client interface that other external packages may expect (ex: sftp) because it's a struct within internal fields not an interface.
Since this request was spawned off a discussion on ControlMaster, I also wanted to give a real-world example of ControlMaster usage where mixing OpenSSH with go could be reasonable:
The use of ControlMaster to enforce MFA but still provide a low friction developer experience is very common in the enterprise world. A typical setup may look like this (configured via an OpenSSH ssh_config file):
workstation -> bastion (MFA) -> production host
ControlMaster is enabled for the bastion host so that the user establishes an active connection once (completing MFA) to the bastion host and then re-uses it for every ongoing request to production hosts allowing them too seamlessly "ssh foo.prod.enterprise.com" without re-prompting for MFA.
Now imagine a go CLI has been written that takes a hostname and runs a command on the host. Using this package as-is, that would mean MFA must be entered on every invocation of the CLI. Alternatively, and entire server/RPC mechanism could be written and forked, but that seems like unnecessary work (and potentially is re-implemented for every CLI), and will still require an extra MFA on first connection. It would be great to be able to specifically use an already established ControlMaster via this Transport method.
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Cyrus Katrak:
Patch Set 5:
Has the crypto committee or Filippo come to a final decision?
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Luke Young:
Patch Set 5:
Patch Set 5:
Has the crypto committee or Filippo come to a final decision?
@Filippo any updates on this pull request?
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from David Cowden:
Patch Set 5: Code-Review+1
I'm also in favor of this change. I've been working with the x/crypto/ssh package recently and would appreciate the ability to use the transport independent of the server implementation.
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Cyrus Katrak:
Patch Set 5:
David, out of curiosity, do you have a desire to be able to swap out the transport on the server interface as well? Not trying to expand the scope of an already contentious change just yet, but given the focus of your employer I figured I would ask.
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from David Cowden:
Patch Set 5:
Cyrus: yes. In fact, it's possible I'd end up more interested in that angle and I guess I should mention I'm also interested the reverse: use the package implementation of the transport protocol with custom connection logic. In any case, I simply stumbled upon this patch set and support decomposing the x/crypto/ssh package with the goal of increased flexibility. I don't see any obvious downsides (save maybe increased API surface) and have gained a recent empathy for creative applications of the ssh protocol.
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Cyrus Katrak:
Patch Set 5:
(1 comment)
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Andrew Bonventre:
Patch Set 4:
(1 comment)
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Han-Wen Nienhuys:
Patch Set 5: Code-Review-1
https://github.com/golang/go/issues/31874#issuecomment-491214121 explains my position on this, but I defer to Filippo and the crypto committee for a final decision.
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Luke Young:
Patch Set 5:
@Han-Wen respectfully, I think you should reconsider your stance on this. I agree that a ControlMaster implementation itself shouldn't be part of this library, however exposing Transport seems like a very reasonable compromise here. If not, this isn't even something that can be solved via external packages as there is no way for an external package to satisfy the *ssh.Client interface that other external packages may expect (ex: sftp) because it's a struct within internal fields not an interface.
Since this request was spawned off a discussion on ControlMaster, I also wanted to give a real-world example of ControlMaster usage where mixing OpenSSH with go could be reasonable:
The use of ControlMaster to enforce MFA but still provide a low friction developer experience is very common in the enterprise world. A typical setup may look like this (configured via an OpenSSH ssh_config file):
workstation -> bastion (MFA) -> production host
ControlMaster is enabled for the bastion host so that the user establishes an active connection once (completing MFA) to the bastion host and then re-uses it for every ongoing request to production hosts allowing them too seamlessly "ssh foo.prod.enterprise.com" without re-prompting for MFA.
Now imagine a go CLI has been written that takes a hostname and runs a command on the host. Using this package as-is, that would mean MFA must be entered on every invocation of the CLI. Alternatively, and entire server/RPC mechanism could be written and forked, but that seems like unnecessary work (and potentially is re-implemented for every CLI), and will still require an extra MFA on first connection. It would be great to be able to specifically use an already established ControlMaster via this Transport method.
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Cy Borg:
Patch Set 5:
Has the crypto committee or Filippo come to a final decision?
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Luke Young:
Patch Set 5:
Patch Set 5:
Has the crypto committee or Filippo come to a final decision?
@Filippo any updates on this pull request?
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Deleted User:
Patch Set 5: Code-Review+1
I'm also in favor of this change. I've been working with the x/crypto/ssh package recently and would appreciate the ability to use the transport independent of the server implementation.
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!
Message from Cy Borg:
Patch Set 5:
David, out of curiosity, do you have a desire to be able to swap out the transport on the server interface as well? Not trying to expand the scope of an already contentious change just yet, but given the focus of your employer I figured I would ask.
Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. After addressing review feedback, remember to publish your drafts!