Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

JS-IPFS misrepresents key IDs when Ed25519 keys are generated #3591

Closed
nadimkobeissi opened this issue Mar 15, 2021 · 11 comments · Fixed by #3693
Closed

JS-IPFS misrepresents key IDs when Ed25519 keys are generated #3591

nadimkobeissi opened this issue Mar 15, 2021 · 11 comments · Fixed by #3693
Labels
kind/bug A bug in existing code (including security flaws) status/ready Ready to be worked

Comments

@nadimkobeissi
Copy link

  • Version:
{
  version: '0.5.4',
  repo: 10,
  commit: '511147bedd51be3151de44a90fefe9425bfbcd50',
  'interface-ipfs-core': '^0.144.2'
}
  • Platform: Darwin Nadims-Mac.home 20.3.0 Darwin Kernel Version 20.3.0: Thu Jan 21 00:07:06 PST 2021; root:xnu-7195.81.3~1/RELEASE_X86_64 x86_64

  • Subsystem: Probably libp2p-crypto

Severity: Medium/High

Description:

JS-IPFS does not correctly encode key IDs in the case of Ed25519 keys:

let keyList = await ipfs.key.list()
console.log(keyList)

Gives us:

[
  {
    name: 'exampleEd25519Key',
    id: '22,81,109,83,110,56,76,85,115,104,119,77,113,74,86,56,99,122,56,90,76,87,87,89,72,113,110,85,80,99,121,76,109,66,119,107,103,101,120,85,121,121,84,69,88,74,54'
  },
  {
    name: 'self',
    id: 'QmWC64HjhUmNjCNksZZzPVhmffx1LZoGobkFx2xFini4p6'
  }
]

The key ID for the self key, an RSA key, appears to be encoded correctly as a Qm multihash. However the key ID for the example Ed25519 key appears to be encoded as an odd string of bytes represented in decimal format. It looks to me like this encoding is incorrect or corrupted somehow.

I'm not sure what the procedure is to turn this bad encoding into a real ID, although, the following does seem to produce something resembling a QmHash:

let idWeirdEncoding = ed25519ExampleKey.id.split(`,`)
let id = []
idWeirdEncoding.forEach((v, i) => {
    id[i] = String.fromCharCode(v)
})

I don't know if I can expect the resulting value from the above to be reliable. Either way the output does not seem correct and clarification on this issue is appreciated.

@nadimkobeissi nadimkobeissi added the need/triage Needs initial labeling and prioritization label Mar 15, 2021
@welcome

This comment has been minimized.

@TheDiscordian
Copy link
Member

The bytes listed decode to �QmSn8LUshwMqJV8cz8ZLWWYHqnUPcyLmBwkgexUyyTEXJ6.

I believe if these are encoded using b58mh, this key should begin with 12D for a Ed25519 key. Strange.

@nadimkobeissi
Copy link
Author

What's odd is that the output of a ipfs.key.list() operation in js-ipfs does not give me the same as the output as ipfs key list -l in go-ipfs. js-ipfs gives QmHash-like values, while go-ipfs gives values that look ready to be used as IPNS names:

~ 2h 17m 14.2s ❱ ipfs key list
self
derp
trains
~ ❱ ipfs key list -l
k51qzi5uqu5dj4xrzah5jq2pyrkivhvex4o0ahuxrwm7502ftn87id0u7tqo6m self
k51qzi5uqu5dku8yffnih1qtxv5fynst4v01du45hzsd9tmuuat3l97ebchlzg derp
k51qzi5uqu5dl6ewcsn0advx9ajfjl0vur383jysnvgjcptgljgqpc32q2suaf trains

How can I get these IPNS name values in js-ipfs? Why this difference?

@jacobheun jacobheun added kind/bug A bug in existing code (including security flaws) status/ready Ready to be worked and removed need/triage Needs initial labeling and prioritization labels Mar 15, 2021
@achingbrain
Copy link
Member

This looks like Ed25519 keys aren't being handled correctly. Would you like to open a PR with a fix?

A test should be added to the interface suite here: https://github.com/ipfs/js-ipfs/blob/master/packages/interface-ipfs-core/src/key/list.js

They are returned from libp2p here: https://github.com/ipfs/js-ipfs/blob/master/packages/ipfs-core/src/components/key/list.js

libp2p returns them here: https://github.com/libp2p/js-libp2p/blob/master/src/keychain/index.js#L250

The bug might be on this line where the key gets added to the datastore, it may not be handling the serialization properly: https://github.com/libp2p/js-libp2p/blob/master/src/keychain/index.js#L227

@nadimkobeissi
Copy link
Author

@achingbrain Fix proposed here: libp2p/js-libp2p-crypto#185

jacobheun pushed a commit to libp2p/js-libp2p-crypto that referenced this issue Mar 15, 2021
As discussed here: ipfs/js-ipfs#3591

Satisfy linter

test: actually verify ids
jacobheun pushed a commit to libp2p/js-libp2p-crypto that referenced this issue Mar 15, 2021
As discussed here: ipfs/js-ipfs#3591

Satisfy linter

test: actually verify ids
@jacobheun
Copy link
Contributor

The fix has been released in libp2p-crypto@0.19.1

@TheDiscordian
Copy link
Member

TheDiscordian commented Mar 16, 2021

Update on this based on chat room. EdDSA/Ed25519 keys are still returning Qm multihashes (should be 12D).

@nadimkobeissi
Copy link
Author

@achingbrain @jacobheun Further fixes to this issue have been proposed in libp2p/js-libp2p-crypto#186.

jacobheun pushed a commit to libp2p/js-libp2p-crypto that referenced this issue Mar 17, 2021
* Fix Ed25519 PeerID generation

This commit pushes further fixes to the generation of Ed25519 peer IDs,
building upon the discussion in ipfs/js-ipfs#3591 and the subsequent
pull request #185.

The purpose of this new pull request is to harmonize the encoding of
PeerIDs for Ed25519 keys such that the same new format is used
everywhere: peer IDs when assigned upon key generation, peer IDs when
shown via key listing, as well as the peer IDs displayed as IPNS names
when the key is used as the basis for an IPNS record.

Concretely, this changes the peer ID representation of Ed25519 keys from
the `Qm...` format to the newer `1...` format.

The accompanying test has been modified accordingly.

* Satisfy linter
@jacobheun
Copy link
Contributor

Latest version is out now, libp2p-crypto@0.19.2

@nadimkobeissi
Copy link
Author

@jacobheun Your quick responses and version updates are helping us push forward with building on top of IPFS. Thank you very much.

@lidel
Copy link
Member

lidel commented May 18, 2021

I created #3693 to explore if we in a place where we can switch js-ipfs to generate ED25519 by default, just like go-ipfs does.

(At some point we should switch both js-ipfs and go-ipfs to output peerids as CIDv1 in Base36, so the same text representation is works in subdomains, but we need ED25519 first)

achingbrain added a commit that referenced this issue Aug 11, 2021
Switch from RSA to ed25519 to match what we already do in go-ipfs.

Closes #3591

Co-authored-by: achingbrain <alex@achingbrain.net>
SgtPooki pushed a commit to ipfs/js-kubo-rpc-client that referenced this issue Aug 18, 2022
Switch from RSA to ed25519 to match what we already do in go-ipfs.

Closes ipfs/js-ipfs#3591

Co-authored-by: achingbrain <alex@achingbrain.net>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug A bug in existing code (including security flaws) status/ready Ready to be worked
Projects
None yet
5 participants