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 support for twofish cipher #457

Merged
merged 2 commits into from
Sep 5, 2018
Merged

Add support for twofish cipher #457

merged 2 commits into from
Sep 5, 2018

Conversation

tomaka
Copy link
Member

@tomaka tomaka commented Sep 4, 2018

Adds support for the twofish cipher mode.

There are random data corruption in polkadot right now, and that might be a problem in the aes library.
Let's add support for twofish in secio, and later, once it's deployed, we can try using it in our local node to see if the data corruption goes away.

@tomaka
Copy link
Member Author

tomaka commented Sep 4, 2018

This is an addition to the specifications of secio, as we add support for an algorithm that Go and JS don't support, with the name TwofishCTR.

I don't know if I should inform someone of that.
cc @mgoelzer

The reason why I went for twofish and not blowfish (which Go and JS support) is because I couldn't find any CBC library in Rust.

@ghost
Copy link

ghost commented Sep 4, 2018

/cc @Stebalien

@Stebalien
Copy link
Member

This shouldn't break anything so I don't really have any objections. However, I don't think there are really any plans to add twofish support to the other implementations.

Are you just using this for testing?

@tomaka
Copy link
Member Author

tomaka commented Sep 4, 2018

This shouldn't break anything so I don't really have any objections.

Sure, but maybe we should document somewhere what the possible algorithms are, instead of having people figure out by reading the source code of the various implementations.

Are you just using this for testing?

There's probably a bug in the AES crypto library we're using, and support for Blowfish would be too long to implement.

@burdges
Copy link

burdges commented Sep 5, 2018

I suppose this does not matter given your reasons, but if it's only twofish in CTR mode being used then that's just a stream cipher, so maybe ChaCha20 works too.

@tomaka
Copy link
Member Author

tomaka commented Sep 5, 2018

(merging this because the Polkadot network is on fire, but we can keep discussing)

ChaCha20 works too.

I went with the algorithm that was the most straight-forward to add support for 😅

@tomaka tomaka merged commit e2960b4 into libp2p:master Sep 5, 2018
@tomaka tomaka deleted the twofish branch September 5, 2018 00:15
@Stebalien
Copy link
Member

Yes, we should totally document this.

@ghost
Copy link

ghost commented Sep 12, 2018

Yes, we should totally document this.

Unless we create an issue where docs people will see it, this will likely never be discovered by them. I made one at libp2p/docs#9. AFAIK, libp2p currently has no docs repo or docs backlog (waffle or zen). So we need to fix that.

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.

4 participants