Re: [PATCH BlueZ] mesh: Add App and Net Key Refresh keyring back-end

["Gix, Brian" <brian.gix@intel.com>]

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