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

Project Status Update #935

Closed
mscdex opened this issue Oct 7, 2020 · 21 comments
Closed

Project Status Update #935

mscdex opened this issue Oct 7, 2020 · 21 comments

Comments

@mscdex
Copy link
Owner

mscdex commented Oct 7, 2020

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

    • 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.)

@mscdex
Copy link
Owner Author

mscdex commented Oct 7, 2020

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)

@wisotzky
Copy link

wisotzky commented Oct 9, 2020

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

@mscdex
Copy link
Owner Author

mscdex commented Oct 9, 2020

@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.

@TimWolla
Copy link

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?

@mscdex
Copy link
Owner Author

mscdex commented Oct 20, 2020

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.

@TimWolla
Copy link

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:

  1. User generates a key-pair as usual.
  2. User sends over the public key to the CA.
  3. 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
  4. CA sends back the signed certificate.
  5. User uses the private key + certificate to connect to the SSH server.
  6. 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).

@TimWolla
Copy link

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).

@theophilusx
Copy link

@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?

@mscdex
Copy link
Owner Author

mscdex commented Dec 10, 2020

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.

@theophilusx
Copy link

theophilusx commented Dec 10, 2020 via email

@wisotzky
Copy link

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.

@mscdex
Copy link
Owner Author

mscdex commented May 23, 2021

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

@wisotzky
Copy link

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

Cheers

@mscdex mscdex closed this as completed May 29, 2021
@mscdex
Copy link
Owner Author

mscdex commented May 29, 2021

v1.0.0 is now released

@wisotzky
Copy link

wisotzky commented May 29, 2021

@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...

@wisotzky
Copy link

@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.

@mscdex
Copy link
Owner Author

mscdex commented May 29, 2021

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

@mscdex
Copy link
Owner Author

mscdex commented May 29, 2021

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...

@wisotzky
Copy link

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.

@SchoofsKelvin
Copy link
Contributor

  • 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.

@mscdex
Copy link
Owner Author

mscdex commented Jun 28, 2021

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.

liranmauda added a commit to liranmauda/noobaa-core that referenced this issue Jul 12, 2021
- node-addon-api from 3.2.1 to 4.0.0
- mocha from 8.4.0 to 9.0.2
- sinon from 10.0.0 to 11.1.1
- @types/node from 15.14.2 to 16.3.1
- googleapis from 73.0.0 to 81.0.0

1. Not migrating ajv from 6 to 8. In order to migrate, we need to follow: [v6-to-v8-migration](https://github.com/ajv-validator/ajv/blob/master/docs/v6-to-v8-migration.md)
2. Not bumping ssh2 [ssh2 breaking changes](mscdex/ssh2#935)

Signed-off-by: liranmauda <liran.mauda@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants