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

Fix tests hostkeys #17786

Closed

Conversation

6543
Copy link
Member

@6543 6543 commented Nov 23, 2021

allow ssh-rsa as hostkey algorithm for integration tests

source: https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/26273

@6543 6543 added this to the 1.16.0 milestone Nov 23, 2021
@6543 6543 added backport/v1.15 backport/done All backports for this PR have been created labels Nov 23, 2021
@zeripath
Copy link
Contributor

zeripath commented Nov 23, 2021

Tests fail as the version of ssh we are using does not have this option - but instead has the older version instead:

pubkeyacceptedkeytypes

Edit:

I should note that the underlying problem requiring this is NOT due to HostKeyAlgorithm acceptance (that was fixed by #17376) but due to the PubkeyAcceptedAlgorithm/PubkeyAcceptedKeyTypes which is a deeper issue with the way in which x/crypto/ssh handshakes over SSH.

Fixing that will require handling an SSH extension. It is also worth noting that there is a design flaw in the way that signers are related to keys and there is no way to separately configure Signers.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 23, 2021
@6543
Copy link
Member Author

6543 commented Nov 23, 2021

so should we update our test environment or let it just stay open until the ssh version is common?

@zeripath
Copy link
Contributor

We should really fix the ssh problem - although that will mean forking X/crypto/ssh

@techknowlogick
Copy link
Member

@zeripath Ive only been casually following the upstream issue, but my understanding is that there were two recent PRs that were merged to resolve this. Would updating our go.mod to use latest x/crypto be suitable enough

@zeripath
Copy link
Contributor

I think you're referring to golang/crypto@b4de73f

This is handling the host key certificate problem in much the same way as my merged PR for Gitea does. It's unfortunately not going to handle the client public key problem that this PR is referring to.

It's worth noting that the above commit has not addressed the design flaw that keys and signers are related but are not the same. Neither Gitea nor the X/crypto/ssh can actually turn off ssh-rsa signature verification for hosts right now.

@techknowlogick
Copy link
Member

Ah yes, that was it. Thanks for that explanation of the situation. It seems the current possible options are rather suboptimal in that case, the least preferred option (at least personally) would be to fork x/crypto.

@zeripath
Copy link
Contributor

zeripath commented Nov 24, 2021

Ah yes, that was it. Thanks for that explanation of the situation. It seems the current possible options are rather suboptimal in that case, the least preferred option (at least personally) would be to fork x/crypto.

Yeah I don't particularly fancy the idea of having to maintain an SSH server... (Although to some extent our hope that we can abstract that problem away to someone else has now failed and I've had to learn (something) about the SSH protocol and KEY_EXT_INFO etc.)

I guess the question is how long do we think upstream will take to fix this issue?

Whilst this issue is still unsolved our SSH server is not a production ready item and cannot be used out of the box with up-to-date ssh. Upstream do not appear to be moving with any speed to resolve this, and from looking at the code there are several fundamental design issues including one that means we cannot in future disable host-key signature verification with the ssh-rsa signing algorithm. This is far from ideal.

For me to work out how to properly fix this and propose a PR would involve me gaining a much more complete understanding of the SSH protocol and the design of x/crypto/ssh. We would need to change the handshake.go file considerably and we're going to need to properly separate signers from keys as I proposed on the golang issue. Such a PR would not only take some time to write but no doubt some time to merge. In the meantime it would entail Gitea using and maintaining that fork until the PR or an equivalent was merged in. The trouble is that I don't have a complete understanding of the SSH protocol or how x/crypto/ssh is designed but do have enough knowledge to say that this is not a trivial task, I therefore expect that x/crypto/ssh might be slow to merge such a PR.

I wonder if gliderlabs' @belak has a better understanding of the design in x/crypto/ssh and whether they would be able to workaround the issue. (However, AFAICS there simply isn't a place we can hook in to to sort this out as it is fundamentally a problem with the way handshake.go is written.)

@zeripath
Copy link
Contributor

We need to collect these issues in one place and open a proper Gitea issue for this.


For speed I'm going to collect things here:


What is actually needed:

  • Signers need to be separately configurable and advertisable apart from the key types that they sign.
  • The Handshake needs to support extension negotiation https://datatracker.ietf.org/doc/html/rfc8308 and in particular the SSH_MSG_EXT_INFO extension described therein.

@belak
Copy link

belak commented Nov 24, 2021

After digging through this for the past hour or so (which is probably a mistake for me since it's almost 4am here), I've come to pretty much the same conclusion - the only way I'm aware of to fix this is in the Go ssh internals, a hack in gliderlabs/ssh wouldn't be enough. I really have no desire at all to fork and maintain the core SSH code, especially when that isn't a part of my job, but I'd be happy to update or review PRs for gliderlabs/ssh to support upstream changes (if needed).

In terms of my knowledge, this was one part of the spec which I wasn't aware of, so I'm not sure how helpful I'll be. I'm a bit more familiar with the layer on top of this (channels, requests, etc).

On a related note, I think your comment here is relevant. It does a good job summarizing the changes which would need to happen to the Signer interface.

I'm also available in Discord for any quick questions, or if you just need someone to bounce ideas off of.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 3, 2021

Could we get this fix in 1.16, or should we leave it to 1.17 (and then backport) ?

@techknowlogick
Copy link
Member

@wxiaoguang if CI passes I'm happy to have it merged before 1.17

@zeripath
Copy link
Contributor

zeripath commented Dec 3, 2021

As I say above - this PR currently relies on an option that our testing version of ssh doesn't have and that only affected versions of ssh have.

If we want to merge this we need to detect if ssh has this option before setting it. There's likely little point setting the old option as only unaffected versions should be affected.

However this is only a workaround bodge for the more substantial problem that the ssh package in x/crypto/ssh is broken and lacks the functionality to handle pubkey authentication with RSA except through the old deprecated ssh-rsa algorithm.

@lunny
Copy link
Member

lunny commented Dec 13, 2021

CI failed is related.

@6543 6543 modified the milestones: 1.16.0, 1.17.0 Jan 5, 2022
@6543 6543 added status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR and removed backport/v1.15 labels Jan 5, 2022
@6543 6543 force-pushed the fix-tests-ssh-hostkeys-frontport branch from 8ac5a4a to 1e899de Compare February 6, 2022 19:00
@6543
Copy link
Member Author

6543 commented Feb 10, 2022

-> #17798 :D

@wxiaoguang wxiaoguang removed the backport/done All backports for this PR have been created label Apr 3, 2022
@lunny lunny modified the milestones: 1.17.0, 1.18.0 Jun 3, 2022
6543 added a commit to 6543-forks/gitea that referenced this pull request Jul 19, 2022
@6543
Copy link
Member Author

6543 commented Jul 19, 2022

I think it could be simpler if we should generate a new ed25519 ssh key and replace the old rsa key with it ...

@lunny lunny modified the milestones: 1.18.0, 1.19.0 Oct 17, 2022
@lunny lunny modified the milestones: 1.19.0, 1.20.0 Jan 31, 2023
@6543 6543 closed this Mar 21, 2023
@6543 6543 deleted the fix-tests-ssh-hostkeys-frontport branch March 21, 2023 03:06
@6543 6543 removed this from the 1.20.0 milestone Mar 21, 2023
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR type/bug type/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants