Project Status Update

Question

Project Status Update

mscdex opened this issue 4 years ago · 21 comments

For the past few months I've been working on a large rewrite of ssh2. The
original reasons for this were performance-related and design-related.

The performance reason came about when I recalled performing some profiling about
a year or so ago and found that the node crypto stuff was dominating the time
spent by the process. Unfortunately I didn't try to reproduce these results
before I started working on reworking the crypto code in ssh2 because I found
out afterwards I was unable to reproduce the profiling results, no matter the
configuration or situation. In hindsight, this makes sense because in most cases
the network should always be the bottleneck.

The design reason came about because I was wanting to add support for
chacha20-poly1305 and doing so was very difficult partly due to how packets were
being parsed and partly because of some confusion around how OpenSSL's
chacha20-poly1305 implementation works (which incidentally is not compatible
with OpenSSH's chacha20-poly1305 implementation).

Just recently I've merged all of my work into the master branch. I would not
recommend using this in production yet, however I am looking for people to give
the code a try as I know between all of the changes and the feature additions
the code coverage is surely to have gone down (this is something that will need
to be remedied in due time). Additionally the documentation may not be 100% yet
either.

The end goal is that once I feel comfortable (especially after sufficient
testing has been performed), I will be releasing this new code as v1.0.0 and
v0.8.x will remain in a separate branch for historical purposes (I do not have
plans to maintain it going forward).

Here's a rundown of some various things I jotted down as I was working on this:

(Potentially) Breaking changes

node v10.16.0 is the new minimum required/supported node version. This allows us to
both use newer code features present in V8 and to remove various polyfills for
older node versions. There are also some other (crypto-related IIRC?) changes
that led to this specific version being the new minimum.
No more internal backpressure (no more 'continue' event, etc.). Reasoning:
- Node never standardized the mechanism that made it possible
  - nodejs/readable-stream#111
  - Specifically no 'drain' equivalent for readable.push() returning
    false
- Very small number of inquiries about the mechanism since its inception
  seemed to indicate few people paid attention to it anyway
- It complicated the codebase
- Along with streams in general, it was the source of various compatibility
  issues with node core over the years
- Since the replacement of the "parser stream" (ssh2-streams) with a
  non-stream-based parser, this would make handling backpressure even
  trickier to implement
SFTP_STATUS_CODE and SFTP_OPEN_MODE are now moved to
the utils.sftp exported namespace as STATUS_CODE and OPEN_MODE
respectively
Client: hostVerifier() now passed a Buffer containing the raw host key
when no (valid) hostHash is set
Client: hostVerifier() will now be called every time a handshake occurs.
This is because the server can potentially change host keys later on
during a rekey.
Client: an error will now be emitted when the connection is severed before
the handshake begins
Server: algorithms.serverHostKey is now mostly ignored, in general the
order of the host key algorithms now comes directly from the order of the
server host keys that are passed in. However, for RSA keys, the order of
algorithms.serverHostKey is used to determine the order of the various
hashing algorithms available (ssh-rsa for SHA1, rsa-sha2-256 for SHA256,
and rsa-sha2-512 for SHA512). If ssh-rsa is not in the list, it is given
the lowest priority amongst the other possible hashing algorithms.
Server: The sigAlgo property of publickey and hostbased authentication
contexts has been removed. It is recommended to instead use the simple verify()
method of keys returned by the parseKey() utility for verifying signatures instead
of using node's crypto API directly to perform the same task.
Server: 'x11' event's info.cookie is now a Buffer instead of hex string

New features

Optional binding for improved crypto performance and resource usage
(in-place encrypt/decrypt also helps to reduce memory consumption and GC)
- node v10.19.0 on i7-3770k
  - 1KB payload
    chacha20-poly1305 encrypt (Native -- Poly1305 WASM) (1024) x 66,964 ops/sec ±1.35% (81 runs sampled)
    chacha20-poly1305 encrypt (Binding) (1024) x 501,103 ops/sec ±0.35% (94 runs sampled)
    
    aes128-gcm encrypt (Native) (1024) x 132,410 ops/sec ±1.32% (85 runs sampled)
    aes128-gcm encrypt (Binding) (1024) x 1,024,079 ops/sec ±0.22% (89 runs sampled)
    
    aes128-ctr/hmac-sha2-256 encrypt (Native) (1024) x 101,521 ops/sec ±2.44% (88 runs sampled)
    aes128-ctr/hmac-sha2-256 encrypt (Binding) (1024) x 255,214 ops/sec ±1.69% (93 runs sampled)
  - 32KB payload (approx. max payload size for single packet)
    chacha20-poly1305 encrypt (Native -- Poly1305 WASM) (32768) x 12,792 ops/sec ±1.78% (86 runs sampled)
    chacha20-poly1305 encrypt (Binding) (32768) x 32,126 ops/sec ±0.20% (96 runs sampled)
    
    aes128-gcm encrypt (Native) (32768) x 31,189 ops/sec ±0.78% (91 runs sampled)
    aes128-gcm encrypt (Binding) (32768) x 44,914 ops/sec ±0.40% (97 runs sampled)
    
    aes128-ctr/hmac-sha2-256 encrypt (Native) (32768) x 9,559 ops/sec ±1.72% (88 runs sampled)
    aes128-ctr/hmac-sha2-256 encrypt (Binding) (32768) x 10,453 ops/sec ±0.77% (95 runs sampled)
- node v14.2.0 on Odroid-C4 (ARM Cortex-A55)
  - 1KB payload
    chacha20-poly1305 encrypt (Native -- Poly1305 WASM) (1024) x 9,767 ops/sec ±7.19% (63 runs sampled)
    chacha20-poly1305 encrypt (Binding) (1024) x 108,616 ops/sec ±2.18% (81 runs sampled)
    
    aes128-gcm encrypt (Native) (1024) x 22,200 ops/sec ±9.19% (67 runs sampled)
    aes128-gcm encrypt (Binding) (1024) x 302,560 ops/sec ±1.58% (75 runs sampled)
    
    aes128-ctr/hmac-sha2-256 encrypt (Native) (1024) x 26,812 ops/sec ±11.08% (65 runs sampled)
    aes128-ctr/hmac-sha2-256 encrypt (Binding) (1024) x 202,023 ops/sec ±3.15% (78 runs sampled)
  - 32KB payload
    chacha20-poly1305 encrypt (Native -- Poly1305 WASM) (32768) x 2,536 ops/sec ±2.29% (77 runs sampled)
    chacha20-poly1305 encrypt (Binding) (32768) x 8,407 ops/sec ±0.05% (81 runs sampled)
    
    aes128-gcm encrypt (Native) (32768) x 10,395 ops/sec ±3.59% (76 runs sampled)
    aes128-gcm encrypt (Binding) (32768) x 19,194 ops/sec ±0.04% (81 runs sampled)
    
    aes128-ctr/hmac-sha2-256 encrypt (Native) (32768) x 8,660 ops/sec ±3.74% (73 runs sampled)
    aes128-ctr/hmac-sha2-256 encrypt (Binding) (32768) x 13,534 ops/sec ±0.11% (81 runs sampled)
- node v12.15.0 on Odroid-C2 (ARM Cortex-A53)
  - 1KB payload
    chacha20-poly1305 encrypt (Native -- Poly1305 WASM) (1024) x 10,398 ops/sec ±8.30% (62 runs sampled)
    chacha20-poly1305 encrypt (Binding) (1024) x 50,428 ops/sec ±0.70% (74 runs sampled)
    
    aes128-gcm encrypt (Native) (1024) x 13,855 ops/sec ±5.09% (68 runs sampled)
    aes128-gcm encrypt (Binding) (1024) x 26,351 ops/sec ±0.73% (75 runs sampled)
    
    aes128-ctr/hmac-sha2-256 encrypt (Native) (1024) x 13,712 ops/sec ±5.43% (72 runs sampled)
    aes128-ctr/hmac-sha2-256 encrypt (Binding) (1024) x 23,959 ops/sec ±0.50% (77 runs sampled)
  - 32KB payload
    chacha20-poly1305 encrypt (Native -- Poly1305 WASM) (32768) x 1,776 ops/sec ±1.85% (75 runs sampled)
    chacha20-poly1305 encrypt (Binding) (32768) x 2,327 ops/sec ±0.12% (80 runs sampled)
    
    aes128-gcm encrypt (Native) (32768) x 881 ops/sec ±0.90% (76 runs sampled)
    aes128-gcm encrypt (Binding) (32768) x 928 ops/sec ±0.04% (79 runs sampled)
    
    aes128-ctr/hmac-sha2-256 encrypt (Native) (32768) x 825 ops/sec ±0.72% (77 runs sampled)
    aes128-ctr/hmac-sha2-256 encrypt (Binding) (32768) x 871 ops/sec ±0.22% (79 runs sampled)
Support for rsa-sha2-256 and rsa-sha2-512 server host key algorithms
Support for -etm HMACs
Support for chacha20-poly1305@openssh.com
- Two implementations:
  - chacha20 (binding) + poly1305 (binding)
  - chacha20 (from node core) + poly1305 WASM
- Priority in default cipher list varies depending on CPU and binding availability. This was done
  after some benchmarking on various platforms
Negotiated handshake algorithms during each key exchange now accessible
More flexible algorithm configuration
- Append/prepend to and remove from default lists instead of being forced
  to set exact lists
- Use RegExps for even greater flexibility

Misc. changes

Server: if there is no 'sftp' session event handler but a 'subsystem'
session event handler exists, it will be called as a fallback
Protocol implementation moved back into ssh2 repo, but still lives
separate from rest of the client/server code. Reasoning:
- Needed to keep both repos in sync all of the time
- People would get confused about where to lodge an issue/PR
- ssh2 was more or less the only user of ssh2-streams
- Note: An sftp stream interface is not yet available, so if you need this
  (e.g. implementing a standalone SFTP subsystem for an OpenSSH server)
  you will want to keep with ssh2-streams for the time being
Code simplified and organized better
(e.g. key exchange handling/logic in a separate file/module/class, ditto
for encryption/decryption, etc.)

Answer 1 · 2020-10-07T20:07:27.000Z

Additionally, I still have two "major" features I'd like to implement but at this point they're not going to be blockers for v1.0.0:

Certificate support for user authentication
~~Custom authentication agent implementations (as opposed to having to use an external agent process) -- this should help resolve some open issues~~ (Done)

Answer 2 · 2020-10-09T10:34:01.000Z

@mscdex, do you have a plan when v1.0.0 will get released?

Answer 3 · 2020-10-09T12:09:34.000Z

@wisotzky

The end goal is that once I feel comfortable (especially after sufficient
testing has been performed), I will be releasing this new code as v1.0.0

So no hard deadline yet.

Answer 4 · 2020-10-20T18:43:38.000Z

Thank you for the update, the lack of response in #808 (which I am the author of) over the last months was very discouraging. I would have appreciated a small heads up or acknowledgement.

As certificate support would allow me to simplify quite a bit of stuff for my use case: Is there anything I can help with getting it in sooner rather than later?

Answer 5 · 2020-10-20T19:21:45.000Z

the lack of response in #808 (which I am the author of) over the last months was very discouraging. I would have appreciated a small heads up or acknowledgement.

Lots of issues and PRs fall through the cracks for various reasons. I generally don't have as much free time to spend on my OSS projects as I used to and would like to.

As certificate support would allow me to simplify quite a bit of stuff for my use case: Is there anything I can help with getting it in sooner rather than later?

I'm not sure. All I can say for now is that I'm really not fond of re-adding the publicKey option because that confused users in the past (they would pass their private key under publicKey and would wonder why it didn't work). The old publicKey existed at a time when ssh2 wasn't yet able to calculate/generate the public key from a private key, so it relied on both types of keys to properly operate.

Since I haven't had time yet to really dig into how certificates are handled and what exactly is needed where, I don't really have any suggestions. Obviously if there is some way to continue to use privateKey for certificates (the more seamless the better), I would prefer that very much.

Answer 6 · 2020-10-20T19:44:41.000Z

Since I haven't had time yet to really dig into how certificates are handled and what exactly is needed where, I don't really have any suggestions. Obviously if there is some way to continue to use privateKey for certificates (the more seamless the better), I would prefer that very much.

It's been a while since I implemented that PR, so the following information might not be 100% accurate. #808 should contain a few pointers as well, I attempted to split my implementation into small, atomic commits with proper commit messages.

The most important resource for me was this: https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys. Certificates embed the existing SSH public key of the connecting user and are signed by the private key of the SSH CA.

So the process is like this:

User generates a key-pair as usual.
User sends over the public key to the CA.
CA signs the public key with their private key resulting in a certificate with additional meta-information embedded: ssh-keygen -s ca -I test_cert -V +60m -O "force-command=env" -O no-agent-forwarding -O no-port-forwarding -O no-pty -O no-user-rc -O no-x11-forwarding user.pub
CA sends back the signed certificate.
User uses the private key + certificate to connect to the SSH server.
The SSH server checks the signing CA against authorized_keys.

So the user would re-use their existing privateKey and provide the proper certificate as the publicKey (or certificate if you want to reduce the naming confusion). On the wire the certificate is sent in place of the public key, just with a different key type (e.g. ssh-rsa-cert-v01@openssh.com in place of ssh-rsa).

Answer 7 · 2020-10-20T19:54:12.000Z

On the wire the certificate is sent in place of the public key, just with a different key type (e.g. ssh-rsa-cert-v01@openssh.com in place of ssh-rsa).

One thing to note here (that I also mentioned in the ssh-streams PR): The signature type still is ssh-rsa and does not know about certificates. Thus the code must be able to handle a signature type != key type. But will be needed for regular RSA keys in the future anyway, because the OpenSSH 8.4 announcement mentions that ssh-rsa signatures will be deprecated (in favor of e.g. rsa-sha2-256).

Answer 8 · 2020-12-10T03:18:06.000Z

@mscdex How is the re-write going? Are you tracking known issues with the new code anywhere? I'm working on updating my module to use the new code base and it would be handy to have somewhere to see known issues and/or potentially report any issues I find. For example, I seem to be running into an issue with fastPut/fastGet under node 12, which I don't get using v0.8x (need to dig deeper to confirm yet). Would like to be able to contribute in a helpful manner and add to existing issue information rather than create new issues or know when something is a known issue rather than flood you with useless data.

It might be useful/helpful if issues are flagged with a version tag or similar?

Answer 9 · 2020-12-10T03:24:10.000Z

I haven't had any free time unfortunately. AFAIK there aren't really that many issues with the new code. The ones that exist are going to be one of the most recent issues. The only showstopping bug that I remember offhand is someone recently reported the fastGet/fastPut issues on both the current and master versions and it seems to be tied to the node version. I'm a bit skeptical about that one still existing in the master branch because of what was rewritten, but it's going to be more difficult to know for sure because AFAIK there is no easily reproducible environment for it.

I'm not sure there are enough issues with the master branch to warrant explicit tagging at this point.

Answer 10 · 2020-12-10T04:10:03.000Z

OK. If I find any issue which doesn't exist with the 0.8x version, I'll clearly flag it in the report as specific to v1.0-snapshot. If it exists in both 0.8x and 1.0-snapshot, I'll assume it is an existing known issue and will only update the relevant issue info if I find something which might help in fixing/diagnosing cause. BTW I noticed that the change in v14 of node which was causing problems with ssh2 has been rolled back in v15. I ran my tests using node v15.3.0 and ssh2 0.8x and all my tests passed. However, I didn't dig deeply and as the issue was intermittent, I might have just been lucky!

Answer 11 · 2021-05-20T07:53:34.000Z

I haven't had any free time unfortunately. AFAIK there aren't really that many issues with the new code.

@mscdex, could you provide an update about when you feel comfortable to release v1.0.0? From my understanding, all new features like algo/ciphers will only be available in v1.0 and not back ported to v0.8 - so there is some need to get upgrades. Even I could include your code directly into my project, I would prefer to expose the package dependencies in npm to load your package automatically.

Answer 12 · 2021-05-23T05:18:25.000Z

@wisotzky Probably in the next couple of days or so.

Answer 13 · 2021-05-24T16:34:32.000Z

@mscdex, thanks your heads up!
Can't wait to get my hands on it to integrate.

Cheers

Answer 14 · 2021-05-29T05:55:43.000Z

v1.0.0 is now released

Answer 15 · 2021-05-29T13:31:27.000Z

@mscdex, thanks for the update!

Seems, the new version v1.0.0 generates an error, when loading this into ATOM (NodeJS).
Following error is produces:

WebAssembly.Compile is disallowed on the main thread, if the buffer size is larger than 4KB. Use WebAssembly.compile, or compile on a worker thread.
Hide Stack Trace
RangeError: WebAssembly.Compile is disallowed on the main thread, if the buffer size is larger than 4KB. Use WebAssembly.compile, or compile on a worker thread.
    at /Users/wiso/.atom/packages/atom-netconf/node_modules/ssh2/lib/protocol/crypto/poly1305.js:18:238
    at /Users/wiso/.atom/packages/atom-netconf/node_modules/ssh2/lib/protocol/crypto/poly1305.js:19:12
    at Object.<anonymous> (/Users/wiso/.atom/packages/atom-netconf/node_modules/ssh2/lib/protocol/crypto/poly1305.js:19:28)
    at Object.<anonymous> (/Users/wiso/.atom/packages/atom-netconf/node_modules/ssh2/lib/protocol/crypto/poly1305.js:23:3)
    at Module.get_Module._compile (/Applications/Atom.app/Contents/Resources/app/static/<embedded>:11:147404)
    at Object.value [as .js] (/Applications/Atom.app/Contents/Resources/app/static/<embedded>:11:150952)
    at Module.load (internal/modules/cjs/loader.js:645:32)
    at Function.Module._load (internal/modules/cjs/loader.js:560:12)
    at Module.require (file:///Applications/Atom.app/Contents/Resources/app.asar/static/index.js:72:46)
    at require (/Applications/Atom.app/Contents/Resources/app/static/<embedded>:11:146720)
    at Object.<anonymous> (/Users/wiso/.atom/packages/atom-netconf/node_modules/ssh2/lib/protocol/crypto.js:160:25)
    at Object.<anonymous> (/Users/wiso/.atom/packages/atom-netconf/node_modules/ssh2/lib/protocol/crypto.js:1612:3)
    at Module.get_Module._compile (/Applications/Atom.app/Contents/Resources/app/static/<embedded>:11:147404)
    at Object.value [as .js] (/Applications/Atom.app/Contents/Resources/app/static/<embedded>:11:150952)
    at Module.load (internal/modules/cjs/loader.js:645:32)
    at Function.Module._load (internal/modules/cjs/loader.js:560:12)
    at Module.require (file:///Applications/Atom.app/Contents/Resources/app.asar/static/index.js:72:46)
    at require (/Applications/Atom.app/Contents/Resources/app/static/<embedded>:11:146720)
    at Object.<anonymous> (/Users/wiso/.atom/packages/atom-netconf/node_modules/ssh2/lib/protocol/keyParser.js:23:25)
    at Object.<anonymous> (/Users/wiso/.atom/packages/atom-netconf/node_modules/ssh2/lib/protocol/keyParser.js:1483:3)
    at Module.get_Module._compile (/Applications/Atom.app/Contents/Resources/app/static/<embedded>:11:147404)
    at Object.value [as .js] (/Applications/Atom.app/Contents/Resources/app/static/<embedded>:11:150952)
    at Module.load (internal/modules/cjs/loader.js:645:32)
    at Function.Module._load (internal/modules/cjs/loader.js:560:12)
    at Module.require (file:///Applications/Atom.app/Contents/Resources/app.asar/static/index.js:72:46)
    at require (/Applications/Atom.app/Contents/Resources/app/static/<embedded>:11:146720)
    at Object.<anonymous> (/Users/wiso/.atom/packages/atom-netconf/node_modules/ssh2/lib/agent.js:9:35)
    at Object.<anonymous> (/Users/wiso/.atom/packages/atom-netconf/node_modules/ssh2/lib/agent.js:1125:3)
    at Module.get_Module._compile (/Applications/Atom.app/Contents/Resources/app/static/<embedded>:11:147404)
    at Object.value [as .js] (/Applications/Atom.app/Contents/Resources/app/static/<embedded>:11:150952)

Need to check, what going wrong here...

Answer 16 · 2021-05-29T13:48:24.000Z

@mscdex, do you know any trick, to tweak memory settings in ATOM/nodeJS to load your module.
It does not look specific to my project... So likely all ATOM plugins using your ssh2 lib will fail.

Answer 17 · 2021-05-29T14:07:04.000Z

@wisotzky That's strange as WebAssembly.Compile is not anywhere in the file....

Answer 18 · 2021-05-29T14:07:27.000Z

Also, can you open this as a new issue?

EDIT: I went ahead and opened the issue myself because I already have a possible solution...

Answer 19 · 2021-05-29T18:17:10.000Z

Your fix seems to work:
#1014 (comment)

Doing the first tests now using v1.0.0... Looks quite promising... Mixing cipher/algo from previous version are now working.

Answer 20 · 2021-06-28T12:14:30.000Z

Note: An sftp stream interface is not yet available, so if you need this
(e.g. implementing a standalone SFTP subsystem for an OpenSSH server)
you will want to keep with ssh2-streams for the time being

Any idea when the SFTPStream class (or something similar) will be ported and made available? It seems that if I want to keep using it, I can use ssh2 v1.0.0 but still require the older ssh2-streams?

On a side note, any plans to add TypeScript typings? There is the community-sourced @types/ssh2 module but the last update there was 7 months ago and it wasn't the most accurate even before the big v1.0.0 change. It'd be very handy to have new and up-to-date (version-specific) TS typings available. Optionally if you have a lot of times on your hands and are in the mood, even rewrite the module in TS.

Answer 21 · 2021-06-28T12:21:05.000Z

On a side note, any plans to add TypeScript typings? There is the community-sourced @types/ssh2 module but the last update there was 7 months ago and it wasn't the most accurate even before the big v1.0.0 change. It'd be very handy to have new and up-to-date (version-specific) TS typings available. Optionally if you have a lot of times on your hands and are in the mood, even rewrite the module in TS.

I'm not really interested in TypeScript, so such "typings" will still need to be maintained by someone else.