Hi Brian,

On Tue, 2019-05-21 at 10:01 -0700, Brian Gix wrote:
> Fill in skeleton App Key and Net Key KeyRing methods that are
> affected
> by the Key Refresh Procedure.

Would be nice to list the implemented methods here.
I would strongly recommned breaking this in to two, maybe even three
patches:
net key methods, app key methods, phase method


> ---
>  doc/mesh-api.txt |   2 +
>  mesh/manager.c   | 198
> ++++++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 170 insertions(+), 30 deletions(-)
> 
> diff --git a/doc/mesh-api.txt b/doc/mesh-api.txt
> index 112990a5d..9644553e8 100644
> --- a/doc/mesh-api.txt
> +++ b/doc/mesh-api.txt
> @@ -434,6 +434,7 @@ Methods:
>  		PossibleErrors:
>  			org.bluez.mesh.Error.InvalidArguments
>  			org.bluez.mesh.Error.DoesNotExist
> +			org.bluez.mesh.Error.Busy
>  
>  	void DeleteSubnet(uint16 net_index)
>  
> @@ -527,6 +528,7 @@ Methods:
>  		PossibleErrors:
>  			org.bluez.mesh.Error.InvalidArguments
>  			org.bluez.mesh.Error.DoesNotExist
> +			org.bluez.mesh.Error.Busy
>  
>  	void DeleteAppKey(uint16 app_index)
>  
> diff --git a/mesh/manager.c b/mesh/manager.c
> index d990ceec2..8ef32d735 100644
> --- a/mesh/manager.c
> +++ b/mesh/manager.c
> @@ -24,9 +24,12 @@
>  #define _GNU_SOURCE
>  #include <ell/ell.h>
>  
> +#include "mesh/mesh-defs.h"
>  #include "mesh/dbus.h"
>  #include "mesh/error.h"
>  #include "mesh/mesh.h"
> +#include "mesh/node.h"
> +#include "mesh/keyring.h"
>  #include "mesh/manager.h"
>  
>  static struct l_dbus_message *add_node_call(struct l_dbus *dbus,
> @@ -86,51 +89,110 @@ static struct l_dbus_message
> *cancel_scan_call(struct l_dbus *dbus,
>  	return dbus_error(msg, MESH_ERROR_NOT_IMPLEMENTED, NULL);
>  }
>  
> +static struct l_dbus_message *store_new_subnet(struct mesh_node
> *node,
> +					struct l_dbus_message *msg,
> +					uint16_t net_idx, uint8_t
> *new_key)
> +{
> +	struct keyring_net_key key;
> +
> +	if (net_idx > MAX_KEY_IDX)
> +		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
> +
> +	if (keyring_get_net_key(node, net_idx, &key)) {
> +		/* Allow redundent calls only if they match */

Allow redundant calls only if the key values match

> +		if (!memcmp(key.old_key, new_key, 16))
> +			return l_dbus_message_new_method_return(msg);
> +
> +		return dbus_error(msg, MESH_ERROR_ALREADY_EXISTS,
> NULL);
> +	}
> +
> +	memcpy(key.old_key, new_key, 16);
> +	key.net_idx = net_idx;
> +	key.phase = KEY_REFRESH_PHASE_NONE;
> +
> +	if (!keyring_put_net_key(node, net_idx, &key))
> +		return dbus_error(msg, MESH_ERROR_FAILED, NULL);
> +
> +	return l_dbus_message_new_method_return(msg);
> +}
> +
>  static struct l_dbus_message *create_subnet_call(struct l_dbus
> *dbus,
>  						struct l_dbus_message
> *msg,
>  						void *user_data)
>  {
> +	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

>  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
>  
> -	/* TODO */
> -	return dbus_error(msg, MESH_ERROR_NOT_IMPLEMENTED, NULL);
> +	/* Generate key and submit */
> +	l_getrandom(key, sizeof(key));
> +
> +	return store_new_subnet(node, msg, net_idx, key);
>  }
>  
>  static struct l_dbus_message *update_subnet_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;
>  
> -	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?

> +
> +		/* Fall Through */
> +
> +	case KEY_REFRESH_PHASE_ONE:
> +		/* Allow redundent calls to start Key Refresh */

"redundant"

> +		return l_dbus_message_new_method_return(msg);
> +
> +	default:
> +		break;
> +	}
> +
> +	/* All other phases mean KR already in progress over-the-air */
> +	return dbus_error(msg, MESH_ERROR_BUSY, "KR in progress");

KR -> Key refresh

