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

Feat/ed25519 ident #4215

Closed
wants to merge 4 commits into from
Closed

Feat/ed25519 ident #4215

wants to merge 4 commits into from

Conversation

whyrusleeping
Copy link
Member

This supercedes #4076 (I couldnt easily push to it).

I think we have some harder problems to solve for this integration. The main pain point I'm seeing is that its not easy, given a public key, to convert it to an ID. It might make sense to add an optional field to the public key protobuf that specifies the method used to convert Key to peer.ID

cc @JustinDrake

JustinDrake and others added 4 commits September 6, 2017 15:41
License: MIT
Signed-off-by: Justin Drake <drakefjustin@gmail.com>
License: MIT
Signed-off-by: Justin Drake <drakefjustin@gmail.com>
License: MIT
Signed-off-by: Justin Drake <drakefjustin@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
@JustinDrake
Copy link
Contributor

JustinDrake commented Sep 11, 2017

Thanks @whyrusleeping for the PR! (Sorry for the delay, I am currently travelling in Israel with limited time and wifi, studying zkSTARKs with Eli Ben-Sasson. Back in the UK on Friday.)

At a glance the PR looks good to me. Did you run the IPFS test suite with the flag to generate Ed25519-id identities?

I think we have some harder problems to solve for this integration.

Are you talking about this PR in particular, or more generally?

its not easy, given a public key, to convert it to an ID. It might make sense to add an optional field to the public key protobuf that specifies the method used to convert Key to peer.ID

Yes I think IDFromPublicKey (and IDFromPrivateKey) should have a second parameter to specify the hash type. If no hash type is provided (i.e. -1) then some sort of default behaviour kicks in. My suggestion for the default is to use mh.ID for Ed25519, and mh.SHA2_256 for RSA. We may want to explicitly disallow certain combinations, e.g. mh.ID for RSA.

@whyrusleeping
Copy link
Member Author

cc @diasdavid @Stebalien @vyzo @Kubuxu

Thoughts here? Would be really great to get this in asap for the open bazaar guys

@daviddias
Copy link
Member

@whyrusleeping just to make sure that this whole endeavor is doing:

  • Peers can have ed25519 based Identities
  • Peers that have an ed25519 based Identity, won't have RSA
  • SECIO needs to adapt
  • Identify needs to adapt

Is this all?

@vyzo
Copy link
Contributor

vyzo commented Sep 13, 2017

Longer term we should strive to make ed25519 the default i think.

@RubenKelevra
Copy link
Contributor

@Stebalien I think this one can be closed. :)

#7579 set it even as default.

@Stebalien
Copy link
Member

Thanks!

@Stebalien Stebalien closed this Apr 12, 2021
@Stebalien Stebalien deleted the feat/ed25519-ident branch April 12, 2021 16:16
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