Hi Inga... I will correct the various spelling issues, and perhaps also segment the patches as you suggest
On Tue, 2019-05-21 at 23:44 -0700, Stotland, Inga wrote:
> On Tue, 2019-05-21 at 10:01 -0700, Brian Gix wrote:
> >
> > + struct mesh_node *node = user_data;
> > + uint8_t key[16];
> > uint16_t net_idx;
> >
> > - if (!l_dbus_message_get_arguments(msg, "q", &net_idx))
> > + if (!l_dbus_message_get_arguments(msg, "q", &net_idx) ||
> > !net_idx)
>
>
> Maybe use explicit value for net_idx check, i.e.,
> net_idx == MESH_PRIMARY_NET_INDEX_DEFAULT
>
> Currently DEFAULT_PRIMARY_NET_INDEX is defined in node.c
> WE should move it into mesh-defs.h and rename accordingly
I can do this... Although I don't know that we will ever have any other Net Index except 0 ever be defined by
the spec as "Primary".
>
> > - if (!l_dbus_message_get_arguments(msg, "q", &net_idx))
> > + if (!l_dbus_message_get_arguments(msg, "q", &net_idx) ||
> > + net_idx > MAX_KEY_IDX)
> > return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
> >
> > - /* TODO */
> > - return dbus_error(msg, MESH_ERROR_NOT_IMPLEMENTED, NULL);
> > + if (!keyring_get_net_key(node, net_idx, &key))
> > + return dbus_error(msg, MESH_ERROR_DOES_NOT_EXIST,
> > NULL);
> > +
> > + switch (key.phase) {
> > + case KEY_REFRESH_PHASE_NONE:
> > + /* Generate Key and update phase */
> > + l_getrandom(key.new_key, sizeof(key.new_key));
> > + key.phase = KEY_REFRESH_PHASE_ONE;
>
> line break?
>
> > + if (!keyring_put_net_key(node, net_idx, &key))
> > + return dbus_error(msg, MESH_ERROR_FAILED,
> > NULL);
>
> Wouldn't we want to revert the key phase back to KEY_REFRESH_PHASE_NONE
> in case of failure?
I don't think this is neccessary. This is the only place in the function where a *Change* is made to the
persistent data... If it fails then it fails, and the old data will still be there.
>
> > +static struct l_dbus_message *store_new_appkey(struct mesh_node
> > *node,
> > + struct l_dbus_message *msg,
> > + uint16_t net_idx, uint16_t
> > app_idx,
> > + uint8_t *new_key)
> > +{
> > + struct keyring_net_key net_key;
> > + struct keyring_app_key app_key;
> > +
> > + if (net_idx > MAX_KEY_IDX || app_idx > MAX_KEY_IDX)
> > + return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
> > +
> > + if (!keyring_get_net_key(node, net_idx, &net_key))
> > + return dbus_error(msg, MESH_ERROR_DOES_NOT_EXIST,
> > NULL);
>
> NULL -> "Bound net key not found"
> Also, I don't think "org.bluez.mesh.Error.DoesNotExist" error is
> documented in mesh-api.txt for either CreateAppKey() or ImportAppKey(),
> needs to be added.
Will add
>
> >
> > static struct l_dbus_message *update_appkey_call(struct l_dbus
> > *dbus,
> > struct l_dbus_message
> > *msg,
> > void *user_data)
> > {
> > - uint16_t net_idx, app_idx;
> > + struct mesh_node *node = user_data;
> > + struct keyring_net_key net_key;
> > + struct keyring_app_key app_key;
> > + uint16_t app_idx;
> >
> > - if (!l_dbus_message_get_arguments(msg, "qq", &net_idx,
> > &app_idx))
> > + if (!l_dbus_message_get_arguments(msg, "q", &app_idx) ||
> > + app_idx > MAX_KEY_IDX)
> > return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
> >
> > - /* TODO */
> > - return dbus_error(msg, MESH_ERROR_NOT_IMPLEMENTED, NULL);
> > + if (!keyring_get_app_key(node, app_idx, &app_key) ||
> > + !keyring_get_net_key(node, app_key.net_idx,
> > &net_key))
> > + return dbus_error(msg, MESH_ERROR_DOES_NOT_EXIST,
> > NULL);
> > +
> > + switch (net_key.phase) {
> > + case KEY_REFRESH_PHASE_NONE:
> > + case KEY_REFRESH_PHASE_ONE:
>
> What if the net key is in key refresh phase one and This method has
> been called mre than once? i.e., do we want to protect the application
> from potentially overwriting already updated application key?
Well, Application keys don't actually have a phase... It is inherited from the bound net key. The controlling
Config Client app needs to set up *all* the updates in the keyring before starting to send the updates to the
remote nodes. We can warn in the mesh-api.txt the correct order of things, but as long as the App does not
start sending the updates over-the-air, there is no problem with Updating the key-ring's App Key redundantly
prior to starting to propagate. As long as the *spec* is followed, we don't have a problem.
>
> >
> > static struct l_dbus_message *set_key_phase_call(struct l_dbus
> > *dbus,
> > struct l_dbus_message
> > *msg,
> > void *user_data)
> > {
> > + struct mesh_node *node = user_data;
> > + struct keyring_net_key key;
> > uint16_t net_idx;
> > uint8_t phase;
> >
> > - if (!l_dbus_message_get_arguments(msg, "qy", &net_idx, &phase))
> > + if (!l_dbus_message_get_arguments(msg, "qy", &net_idx, &phase)
> > > >
> >
> > + phase == KEY_REFRESH_PHASE_ONE
> > > >
> >
> > + phase >
> > KEY_REFRESH_PHASE_THREE)
> > return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
> >
> > - /* TODO */
> > - return dbus_error(msg, MESH_ERROR_NOT_IMPLEMENTED, NULL);
> > + if (!keyring_get_net_key(node, net_idx, &key))
> > + return dbus_error(msg, MESH_ERROR_DOES_NOT_EXIST,
> > NULL);
> > +
> > + if (phase == KEY_REFRESH_PHASE_THREE &&
> > + key.phase !=
> > KEY_REFRESH_PHASE_NONE) {
> > + memcpy(key.old_key, key.new_key, 16);
> > + key.phase = KEY_REFRESH_PHASE_NONE;
> > + } else
> > + key.phase = phase;
> > +
> > + if (!keyring_put_net_key(node, net_idx, &key))
> > + return dbus_error(msg, MESH_ERROR_FAILED, NULL);
>
> Restore the phase to the previous value in case of failure?
As with the above comment, a failure is a failure... If this method fails for *any* reason, it means nothing
about the keys were updated, so there is nothing to restore.
>
> >
> Thank you,
>
>
> Inga
