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

Update SSH server #18711

Closed
wants to merge 30 commits into from
Closed

Update SSH server #18711

wants to merge 30 commits into from

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Feb 10, 2022

- Update SSH server libraries to support extensions negotations.
- The extensions negotations are needed to communitcate with algorithms are accepted for "publickey" auth.
- This PR adds 2 libraries. The modifed golang.org/x/crypto libraries(this in order to not mismatch with types in ssh.go) and a patched "github.com/gliderlabs/ssh" that has been modified in order to use the modified crypto library.
- Resolves go-gitea#17798
@Gusted Gusted added this to the 1.17.0 milestone Feb 10, 2022
modules/ssh/ssh.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 10, 2022
@singuliere
Copy link
Contributor

Would it be better to have the fork that is currently in codeberg.org/gusted/gliderlabs-ssh in gitea.com/gitea/gliderlabs-ssh so that its maintenance can be shared with other Gitea maintainers?

@Gusted
Copy link
Contributor Author

Gusted commented Feb 12, 2022

I don't have the permission to add new repos to the gitea org. So if a gitea owner would like to do that. CC @zeripath @techknowlogick

@zeripath
Copy link
Contributor

Sorry I don't have owner control of the gitea org on gitea.com 😝

@Gusted - I think migrate the repo - then open a transfer request to give it to the gitea org.

@Gusted
Copy link
Contributor Author

Gusted commented Feb 12, 2022

@zeripath Done!

@zeripath
Copy link
Contributor

zeripath commented Feb 12, 2022

well that worked!

repo added to the appropriate teams and trust model changed to committer.

@lunny
Copy link
Member

lunny commented Feb 12, 2022

@zeripath Forgot that. Now you have the power. I have set the protected branch on that new repository.

Switch to the gitea-owned version
@Gusted
Copy link
Contributor Author

Gusted commented Feb 12, 2022

Would it be better to have the fork that is currently in codeberg.org/gusted/gliderlabs-ssh in gitea.com/gitea/gliderlabs-ssh so that its maintenance can be shared with other Gitea maintainers?

Updated the PR.

Side-note: we currently have some other depedency under the ownership of gitea maintainers(and some even on github), I guess it's better to slowly move them as well to a gitea instance?

@zeripath
Copy link
Contributor

I think we need the gliderlabs patch to also use a gitea fork of go-crypto too

Gusted added 3 commits February 16, 2022 23:31
- Avoid some problems with golang not picking up the replace on the
forked version.
@Gusted
Copy link
Contributor Author

Gusted commented Feb 16, 2022

The commit that's patched on top of x/crypto: https://gitea.com/gitea/go-crypto/commit/31cfbd2326cbe371d5e8ea590d60a70fc98f85a5

@zeripath
Copy link
Contributor

I think we should just move the requisite parts of crypto/ssh into our fork of gliderlabs/ssh that way we can avoid a fork of x/crypto because whilst I don't mind forking the SSH server, I think forking x/crypto is too much.

@Gusted
Copy link
Contributor Author

Gusted commented Feb 21, 2022

I think we should just move the requisite parts of crypto/ssh into our fork of gliderlabs/ssh that way we can avoid a fork of x/crypto because whilst I don't mind forking the SSH server, I think forking x/crypto is too much.

So the problem is as follows:

  • Gliderlabs uses golang.org/x/crypto's struct, so if send our forked one pass one of their functions, golang will error. So we somehow we need to be in-par in which package's structs we send.
  • For some reason, if we use replace within Gliderlabs's go.mod, golang won't comply with that if we use gliderlabs as a dependency, so we have to hard-fork(replacing golang.org/x/crypto with gitea.com/gitea/go-crypto).
  • So we can either use replace twice(current situation) or we can remove gliderlab's dependency and replace it with gitea.com/gitea/gliderlabs(which will use gitea.com/gitea/go-crypto) and we add gitea.com/gitea/go-crypto as dependency and use that package only for the ssh module so golang won't error because of mismatched structs. (This should be situation as per your comment?)

It's more or less dependency hell as of now, but that's because of how the tooling works with golang.

@codecov-commenter
Copy link

codecov-commenter commented Feb 25, 2022

Codecov Report

