Provide more meaningful error message when adding SSH keys fails

[wcarson]

• Gitea version (or commit ref): 1.4.1
• Git version: 2.16.2
• Operating system: Windows Server 2016
• Database (use [x]):
  • PostgreSQL
  • MySQL
  • MSSQL
  • SQLite
• Can you reproduce the bug at https://try.gitea.io:
  • Yes (provide example URL)
  • No
  • Not relevant
• Log gist:

Description

When trying to add a SSH key to a user profile, any type of failure with the ssh-keygen command returns a generic HTTP 500 error to the UI (screenshot attached). In the log you can see what the actual error was, but none of this info is conveyed to the end user.

Reason: Path to ssh-keygen not configured correctly
2018/05/16 17:00:23 [...ters/user/setting.go:430 SettingsKeysPost()] [E] AddPublicKey: 'ssh-keygen -lf C:\Windows\TEMP\gitea_keytest659272622' failed with error 'exec: "ssh-keygen": executable file not found in %PATH%':

Reason: SSH key is not a valid public key - OR - the key is password protected
2018/05/16 17:04:14 [...ters/user/setting.go:430 SettingsKeysPost()] [E] AddPublicKey: 'ssh-keygen -lf C:\Windows\TEMP\gitea_keytest763165203' failed with error 'exec(4:AddPublicKey) failed: exit status 255(<nil>) stdout: stderr: C:\\Windows\\TEMP\\gitea_keytest763165203 is not a public key file. ': C:\\Windows\\TEMP\\gitea_keytest763165203 is not a public key file.

It would be helpful to have a better error message returned to the user instead of a generic HTTP 500 error (e.g. "Invalid public key or key is password protected", "Path to ssh-keygen is invalid. Contact administrator", etc.)

Screenshots

[lunny]

Both the error message should not be displayed in the user's UI for security reason.

[wcarson]

I agree that level of detail should not be in the user's UI, but even a general error would be better than generic 500 (e.g. "Invalid key", "Invalid SSH configuration. Contact administrator"). That level of detail isn't a security issue and provides a better user experience.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 weeks. Thank you for your contributions.

[zeripath]

Sorry stale, it's a real bug

[pbodnar]

Reason: SSH key is not a valid public key - OR - the key is password protected

@wcarson, can you please explain how a public key can be password protected? Isn't this a typo?

Also, please note that you can get the error is not a public key file from OpenSSH executables even when the key is a perfectly valid key file that just doesn't meet a hard-coded key size limit since OpenSSH version 7.6! See bug 3127 I've just created in their bugtracker.

Both the error message should not be displayed in the user's UI for security reason.

@lunny, can you please elaborate on this? What exactly is wrong on telling the user the key he uploads is not valid or accepted by the server?

[guillep2k]

@pbodnar By policy we never allow the users to get more information than they need about the system, like paths, versions, etc. What is acceptable is to make our best effort to parse the message and present the user with a sanitized version of it, but that's complicated because tools might change their message format in different versions, or under different locales.

[pbodnar]

@guillep2k, thank you for the clarification. So is it OK to display a meaningful message at least in the situation when you are actually able to call the ssh-keygen and it returns an error? Or is it too hard to recognize that in Go application?

If one looks at Github as the inspiration, it returns this message: "Key is invalid. You must supply a key in OpenSSH public key format". Moreover, it also displays a hint in the key field that is surely helpful for newbies who don't necessarily always choose the right format for upload.

[zeripath]

Hmm... I actually think this is already fixed.

The first case is a server configuration issue for which a 500: Internal Server Error is completely reasonable. The second is already handled I think.

@pbodnar have you actually been able to replicate this issue?

[pbodnar]

@zeripath, yes, the issue is still reproducible - you can try it for yourself on https://try.gitea.io/, which I suppose is the latest testable Gitea version. Here are the various inputs tested:

1. Supply a valid key of length at least 1024 bits -> accepted.
2. Supply a (syntactically) valid key of length of 1023 bits (or possibly shorter) -> still returns error 500. As explained above, you should see the misleading is not a public key file error in the logs.
3. Supply an invalid key, e. g. supply an invalid algorithm name or change a letter in the encoded key -> correctly returns appropriate errors:
   3.1 Can not verify your SSH key: key type and content does not match: ssh-rsb - ssh-rsa
   3.2 Can not verify your SSH key: extractTypeFromBase64Key: invalid key format: not enough length 263

[zeripath]

Ok so mostly it is fixed - just this second case. Could you provide the logs for the second case.

[pbodnar]

@zeripath, there is actually nothing interesting logged apart from the error itself that is still nearly the same as in 2018, here it goes (paths shortened):

2020/03/13 08:00:19 ...user/setting/keys.go:96:KeysPost() [E] AddPublicKey: calcFingerprintSSHKeygen: 'ssh-keygen -lf C:\Temp\gitea_keytest395065083' failed with error 'exec(8:AddPublicKey) failed: exit status 255(<nil>) stdout: stderr: C:\\Temp\\gitea_keytest395065083 is not a public key file.\01503d ': C:\\Temp\\gitea_keytest395065083 is not a public key file.\01503d