>  }
>  
>  static struct l_dbus_message *delete_subnet_call(struct l_dbus
> *dbus,
>  						struct l_dbus_message
> *msg,
>  						void *user_data)
>  {
> +	struct mesh_node *node = user_data;
>  	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 > MAX_KEY_IDX)
>  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
>  
> -	/* TODO */
> -	return dbus_error(msg, MESH_ERROR_NOT_IMPLEMENTED, NULL);
> +	keyring_del_net_key(node, net_idx);
> +
> +	return l_dbus_message_new_method_return(msg);
>  }
>  
>  static struct l_dbus_message *import_subnet_call(struct l_dbus
> *dbus,
>  						struct l_dbus_message
> *msg,
>  						void *user_data)
>  {
> -	uint16_t net_idx;
> +	struct mesh_node *node = user_data;
>  	struct l_dbus_message_iter iter_key;
> +	uint16_t net_idx;
>  	uint8_t *key;
>  	uint32_t n;
>  
> @@ -142,55 +204,116 @@ static struct l_dbus_message
> *import_subnet_call(struct l_dbus *dbus,
>  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
>  							"Bad network
> key");
>  
> -	/* TODO */
> -	return dbus_error(msg, MESH_ERROR_NOT_IMPLEMENTED, NULL);
> +	return store_new_subnet(node, msg, net_idx, key);
> +}
> +
> +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.

> +
> +	if (keyring_get_app_key(node, app_idx, &app_key)) {
> +		/* Allow redundent calls with identical data */

"Allow redundant calls with identical values"

> +		if (!memcmp(app_key.old_key, new_key, 16) &&
> +						app_key.net_idx ==
> net_idx)
> +			return l_dbus_message_new_method_return(msg);
> +
> +		return dbus_error(msg, MESH_ERROR_ALREADY_EXISTS,
> NULL);
> +	}
> +
> +	memcpy(app_key.old_key, new_key, 16);
> +	app_key.net_idx = net_idx;
> +	app_key.app_idx = app_idx;
> +
> +	if (!keyring_put_app_key(node, app_idx, net_idx, &app_key))
> +		return dbus_error(msg, MESH_ERROR_FAILED, NULL);
> +
> +	return l_dbus_message_new_method_return(msg);
>  }
>  
>  static struct l_dbus_message *create_appkey_call(struct l_dbus
> *dbus,
>  						struct l_dbus_message
> *msg,
>  						void *user_data)
>  {
> +	struct mesh_node *node = user_data;
>  	uint16_t net_idx, app_idx;
> +	uint8_t key[16];
>  
>  	if (!l_dbus_message_get_arguments(msg, "qq", &net_idx,
> &app_idx))
>  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
>  
> -	/* TODO */
> -	return dbus_error(msg, MESH_ERROR_NOT_IMPLEMENTED, NULL);
> +	l_getrandom(key, sizeof(key));
> +
> +	return store_new_appkey(node, msg, net_idx, app_idx, key);
>  }
>  
>  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?

> +		/* Generate Key if in acceptable phase */
> +		l_getrandom(app_key.new_key, sizeof(app_key.new_key));

line break

> +		if (!keyring_put_app_key(node, app_idx,
> app_key.net_idx,
> +								&app_ke
> y))
> +			return dbus_error(msg, MESH_ERROR_FAILED,
> NULL);
> +
> +		return l_dbus_message_new_method_return(msg);
> +
> +	default:
> +		break;
> +	}
> +
> +	/* Don't allow Updates from invalid phase */
> +	return dbus_error(msg, MESH_ERROR_BUSY, "KR in progress");

KR -> Key refresh

>  }
>  
>  static struct l_dbus_message *delete_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;
> +	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))
>  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
>  
> -	/* TODO */
> -	return dbus_error(msg, MESH_ERROR_NOT_IMPLEMENTED, NULL);
> +	keyring_del_app_key(node, app_idx);
> +
> +	return l_dbus_message_new_method_return(msg);
>  }
>  
>  static struct l_dbus_message *import_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 l_dbus_message_iter iter_key;
> +	uint16_t net_idx, app_idx;
>  	uint8_t *key;
>  	uint32_t n;
>  
> @@ -203,22 +326,37 @@ static struct l_dbus_message
> *import_appkey_call(struct l_dbus *dbus,
>  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
>  							"Bad
> application key");
>  
> -	/* TODO */
> -	return dbus_error(msg, MESH_ERROR_NOT_IMPLEMENTED, NULL);
> +	return store_new_appkey(node, msg, net_idx, app_idx, key);
>  }
>  
>  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?

> +
> +	return l_dbus_message_new_method_return(msg);
>  }
>  
>  static void setup_management_interface(struct l_dbus_interface
> *iface)
> @@ -242,9 +380,9 @@ static void setup_management_interface(struct
> l_dbus_interface *iface)
>  	l_dbus_interface_method(iface, "CreateAppKey", 0,
> create_appkey_call,
>  					"", "qq", "", "net_index",
> "app_index");
>  	l_dbus_interface_method(iface, "UpdateAppKey", 0,
> update_appkey_call,
> -					"", "qq", "", "net_index",
> "app_index");
> +					"", "q", "", "app_index");
>  	l_dbus_interface_method(iface, "DeleteAppKey", 0,
> delete_appkey_call,
> -					"", "qq", "", "net_index",
> "app_index");
> +					"", "q", "", "app_index");
>  	l_dbus_interface_method(iface, "ImportAppKey", 0,
> import_appkey_call,
>  				"", "qqay", "", "net_index",
> "app_index",
>  								"app_ke
> y");


Thank you,


Inga