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

TypeScript rewrite #14

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

SamuelMarks
Copy link
Contributor

Begun the rewrite 🐳

Importantly this will enable:

  • Better modularisation
  • Compile-time type checking (less bugs)
  • Access to latest ECMAScript standard features (compiles down to earlier JS versions)

Still TODO on the reimplementation side:

  • Replace all var with const and let
  • Ensure everything compiles
  • Show examples of new library interface, e.g.: constructing multiple SSH connections, each in a different on-page <div>

@stuicey
Copy link
Owner

stuicey commented Sep 15, 2018

Sounds great, will be following progress :)

Let me know if you need any help

@SamuelMarks
Copy link
Contributor Author

Thanks.

Just pushed up another commit. Trying to remove all your minified files, your globals, and start using package.json.

Can you help me figure out the types of everything, and replace all globals with parameters?

E.g.: your BigInteger library, is that something custom you've built? - Could we switch to the popular https://github.com/peterolson/BigInteger.js ?

@stuicey
Copy link
Owner

stuicey commented Sep 15, 2018

Yeah sure, where'd you like to start?

I think the bigInteger library in use is jsbn. As far as I can remember its only used in key exchange ( Diffie Hellman ) to do the modPow operations. I'll look into the performance differences between the two and see if it'll be beneficial to switch.

@stuicey
Copy link
Owner

stuicey commented Sep 15, 2018

Hacked together a quick benchmark and it'll be a massive performance degradation to move libraries. JSBN is shipped as two parts (minimal & additional) and was manually stitched together to include the necessary functions.

g = new BigInteger('2', 10)
x = new BigInteger('9795623379720348785398208849028643823099584654670096560148223081263165803405717427716668286547942879498223038541687019132168677797815517353006547468241675616931239404189932808331911798558610441278084027794187224439305604841329239516110745632237563668589743848610864818215043177790871953876989330545035358736424240161712235694692878610815824796784727412033324103814339668136112370347089690842960560222718991708482872912954111722989789153360787872547391996599860219249651476099365174891321830481015184683492897794958183640665168139939664138061058047194373802532973070358387774971459887259727180174773340136475538435291', 10)
p = new BigInteger('27672893231078482414316412291382054668961850794078055737740885327219301644519185062523772252402554472043622113499052135038014438034231013974319591759922177454902042906854207562071956105114464012639538617812152802413006824376872270593397207538940915646193407317442527833642815885546990867297219159494157722538219224320598501999223606731202211414832674646768386746416618919936967926684802530604724865683415139512386953639232897141119671159391823414462672076497688454213821940860127227742127183998157286882320495558655630141964820022469843750342248821499186469187641912775497390399744993247505467402280014319222980039563', 10)

var t0 = performance.now()
var r = g.modPow(x, p)
var t1 = performance.now()

jsbn =~ 150ms
BigInt =~ 2000ms

@SamuelMarks
Copy link
Contributor Author

Thanks for that, okay I've switched to my fork which incorporates the two relevant open PRs.

Been thinking of replacing:

new SSHyClient.hash.SHA1(m.toString()).digest()

With

sjcl.hash.sha1.hash(m.toString())

Also trying to figure out:

  1. what to make of the self in DiffieHellman, i.e.: what this.transport is set to;
  2. where randomart comes from, e.g.: https://github.com/Risto-Stevcev/randomart-js
  3. what you expect inflate_long in utilities to do
  4. where struct.pack and struct.unpack comes from
  5. Is socket a WebSocket?

But yeah, if you can take a quick look through the code, I'm sure 5 minutes with you would mean many times that for me.

Thanks

@stuicey
Copy link
Owner

stuicey commented Sep 16, 2018

1 - this.transport is set here, its simply used as a reference back to SSHyClient.Transport so that key exchange has access to other modules ( parceler )

2 - randomart is from - https://github.com/purpliminal/randomart

3 - [inflate|deflate]_long converts between multiple precision hex encoded (two's complement) strings and their decimal string representations. There is probably a better way of doing it but have never gotten around to looking at it.

RFC 4251               SSH Protocol Architecture            January 2006
         Examples:

         value (hex)        representation (hex)
         -----------        --------------------
         0                  00 00 00 00
         9a378f9b2e332a7    00 00 00 08 09 a3 78 f9 b2 e3 32 a7
         80                 00 00 00 02 00 80
         -1234              00 00 00 02 ed cc
         -deadbeef          00 00 00 05 ff 21 52 41 11

4 - Can't remember where I found struct stuff. I think we only use integer structs so really could just cut it down to only handle ints:

pack : function(n) {
    var result = "";
    result += String.fromCharCode(n >>> 24 & 0xff);
    result += String.fromCharCode(n >>> 16 & 0xff);
    result += String.fromCharCode(n >>> 8  & 0xff);
    result += String.fromCharCode(n & 0xff);
    return result;

unpack : function(s) {
    var result = [];
    var t = 0;
    t += str.charCodeAt(0) << 24 >>> 0;
    t += str.charCodeAt(1) << 16 >>> 0;
    t += str.charCodeAt(2) << 8  >>> 0;
    t += str.charCodeAt(3) << 0  >>> 0;
    result.push(v);
    return result;
}

5 - Yeah when the client is created the ws object is passed into SSHyClient.parceler via SSHyClient.transport


Had a brief scan over most recent commit and think that the const m = new SSHyClientMessage(); will fail as m is always changed when building the packet?

@SamuelMarks
Copy link
Contributor Author

const doesn't apply to the RHS if the RHS has its own memory, e.g.: to do this to a regular object, run Object.freeze.

Oh, I meant that inflate_long is used all over the place as if it supports two arguments, whereas it only supports one.

Same as deflate_long, so I just move from:

export const inflate_long = (a): BigInteger => {
    let c = new BigInteger('0', 10);

To:

export const inflate_long = (a, c?): BigInteger => {
    if(c == null) c = new BigInteger('0', 10);

parceler.decrypt seems to want to accept an argument, but it doesn't have one.

Anyway, I think I've gone about as far as I'm comfortable going. TBH would prefer moving the actual crypto code—HMAC, SHA1, SHA256, MD5—to someone else's library, such as sjcl (which is already being used). Maybe keep the MD5 code

After that, was thinking to integrate webpack, and bifurcate SSHy into SSHy-lib and SSHy-client, so that the lib version can be used in other frontends, ensuring that all globals are removed in the process.

@stuicey
Copy link
Owner

stuicey commented Sep 18, 2018

Great work so far :)

Decryption is a bit weird; cause there wasn't a reliable way to disable nagle and the code being event based, the data gets added to a buffer here which is interacted with by parceler.decrypt()

I might look at replacing some of the crypto stuff with WebCrypto API as it should be a significant improvement over JS.

I like the idea of splitting it up too

@SamuelMarks
Copy link
Contributor Author

@stuicey Thanks, yeah I think this is where I leave it in your hands (for a time).

Lots of types still need to be annotated, but the bulk of the work has been done. Once you have it compiling with TypeScript, I'll finish integrating WebPack and expose a neater interface (such that it is usable without expecting HTML elements to be on the page, and allows for explicit reference of them by name (so you can have multiple on the one page), and also exposes a decoupled library for use in frameworks (like Angular, Vue and React).

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

Successfully merging this pull request may close these issues.

2 participants