ssh: export a transport interface #100
Conversation
|
This PR (HEAD: 4f34f86) 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 |
|
This PR (HEAD: 525bfca) 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 |
y3llowcake
force-pushed
the
ssh_transport_iface
branch
from
525bfca to
797f6aa
Compare
|
This PR (HEAD: 797f6aa) 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 |
|
Message from Andrew Bonventre: Patch Set 4: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. |
|
Message from Han-Wen Nienhuys: Patch Set 5: Code-Review-1 golang/go#31874 (comment) 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. |
|
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 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):
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. |
|
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. |
|
Message from Luke Young: Patch Set 5:
@filippo any updates on this pull request? Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. |
|
@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. |
|
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. |
|
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. |
|
Message from Cyrus Katrak: Patch Set 5: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. |
|
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 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):
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. |
|
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. |
|
Message from Luke Young: Patch Set 5:
@filippo any updates on this pull request? Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. |
|
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. |
|
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. |
|
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. |
|
Message from Cyrus Katrak: Patch Set 5: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. |
|
Message from Andrew Bonventre: Patch Set 4: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. |
|
Message from Han-Wen Nienhuys: Patch Set 5: Code-Review-1 golang/go#31874 (comment) 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. |
|
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 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):
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. |
|
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. |
|
Message from Luke Young: Patch Set 5:
@filippo any updates on this pull request? Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. |
|
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. |
|
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. |
|
Message from Deleted User: 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. |
|
Message from Cy Borg: Patch Set 5: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/193117. |
|
Great work on this @y3llowcake. Using this, I was able to use an existing ControlMaster to establish an ssh client from within golang. Did you by chance have an example of creating the ControlMaster from go? It sounds like we’re working on very similar problems you went through (now several years ago). |
|
There was an alternate approach which looked more sensible to me at https://go-review.googlesource.com/c/crypto/+/383374. However, the author never responded to any of the code review comments. |
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