crypto
crypto copied to clipboard
golang
ssh: add support for extension negotiation (rfc 8308)
This adds support for SSH extension negotiation via SSH_MSG_EXT_INFO as defined in RFC 8308 [1]. It also adds support for the server-sig-algs extension on both the client and server sides.
[1] https://datatracker.ietf.org/doc/html/rfc8308
Fixes golang/go#49269
This PR isn't ready for merging yet! I created the PR to get some initial feedback to see if the direction it's going is appropriate or if there are larger changes that need to be handled. I have a number of // TODO(kxd) comments where I called out specific questions I was hoping to get upstream feedback on as well!
The client side of server-sig-algs isn't implemented yet because I wanted to have a discussion about how that should integrate into choosing a pubkey auth algorithm.
This PR (HEAD: 29bf9ec043429558b99ff88e27422ccd0873e235) has been imported to Gerrit for code review.
Please visit https://go-review.googlesource.com/c/crypto/+/360195 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 Go Bot:
Patch Set 1:
Congratulations on opening your first change. Thank you for your contribution!
Next steps: A maintainer will review your change and provide feedback. See https://golang.org/doc/contribute.html#review for more info and tips to get your patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be surprising to people new to the project. The careful, iterative review process is our way of helping mentor contributors and ensuring that their contributions have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which little code gets reviewed or merged. If a reviewer responds with a comment like R=go1.11 or adds a tag like "wait-release", it means that this CL will be reviewed as part of the next development cycle. See https://golang.org/s/release for more details.
Please don’t reply on this GitHub thread. Visit golang.org/cl/360195. After addressing review feedback, remember to publish your drafts!
Message from Kristin Davidson:
Patch Set 1:
(1 comment)
Please don’t reply on this GitHub thread. Visit golang.org/cl/360195. After addressing review feedback, remember to publish your drafts!
Hi @aphistic, I tried your changes along with the recently merged support for SHA2 signatures for RSA host keys (commit: b4de73f9ece8163b492578e101e4ef8923ac2c5c). Both my OpenSSH 8.8 client and Go SSH server seemed happy about it.
It looks like @FiloSottile is marked as the sole Gerrit reviewer... Perhaps another Go maintainer CC'ed on the change (such as @agl) can take a look and offer feedback about this implementation?
Hey @stephen-fox! Those are the same changes we're running prod now, so it's good to hear it's working for you!
With regard to the review, I talked with @FiloSottile on the Gopher Slack about this PR and the next steps are to document the public changes to the API that I'd like to propose as part of merging this upstream. The code right now is not ready for merging and I think the public API for configuring extensions isn't correct, so this is definitely something that needs to happen first. I haven't had a chance to do this documentation yet, though, but I will redouble my efforts once I'm back in the office on Monday!.
Ah, I didn't realize there is a slack! And no worries - thank you for implementing this feature. I look forward to using it once it is merged :)
With regard to the review, I talked with @FiloSottile on the Gopher Slack about this PR and the next steps are to document the public changes to the API that I'd like to propose as part of merging this upstream. The code right now is not ready for merging and I think the public API for configuring extensions isn't correct, so this is definitely something that needs to happen first. I haven't had a chance to do this documentation yet, though, but I will redouble my efforts once I'm back in the office on Monday!.
@aphistic great to see this! Do you also plan to tackle automatically setting the right algorithm for the rsa signers (which are right now hardcoded to ssh-rsa?
With regard to the review, I talked with @FiloSottile on the Gopher Slack about this PR and the next steps are to document the public changes to the API that I'd like to propose as part of merging this upstream.
@aphistic FYI, managed to leverage your work for the ssh client signers with your current api exposure and without having to change the signatures of the signers themselves: https://github.com/rmohr/crypto/commit/e4ed9664ac545e290a287dc913e01b6939c837ea. Maybe it is helpful when looking into the API changes.
AFAIK it's not complete. It does not, for example wire in the negotiation extension to the client (only the server). There are other things, too.
Hi @owenthereal! @pete-woods is correct, this PR shouldn't be merged in its current state. The server-side of this is implemented but the configuration side of things will likely change, and the client-side isn't fully implemented either. If you only need server-sig-algs on the server side, this could work for you in its current state. We've been using it in prod for a few months now as it is.
I'd still like to get some feedback on the proposed config changes from the maintainers before I continue down the road for this PR, though. I have some discussion in the tracking issue for this PR https://github.com/golang/go/issues/49269.
I think people are starting to get nervous about this now the GitHub March 15th date (when rsa with SHA1 is deprecated) is getting pretty close. Any Go based Git client not using this patch plus extra stuff will break for a sizeable chunk of users.
@aphistic do you plan to rebase this patch on the master branch? For what I can understand the upstream seems quite unresponsive. I'm a little short on time but if you're not going to work on this I might give it a try in the next few weeks, thanks
@drakkan I am not sure if you need client or server support, but client support got merged a few days ago. Bumping golang.org/x/crypto should fix client issues.
@drakkan I am not sure if you need client or server support, but client support got merged a few days ago. Bumping golang.org/x/crypto should fix client issues.
@rmohr thanks, I'm aware that client support was merged. I would like to add server support to SFTPGo and so I need this patch rebased without the client specific parts that were implemented in a different way
Here is a rebase of this patch against current master:
0001-ssh-add-support-for-extension-negotiation-rfc-8308.zip
I just modified extInfoMsg marshal/unmarshal to match the one introduced upstream and added support for client certificate authentication which didn't work in the previous patch.
I don't think this patch is production ready, one of the many companies interested in this feature should hire one of the upstream maintainers to get a proper patch and so give something back to the community (I know at least one company that is developing a new SFTP server in Go and it is silently waiting for this patch ...)
@aphistic FYI, managed to leverage your work for the ssh client signers with your current api exposure and without having to change the signatures of the signers themselves: rmohr@e4ed966. Maybe it is helpful when looking into the API changes.
That fork apparently also fixed the server part for me. Adding
replace golang.org/x/crypto => github.com/rmohr/crypto v0.0.0-20211203105847-e4ed9664ac54
to go.mod worked!
