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

Add interop tests for different key types #20

Closed
jacobheun opened this issue Jun 7, 2019 · 9 comments
Closed

Add interop tests for different key types #20

jacobheun opened this issue Jun 7, 2019 · 9 comments
Labels
exp/expert Having worked on the specific codebase is important help wanted Seeking public contribution on this issue kind/enhancement A net-new feature or improvement to an existing feature P2 Medium: Good to have, but can wait until someone steps up

Comments

@jacobheun
Copy link
Contributor

After some discussion on the libp2p discuss forums, it would be ideal to have interop tests to verify different peer key types are working properly across implementations. The current interop tests are only validating RSA keys.

@jacobheun jacobheun added kind/enhancement A net-new feature or improvement to an existing feature help wanted Seeking public contribution on this issue exp/expert Having worked on the specific codebase is important P2 Medium: Good to have, but can wait until someone steps up labels Jun 7, 2019
@aknuds1
Copy link
Contributor

aknuds1 commented Jul 1, 2019

I would like to try to solve this, as it's of relevance to our project (Quorum Control).

@aknuds1
Copy link
Contributor

aknuds1 commented Jul 3, 2019

@jacobheun Could you give me a hint as to where the peer key type is configured in test/connect/js2go.js? I'm not familiar enough with libp2p internals to tell yet; I scoured through the tests and the immediate API and couldn't figure out so far, I suppose RSA use is implicit?

@jacobheun
Copy link
Contributor Author

@aknuds1 spawnDaemons takes an optional 3rd argument of options. These are the libp2p-daemon flags, and would be something like: daemons = await spawnDaemons(2, ['js', 'go'], { id: '/path/to/key' }). The tests use the https://github.com/libp2p/go-libp2p-daemon and https://github.com/libp2p/js-libp2p-daemon libraries. Support for key file reading should already exist, the key files will just need to be generated.

These tests should end up failing. JS currently has some open issues we need to get resolved for supporting the other keys, namely:

@aknuds1
Copy link
Contributor

aknuds1 commented Jul 3, 2019

Thanks @jacobheun! I also came to the conclusion I had to provide a key through the -id parameter to the daemon. I'm in the process of producing an ECDSA private key file.

@aknuds1
Copy link
Contributor

aknuds1 commented Jul 3, 2019

@jacobheun I'm able to break the tests now, by using ECDSA keys.

@jacobheun
Copy link
Contributor Author

@aknuds1 that's great! Ideally what I think interop should be doing is running through all the tests with different keys, but that's a scenario I think we can add later. The advantage of doing that is making sure things like pubsub signature verification is working correctly with various keys types.

I think we could look at pushing up these failing tests and allow them to fail in CI so we at least have visibility while we work on finishing support in JS.

@aknuds1
Copy link
Contributor

aknuds1 commented Jul 3, 2019

@jacobheun I'll make a PR then, so you can provide feedback.

@aknuds1
Copy link
Contributor

aknuds1 commented Jul 3, 2019

@jacobheun Please have a look at my PR #21. I'm open to feedback on this as a starting point.

@vasco-santos
Copy link
Member

This seems done! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/expert Having worked on the specific codebase is important help wanted Seeking public contribution on this issue kind/enhancement A net-new feature or improvement to an existing feature P2 Medium: Good to have, but can wait until someone steps up
Projects
None yet
Development

No branches or pull requests

3 participants