puppeth: handle encrypted ssh keys (closes #15442)

[bohendo]

It looks like the ssh functionality of puppeth depends on a local version of golang.org/x/crypto/ssh which has a couple methods removed compared to the original. I'm not sure why they were removed but I copy/pasted two of the missing methods back in:

• ParsePrivateKeyWithPassphrase
• ParseRawPrivateKeyWithPassphrase

Then, I added a check into the dial function defined in puppeth/ssh.go:80 to detect encrypted keys. If any are spotted, we'll ask for the password and then call the methods above to decrypt & parse them.

Now, puppeth is just a tiny bit smarter. Booyea!

[holiman]

LGTM, but I think it may be better to update the entire upstream file instead of copying individual methods.

[karalabe]

Thank for this!

Martin is right btw, these methods weren't deleted compared to the upstream crypto library, rather they didn't exist when we last updated the dependency (they were added this June golang/crypto@fea6c2c#diff-aab72d98d6b840106cb1ffed0390f951).

The "proper" way to pull in these changes is to update the dependency with the current upstream version. You can do that by installing govendor, and then asking it to update the dependency:

$ go get -u github.com/kardianos/govendor
$ govendor fetch golang.org/x/crypto/...

Note, do the second command from the go-ethereum repo root.

[karalabe]

You don't need the string case here, Error() already returns a string.

Also this is a very brittle way to check for the error as it could change at any moment, however I do notice that the crypto package does not have an expose error constant for this. I'd suggest leaving this check as you implemented for now, but opening a PR against the upstream repo to make the error a constant.

[karalabe]

Please no empty lines here :)

[bohendo]

So in general, no empty lines padding the beginning or end of blocks?

[karalabe]

no echo -> won't be echoed to keep it consistent with all other password read instances.

[karalabe]

Also Puppeth's "labels" generally take the form of questions, not statements, so I'd rather formulate this as:

fmt.Printf("What's the decryption password for the SSH key? (won't be echoed)\n> ")

[karalabe]

Perhaps instead of Bad SSH key, output Failed to decrypt SSH key here, since it better highlights the issue.

[karalabe]

Minor nitpicks and please update the entire crypto dep, not just a single file manually :)

[bohendo]

Thank you for the feedback!

Proposal: decrypting ssh keys is a common enough situation that we can use it as the default next-action rather than specifically making that silly, fragile if err == "specific string" check. Simpler & avoids upstream error handling issues.

Running govendor fetch golang.org/x/crypto/... means we also need to upgrade a unix dependency (golang.org/x/sys/unix). All in all, these two fetches made some really heavy (& for now, unnecessary) dependency additions. Instead, I ran govendor fetch golang.org/x/crypto/ssh. No extra fetches required & it gives us exactly what we need.

Is it best practice for me to squash my commits or leave that up to you?

[holiman]

LGTM; Peter can make the call regarding squashing

[karalabe]

Yup, I agree with the decryption fallback in case of any error! Regarding squashing, we usually try to keep 1 commit per significant change, so either you squash it, or we can on merge, doesn't matter, so you can leave it as is.

Regarding the crypto package update however, only partially updating a dependent repo seems weird to me because we end up with different commits on different paths. I'd really rather have the entire reporisoty bumped to the latest version; and if that entails more deps, so be it.

[bohendo]

Awesome, golang.org/x/crypto is fully upgraded and my commits have been squashed. I think we're good to go.

A recent commit removed the syscall import from cmd/puppeth/ssh.go so I followed suit and changed my int(syscall.Stdin) into int(os.Stdin.Fd()) like was done later in that same file.

[karalabe]

Pushed a tiny fix for the unconvert linter. Otherwise LGTM! Thanks for the PR!