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

Add some functions for converting from ed25519 to curve25519 #361

Closed
wants to merge 2 commits into from

Conversation

aleksuss
Copy link
Contributor

@aleksuss aleksuss commented Aug 8, 2019

The PR added some functions for converting public and secret keys from ed25519 format to curve25519.

@coveralls
Copy link

coveralls commented Aug 8, 2019

Pull Request Test Coverage Report for Build 1347

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 95.389%

Files with Coverage Reduction New Missed Lines %
crypto/sign/ed25519.rs 1 99.71%
Totals Coverage Status
Change from base Build 1334: 0.2%
Covered Lines: 3165
Relevant Lines: 3318

💛 - Coveralls

ozkriff
ozkriff previously approved these changes Aug 8, 2019
@dnaq
Copy link
Collaborator

dnaq commented Aug 8, 2019

Thanks for the pull request. Unfortunately I’ve closed pull requests adding this functionality more times than I can count. Please look through the history of closed issues for the reasoning why.

@dnaq dnaq closed this Aug 8, 2019
@kpp
Copy link
Member

kpp commented Aug 8, 2019

So let's bring it up.
https://github.com/sodiumoxide/sodiumoxide/issues/85
#89

@dnaq I want to reach a compromise. There were some PRs to add this functionality. You are not sure whether these functions are safe. That's a good position.

How about marking them unsafe and describing in comments why it is unsafe? Let the people shoot their leg off. They need this functionality and they will fork the project. But if you let them add this function as unsafe they will probably fix and fix some bugs.

@dnaq
Copy link
Collaborator

dnaq commented Aug 8, 2019

I prefer using unsafe to mean memory unsafety. You’re telling me that people need this functionality, but every time I’ve asked for an example of why they need it no one has been able to give me one. There is nothing stopping people from either using two different keys, or deriving keys from a single master key, instead of using a single key for two completely different purposes. Every time this discussion has come up I’ve asked for the use-case.

@aleksuss
Copy link
Contributor Author

aleksuss commented Aug 8, 2019

Actually, @dnaq is right. It's a good idea to have different key pairs for different tasks. But I don't see a reason why wouldn't add this functionality if libsodium does. In the end this is not user end application. It's a little bit strange to have such restrictions for developers as for me.

@tasn
Copy link
Contributor

tasn commented Jul 19, 2020

@dnaq, the main use-case is compatibility.

There are already protocols, such as scuttlebut and Signal that do this. If you want to be able to write something that interacts with them, you need to be able to do it. I'm now stuck trying to figure out if it's better to fork this lib for my use-case, bind the functions myself (need to see if possible) or figure out another way. It seems these fellas are in the same boat: Kuska-ssb/handshake#32

I understand and respect your position about it not being proved secure and not wanting to let developers implement anything insecure, though this makes this library incompatible with libsodium. Is it maybe possible to just hide it behind a configuration flag or something?

@dnaq
Copy link
Collaborator

dnaq commented Jul 21, 2020

@tasn This is actually the first example of protocols using the key conversion functions that anyone has given me in all the years this feature has been requested. Almost all conversations about key conversion have ended after I've asked for a concrete example of why the user couldn't use two keys instead (or derive both from a single master key).

So, I've officially changed my stance and will allow pull requests containing this functionality. I want the functions to be well documented and well tested of course, and I would like some warning that these should only be used to implement existing protocols.

@dnaq dnaq reopened this Jul 21, 2020
@tasn
Copy link
Contributor

tasn commented Jul 21, 2020

@dnaq, great to hear, thank you!

@aleksuss, could you please resolve the conflicts in the PR so it can be merged?

# Conflicts:
#	src/crypto/sign/ed25519.rs
@kpp
Copy link
Member

kpp commented Jan 29, 2021

@aleksuss I am so sorry for the delay. This PR is closed in favour of #456. If you want to add more functionality to #456 please file a new PR. We will review it far more quickly.

@kpp kpp closed this Jan 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants