Obscure error messages with incorrect private key passphrases

karlhorky commented 7 years ago

Given an encrypted private key and an incorrect passphrase, obscure error messages are thrown from the node-asn1 library:

Long, non-empty, incorrect passphrase

InvalidAsn1Error: encoding too long
    at newInvalidAsn1Error (/Users/k/p/lagoon/cli/node_modules/asn1/lib/ber/errors.js:7:13)
    at Reader.readLength (/Users/k/p/lagoon/cli/node_modules/asn1/lib/ber/reader.js:102:13)
    at Reader.readSequence (/Users/k/p/lagoon/cli/node_modules/asn1/lib/ber/reader.js:135:16)
    at genPublicKey (/Users/k/p/lagoon/cli/node_modules/ssh2-streams/lib/utils.js:429:19)
    at Client.connect (/Users/k/p/lagoon/cli/node_modules/ssh2/lib/client.js:239:29)
    at /Users/k/p/lagoon/cli/src/util/ssh.js:42:16
    at Promise (<anonymous>)
    at sshConnect$ (/Users/k/p/lagoon/cli/src/util/ssh.js:31:10)
    at tryCatch (/Users/k/p/lagoon/cli/node_modules/regenerator-runtime/runtime.js:65:40)
    at Generator.invoke [as _invoke] (/Users/k/p/lagoon/cli/node_modules/regenerator-runtime/runtime.js:303:22)

Empty, incorrect passphrase

InvalidAsn1Error: Expected 0x2: got 0xa3
    at newInvalidAsn1Error (/Users/k/p/cli/node_modules/asn1/lib/ber/errors.js:7:13)
    at Reader._readTag (/Users/k/p/cli/node_modules/asn1/lib/ber/reader.js:229:11)
    at Reader.readInt (/Users/k/p/cli/node_modules/asn1/lib/ber/reader.js:145:15)
    at genPublicKey (/Users/k/p/cli/node_modules/ssh2-streams/lib/utils.js:437:19)
    at Client.connect (/Users/k/p/cli/node_modules/ssh2/lib/client.js:239:29)
    at /Users/k/p/cli/src/util/ssh.js:41:16
    at Promise (<anonymous>)
    at sshConnect$ (/Users/k/p/cli/src/util/ssh.js:30:10)
    at tryCatch (/Users/k/p/cli/node_modules/regenerator-runtime/runtime.js:65:40)
    at Generator.invoke [as _invoke] (/Users/k/p/cli/node_modules/regenerator-runtime/runtime.js:303:22)

This appears to be as a result of asnReader.readSequence() and asnReader.readInt() throwing here and here and not being caught in lib/utils.js.

Would it make sense to wrap these (and maybe other calls to node-asn1 methods) in try / catch blocks?

mofux commented 7 years ago

At the moment, in the real world, you have to wrap the client.connect call into a try / catch block anyway, because there are other scenarios where the client will throw synchronously (for example if the username is empty), even if you attached an error handler to it (client.on("error", ...)). But I agree that it would be more consistent if those synchronous errors would be thrown through the client error handler.

@mscdex I could create a PR that essentially surrounds the client.connect code with a try/catch block and emits the "error" event rather than throwing directly. I would make sure that the error message follows the current design and throws with a meaningful "level" property. Any objections?

mscdex commented 7 years ago

The problem is that there is no way to distinguish between an encrypted key and a corrupt unencrypted key, so I'm not sure that there is much that can be done here.

karlhorky commented 7 years ago

I think even the messages that are in lib/utils.js already are much better than the messages from node-asn1:

      errMsg = 'Malformed private key (expected sequence)';
      if (keyInfo._decrypted)
        errMsg += '. Bad passphrase?';

Maybe these can just be thrown in a catch block?

mscdex / ssh2-streams

Obscure error messages with incorrect private key passphrases #76