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 XChaChaPoly cipher #73

Merged
merged 2 commits into from
Apr 22, 2020
Merged

Add XChaChaPoly cipher #73

merged 2 commits into from
Apr 22, 2020

Conversation

Frando
Copy link
Contributor

@Frando Frando commented Jan 27, 2020

This adds the XChaChaPoly cipher to snow. The crypto used is the chacha20poly1305 crate, which also has a dependency on the aead crate. I added a simple test that follows the ChaChaPoly test. Likely more tests are needed.

I notices that XChaChaPoly is missing when trying to handshake with a hypercore-protocol stream from nodejs

@mcginty
Copy link
Owner

mcginty commented Feb 8, 2020

Rad! Thanks for taking this on. I'd love to get this merged in.

Because XChaChaPoly isn't in the official Noise specification, I'm thinking this should be behind a feature gate. Thoughts?

@Frando
Copy link
Contributor Author

Frando commented Mar 31, 2020

This makes sense, I added a feature gate.

@Frando
Copy link
Contributor Author

Frando commented Mar 31, 2020

I rebased this on master and simplified the implementation similar to the ChaChaPoly cipher after the move to the aead crate.

Copy link
Owner

@mcginty mcginty left a comment

Choose a reason for hiding this comment

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

The code looks good, thanks for updating it!

Before merging this, we need to update ci-tests.sh since xchachapoly is not a default feature but we want to test it regardless.

When enabling the feature a la:

 set -e
 TARGET="$([ -n "$1" ] && echo "--target $1" || echo "")"

+COMMON_FEATURES="xchachapoly vector-tests"
 set -x
 cargo check --benches
 cargo test $TARGET --no-default-features
-cargo test $TARGET --features "vector-tests"
-cargo test $TARGET --features "ring-resolver vector-tests"
-cargo test $TARGET --features "ring-accelerated vector-tests"
-cargo test $TARGET --features "hfs pqclean_kyber1024 vector-tests"
-cargo test $TARGET --features "ring-resolver hfs pqclean_kyber1024 vector-tests"
-cargo test $TARGET --features "libsodium-resolver vector-tests"
-cargo test $TARGET --features "libsodium-accelerated vector-tests"
+cargo test $TARGET --features "$COMMON_FEATURES"
+cargo test $TARGET --features "ring-resolver $COMMON_FEATURES"
+cargo test $TARGET --features "ring-accelerated $COMMON_FEATURES"
+cargo test $TARGET --features "hfs pqclean_kyber1024 $COMMON_FEATURES"
+cargo test $TARGET --features "ring-resolver hfs pqclean_kyber1024 $COMMON_FEATURES"
+cargo test $TARGET --features "libsodium-resolver $COMMON_FEATURES"
+cargo test $TARGET --features "libsodium-accelerated $COMMON_FEATURES"

we'll see some tests fail when other resolvers are enabled, so we'll have to fix those before pushing.

@Frando
Copy link
Contributor Author

Frando commented Apr 7, 2020

Right, I added the feature to CI tests and updated the ring resolver to not crash.

@Frando Frando requested a review from mcginty April 7, 2020 17:14
Frando added 2 commits April 11, 2020 17:53
It's behind a feature gate "xchachapoly" because it is a the moment a
quite uncommon cipher for NOISE handshaking (but used in some protocols,
e.g. the Hypercore protocol).
@Frando
Copy link
Contributor Author

Frando commented Apr 22, 2020

Hei, can this be merged now that the CI changes are done?

@mcginty mcginty merged commit a10103c into mcginty:master Apr 22, 2020
@mcginty
Copy link
Owner

mcginty commented Apr 22, 2020

Sorry for the delay and thanks for rebasing. Merged :).

@cryptoquick
Copy link

Does this close #71 ?

@Frando
Copy link
Contributor Author

Frando commented May 6, 2020

Yes, it does! There's been no release though since it has been merged.

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.

3 participants