Ensure getDefaultKey always returns a valid key#12
Conversation
…refer user keyPath
…ig) can NEVER be an empty string
noorvir
left a comment
There was a problem hiding this comment.
Looks good except for ui error formatting
cmd/code.go
Outdated
| ui.Errorf("expected private key to be none empty string") | ||
| os.Exit(1) | ||
| } | ||
| if err != nil { | ||
| ui.Errorf("failed to get private key: %s", err) |
There was a problem hiding this comment.
The ui.Errof should be human consumable error (i.e. capitalized and only the info that the user needs to see).
There was a problem hiding this comment.
Great Ty will sort this
There was a problem hiding this comment.
So I'm tempted to say that the best solution to this is drop a line in ui.Errorf to ensure errors are human readable. I've updated the error here. What do you think?
cmd/ssh.go
Outdated
| ui.Errorf("expected private key to be none empty string") | ||
| os.Exit(1) | ||
| } | ||
| if err != nil { | ||
| ui.Errorf("failed to get private key: %s", err) |
There was a problem hiding this comment.
Same as above for error formatting
|
Updated @noorvir how about we add some handling in ui.Errorf to alphabetize the first alphabetical character of any error as a catch all before merging on approval? |
I'm not sure we want that. I think we want to be very specific about what we print to the user rather than wrapping internal errors and printing them to the user. So any |
| @@ -32,7 +32,7 @@ func Code(cmd *cobra.Command, args []string) error { | |||
| os.Exit(1) | |||
There was a problem hiding this comment.
Still missing the capitalization on the line above
There was a problem hiding this comment.
oh sorry - i should have caught this
Currently, the getDefaultKey method may return an empty string if no user key is specified. Really what we want is to prefer a specified user key and ensure that there is always some kind of key available for an instance, even if it means generating a new one. This way, even if a user ends up in a peculiar state where they attempt to SSH with an invalid key (due to tampering with the unweave managed/global keystore), they will at least receive a meaningful "bad public key" error in SSH.
This PR:
Bugfixes
no argument after keyword "identityfile". If you're still getting the error you will need to go to~/.unweave_global/ssh_configand manually delete all the SSH configs with no argument after the identity file keyword or just delete the whole file. SSH config really cares about whether it can parse the file at all