-
Notifications
You must be signed in to change notification settings - Fork 453
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
fix!: remove private key field from peer id #2660
fix!: remove private key field from peer id #2660
Conversation
Opened as draft so we can agree early on the nomenclature.
I suppose we could change the creation of Peer Ids so you create a keypair, then turn the public key into a PeerId? |
export interface PeerId { | ||
type: PeerIdType | ||
multihash: MultihashDigest | ||
privateKey?: Uint8Array | ||
publicKey?: Uint8Array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we take this opportunity to do something that requires a little less maintenance for these very common interfaces?
export interface LocalPeerIdBase {
readonly publicKey: Uint8Array
readonly privateKey: Uint8Array
}
export interface PeerIdBase <pType extends PeerIdType, key extends Uint8Array = Uint8Array> {
readonly type: pType
readonly publicKey: key
readonly multihash: MultihashDigest
toString(): string
toCID(): CID
toBytes(): Uint8Array
equals(other?: PeerId | Uint8Array | string): boolean
}
export type RSAPeerId = PeerIdBase<'RSA'>
export type LocalRSAPeerId = RSAPeerId & LocalPeerIdBase
export type PeerId = RSAPeerId | Ed25519PeerId | Secp256k1PeerId | URLPeerId
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk that we're planning on adding many more peerId types or anything, but it results in less code that seems easier to reason about for me at least. and will be less lines in the future if we need to extend PeerIdBase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I somewhat did this on purpose. As suggested the generated API docs will have you clicking through from PeerId
-> Ed25519PeerId
-> PeerIdBase
to see the docs for the fields on the type.
No-one will ever deal with a PeerIdBase
in day to day code so I just copied the fields/methods to make the docs simpler.
They are interfaces so are removed at build time so there's no bundle size impact.
I think that's what you and @wemeetagain agreed to on the maintainers' call. Is there prior art in go-libp2p or rust-libp2p on how they generate peer IDs? Do we have a need to accept the full keypair, so we're validating the person providing that public key so we get out of any impersonation attempts, or noise, with duplicate peerIds on the network? |
939c741
to
de57508
Compare
The only time you'll ever see a private key on a peer id is for the id of the currently running node. At runtime a service can obtain the unmarhsalled private key by adding a `privateKey: PrivateKey` field to it's components map so there's no need to have the field on every `PeerId`. BREAKING CHANGE: the `.privateKey` field of the `PeerId` interface has been removed
de57508
to
a3226cd
Compare
…ivate-key-from-peer-id
We don't need the keypair to create a We validate the remote peer has the private key corresponding to the public key during the connection upgrade process. Duplicate |
@@ -3,7 +3,7 @@ syntax = "proto3"; | |||
enum KeyType { | |||
RSA = 0; | |||
Ed25519 = 1; | |||
Secp256k1 = 2; | |||
secp256k1 = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's referred to as secp256k1
in it's paper, plus where we have it as a string constant we have it as "secp256k1" so it's just bringing this in line with that.
On the wire 2
is still 2
so there's no difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should include it in the breaking changes list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The only time you should ever see a private key on a `PeerId` is for the id of the currently running node. An exception to this is the keychain which used `PeerId`s to export private keys, but a nicer API there is to just deal with `PrivateKey` instances directly. At runtime a service can obtain the unmarshaled private key by adding a `privateKey: PrivateKey` field to it's components map so there's no need to have the field on every `PeerId`. The `publicKey` field of a `PeerId` was a `Uint8Array` which (if present) held the public key bytes in a protobuf wrapper along with a field saying what type of key it was. This was because we wanted to avoid pulling `@libp2p/crypto` into front ends, since it had a dependency on `node-forge` which was a large blob of untreeshakable CSJ code. This dependency has been removed so `@libp2p/crypto` is now comparatively lightweight so we can use the `PublicKey` type instead of `Uint8Array`, which also saves CPU time since we don't need to unmarshal the key before we can use it. Fixes #2659 BREAKING CHANGE: - The `.privateKey` field of the `PeerId` interface has been removed - The `.publicKey` field of the `PeerId` interface is now a `PublicKey` instead of a `Uint8Array` - `createLibp2p` now accepts a `privateKey` instead of a `peerId` - The keychain operates on `PrivateKey` instances instead of `PeerId`s with `.privateKey` fields - `@libp2p/peer-id-factory` has been removed, use `generateKeyPair` and `peerIdFromPrivateKey` instead
The only time you should ever see a private key on a `PeerId` is for the id of the currently running node. An exception to this is the keychain which used `PeerId`s to export private keys, but a nicer API there is to just deal with `PrivateKey` instances directly. At runtime a service can obtain the unmarshaled private key by adding a `privateKey: PrivateKey` field to it's components map so there's no need to have the field on every `PeerId`. The `publicKey` field of a `PeerId` was a `Uint8Array` which (if present) held the public key bytes in a protobuf wrapper along with a field saying what type of key it was. This was because we wanted to avoid pulling `@libp2p/crypto` into front ends, since it had a dependency on `node-forge` which was a large blob of untreeshakable CSJ code. This dependency has been removed so `@libp2p/crypto` is now comparatively lightweight so we can use the `PublicKey` type instead of `Uint8Array`, which also saves CPU time since we don't need to unmarshal the key before we can use it. Fixes #2659 BREAKING CHANGE: - The `.privateKey` field of the `PeerId` interface has been removed - The `.publicKey` field of the `PeerId` interface is now a `PublicKey` instead of a `Uint8Array` - `createLibp2p` now accepts a `privateKey` instead of a `peerId` - The keychain operates on `PrivateKey` instances instead of `PeerId`s with `.privateKey` fields - `@libp2p/peer-id-factory` has been removed, use `generateKeyPair` and `peerIdFromPrivateKey` instead
The only time you should ever see a private key on a `PeerId` is for the id of the currently running node. An exception to this is the keychain which used `PeerId`s to export private keys, but a nicer API there is to just deal with `PrivateKey` instances directly. At runtime a service can obtain the unmarshaled private key by adding a `privateKey: PrivateKey` field to it's components map so there's no need to have the field on every `PeerId`. The `publicKey` field of a `PeerId` was a `Uint8Array` which (if present) held the public key bytes in a protobuf wrapper along with a field saying what type of key it was. This was because we wanted to avoid pulling `@libp2p/crypto` into front ends, since it had a dependency on `node-forge` which was a large blob of untreeshakable CSJ code. This dependency has been removed so `@libp2p/crypto` is now comparatively lightweight so we can use the `PublicKey` type instead of `Uint8Array`, which also saves CPU time since we don't need to unmarshal the key before we can use it. Fixes #2659 BREAKING CHANGE: - The `.privateKey` field of the `PeerId` interface has been removed - The `.publicKey` field of the `PeerId` interface is now a `PublicKey` instead of a `Uint8Array` - `createLibp2p` now accepts a `privateKey` instead of a `peerId` - The keychain operates on `PrivateKey` instances instead of `PeerId`s with `.privateKey` fields - `@libp2p/peer-id-factory` has been removed, use `generateKeyPair` and `peerIdFromPrivateKey` instead
The only time you should ever see a private key on a
PeerId
is for the id of the currently running node. An exception to this is the keychain which usedPeerId
s to export private keys, but a nicer API there is to just deal withPrivateKey
instances directly.At runtime a service can obtain the unmarshaled private key by adding a
privateKey: PrivateKey
field to it's components map so there's no need to have the field on everyPeerId
.The
publicKey
field of aPeerId
was aUint8Array
which (if present) held the public key bytes in a protobuf wrapper along with a field saying what type of key it was. This was because we wanted to avoid pulling@libp2p/crypto
into front ends, since it had a dependency onnode-forge
which was a large blob of untreeshakable CSJ code. This dependency has been removed so@libp2p/crypto
is now comparatively lightweight so we can use thePublicKey
type instead ofUint8Array
, which also saves CPU time since we don't need to unmarshal the key before we can use it.Fixes #2659
BREAKING CHANGE:
.privateKey
field of thePeerId
interface has been removed.publicKey
field of thePeerId
interface is now aPublicKey
instead of aUint8Array
createLibp2p
now accepts aprivateKey
instead of apeerId
PrivateKey
instances instead ofPeerId
s with.privateKey
fields@libp2p/peer-id-factory
has been removed, usegenerateKeyPair
andpeerIdFromPrivateKey
insteadChange checklist