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

Switch built-in SSH server to github.com/gliderlabs/ssh #3896

Closed
wants to merge 6 commits into from

Conversation

belak
Copy link

@belak belak commented May 4, 2018

This pull request is the first step towards making the built-in SSH server a good option. As discussed in discord, gliderlabs/ssh does a fairly good job at abstracting away most of the error-prone x/crypto/ssh code while still maintaining the flexibility we need.

A few things worth noting:

  • The change in cmd/serv.go probably isn't needed (I can revert it if requested), but may be worthwhile anyway. I was using that before I was properly shell escaping command arguments.
  • The copyright was changed from Gogs to Gitea because almost the entire file was rewritten, mostly from scratch.
  • There's a TODO in Listen because gliderlabs/ssh doesn't currently support choosing ciphers, key exchanges, and MACs. I'd be happy to open a PR there to get that support in.
  • It would be really nice if there was a way to use cmd/serv.go directly, rather than having to call it like this. That would allow us to avoid re-escaping command args as they come in.
  • This now checks the incoming user against setting.SSH.BuiltinServerUser. I don't believe that was done before.

If anyone has a recommended test suite or things they'd like to see tested, please let me know and I will do my best.

@lunny lunny added the type/enhancement An improvement of existing functionality label May 5, 2018
@lunny
Copy link
Member

lunny commented May 5, 2018

CI failed.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 5, 2018
@codecov-io
Copy link

codecov-io commented May 5, 2018

Codecov Report

Merging #3896 into master will decrease coverage by 1.3%.
The diff coverage is 67.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3896      +/-   ##
==========================================
- Coverage   38.73%   37.42%   -1.31%     
==========================================
  Files         339      312      -27     
  Lines       49013    46323    -2690     
==========================================
- Hits        18983    17338    -1645     
+ Misses      27284    26509     -775     
+ Partials     2746     2476     -270
Impacted Files Coverage Δ
modules/ssh/ssh.go 63.9% <67.54%> (+8.42%) ⬆️
routers/api/v1/org/org.go 0% <0%> (-42.39%) ⬇️
models/issue_mail.go 12% <0%> (-41.85%) ⬇️
modules/util/compare.go 12.12% <0%> (-33.34%) ⬇️
models/mail.go 1.73% <0%> (-22.4%) ⬇️
models/repo_mirror.go 2.62% <0%> (-21.65%) ⬇️
modules/notification/ui/ui.go 58.92% <0%> (-21.56%) ⬇️
modules/mailer/mailer.go 3.93% <0%> (-18.12%) ⬇️
models/update.go 31.65% <0%> (-17.59%) ⬇️
routers/repo/issue_stopwatch.go 26.66% <0%> (-15.44%) ⬇️
... and 134 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 baffea1...179f2ad. Read the comment docs.

@belak
Copy link
Author

belak commented May 5, 2018

Sorry about that. Managed to figure out how to run drone locally and fixed the issues I saw - most notably, the BUILTIN_SSH_SERVER_USER option needs to be set and generated URLs use setting.SSH.BuiltinServerUser over setting.RunUser so there was a test which needed to be fixed.

modules/ssh/ssh.go Outdated Show resolved Hide resolved
@lunny lunny added this to the 1.x.x milestone May 7, 2018
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update to use dep

@lafriks lafriks modified the milestones: 1.x.x, 1.6.0 Jun 22, 2018
@belak
Copy link
Author

belak commented Jun 28, 2018

I've been really busy lately but I still plan on updating this. I don't know if I can give a specific date though.

Signed-off-by: Kaleb Elwert <belak@coded.io>
Signed-off-by: Kaleb Elwert <belak@coded.io>
Signed-off-by: Kaleb Elwert <belak@coded.io>
@belak
Copy link
Author

belak commented Aug 9, 2018

This has been updated to use dep, though there were some oddities with the Godep.lock file which seems like it added something for all existing deps as well. Not sure how that should be handled.

@techknowlogick
Copy link
Member

The lock file seems fine. Those additions are probably just from a newer version of dep.

@techknowlogick techknowlogick modified the milestones: 1.6.0, 1.7.0 Aug 28, 2018
@techknowlogick
Copy link
Member

@belak any update?

@belak
Copy link
Author

belak commented Nov 2, 2018

I'll get that change into gliderlabs/ssh which should let us have the same level of granularity with the internal algorithms (though I'm not 100% we need to expose that) and then fix up the Gopkg files. Is there anything else that needs to be done I'm missing?

Gopkg.toml Outdated Show resolved Hide resolved
@techknowlogick techknowlogick modified the milestones: 1.7.0, 1.8.0 Dec 19, 2018
@belak
Copy link
Author

belak commented Feb 12, 2019

My apologies, I completely forgot about this. I'd be happy to clean this up some time this week.

@techknowlogick
Copy link
Member

@belak no need to apologize, we appreciate your PR, and I'm the one who should be sorry as we should've done a better job of reviewing your PR sooner.

I'm just going through right now and doing some maintenance patches on older PRs so they aren't conflicted and make it easier for them to get reviewed, so I hope you don't mind that I pushed directly to your branch.

@techknowlogick
Copy link
Member

@belak I've just created #7250 which resolves the conflicts from this PR. I am going to close this PR, let me know if you want access to the other PR if you want to make any changes to it. Thank you for the work you did for this PR :)

@techknowlogick techknowlogick removed this from the 1.10.0 milestone Jun 19, 2019
techknowlogick added a commit that referenced this pull request Jul 7, 2019
resolves git conflicts from #3896 (credit to @belak, in case github doesn't keep original author during squash)

Co-Authored-By: Matti Ranta <techknowlogick@gitea.io>
jeffliu27 pushed a commit to jeffliu27/gitea that referenced this pull request Jul 18, 2019
resolves git conflicts from go-gitea#3896 (credit to @belak, in case github doesn't keep original author during squash)

Co-Authored-By: Matti Ranta <techknowlogick@gitea.io>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
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. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants