Test JS to Go connections with both RSA and Secp256k1 keys #21

aknuds1 · 2019-07-03T15:30:07Z

As a starting point for solving #20, test JS -> Go connections for both RSA and ECDSA keys.

I'm open to discuss alternative ways of parameterizing the tests over key type. I'm used to testing with AVA, so not sure how to best implement data driven testing with Mocha :)

src/daemon.js

jacobheun · 2019-07-04T09:33:28Z

test/connect/js2go.js

-  // Start Daemons
-  before(async function () {
-    this.timeout(20 * 1000)
+  return spawnDaemons(2, [{type: 'js', keyType}, {type: 'go', keyType}])


If we change how start is called we can leverage the third param of spawnDaemons to pass { id: keyPath }

aknuds1 · 2019-07-04T13:52:11Z

@jacobheun In my master branch, where I use my own versions of various libp2p dependencies (due to bugs I've fixed), I've got to the point of being able to launch JS and Go daemons, but the RSA test fails in a way I don't understand:

1) connecting js peer to go peer
       with RSA keys
         should work:
     Error: Key or message is invalid!: error:04075070:rsa routines:RSA_sign:digest too big for rsa key
      at createError (node_modules/err-code/index.js:4:44)
      at Client.connect (node_modules/@arve.knudsen/libp2p-daemon-client/src/index.js:168:13)
      at processTicksAndRejections (internal/process/task_queues.js:82:5)
      at async performTest (test/connect/js2go.js:47:3)
      at async Context.<anonymous> (test/connect/js2go.js:72:7)

Are you able to understand what would cause this issue? I'm guessing the Go daemon key is the problematic one, but I've generated it with crypto.GenerateRSAKeyPair and 2048 bits which seems to be what the daemon also does, so I don't see how to fix this. If you could help me out with this one, I'd appreciate it very much :)

aknuds1 · 2019-07-04T13:53:35Z

@jacobheun Also if you don't mind, I can update this PR to use my forked dependencies, so you we can get the tests running. We should revert to non-forked dependencies before merging this PR of course.

vasco-santos · 2019-07-04T14:14:26Z

@jacobheun Also if you don't mind, I can update this PR to use my forked dependencies, so you we can get the tests running. We should revert to non-forked dependencies before merging this PR of course.

@aknuds1 yes, feel free to install in package.json the dependencies pointing to your branches

aknuds1 · 2019-07-04T14:21:03Z

@jacobheun @vasco-santos I pushed package.json with my forked dependencies.

I would suggest you check the RSA test in test/connect/js2go.js in isolation, to figure out why the RSA key error occurs.

aknuds1 · 2019-07-04T14:58:51Z

@vasco-santos @jacobheun Never mind, I found out what the issue was with the RSA key - I had messed up in my key generator and used 256 bits for both RSA and ECDSA :/ Sorry!

aknuds1 · 2019-07-04T15:16:16Z

My current problem is that the test client can't connect to the JS daemon in the RSA test, although connecting to the Go daemon works. It seems to hang on trying to obtain a connection to /tmp/p2pd-0.sock. Have to go now, so will have to pick up debugging again later.

aknuds1 · 2019-07-05T09:29:05Z

Fixed the issue I was having, so the RSA test works. ECDSA test fails, but the response is empty so can't tell why yet.

jacobheun · 2019-07-05T10:35:43Z

FYI, if you rebase CI should stop failing commitlint for you, I removed that from the build since it's annoying :)

I assume ECDSA is likely failing because JS probably just assumes it's being given an RSA key at the moment, so when it goes to do actual encryption it fails.

aknuds1 · 2019-07-07T12:21:56Z

@jacobheun @vasco-santos I figured out what makes SECP256k1 keys incompatible between Go and JS daemons. The Go daemon calculates a different peer ID, due to the AdvancedEnableInlining option in github.com/libp2p/go-libp2p-core/peer. I.e., since the SECP256k1 public key is less than 42 bytes, the Go daemon uses the multihash ID algorithm to compute the PK digest and thus produces a different ID as the JS daemon doesn't have this logic.

When the JS daemon calculates a different peer ID from the Go daemon, on SECIO handshake, it produces an error (dialed to the wrong peer, Ids do not match).

What do you think, is it a bug that the Go and JS daemons don't agree on the logic for producing peer IDs?

tobowers · 2019-07-08T07:28:09Z

Hi hi, Adding this for context too: libp2p/specs#138

aknuds1 · 2019-07-08T12:50:42Z

@vasco-santos @jacobheun I worked around the problem I found of the Go daemon producing incompatible peer IDs by forking go-libp2p-dep, and making it use a version of go-libp2p-daemon that allows you to disable PK inlined peer IDs. The secp256k1 test now works :)