Merging #18711 (23e009f) into main (548adb9) will decrease coverage by 0.06%.
The diff coverage is 30.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #18711      +/-   ##
==========================================
- Coverage   46.62%   46.55%   -0.07%     
==========================================
  Files         854      854              
  Lines      122587   122921     +334     
==========================================
+ Hits        57153    57231      +78     
- Misses      58537    58769     +232     
- Partials     6897     6921      +24     
Impacted Files Coverage Δ
cmd/admin.go 0.00% <0.00%> (ø)
cmd/cmd.go 40.74% <0.00%> (ø)
models/issue_comment.go 51.30% <0.00%> (-0.12%) ⬇️
models/lfs.go 21.93% <0.00%> (-0.15%) ⬇️
models/user/search.go 69.38% <ø> (ø)
modules/lfs/pointer.go 78.78% <0.00%> (-10.87%) ⬇️
modules/markup/csv/csv.go 28.28% <0.00%> (-0.59%) ⬇️
modules/markup/external/external.go 1.35% <0.00%> (-0.04%) ⬇️
modules/markup/orgmode/orgmode.go 51.88% <0.00%> (-1.00%) ⬇️
modules/migration/comment.go 100.00% <ø> (ø)
... and 59 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fdd304...23e009f. Read the comment docs.

@owenthereal
Copy link

Would it be possible to contribute the fixes in https://gitea.com/gitea/go-crypto upstream so that others are benefit?

@6543
Copy link
Member

6543 commented Feb 26, 2022

with this we need to maintain the golang crypto lib ourselve :/

and make sure we dont miss related security fixes etc ...!

@Gusted
Copy link
Contributor Author

Gusted commented Feb 26, 2022

Would it be possible to contribute the fixes in https://gitea.com/gitea/go-crypto upstream so that others are benefit?

Ah well, it's already a PR which includes more standardized specification golang/crypto#197, the PR hasn't been active, hence why we're now forking it.

with this we need to maintain the golang crypto lib ourselve :/

and make sure we dont miss related security fixes etc ...!

I'm looking into using the forked crypto library only for the SSH server part, such we can still use the regular x/crypto library. It was already proposed by zeripath to create our own SSH library which would be much more flexible. Such we don't have to mess around in the package itself but rather can implement such changes within gitea, but that's a long-term option.

Also the SSH part of the crypto library is such a mess and most of the code was made a decade ago, that's already a security risk on it's own.

@Gusted Gusted added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Mar 24, 2022
@Gusted
Copy link
Contributor Author

Gusted commented Mar 24, 2022

Adding blocked status because gitea.com/gitea/go-crypto needs to be updated to include the latest changes from upstream x/crypto

@6543
Copy link
Member

6543 commented Mar 25, 2022

Adding blocked status because gitea.com/gitea/go-crypto needs to be updated to include the latest changes from upstream x/crypto

we only use the ssh part - so it should be fine!
or is the ssh part affected?

@Gusted
Copy link
Contributor Author

Gusted commented Mar 25, 2022

we only use the ssh part - so it should be fine!
or is the ssh part affected?

the ssh package is part of x/crypto and has recently got some nice updates: https://github.com/golang/crypto/commits/master and allowed me for a recent PR as well: #19097

@Gusted
Copy link
Contributor Author

Gusted commented Mar 25, 2022

PR that needs to be merged first: https://gitea.com/gitea/go-crypto/pulls/3

@6543
Copy link
Member

6543 commented Mar 25, 2022

☝️ reviewed

@Gusted
Copy link
Contributor Author

Gusted commented Mar 25, 2022

All good from my side now.

@Gusted Gusted removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Mar 25, 2022
@lunny lunny modified the milestones: 1.17.0, 1.18.0 Jun 16, 2022
@lunny lunny modified the milestones: 1.18.0, 1.19.0 Oct 17, 2022
@Gusted
Copy link
Contributor Author

Gusted commented Nov 7, 2022

Closing, Go finally seems intrested to fix this. https://go-review.googlesource.com/c/crypto/+/447757

@Gusted Gusted closed this Nov 7, 2022
@Gusted Gusted deleted the fix-ssh-server branch November 7, 2022 19:09
@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 1 This PR needs approval from one additional maintainer to be merged. type/bug
Projects
None yet
10 participants