Skip to content

Commit 7a8a4f5

Browse files
authored
Prefer native parser for SSH public key parsing (#23798)
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 so it might be an acceptable risk. Fixes #23363 EDIT: this message was garbled when I tried to get the commit description back in.. Trying to reconstruct it: ## ⚠️ BREAKING ⚠️ Users who don't have SSH.KeygenPath explicitly set and rely on the ssh-keygen binary need to set SSH.KeygenPath to 'ssh-keygen' in order to be able to continue using it for public key parsing. There was something else but I can't remember at the moment. EDIT2: It was about `make test` and `make lint`. Can't get them to run. To reproduce the issue, I installed `golang` in `docker.io/node:16` and got: ``` ... go: mvdan.cc/xurls/[email protected]: unknown revision mvdan.cc/xurls/v2.4.0 go: gotest.tools/[email protected]: unknown revision gotest.tools/v3.4.0 ... go: gotest.tools/[email protected]: unknown revision gotest.tools/v3.0.3 ... go: error loading module requirements ``` Signed-off-by: Leon M. Busch-George <[email protected]>
1 parent ef7fd78 commit 7a8a4f5

File tree

5 files changed

+20
-7
lines changed

5 files changed

+20
-7
lines changed

custom/conf/app.example.ini

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -186,8 +186,8 @@ RUN_MODE = ; prod
186186
;; default is the system temporary directory.
187187
;SSH_KEY_TEST_PATH =
188188
;;
189-
;; Path to ssh-keygen, default is 'ssh-keygen' which means the shell is responsible for finding out which one to call.
190-
;SSH_KEYGEN_PATH = ssh-keygen
189+
;; Use `ssh-keygen` to parse public SSH keys. The value is passed to the shell. By default, Gitea does the parsing itself.
190+
;SSH_KEYGEN_PATH =
191191
;;
192192
;; Enable SSH Authorized Key Backup when rewriting all keys, default is true
193193
;SSH_AUTHORIZED_KEYS_BACKUP = true

docs/content/doc/administration/config-cheat-sheet.en-us.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,7 @@ The following configuration set `Content-Type: application/vnd.android.package-a
345345
- `SSH_SERVER_MACS`: **[email protected], 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
346346
- `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.
347347
- `SSH_KEY_TEST_PATH`: **/tmp**: Directory to create temporary files in when testing public keys using ssh-keygen, default is the system temporary directory.
348-
- `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.
348+
- `SSH_KEYGEN_PATH`: **\<empty\>**: Use `ssh-keygen` to parse public SSH keys. The value is passed to the shell. By default, Gitea does the parsing itself.
349349
- `SSH_EXPOSE_ANONYMOUS`: **false**: Enable exposure of SSH clone URL to anonymous visitors, default is false.
350350
- `SSH_PER_WRITE_TIMEOUT`: **30s**: Timeout for any write to the SSH connections. (Set to
351351
-1 to disable all timeouts.)

models/asymkey/ssh_key_parse.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ func CheckPublicKeyString(content string) (_ string, err error) {
179179
keyType string
180180
length int
181181
)
182-
if setting.SSH.StartBuiltinServer {
182+
if len(setting.SSH.KeygenPath) == 0 {
183183
fnName = "SSHNativeParsePublicKey"
184184
keyType, length, err = SSHNativeParsePublicKey(content)
185185
} else {
@@ -285,7 +285,12 @@ func SSHKeyGenParsePublicKey(key string) (string, int, error) {
285285
}
286286
}()
287287

288-
stdout, stderr, err := process.GetManager().Exec("SSHKeyGenParsePublicKey", setting.SSH.KeygenPath, "-lf", tmpName)
288+
keygenPath := setting.SSH.KeygenPath
289+
if len(keygenPath) == 0 {
290+
keygenPath = "ssh-keygen"
291+
}
292+
293+
stdout, stderr, err := process.GetManager().Exec("SSHKeyGenParsePublicKey", keygenPath, "-lf", tmpName)
289294
if err != nil {
290295
return "", 0, fmt.Errorf("fail to parse public key: %s - %s", err, stderr)
291296
}

models/asymkey/ssh_key_test.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,14 @@ func Test_SSHParsePublicKey(t *testing.T) {
5757
assert.Equal(t, tc.keyType, keyTypeK)
5858
assert.EqualValues(t, tc.length, lengthK)
5959
})
60+
t.Run("SSHParseKeyNative", func(t *testing.T) {
61+
keyTypeK, lengthK, err := SSHNativeParsePublicKey(tc.content)
62+
if err != nil {
63+
assert.Fail(t, "%v", err)
64+
}
65+
assert.Equal(t, tc.keyType, keyTypeK)
66+
assert.EqualValues(t, tc.length, lengthK)
67+
})
6068
})
6169
}
6270
}

modules/setting/ssh.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ var SSH = struct {
5858
ServerCiphers: []string{"[email protected]", "aes128-ctr", "aes192-ctr", "aes256-ctr", "[email protected]", "[email protected]"},
5959
ServerKeyExchanges: []string{"curve25519-sha256", "ecdh-sha2-nistp256", "ecdh-sha2-nistp384", "ecdh-sha2-nistp521", "diffie-hellman-group14-sha256", "diffie-hellman-group14-sha1"},
6060
ServerMACs: []string{"[email protected]", "hmac-sha2-256", "hmac-sha1"},
61-
KeygenPath: "ssh-keygen",
61+
KeygenPath: "",
6262
MinimumKeySizeCheck: true,
6363
MinimumKeySizes: map[string]int{"ed25519": 256, "ed25519-sk": 256, "ecdsa": 256, "ecdsa-sk": 256, "rsa": 2047},
6464
ServerHostKeys: []string{"ssh/gitea.rsa", "ssh/gogs.rsa"},
@@ -134,7 +134,7 @@ func loadSSHFrom(rootCfg ConfigProvider) {
134134
}
135135
}
136136

137-
SSH.KeygenPath = sec.Key("SSH_KEYGEN_PATH").MustString("ssh-keygen")
137+
SSH.KeygenPath = sec.Key("SSH_KEYGEN_PATH").String()
138138
SSH.Port = sec.Key("SSH_PORT").MustInt(22)
139139
SSH.ListenPort = sec.Key("SSH_LISTEN_PORT").MustInt(SSH.Port)
140140
SSH.UseProxyProtocol = sec.Key("SSH_SERVER_USE_PROXY_PROTOCOL").MustBool(false)

0 commit comments

Comments
 (0)