Configurable SSH cipher suite #913

spacetourist · 2017-02-12T11:43:53Z

As requested, merging change from Gogs.io

lunny · 2017-02-12T11:51:01Z

LGTM

bkcsoft · 2017-02-12T14:54:07Z

conf/app.ini

@@ -110,6 +110,8 @@ SSH_PORT = 22
 SSH_LISTEN_PORT = %(SSH_PORT)s
 ; Root path of SSH directory, default is '~/.ssh', but you have to use '/home/git/.ssh'.
 SSH_ROOT_PATH =
+; Choose the ciphers to support for SSH connections


Please make it clear that this only affects Built-In SSH-server

; For built-in SSH server only, choose the ciphers to support for SSH connections, ; for system SSH this setting has no effect

bkcsoft · 2017-02-12T14:55:05Z

routers/init.go

-		ssh.Listen(setting.SSH.ListenHost, setting.SSH.ListenPort)
-		log.Info("SSH server started on %s:%v", setting.SSH.ListenHost, setting.SSH.ListenPort)
+               ssh.Listen(setting.SSH.ListenHost, setting.SSH.ListenPort, setting.SSH.ServerCiphers)
+               log.Info("SSH server started on %s:%v. Cipher list (%v)", setting.SSH.ListenHost, setting.SSH.ListenPort, setting.SSH.ServerCiphers)


ListenPort should use %d not %v

bkcsoft · 2017-02-12T14:57:27Z

Not exactly happy about having configs that only affect the build-in server, but for this it's hard to do otherwise 🙁

ethantkoenig · 2017-02-12T18:24:17Z

routers/init.go

