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

Provide more meaningful error message when adding SSH keys fails #3985

Closed
2 of 7 tasks
wcarson opened this issue May 17, 2018 · 11 comments · Fixed by #10863
Closed
2 of 7 tasks

Provide more meaningful error message when adding SSH keys fails #3985

wcarson opened this issue May 17, 2018 · 11 comments · Fixed by #10863
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented topic/ui Change the appearance of the Gitea UI type/bug

Comments

@wcarson
Copy link

wcarson commented May 17, 2018

  • 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

sshkey-500error

@lunny
Copy link
Member

lunny commented May 18, 2018

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

@wcarson
Copy link
Author

wcarson commented May 18, 2018

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
Copy link

stale bot commented Jan 26, 2019

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.

@stale stale bot added the issue/stale label Jan 26, 2019
@zeripath zeripath added topic/ui Change the appearance of the Gitea UI issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented labels Jan 26, 2019
@zeripath
Copy link
Contributor

Sorry stale, it's a real bug

@lunny lunny removed the issue/stale label Feb 6, 2019
@pbodnar
Copy link

pbodnar commented Feb 29, 2020

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
Copy link
Member

@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
Copy link

pbodnar commented Feb 29, 2020

@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
Copy link
Contributor

zeripath commented Mar 5, 2020

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?

@zeripath zeripath added issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail and removed issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented labels Mar 5, 2020
@pbodnar
Copy link

pbodnar commented Mar 10, 2020

@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
Copy link
Contributor

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

@pbodnar
Copy link

pbodnar commented Mar 13, 2020

@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

zeripath added a commit to zeripath/gitea that referenced this issue Mar 28, 2020
@zeripath zeripath added issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented and removed issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail labels Mar 28, 2020
lafriks pushed a commit that referenced this issue Mar 28, 2020
…10863)

* Use ErrKeyUnableToVerify if fail to calc fingerprint in ssh-keygen

Fix #3985

Signed-off-by: Andrew Thornton <[email protected]>

* Pass up the unable to verify
@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
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented topic/ui Change the appearance of the Gitea UI type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants