Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

[WIP] Refactor to use pull streams #8

Merged
merged 1 commit into from
Sep 6, 2016
Merged

[WIP] Refactor to use pull streams #8

merged 1 commit into from
Sep 6, 2016

Conversation

dignifiedquire
Copy link
Member

@dignifiedquire dignifiedquire commented Jun 13, 2016

@dominictarr
Copy link

the two "fix and publish" tasks are done now.

@dignifiedquire
Copy link
Member Author

thanks @dominictarr :) just on my flight back from holidays so will hopefully be able to finish this in the next days

@dominictarr
Copy link

btw, I quite like how you have used run-series to separate the parts of the handshake.
but I would like to suggest completely separating the crypto from the IO.
I did that here https://github.com/auditdrivencrypto/secret-handshake/blob/master/state.js
but in an OO style, which I will change.
better would be to put the crypto in pure functions (pass in state, message, return new state)
because then you can generate test vectors, and test the implementation in every language you port to, without having to test the IO part.

my plan is to refactor secret-handshake to work like that, just gotta make the one random thing ephemeral key generation accept a seed passed in as initial state.

@dignifiedquire
Copy link
Member Author

@dominictarr I'm getting close to working :) but I have this nasty issue when I started using pull-defer and I'm not sure if it's a bug in my usage or in the library.

You can run it yourself with DEBUG=libp2p* npm run test:node, and here is the log output: https://gist.github.com/dignifiedquire/fd6623ea88f8b8d2e5cbd4f95abcb9d4
Any pointers would be appreciated :)

@dominictarr
Copy link

the problem is here:
https://github.com/libp2p/js-libp2p-secio/blob/pull/src/handshake/finish.js#L20-L29

you are trying to read to the same stream in from two different places. that isn't allowed.
with pull streams each stream should be attached to exactly one stream.
(like plumming, if you want to attach two pipes to one, you need a special T or Y connector)

you need to take the stream, and put the mac into the start, and intercept it coming out.
This is part of the "body" encryption, but really it's still a part of the handshake, because you don't truely know if the handshake has succeed until you get the right value out.

var shake = Handshake()

pull(
   stream,
   etm.createUnboxStream(proto.local.cipher, proto.local.mac),
   shake,
   etm.createBoxStream(proto.remote.cipher, proto.remote.mac),
   stream
)
shake.write(state.proposal.in.rand)
shake.read(state.proposal.in.rand.length, function (err, buf) {
  if(deepEqual(buf, state.proposal.in.rand)) //SUCCESS
    state.secure.resolve(shake.rest())
  else
     state.secure.resolve(pull.error(new Error('fail')))
})

it seems silly to use two handshake phases, but since it sends a fixed value to confirm the session, part of the handshake is in the body, so that is kinda just how this protocol works.

@dignifiedquire
Copy link
Member Author

@dominictarr thank you so much! I finally have passing tests again 😂

@daviddias
Copy link
Member

I believe you have successfully tested this in libp2p-swarm, right?

@dignifiedquire
Copy link
Member Author

Yes the secio branch in libp2p-swarm adds tests using secio which passed with the current version of this branch

@dignifiedquire
Copy link
Member Author

This probably still needs some cleanup, need to go through it once more before merging

@@ -2,7 +2,7 @@
"name": "libp2p-secio",
"version": "0.3.1",
"description": "Secio implementation in JavaScript",
"main": "lib/index.js",
"main": "src/index.js",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • change before merge

@daviddias
Copy link
Member

daviddias commented Sep 2, 2016

Last things to do before merge:

  • point to lib
  • rebase commits to follow js-commit guidelines
  • add documentation about it using pull-streams
  • complete the readme (API + Flow of the handshake (even it just a drawing))

@daviddias
Copy link
Member

Extra, required for js and go interop #4 (comment)

@dignifiedquire
Copy link
Member Author

Extra, required for js and go interop #4 (comment)

This should be simple with this: dignifiedquire/pull-length-prefixed#5

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants