Skip to content
This repository has been archived by the owner on Sep 1, 2022. It is now read-only.

Crypto: implement TweetNaCl via Crypto++, remove SUPERCOP (ref10) #909

Merged
merged 3 commits into from
Jul 19, 2018

Conversation

anonimal
Copy link
Collaborator


By submitting this pull-request, I confirm the following:

  • I have read and understood the developer guide in kovri-docs.
  • I have checked that another pull-request for this purpose does not exist.
  • I have considered and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used and that this pull-request may be closed by the will of the maintainer.
  • I give this submission freely under the BSD 3-clause license.

See git-log for details. References anonimal/cryptopp@caa06bf and #784. Resolves #485. Closes #345.

@anonimal
Copy link
Collaborator Author

anonimal commented Jun 14, 2018

Unit test passes. Live test fires an assertion. Live tests pass for transports (yes, sessions are successful with ed25519 router ident) and tunnel pool tunnel tests are passing - but not client destinations.

This is most likely because, according to the spec, I2P requires 32byte sk buffer (I hate I2P) but, if 32byte sk were the case, then nothing should work... Certainly, a quick look at the identity code isn't helpful (#366) either.

TBD.

Update: note as to why 32byte sk buffer, the java implementation appears to take a lazy approach to interop'ing with pre-hashed ed25519 for their offline signing (see EdDSAPublicKey.java and EdDSAPrivateKey.java). Offline signing (according to spec) has nothing to do with router identities or client destinations.

If they enforce 32byte sk buffer in place of pure ed25519 for purely implementation reasons and not coherent mathematical reasons, then [insert rage comment].

I'll need more time to confirm (after I return next week).

@coneiric
Copy link
Contributor

Just ran this branch in the Docker testnet, and Kovri exited with the following errors:

[2018.06.26 23:03:48.636067] [0x00007fc0af0a0440] [info]    The Kovri I2P Router Project
[2018.06.26 23:03:48.636154] [0x00007fc0af0a0440] [info]    0.1.0-pre-alpha-  "In the beginning"
[2018.06.26 23:03:48.636363] [0x00007fc0af0a0440] [info]    DaemonSingleton: configured
kovri: /home/kovri/kovri/src/core/crypto/impl/cryptopp/signature.cc:846: bool kovri::core::Ed25519Verifier::Ed25519VerifierImpl::Verify(const uint8_t*, std::size_t, const uint8_t*) const: Assertion `rmlen == sm.size()' failed.
[2018.06.26 23:09:12.322998] [0x00007f0b2ee11440] [info]    The Kovri I2P Router Project
[2018.06.26 23:09:12.323090] [0x00007f0b2ee11440] [info]    0.1.0-pre-alpha-  "In the beginning"
[2018.06.26 23:09:12.323325] [0x00007f0b2ee11440] [info]    DaemonSingleton: configured
kovri: /home/kovri/kovri/src/core/crypto/impl/cryptopp/signature.cc:846: bool kovri::core::Ed25519Verifier::Ed25519VerifierImpl::Verify(const uint8_t*, std::size_t, const uint8_t*) const: Assertion `rmlen == sm.size()' failed.

Tracking down the cause of the assertion firing, but wanted to leave this here in case it's helpful.

@coneiric
Copy link
Contributor

coneiric commented Jul 12, 2018

Just made a PR to your branch, anonimal/kovri#4 with updates that resolves the failed assertion.

After applying those patches, rebasing on current master, and resolving some merge conflicts, this branch passes all the live tests.

I was able to connect to a server tunnel, and use the IRC + HTTP proxies.

Everything looks good to me, unless there was another client destination test you ran that was failing.

Update: ironically, shortly after posting this comment, the router's NetDb stalled with boost::bad_any_cast exceptions. I have a backtrace of the failure, hopefully it helps: netdb-log.txt

@anonimal anonimal changed the title Crypto: implement TweetNaCl via Crypto++, remove SUPERCOP (ref10) [HOLD] Crypto: implement TweetNaCl via Crypto++, remove SUPERCOP (ref10) Jul 14, 2018
noloader referenced this pull request in weidai11/cryptopp Jul 17, 2018
This should allow users to convert a ed25519 seret key to a public key without rolling their own code
weidai11/cryptopp#668

The fix is useful for implementations whose interface can't clobber
the existing sk buffer (or *must* re-use the privkey). We require both.

Thanks to noloader for the assistance. Referencing monero-project#909.
anonimal added a commit to anonimal/kovri that referenced this pull request Jul 19, 2018
Also re-introduces the crypto namespace (lifetime TBD), and implements
SecByteBlock (see monero-project#784).

Thanks to noloader for the assistance. Referencing monero-project#909.

Resolves monero-project#485. Closes monero-project#345.
anonimal added 2 commits July 19, 2018 04:21
Also re-introduces the crypto namespace (lifetime TBD), and implements
SecByteBlock (see monero-project#784).

Thanks to noloader for the assistance. Referencing monero-project#909.

Resolves monero-project#485. Closes monero-project#345.
@anonimal anonimal changed the title [HOLD] Crypto: implement TweetNaCl via Crypto++, remove SUPERCOP (ref10) Crypto: implement TweetNaCl via Crypto++, remove SUPERCOP (ref10) Jul 19, 2018
@anonimal
Copy link
Collaborator Author

Just made a PR to your branch, anonimal#4 with updates that resolves the failed assertion.

I've closed that PR because this PR resolves that issue (safer than what you were proposing).

Update: ironically, shortly after posting this comment, the router's NetDb stalled with boost::bad_any_cast exceptions. I have a backtrace of the failure, hopefully it helps: netdb-log.txt

Unrelated to this PR. I've seen it without this PR.

@anonimal
Copy link
Collaborator Author

Live tests pass. This PR should be ready to merge.

Thank you very much to @noloader for his assistance in weidai11/cryptopp#668. I've credited him in the git log.

@anonimal anonimal merged commit 5831103 into monero-project:master Jul 19, 2018
anonimal added a commit that referenced this pull request Jul 19, 2018
5831103 Tests: update ed25519 unit-test + benchmark (anonimal)
3b39ef3 Crypto: implement TweetNaCl via Crypto++, remove SUPERCOP (ref10) (anonimal)
e2cf677 Build: bump cryptopp to 8d6b1af (NaCl-related API patch) (anonimal)
@anonimal anonimal deleted the crypto branch July 19, 2018 06:15
anonimal added a commit to anonimal/kovri-docs that referenced this pull request Jul 19, 2018
anonimal added a commit to anonimal/kovri that referenced this pull request Jul 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Swapout ref10 from SUPERCOP with TweetNaCl Crypto: upgrade to supercop to latest release
2 participants