Hi Michal, > From: Michał Lowas-Rzechonek [mailto:michal.lowas-rzechonek@xxxxxxxxxxx] > > +try_remote: > > + /* Try remote device key if available */ > > + dev_key = node_get_remote_device_key(node, src); > > + if (!dev_key) > > + return -1; > > + > > + if (mesh_crypto_payload_decrypt(NULL, 0, data, size, szmict, src, > > + dst, key_id, seq, iv_idx, out, > dev_key)) > > + return APP_IDX_DEV_RMT; > > + > What's the purpose of APP_IDX_DEV_RMT? App Index is an internal way to track which credentials were used to decrypt a message. In theory, if the incoming message used the local device key, then the message is addressed to one of the local servers, and if the incoming message was decrypted by a *remote* device key it should be addressed to a local Client (as a response from a remote server). However, also importantly, if a message that requires a response is received on *any* key (local dev, remote dev, or App Key) the response is supposed to be sent with the same key. To enable that, we need to differentiate between the two possible device keys. In any case, these "Magic Numbers" are internal use only, and do not show up on the external interface. When *sending* Dev Key message from the Application, we will try the remote nodes device key first, and if it does not exist we fall back to the local key. > > I'm not a fan of magic values (and I hope APP_IDX_DEV goes away afterw we > manage to implement proposed Config/Provisioning API), but at least > APP_IDX_DEV is used to indicate 'privileged' loopback messages. > > Adding IDX_DEV_RMT doesn't seem necessary. > > > diff --git a/mesh/node.c b/mesh/node.c index 157991dad..fe0488108 > > 100644 > > --- a/mesh/node.c > > +++ b/mesh/node.c > > @@ -54,6 +54,8 @@ > > #define DEFAULT_CRPL 10 > > #define DEFAULT_SEQUENCE_NUMBER 0 > > > > +#define REMOTE_CACHE_LEN 5 > By the way: why the limits here are set so low (and there doesn't seem to be > a way to change them...)? I don't think we should put artificial constraints on > network size, so both CRPL and remote cache could safely be set to cover full > unicast space (32768). In any practical application, the device running Linux + > bluetooth will have more than enough RAM for this. Under normal circumstances (a non-Provisioner node for example) There will only ever be the local device key available, and so the size of the remote cache is moot. When being used as a Config Client, we are just trying to avoid reopening the file to fetch the device key to decode the expected response. I was thinking of actually making it *smaller*, but there will be times (like during a key update) that multiple messages to multiple servers will be "in flight" simultaneously. However in most cases the configuration messages are rarely used outside of initial node setup. > > Getting rid of these constraints would make the implementation > substiantially simpler (no MRU, for example). Increasing the Cache increases (in an open-ended way, depending on number of remote nodes) RAM usage for something that as stated above is rarely used. All the device keys are always available... they just may need to be read into RAM from storage. > > FYI, we're running networks with couples of hundreds devices, and plan to > expand them even further. > > > +/* This is a thread-safe always malloced version of dirname which > > +will work > > + * regardless of which underlying dirname implimentation is used */ > > +static char *alloc_dirname(const char *path) { > I was under impression that mesh daemon operates inside a main loop, so it > doesn't need to worry about thread safety? > > > +bool storage_get_remote_dev_key(struct mesh_node *node, uint16_t > unicast, > > + uint8_t dev_key[16]) > > +{ > > (...) > > + dir_name = alloc_dirname(cfgname); > I don't see why the remote key storage should be kept separately (and in > binary files?) from the main node json. It makes the implementation much > more complicated, as you need to worry about directories etc. > > I believie that keeping remote keys in the same place, as a JSON object, > would be much simpler. Again, this creates an open-ended RAM requirement that depends on the existence of external resources.... What if there are only 10 remote nodes? Do we still commit enough RAM to handle 32,767 remote nodes just in case? This implementation leave the open-ended burden of storage out of RAM, and acknowledges that Config messages, while important, are not the regular traffic... Things like generic On/Off and Level messages should make up 99% of normal traffic, and those are sent with normal App Keys. > > regards > -- > Michał Lowas-Rzechonek <michal.lowas-rzechonek@xxxxxxxxxxx> > Silvair http://silvair.com > Jasnogórska 44, 31-358 Krakow, POLAND
