From 14ccebe75df1669beb78ab23125d0c9de3a8d50d Mon Sep 17 00:00:00 2001 From: "Leon M. Busch-George" Date: Wed, 29 Mar 2023 13:47:50 +0200 Subject: [PATCH 1/3] Prefer native parser for SSH public key parsing Without this patch, the setting SSH.StartBuiltinServer decides whether the native (Go) implementation is used rather than calling 'ssh-keygen'. It's possible for 'using ssh-keygen' and 'using the built-in server' to be independent. In fact, the gitea rootless container doesn't ship ssh-keygen and can be configured to use the host's SSH server - which will cause the public key parsing mechanism to break. This commit changes the decision to be based on SSH.KeygenPath instead. Any existing configurations with a custom KeygenPath set will continue to function. The new default value of '' selects the native version. The downside of this approach is that anyone who has relying on plain 'ssh-keygen' to have special properties will now be using the native version instead. I assume the exec-variant is only there because /x/crypto/ssh didn't support ssh-ed25519 until 2016. I don't see any other reason for using it. Fixes #23363 Co-authored-by: wxiaoguang Signed-off-by: Leon M. Busch-George --- custom/conf/app.example.ini | 4 ++-- docs/content/doc/administration/config-cheat-sheet.en-us.md | 2 +- models/asymkey/ssh_key_parse.go | 2 +- modules/setting/ssh.go | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 0543c85d50daa..88297fe0d975e 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -186,8 +186,8 @@ RUN_MODE = ; prod ;; default is the system temporary directory. ;SSH_KEY_TEST_PATH = ;; -;; Path to ssh-keygen, default is 'ssh-keygen' which means the shell is responsible for finding out which one to call. -;SSH_KEYGEN_PATH = ssh-keygen +;; Use `ssh-keygen` to parse public SSH keys. The value is passed to the shell. By default, Gitea does the parsing itself. +;SSH_KEYGEN_PATH = ;; ;; Enable SSH Authorized Key Backup when rewriting all keys, default is true ;SSH_AUTHORIZED_KEYS_BACKUP = true diff --git a/docs/content/doc/administration/config-cheat-sheet.en-us.md b/docs/content/doc/administration/config-cheat-sheet.en-us.md index f976458f2932a..76952df40e751 100644 --- a/docs/content/doc/administration/config-cheat-sheet.en-us.md +++ b/docs/content/doc/administration/config-cheat-sheet.en-us.md @@ -345,7 +345,7 @@ The following configuration set `Content-Type: application/vnd.android.package-a - `SSH_SERVER_MACS`: **hmac-sha2-256-etm@openssh.com, hmac-sha2-256, hmac-sha1**: For the built-in SSH server, choose the MACs to support for SSH connections, for system SSH this setting has no effect - `SSH_SERVER_HOST_KEYS`: **ssh/gitea.rsa, ssh/gogs.rsa**: For the built-in SSH server, choose the keypairs to offer as the host key. The private key should be at `SSH_SERVER_HOST_KEY` and the public `SSH_SERVER_HOST_KEY.pub`. Relative paths are made absolute relative to the `APP_DATA_PATH`. If no key exists a 4096 bit RSA key will be created for you. - `SSH_KEY_TEST_PATH`: **/tmp**: Directory to create temporary files in when testing public keys using ssh-keygen, default is the system temporary directory. -- `SSH_KEYGEN_PATH`: **ssh-keygen**: Path to ssh-keygen, default is 'ssh-keygen' which means the shell is responsible for finding out which one to call. +- `SSH_KEYGEN_PATH`: **\**: Use `ssh-keygen` to parse public SSH keys. The value is passed to the shell. By default, Gitea does the parsing itself. - `SSH_EXPOSE_ANONYMOUS`: **false**: Enable exposure of SSH clone URL to anonymous visitors, default is false. - `SSH_PER_WRITE_TIMEOUT`: **30s**: Timeout for any write to the SSH connections. (Set to -1 to disable all timeouts.) diff --git a/models/asymkey/ssh_key_parse.go b/models/asymkey/ssh_key_parse.go index 7e61e61dae553..012a10fe17a57 100644 --- a/models/asymkey/ssh_key_parse.go +++ b/models/asymkey/ssh_key_parse.go @@ -179,7 +179,7 @@ func CheckPublicKeyString(content string) (_ string, err error) { keyType string length int ) - if setting.SSH.StartBuiltinServer { + if len(setting.SSH.KeygenPath) == 0 { fnName = "SSHNativeParsePublicKey" keyType, length, err = SSHNativeParsePublicKey(content) } else { diff --git a/modules/setting/ssh.go b/modules/setting/ssh.go index e8796f98d6f14..a5a9da0b3676b 100644 --- a/modules/setting/ssh.go +++ b/modules/setting/ssh.go @@ -58,7 +58,7 @@ var SSH = struct { ServerCiphers: []string{"chacha20-poly1305@openssh.com", "aes128-ctr", "aes192-ctr", "aes256-ctr", "aes128-gcm@openssh.com", "aes256-gcm@openssh.com"}, ServerKeyExchanges: []string{"curve25519-sha256", "ecdh-sha2-nistp256", "ecdh-sha2-nistp384", "ecdh-sha2-nistp521", "diffie-hellman-group14-sha256", "diffie-hellman-group14-sha1"}, ServerMACs: []string{"hmac-sha2-256-etm@openssh.com", "hmac-sha2-256", "hmac-sha1"}, - KeygenPath: "ssh-keygen", + KeygenPath: "", MinimumKeySizeCheck: true, MinimumKeySizes: map[string]int{"ed25519": 256, "ed25519-sk": 256, "ecdsa": 256, "ecdsa-sk": 256, "rsa": 2047}, ServerHostKeys: []string{"ssh/gitea.rsa", "ssh/gogs.rsa"}, @@ -134,7 +134,7 @@ func loadSSHFrom(rootCfg ConfigProvider) { } } - SSH.KeygenPath = sec.Key("SSH_KEYGEN_PATH").MustString("ssh-keygen") + SSH.KeygenPath = sec.Key("SSH_KEYGEN_PATH").String() SSH.Port = sec.Key("SSH_PORT").MustInt(22) SSH.ListenPort = sec.Key("SSH_LISTEN_PORT").MustInt(SSH.Port) SSH.UseProxyProtocol = sec.Key("SSH_SERVER_USE_PROXY_PROTOCOL").MustBool(false) From 56e072019494acbcda339e2fce3fd51d1e0e9741 Mon Sep 17 00:00:00 2001 From: "Leon M. Busch-George" Date: Mon, 10 Apr 2023 21:21:08 +0200 Subject: [PATCH 2/3] Add test for native SSH key parser Without this patch, the native SSH key parser is not tested. Signed-off-by: Leon M. Busch-George --- models/asymkey/ssh_key_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/models/asymkey/ssh_key_test.go b/models/asymkey/ssh_key_test.go index afd79ae6ded50..5378ef66ba29a 100644 --- a/models/asymkey/ssh_key_test.go +++ b/models/asymkey/ssh_key_test.go @@ -57,6 +57,14 @@ func Test_SSHParsePublicKey(t *testing.T) { assert.Equal(t, tc.keyType, keyTypeK) assert.EqualValues(t, tc.length, lengthK) }) + t.Run("SSHParseKeyNative", func(t *testing.T) { + keyTypeK, lengthK, err := SSHNativeParsePublicKey(tc.content) + if err != nil { + assert.Fail(t, "%v", err) + } + assert.Equal(t, tc.keyType, keyTypeK) + assert.EqualValues(t, tc.length, lengthK) + }) }) } } From bfdf35cf55da89275e2ba45d79052fd93df4ec10 Mon Sep 17 00:00:00 2001 From: "Leon M. Busch-George" Date: Mon, 10 Apr 2023 21:23:53 +0200 Subject: [PATCH 3/3] Supply default KeygenPath in SSH key parser This allows the parser to be used even if it's not configured (useful for automatic tests). Signed-off-by: Leon M. Busch-George --- models/asymkey/ssh_key_parse.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/models/asymkey/ssh_key_parse.go b/models/asymkey/ssh_key_parse.go index 012a10fe17a57..94b1cf112b56c 100644 --- a/models/asymkey/ssh_key_parse.go +++ b/models/asymkey/ssh_key_parse.go @@ -285,7 +285,12 @@ func SSHKeyGenParsePublicKey(key string) (string, int, error) { } }() - stdout, stderr, err := process.GetManager().Exec("SSHKeyGenParsePublicKey", setting.SSH.KeygenPath, "-lf", tmpName) + keygenPath := setting.SSH.KeygenPath + if len(keygenPath) == 0 { + keygenPath = "ssh-keygen" + } + + stdout, stderr, err := process.GetManager().Exec("SSHKeyGenParsePublicKey", keygenPath, "-lf", tmpName) if err != nil { return "", 0, fmt.Errorf("fail to parse public key: %s - %s", err, stderr) }