Repository navigation
Refactor NodeId as a proxy/singleton instance #254
Description
Specification
The node ID of a Polykey keynode is the public key fingerprint (based on the root keypair of the keynode). As such, on a key renewal/refresh, the node ID of the keynode will also change. Therefore, the node ID can be seen as dynamic state.
Currently we've had to be careful when we retrieve the node ID. We shouldn't be storing it as local, static state, otherwise this state needs to be updated when the root key changes.
The current solution has been to inject the KeyManager into any class where we require the node ID to be retrieved, such that we can call getNodeId to compute the node ID directly from the root key. This isn't the ideal solution: we shouldn't have to expose all of the KeyManager whenever we need just the node ID.
We should look into changing the node ID to be similar to the Id class in js-id, or making use of the js-permaproxy library to do this (https://github.com/matrixai/js-permaproxy). Therefore, only the node ID's proxy instance would need to be injected where we need the node ID.
Would we expose any other functionality other than a getter? If we wanted to do an "update" functionality, we'd actually need to perform an update on the root key itself (given that the node ID is derived from the public key). So perhaps instead, we could look into abstracting the root key as a proxy, such that a key update/renewal can be called without the KeyManager... this is potentially completely unnecessary though.
Additional context
- original discussion of changing the node ID to be a proxy/singleton instance https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/205#note_708488228
- previous thread from !209 (nodes claims) discussing the problems arising in the
nodesdomain when injecting the node ID as a local, static property https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/209#note_675531967 - Propagate root keypair changes to domains external to
KeyManager#312 - Propagate keypair changes to other domains - Review js-id usage for NodeId, VaultId, NotificationId, PermId, ClaimId, Gestalts and
GenericIdTypes.ts#299 - NodeId applied to thejs-id
Tasks
- ...
- ...
- ...
Activity
CMCDragonkai commentedon Jan 10, 2022
#312 has discussion about propagating changes of the key reset. Which has implications regarding how the NodeId is being acquired.
CMCDragonkai commentedon Jan 19, 2022
Some notes about encoded form of the
NodeIdandVaultId:NodeId- should be usingbase32hex- because this is fixed size and it preserves lexicographic order. Fixed size was originally due to node graph requirements, but this is less important now, thatIditself is already fixed size buffer.VaultId- should be usingbase58btc- we actually preferbase58btcfor most encodings anyway (usingbase64when required by a particular spec). The vault ids are random, so lexicographic order is not important, and fixed size is also not important.When sortable base64 is available, we can switch to that for
NodeId. Base evolution will be fine because our decoders always accept any multibase encoding. It's our encoders that specify these settings.CMCDragonkai commentedon Jan 19, 2022
@tegefaulkes another important point is generation.
NodeIdis generated by public key creation. There's a buffer form of public keys. I just realised that because it may not fit the size ofId, then it actually doesn't quite make sense to make it anId.VaultIdis generated byIdRandomIdSortable.CMCDragonkai commentedon Jan 19, 2022
So the important function here is in
keys/utils.ts:It looks like this has been changed to have
makeNodeIdincluded which took the binary array and encoded it asbase32hex.This can be vastly simplified.
publicKeyToFingerprintByteswhich uses some standard RSA algorithm to essentially hash the public key, this is using the "SubjectPublicKeyInfo" standard defined in X509 PKi standards. This i currently programmed inpublicKeyToFingerprintBytes. This keeps the key size to about 32 bytes which enables us to move to using ed25519 eventually.NodeIdis therefore meant to be 32 bytes, and does not fit the standard ofjs-idwhere theIdis supposed to be 16 bytes. Note thatIdInternaldoes not have this constraint, it's the utility functions forjs-idthat enforces 16 bytes. So even if we could useIdInternalfor wrapping theNodeId, it's not a good idea since users would think that you could useidUtilson it.NodeIdrequires its own type/class which should be similar toIdInternalandId. Although it may be better to remove the 16 byte constraint from thejs-idutils, so that way we can reuse theIdInternalclass andIdtype fromjs-id.CMCDragonkai commentedon Jan 19, 2022
The reason the 16 byte limit is in the
js-idutils was due to the UUID conversion. UUID only works on 16 bytes as a standard.If UUID doesn't matter, then the 16 byte requirement can be dropped.
What we could do is make
idUtils.toUUIDcheck if the ID is within 16 bytes, and then throw an error if it cannot be made into a UUID.All other conversion functions do not have a requirement to be 16 bytes.
If we do this, we can update
js-idto3.1.0and then reuseIdforNodeId. Then we can just useidUtils.fromBufferto convert the public key fingerprint as discussed above intoIdInternal.CMCDragonkai commentedon Jan 19, 2022
Subsequently this would mean that
NodeIdisIdInternaland no longer a string.Thus when we want to use it as a string, we must first encode it.
The encoding functions can take place inside
nodesdomain rather thankeysdomain.CMCDragonkai commentedon Jan 19, 2022
New PR here MatrixAI/js-id#10 to address this for
js-id.CMCDragonkai commentedon Jan 19, 2022
Released
3.1.0ofjs-id. This should be possible now:CMCDragonkai commentedon Jan 19, 2022
Here's a demonstration on how it can work:
Which outputs like this:
CMCDragonkai commentedon Jan 19, 2022
There's still the question of where node id is acquired.
So far I like the idea of
KeyManagerfocusing on keys. So you can get the root key pair from there.But the conversion to node id is something that should be in the nodes domain. There's some functionality that's sprawled out.
network/utils.ts- this seems like something that should be in the keys utils, given that key manager is aware of both node ids and also certificates and the relevant field to pull it from.encodeNodeIdanddecodeNodeIdthat fixesbase32hexas the multibase option during encoding be placed? At first it seems thatnodes/utils.tsshould be where it is, but ifKeyManagerneeds this during certificate generation, then it should be inkeys. We just want to avoid mutual recursion betweennodesUtilsandkeysUtils.getNodeIddoesn't fit in the nodes domain, because the nodes domain doesn't manage itself. ThegetNodeIdof the current agent does make sense to put inKeyManager. But nodes domain may use other node ids. It's important to realise thatNodeIdis just a type, and other node's ids can be a value of this type too.CMCDragonkai commentedon Jan 19, 2022
@tegefaulkes suggests that we can inject just a lambda that acquires the node id. That way you don't pass the key manager around, but you also don't pass a static node id around. You pass a lambda that must be called to acquire the node id when you need it.
The lambda can be easily created as
const getNodeId = keyManager.getNodeId.bind(keyManager);.CMCDragonkai commentedon Jan 20, 2022
Ideas for refactoring
NodeIdwith respect tojs-idis here: #299 (comment).#299 is a more general issue addressing the
js-idusage everywhere in PK. But this issue can be fixed by the new PR that makes the relevant changes toNodeId.@tegefaulkes has some ideas regarding how to use generics for
NodeId, but if it's too complicated, it can just be done with just the regular encoding functions put insidenodes/utils.ts.