@@ -75,7 +75,7 @@ func GlobalInit() {
 	checkRunMode()

 	if setting.InstallLock && setting.SSH.StartBuiltinServer {
-		ssh.Listen(setting.SSH.ListenHost, setting.SSH.ListenPort)
-		log.Info("SSH server started on %s:%v", setting.SSH.ListenHost, setting.SSH.ListenPort)
+               ssh.Listen(setting.SSH.ListenHost, setting.SSH.ListenPort, setting.SSH.ServerCiphers)


Please use tabs for indentation

aaronjwood · 2017-02-12T21:39:48Z

Can I suggest that you prioritize AES-GCM above everything else? I think an authenticated encryption mode should be first on everyone's list.

It might be nice to prevent the use of the arcfour suites entirely. Since most modern chips have the AES-NI instruction set to accelerate AES operations most people would see better performance too.

lunny · 2017-02-14T02:21:34Z

@aaronjwood Yes. I think the default values should be security enough.

lunny · 2017-02-24T06:27:41Z

It seems no response here, let's move it to v1.2

evantbyrne · 2017-02-25T19:29:23Z

According to the Go SSH documentation on ssh.Config,

// The allowed cipher algorithms. If unspecified then a sensible
// default is used.
Ciphers []string

So it might be best to leave that ini option undefined by default, unless this cipher set differs from the default Go one and there is a specific rationale for such.

tboerger · 2017-02-27T00:20:00Z

modules/setting/setting.go

@@ -618,6 +619,7 @@ please consider changing to GITEA_CUSTOM`)
 	}

 	SSH.RootPath = path.Join(homeDir, ".ssh")
+	SSH.ServerCiphers = sec.Key("SSH_SERVER_CIPHERS").Strings(",")


Requires a default value?

strk · 2017-03-16T12:05:20Z

It's somewhat ironic that Gogs is ahead of the community fork made to speed things up...
How about reviewers are less picky unless they have time to fix things themselves ?
I'll start with my LGTM here

lunny · 2017-03-17T02:21:58Z

As @evantbyrne said, keep the default as Go did maybe a good choice. @spacetourist maybe you can change this?

sapk · 2017-04-27T09:46:51Z

Any news ?

lunny · 2017-04-27T09:54:00Z

If @evantbyrne no response. I think maintainers could do something.
I think this PR needs two fixes:

add more comment as @cdslashetc said.
change the default value as @evantbyrne said

sapk · 2017-04-27T10:04:17Z

fix indentation

lafriks · 2017-10-18T21:44:55Z

@bkcsoft @ethantkoenig fixed

codecov-io · 2017-10-18T22:36:12Z

Codecov Report

Merging #913 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #913   +/-   ##
=======================================
  Coverage   26.91%   26.91%           
=======================================
  Files          87       87           
  Lines       17286    17286           
=======================================
  Hits         4652     4652           
  Misses      11955    11955           
  Partials      679      679

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 985a395...3b66029. Read the comment docs.

lafriks · 2017-10-21T13:06:18Z

Make LG-TM work again

ncwgf · 2017-10-21T16:27:13Z

This patch will report below error using SSH to git fetch who is not from fresh install
because conf/app.ini missed config SSH_SERVER_CIPHERS
problem solved by manually copy the setting from this commit to conf/app.ini

git fetch gitea
FATAL ERROR: Couldn't agree a client-to-server cipher (available: )
fatal: Could not read from remote repository.

Configurable SSH cipher suite

139a538

lunny added this to the 1.1.0 milestone Feb 12, 2017

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Feb 12, 2017

tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 12, 2017

bkcsoft suggested changes Feb 12, 2017

View reviewed changes

ethantkoenig suggested changes Feb 12, 2017

View reviewed changes

lunny added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Feb 24, 2017

lunny modified the milestones: 1.2.0, 1.1.0 Feb 24, 2017

tboerger reviewed Feb 27, 2017

View reviewed changes

lunny mentioned this pull request Mar 16, 2017

Add configurable SSH cipher suite #1284

Closed

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 16, 2017

lunny modified the milestones: 1.3.0, 1.2.0 Apr 28, 2017

lafriks added 4 commits October 19, 2017 00:36

Merge branch 'master' into master

9286f58

Update configuration file comment

5865a28

Add default in settings loading code

372a1c7

Fix fmt and log messsage

c3a8c48

Remove default from code as this could probably might not be good idea

99633ae

ethantkoenig approved these changes Oct 18, 2017

View reviewed changes

lafriks removed the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Oct 19, 2017

bkcsoft approved these changes Oct 21, 2017

View reviewed changes

Merge branch 'master' into master

3b66029

lafriks merged commit 7131c7d into go-gitea:master Oct 21, 2017

lunny mentioned this pull request Oct 22, 2017

Add default ssh ciphers #2761

Merged

tgurr mentioned this pull request Jan 10, 2020

Configurable SSL cipher suite #9691

Closed

2 tasks

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable SSH cipher suite #913

Configurable SSH cipher suite #913

spacetourist commented Feb 12, 2017

lunny commented Feb 12, 2017

bkcsoft Feb 12, 2017

cdslashetc Mar 1, 2017

bkcsoft Feb 12, 2017

bkcsoft commented Feb 12, 2017

ethantkoenig Feb 12, 2017

aaronjwood commented Feb 12, 2017

lunny commented Feb 14, 2017

lunny commented Feb 24, 2017

evantbyrne commented Feb 25, 2017 •

edited

Loading

tboerger Feb 27, 2017

strk commented Mar 16, 2017

lunny commented Mar 17, 2017

sapk commented Apr 27, 2017

lunny commented Apr 27, 2017

sapk commented Apr 27, 2017

lafriks commented Oct 18, 2017

codecov-io commented Oct 18, 2017 •

edited

Loading

lafriks commented Oct 21, 2017

ncwgf commented Oct 21, 2017

Configurable SSH cipher suite #913

Configurable SSH cipher suite #913

Conversation

spacetourist commented Feb 12, 2017

lunny commented Feb 12, 2017

bkcsoft Feb 12, 2017

Choose a reason for hiding this comment

cdslashetc Mar 1, 2017

Choose a reason for hiding this comment

bkcsoft Feb 12, 2017

Choose a reason for hiding this comment

bkcsoft commented Feb 12, 2017

ethantkoenig Feb 12, 2017

Choose a reason for hiding this comment

aaronjwood commented Feb 12, 2017

lunny commented Feb 14, 2017

lunny commented Feb 24, 2017

evantbyrne commented Feb 25, 2017 • edited Loading

tboerger Feb 27, 2017

Choose a reason for hiding this comment

strk commented Mar 16, 2017

lunny commented Mar 17, 2017

sapk commented Apr 27, 2017

lunny commented Apr 27, 2017

sapk commented Apr 27, 2017

lafriks commented Oct 18, 2017

codecov-io commented Oct 18, 2017 • edited Loading

Codecov Report

lafriks commented Oct 21, 2017

ncwgf commented Oct 21, 2017

evantbyrne commented Feb 25, 2017 •

edited

Loading

codecov-io commented Oct 18, 2017 •

edited

Loading