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 draft spec for plaintext key exchange protocol #186

Merged
merged 5 commits into from
Jul 9, 2019

Conversation

yusefnapora
Copy link
Contributor

This adds a spec for exchanging keys and peer ids when setting up the plaintext "security" protocol, as discussed in #184.

This sends both peer ids and keys as @raulk outlined - we could potentially just send the keys as in @Warchant's c++ implementation and derive the ids.

Also, I bumped the version in the protocol id, since it seems like sending the key message over will break anyone using the existing /plaintext/1.0.0 protocol id.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

LGTM; few minor comments.

plaintext/README.md Outdated Show resolved Hide resolved
plaintext/README.md Show resolved Hide resolved
plaintext/README.md Outdated Show resolved Hide resolved
plaintext/README.md Outdated Show resolved Hide resolved
plaintext/README.md Outdated Show resolved Hide resolved
plaintext/README.md Outdated Show resolved Hide resolved
plaintext/README.md Outdated Show resolved Hide resolved
@Kubuxu
Copy link
Member

Kubuxu commented Jul 8, 2019

I would be for adding a big disclaimer that this spec is only for development purposes and should never be shipped in actual application or library.

Copy link

@Warchant Warchant left a comment

Choose a reason for hiding this comment

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

Hm. We have a little bit different encoding of keys on the wire.

We took it from js-libp2p I guess.

Seems rust-libp2p also implements key encoding according to spec, so we're gonna fix this soon.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I kind of want to make both sides send their private keys just to make it clear that this should never be enabled in production but that's probably asking for trouble.

Copy link
Contributor

@mhchia mhchia left a comment

Choose a reason for hiding this comment

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

Looks pretty complete. Thanks for the work on specifying the protocol. 🙏

@yusefnapora
Copy link
Contributor Author

Awesome, thanks for all the reviews! I added everyone in the thread to the IG - please PR or ping me if you'd rather not be. I also added another warning about not using in production - hopefully people get the message.

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.

6 participants