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

Refactor NodeId as a proxy/singleton instance #254

Closed
joshuakarp opened this issue Oct 21, 2021 · 12 comments · Fixed by #310 or #318
Closed

Refactor NodeId as a proxy/singleton instance #254

joshuakarp opened this issue Oct 21, 2021 · 12 comments · Fixed by #310 or #318
Assignees
Labels
development Standard development discussion Requires discussion epic Big issue with multiple subissues r&d:polykey:core activity 3 Peer to Peer Federated Hierarchy

Comments

@joshuakarp
Copy link
Contributor

joshuakarp commented Oct 21, 2021

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

Tasks

  1. ...
  2. ...
  3. ...
@CMCDragonkai
Copy link
Member

#312 has discussion about propagating changes of the key reset. Which has implications regarding how the NodeId is being acquired.

@CMCDragonkai
Copy link
Member

Some notes about encoded form of the NodeId and VaultId:

  1. NodeId - should be using base32hex - 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, that Id itself is already fixed size buffer.
  2. VaultId - should be using base58btc - we actually prefer base58btc for most encodings anyway (using base64 when 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
Copy link
Member

@tegefaulkes another important point is generation.

  1. NodeId is generated by public key creation. There's a buffer form of public keys. I just realised that because it may not fit the size of Id, then it actually doesn't quite make sense to make it an Id.
  2. VaultId is generated by IdRandom
  3. Other Ids may be using IdSortable.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Jan 19, 2022

So the important function here is in keys/utils.ts:

function publicKeyToFingerprint(publicKey: PublicKey): PublicKeyFingerprint {
  const fString = publicKeyToFingerprintBytes(publicKey);
  const fTypedArray = forgeUtil.binary.raw.decode(fString);
  const f = makeNodeId(fTypedArray);
  return f;
}

It looks like this has been changed to have makeNodeId included which took the binary array and encoded it as base32hex.

This can be vastly simplified.

  1. The public key is ASN1 format. There is a "buffer" version of the public key depends on the asymmetric crypto we are using. Because we are using RSA, public key size can be different size. And when we change to Ed25519, then it will be 32 bytes. Anyway, this means there's no need to decode the bytes, if we don't encode the bytes into a string in the first place. We just keep it as bytes.
  2. We deliberately use the publicKeyToFingerprintBytes which 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 in publicKeyToFingerprintBytes. This keeps the key size to about 32 bytes which enables us to move to using ed25519 eventually.
  3. So NodeId is therefore meant to be 32 bytes, and does not fit the standard of js-id where the Id is supposed to be 16 bytes. Note that IdInternal does not have this constraint, it's the utility functions for js-id that enforces 16 bytes. So even if we could use IdInternal for wrapping the NodeId, it's not a good idea since users would think that you could use idUtils on it.
  4. This means NodeId requires its own type/class which should be similar to IdInternal and Id. Although it may be better to remove the 16 byte constraint from the js-id utils, so that way we can reuse the IdInternal class and Id type from js-id.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Jan 19, 2022

The reason the 16 byte limit is in the js-id utils 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.toUUID check 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-id to 3.1.0 and then reuse Id for NodeId. Then we can just use idUtils.fromBuffer to convert the public key fingerprint as discussed above into IdInternal.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Jan 19, 2022

Subsequently this would mean that NodeId is IdInternal and 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 nodes domain rather than keys domain.

@CMCDragonkai
Copy link
Member

New PR here MatrixAI/js-id#10 to address this for js-id.

@CMCDragonkai
Copy link
Member

Released 3.1.0 of js-id. This should be possible now:

  const fString = publicKeyToFingerprintBytes(publicKey);
  const fTypedArray = forgeUtil.binary.raw.decode(fString);
  const nodeId = Id.create(fTypedArray);

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Jan 19, 2022

Here's a demonstration on how it can work:

import { util as forgeUtil } from 'node-forge';
import { IdInternal, utils as idUtils } from '@matrixai/id';
import * as keysUtils from './src/keys/utils';

async function main() {
  const keyPair = await keysUtils.generateKeyPair(2048);
  const fString = keysUtils.publicKeyToFingerprintBytes(keyPair.publicKey);
  const fTypedArray = forgeUtil.binary.raw.decode(fString);
  // This returns the type of `Id`, not type of `IdInternal`
  const nodeId = IdInternal.create(fTypedArray);
  console.log(nodeId);
  const pojo = {
    [nodeId]: 'hello world'
  };
  console.log(pojo[nodeId]);
  const nodeIdEncoded = idUtils.toMultibase(nodeId, 'base32hex');
  console.log(nodeIdEncoded);
  console.log(idUtils.fromMultibase(nodeIdEncoded));
}

main();

Which outputs like this:

[nix-shell:~/Projects/js-polykey]$ npm run ts-node -- ./test-ids.ts 

> @matrixai/[email protected] ts-node /home/cmcdragonkai/Projects/js-polykey
> ts-node --require tsconfig-paths/register "./test-ids.ts"

IdInternal(32) [Uint8Array] [
  117, 235, 209, 99, 170, 124,  67, 184,
    9, 234, 152, 45, 117, 166, 112, 107,
  126, 137, 100, 22,  69, 164,  54, 203,
   12,  29, 107, 44, 156, 165,  76,   5
]
hello world
venlt2otafh1rg2faj0mnb9jgddv8ip0m8mi3dioc3llip7559g2g
IdInternal(32) [Uint8Array] [
  117, 235, 209, 99, 170, 124,  67, 184,
    9, 234, 152, 45, 117, 166, 112, 107,
  126, 137, 100, 22,  69, 164,  54, 203,
   12,  29, 107, 44, 156, 165,  76,   5
]

@CMCDragonkai
Copy link
Member

There's still the question of where node id is acquired.

So far I like the idea of KeyManager focusing 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.

  • The certificate inside the key manager put the encoded node Id in one of the relevant fields. I think this is in the "common name". This is already done.
  • The extraction of node id from certificates is in a utility function put into 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.
  • Where should encodeNodeId and decodeNodeId that fixes base32hex as the multibase option during encoding be placed? At first it seems that nodes/utils.ts should be where it is, but if KeyManager needs this during certificate generation, then it should be in keys. We just want to avoid mutual recursion between nodesUtils and keysUtils.
  • Finally the call getNodeId doesn't fit in the nodes domain, because the nodes domain doesn't manage itself. The getNodeId of the current agent does make sense to put in KeyManager. But nodes domain may use other node ids. It's important to realise that NodeId is just a type, and other node's ids can be a value of this type too.

@CMCDragonkai
Copy link
Member

@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
Copy link
Member

Ideas for refactoring NodeId with respect to js-id is here: #299 (comment).

#299 is a more general issue addressing the js-id usage everywhere in PK. But this issue can be fixed by the new PR that makes the relevant changes to NodeId.

@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 inside nodes/utils.ts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development discussion Requires discussion epic Big issue with multiple subissues r&d:polykey:core activity 3 Peer to Peer Federated Hierarchy
3 participants