The tests fail on Windows, due to what looks like a timeout getting the go-libp2p-daemon archive for Windows off of IPFS. I am serving it from a DigitalOcean VM, but it's really slow to get hold of for some reason.

jacobheun · 2019-07-09T09:44:34Z

@Stebalien @raulk did we come to a resolution on key inlining in go? I know js will need some updates for non RSA keys, but I'm not sure where/if we came to a resolution for handling inlining going forward.

aknuds1 · 2019-07-09T09:48:12Z

@jacobheun On our side (Quorum Control) in the meantime, I made another workaround by modifying libp2p-secio to handle also peer IDs with inlined public key on protocol handshake (i.e. if the remote ID doesn't match, it tries also inlining the PK). This seems to work! Is this a possible (transitional) fix, instead of changing the Go logic?

jacobheun · 2019-07-09T10:02:18Z

@aknuds1 can you open up a Draft PR of that over at libp2p-secio and we can talk through that potential solution there?

aknuds1 · 2019-07-09T10:04:13Z

@aknuds1 can you open up a Draft PR of that over at libp2p-secio and we can talk through that potential solution there?

@jacobheun Will do!

aknuds1 · 2019-07-11T10:16:34Z

@jacobheun @vasco-santos I could use your advice on what's the best solution to harmonize Go and JS implementations of libp2p with regards to calculation of peer ID in case of Secp256k1 keys?

So far I tried to build robustness into both libraries, so that on connection JS libp2p will test if the remote's peer ID is calculated with the identity scheme and vice versa for Go libp2p. However, it doesn't work perfectly since for example when you connect a JS peer to a Go peer and you list the peers of the Go peer, it will give you a different ID for the JS peer (since it uses the identity scheme to calculate it from the given public key).

Given the problem described above, I wonder if it would be best to modify JS libp2p to also calculate peer IDs with identity scheme if the PK is shorter than 43 bytes, since it would then behave the same as the Go implementation.

jacobheun · 2019-07-11T10:40:08Z

Given the problem described above, I wonder if it would be best to modify JS libp2p to also calculate peer IDs with identity scheme if the PK is shorter than 43 bytes, since it would then behave the same as the Go implementation.

I think we'll need to do this for now so that we match go and can leverage the keys. This may cause issues in the future if/when we upgrade how this is calculated, but this will need to be done in go anyway. I'd rather us having a working implementation now and cross that bridge when we come to it than to delay this until that change happens, since that is currently an unknown.

aknuds1 · 2019-07-11T11:10:21Z

@jacobheun Thanks for the helpful answer! I'm going to prototype porting Go's peer ID algorithm to JS and see how it goes.

aknuds1 · 2019-07-11T15:12:10Z

Using my modified peer-id package, this scenario looks to be fixed for me locally at least.

aknuds1 · 2019-07-11T17:45:21Z

@vasco-santos @jacobheun Can you please blow away the Travis dependency cache? It works locally, but I think the dependencies must be updated.

Stebalien · 2019-07-11T18:49:24Z

@jacobheun my current plan is to leave it as it is and to try to find a way to help OpenBazaar upgrade. At this point, too many are relying on key inlining to easily revert it. Thoughts @raulk?

jacobheun · 2019-07-11T19:49:36Z

@aknuds1 cache cleared and tests are passing here now 👍

aknuds1 · 2019-07-11T20:35:05Z

Yay!!!

…

On Thu, Jul 11, 2019, 21:49 Jacob Heun ***@***.***> wrote: @aknuds1 <https://github.com/aknuds1> cache cleared and tests are passing here now 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21?email_source=notifications&email_token=AACEVV72C4UG3YG7KLDEC4DP66FFHA5CNFSM4H5HCIP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZXZIEQ#issuecomment-510628882>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACEVV6BUMGZFAVFQYIBVZ3P66FFHANCNFSM4H5HCIPQ> .

aknuds1 · 2019-07-12T08:44:31Z

I revised according to @jacobheun's suggestions, please have another look.

jacobheun

Just two nits that I think could be helpful for readability if the tests are expanded in the future. Other than that, this LGTM.

test/utils/spawnDaemons.js

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

aknuds1 · 2019-07-12T09:06:03Z

@jacobheun Cool, committed your suggestions.

aknuds1 · 2019-07-15T09:07:37Z

src/daemon.js

-      daemon.stderr.on('data', (data) => {
-        if (!data.toString().includes('Warning')) {
-          return reject(data.toString())
+      daemon.on('exit', (code, signal) => {


I figure it's more robust to fail if the daemon fails, instead of when it simply prints a warning?

I agree with that!

vasco-santos · 2019-07-15T09:26:39Z

@aknuds1 I will not merge this PR yet because I have been experiencing issues with the CI for js-libp2p-daemon for the following test: should be able to load a Secp256k1 private key:

Can you have a look as that might have impact on this PR?

aknuds1 · 2019-07-15T09:37:44Z

@aknuds1 I will not merge this PR yet because I have been experiencing issues with the CI for js-libp2p-daemon for the following test: should be able to load a Secp256k1 private key:

Can you have a look as that might have impact on this PR?

@vasco-santos Please have a look at this PR.

vasco-santos · 2019-07-15T10:02:24Z

I haven't noticed your PR! We are good to go here

vasco-santos

LGTM!

aknuds1 mentioned this pull request Jul 3, 2019

Add interop tests for different key types #20

Closed

aknuds1 force-pushed the bugfix/test-ecdsa-keys branch 5 times, most recently from 2e718c4 to 3558e77 Compare July 4, 2019 07:43

jacobheun reviewed Jul 4, 2019

View reviewed changes

src/daemon.js Outdated Show resolved Hide resolved

jacobheun reviewed Jul 4, 2019

View reviewed changes

src/daemon.js Outdated Show resolved Hide resolved

jacobheun reviewed Jul 4, 2019

View reviewed changes

aknuds1 force-pushed the bugfix/test-ecdsa-keys branch from 3558e77 to 235aa5a Compare July 4, 2019 13:46

aknuds1 changed the title ~~Test JS to Go connections with both RSA and ECDSA keys~~ WIP: Test JS to Go connections with both RSA and ECDSA keys Jul 4, 2019

aknuds1 force-pushed the bugfix/test-ecdsa-keys branch from 4748b9f to d27a4c3 Compare July 5, 2019 08:44

aknuds1 force-pushed the bugfix/test-ecdsa-keys branch from 5779d72 to 224c829 Compare July 5, 2019 10:41

aknuds1 force-pushed the bugfix/test-ecdsa-keys branch 2 times, most recently from a1b072c to 1720541 Compare July 8, 2019 12:47

aknuds1 changed the title ~~WIP: Test JS to Go connections with both RSA and ECDSA keys~~ WIP: Test JS to Go connections with both RSA and Secp256k1 keys Jul 8, 2019

aknuds1 force-pushed the bugfix/test-ecdsa-keys branch from 1720541 to 616e741 Compare July 8, 2019 13:58

aknuds1 force-pushed the bugfix/test-ecdsa-keys branch from 616e741 to 4996f42 Compare July 11, 2019 10:09

aknuds1 mentioned this pull request Jul 11, 2019

Support creating from Secp256k1 / harmonize algorithm with Go libp2p/js-peer-id#95

Merged

aknuds1 force-pushed the bugfix/test-ecdsa-keys branch from 42aa66b to 30e7d26 Compare July 11, 2019 17:40

aknuds1 changed the title ~~WIP: Test JS to Go connections with both RSA and Secp256k1 keys~~ Test JS to Go connections with both RSA and Secp256k1 keys Jul 11, 2019

aknuds1 mentioned this pull request Jul 12, 2019

Test connecting Go to JS peer with Secp256k1 keys #24

Merged

Test JS to Go connections with both RSA and ECDSA keys

7ba851b

aknuds1 force-pushed the bugfix/test-ecdsa-keys branch from 30e7d26 to 7ba851b Compare July 12, 2019 08:44

jacobheun approved these changes Jul 12, 2019

View reviewed changes

test/utils/spawnDaemons.js Outdated Show resolved Hide resolved

test/utils/spawnDaemons.js Outdated Show resolved Hide resolved

jacobheun requested a review from vasco-santos July 12, 2019 09:03

aknuds1 and others added 2 commits July 12, 2019 11:04

Update test/utils/spawnDaemons.js

ede0da3

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

Update test/utils/spawnDaemons.js

f799cef

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

aknuds1 commented Jul 15, 2019

View reviewed changes

vasco-santos approved these changes Jul 15, 2019

View reviewed changes

vasco-santos merged commit 28b2d80 into libp2p:master Jul 15, 2019

aknuds1 deleted the bugfix/test-ecdsa-keys branch July 15, 2019 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test JS to Go connections with both RSA and Secp256k1 keys #21

Test JS to Go connections with both RSA and Secp256k1 keys #21

aknuds1 commented Jul 3, 2019

jacobheun Jul 4, 2019

aknuds1 commented Jul 4, 2019

aknuds1 commented Jul 4, 2019

vasco-santos commented Jul 4, 2019

aknuds1 commented Jul 4, 2019 •

edited

Loading

aknuds1 commented Jul 4, 2019

aknuds1 commented Jul 4, 2019

aknuds1 commented Jul 5, 2019

jacobheun commented Jul 5, 2019

aknuds1 commented Jul 7, 2019 •

edited

Loading

tobowers commented Jul 8, 2019

aknuds1 commented Jul 8, 2019 •

edited

Loading

jacobheun commented Jul 9, 2019

aknuds1 commented Jul 9, 2019 •

edited

Loading

jacobheun commented Jul 9, 2019

aknuds1 commented Jul 9, 2019

aknuds1 commented Jul 11, 2019 •

edited

Loading

jacobheun commented Jul 11, 2019

aknuds1 commented Jul 11, 2019

aknuds1 commented Jul 11, 2019

aknuds1 commented Jul 11, 2019

Stebalien commented Jul 11, 2019

jacobheun commented Jul 11, 2019

aknuds1 commented Jul 11, 2019 via email

aknuds1 commented Jul 12, 2019

jacobheun left a comment

aknuds1 commented Jul 12, 2019

aknuds1 Jul 15, 2019

vasco-santos Jul 15, 2019

vasco-santos commented Jul 15, 2019

aknuds1 commented Jul 15, 2019

vasco-santos commented Jul 15, 2019

vasco-santos left a comment

Test JS to Go connections with both RSA and Secp256k1 keys #21

Test JS to Go connections with both RSA and Secp256k1 keys #21

Conversation

aknuds1 commented Jul 3, 2019

jacobheun Jul 4, 2019

Choose a reason for hiding this comment

aknuds1 commented Jul 4, 2019

aknuds1 commented Jul 4, 2019

vasco-santos commented Jul 4, 2019

aknuds1 commented Jul 4, 2019 • edited Loading

aknuds1 commented Jul 4, 2019

aknuds1 commented Jul 4, 2019

aknuds1 commented Jul 5, 2019

jacobheun commented Jul 5, 2019

aknuds1 commented Jul 7, 2019 • edited Loading

tobowers commented Jul 8, 2019

aknuds1 commented Jul 8, 2019 • edited Loading

jacobheun commented Jul 9, 2019

aknuds1 commented Jul 9, 2019 • edited Loading

jacobheun commented Jul 9, 2019

aknuds1 commented Jul 9, 2019

aknuds1 commented Jul 11, 2019 • edited Loading

jacobheun commented Jul 11, 2019

aknuds1 commented Jul 11, 2019

aknuds1 commented Jul 11, 2019

aknuds1 commented Jul 11, 2019

Stebalien commented Jul 11, 2019

jacobheun commented Jul 11, 2019

aknuds1 commented Jul 11, 2019 via email

aknuds1 commented Jul 12, 2019

jacobheun left a comment

Choose a reason for hiding this comment

aknuds1 commented Jul 12, 2019

aknuds1 Jul 15, 2019

Choose a reason for hiding this comment

vasco-santos Jul 15, 2019

Choose a reason for hiding this comment

vasco-santos commented Jul 15, 2019

aknuds1 commented Jul 15, 2019

vasco-santos commented Jul 15, 2019

vasco-santos left a comment

Choose a reason for hiding this comment

aknuds1 commented Jul 4, 2019 •

edited

Loading

aknuds1 commented Jul 7, 2019 •

edited

Loading

aknuds1 commented Jul 8, 2019 •

edited

Loading

aknuds1 commented Jul 9, 2019 •

edited

Loading

aknuds1 commented Jul 11, 2019 •

edited

Loading