* [PATCH BlueZ v3 0/3] mesh: Fix Remote/Local dev key usage
@ 2019-09-26 18:14 Brian Gix
2019-09-26 18:14 ` [PATCH BlueZ v3 1/3] mesh: Add remote boolean to DevKey transactions Brian Gix
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Brian Gix @ 2019-09-26 18:14 UTC (permalink / raw)
To: linux-bluetooth; +Cc: brian.gix, inga.stotland, michal.lowas-rzechonek
V3: By popular demand, the name "remote" is now used for both DevKeySend()
and DevKeyMessageReceived().
In DevKeySend(), setting remote == true means that the Key Ring *must* be
used to encrypt the outgoing message, and a failure will be returned if
the requested destination address does not include a device key in the
local key ring. For remote == false requests, the request will be rejected
if the destination is an element on the local node.
In DevKeyMessageReceived(), the remote boolean will be set == true if it
required the key ring to decrypot the message. If remote == false, this
means that the local nodes Device Key successfully decrypted the message,
and the message may be used to change or query privileged states.
Brian Gix (3):
mesh: Add local/remote bools to DevKey transactions
mesh: Use explicit Local vs Remote Device key usage
mesh: Fix Key Ring permissions for local nodes
doc/mesh-api.txt | 17 ++++++++++++++---
mesh/manager.c | 5 -----
mesh/model.c | 11 +++++++----
mesh/node.c | 40 +++++++++++++++-------------------------
4 files changed, 36 insertions(+), 37 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH BlueZ v3 1/3] mesh: Add remote boolean to DevKey transactions 2019-09-26 18:14 [PATCH BlueZ v3 0/3] mesh: Fix Remote/Local dev key usage Brian Gix @ 2019-09-26 18:14 ` Brian Gix 2019-09-26 18:14 ` [PATCH BlueZ v3 2/3] mesh: Explicit Remote/Local Device key usage Brian Gix ` (3 subsequent siblings) 4 siblings, 0 replies; 10+ messages in thread From: Brian Gix @ 2019-09-26 18:14 UTC (permalink / raw) To: linux-bluetooth; +Cc: brian.gix, inga.stotland, michal.lowas-rzechonek DevKey operations require authorization on the part of the applications making the requests. Messages to state changing Servers should use device keys from the remote (destination) to demonstrate authorization. --- doc/mesh-api.txt | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/doc/mesh-api.txt b/doc/mesh-api.txt index 9b9f4e3de..a589616eb 100644 --- a/doc/mesh-api.txt +++ b/doc/mesh-api.txt @@ -245,7 +245,7 @@ Methods: org.bluez.mesh.Error.InvalidArguments org.bluez.mesh.Error.NotFound - void DevKeySend(object element_path, uint16 destination, + void DevKeySend(object element_path, uint16 destination, boolean remote, uint16 net_index, array{byte} data) This method is used to send a message originated by a local @@ -259,6 +259,12 @@ Methods: destination must be a uint16 to a unicast address, or a well known group address. + The remote parameter, if true, looks up the device key by the + destination address in the key database to encrypt the message. + If remote is true, but requested key does not exist, a NotFound + error will be returned. If set to false, the local node's + device key is used. + The net_index parameter is the subnet index of the network on which the message is to be sent. @@ -782,8 +788,8 @@ Methods: The data parameter is the incoming message. - void DevKeyMessageReceived(uint16 source, uint16 net_index, - array{byte} data) + void DevKeyMessageReceived(uint16 source, boolean remote, + uint16 net_index, array{byte} data) This method is called by meshd daemon when a message arrives addressed to the application, which was sent with the remote @@ -792,6 +798,11 @@ Methods: The source parameter is unicast address of the remote node-element that sent the message. + The remote parameter if true indicates that the device key + used to decrypt the message was from the sender. False + indicates that the local nodes device key was used, and the + message has permissions to modify local states. + The net_index parameter indicates what subnet the message was received on, and if a response is required, the same subnet must be used to send the response. -- 2.21.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH BlueZ v3 2/3] mesh: Explicit Remote/Local Device key usage 2019-09-26 18:14 [PATCH BlueZ v3 0/3] mesh: Fix Remote/Local dev key usage Brian Gix 2019-09-26 18:14 ` [PATCH BlueZ v3 1/3] mesh: Add remote boolean to DevKey transactions Brian Gix @ 2019-09-26 18:14 ` Brian Gix 2019-09-26 20:40 ` Stotland, Inga 2019-09-26 18:14 ` [PATCH BlueZ v3 3/3] mesh: Fix Key Ring permissions for local nodes Brian Gix ` (2 subsequent siblings) 4 siblings, 1 reply; 10+ messages in thread From: Brian Gix @ 2019-09-26 18:14 UTC (permalink / raw) To: linux-bluetooth; +Cc: brian.gix, inga.stotland, michal.lowas-rzechonek When sending or receiving Device Key (privileged) mesh messages, the remote vs local device key must be specified. This allows Apps to specify Key Ring stored device keys, and sanity checks that the correct key exists before allowing the transmission. Loopback messages to local servers *must* use keys from the Key Ring to indicate privilege has been granted. --- mesh/model.c | 11 +++++++---- mesh/node.c | 25 +++++++++++++++---------- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/mesh/model.c b/mesh/model.c index a06b684a5..e9b346102 100644 --- a/mesh/model.c +++ b/mesh/model.c @@ -735,14 +735,16 @@ static int add_sub(struct mesh_net *net, struct mesh_model *mod, } static void send_dev_key_msg_rcvd(struct mesh_node *node, uint8_t ele_idx, - uint16_t src, uint16_t net_idx, - uint16_t size, const uint8_t *data) + uint16_t src, uint16_t app_idx, + uint16_t net_idx, uint16_t size, + const uint8_t *data) { struct l_dbus *dbus = dbus_get_bus(); struct l_dbus_message *msg; struct l_dbus_message_builder *builder; const char *owner; const char *path; + bool remote = (app_idx != APP_IDX_DEV_LOCAL); owner = node_get_owner(node); path = node_get_element_path(node, ele_idx); @@ -758,6 +760,7 @@ static void send_dev_key_msg_rcvd(struct mesh_node *node, uint8_t ele_idx, builder = l_dbus_message_builder_new(msg); l_dbus_message_builder_append_basic(builder, 'q', &src); + l_dbus_message_builder_append_basic(builder, 'b', &remote); l_dbus_message_builder_append_basic(builder, 'q', &net_idx); dbus_append_byte_array(builder, data, size); @@ -936,8 +939,8 @@ bool mesh_model_rx(struct mesh_node *node, bool szmict, uint32_t seq0, else if (decrypt_idx == APP_IDX_DEV_REMOTE || (decrypt_idx == APP_IDX_DEV_LOCAL && mesh_net_is_local_address(net, src, 1))) - send_dev_key_msg_rcvd(node, i, src, 0, - forward.size, forward.data); + send_dev_key_msg_rcvd(node, i, src, decrypt_idx, + 0, forward.size, forward.data); } /* diff --git a/mesh/node.c b/mesh/node.c index b6824f505..833377e99 100644 --- a/mesh/node.c +++ b/mesh/node.c @@ -1976,7 +1976,8 @@ static struct l_dbus_message *dev_key_send_call(struct l_dbus *dbus, const char *sender, *ele_path; struct l_dbus_message_iter iter_data; struct node_element *ele; - uint16_t dst, net_idx, src; + uint16_t dst, app_idx, net_idx, src; + bool remote; uint8_t *data; uint32_t len; @@ -1987,8 +1988,12 @@ static struct l_dbus_message *dev_key_send_call(struct l_dbus *dbus, if (strcmp(sender, node->owner)) return dbus_error(msg, MESH_ERROR_NOT_AUTHORIZED, NULL); - if (!l_dbus_message_get_arguments(msg, "oqqay", &ele_path, &dst, - &net_idx, &iter_data)) + if (!l_dbus_message_get_arguments(msg, "oqbqay", &ele_path, &dst, + &remote, &net_idx, &iter_data)) + return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL); + + /* Loopbacks to local servers must use *remote* addressing */ + if (!remote && mesh_net_is_local_address(node->net, dst, 1)) return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL); ele = l_queue_find(node->elements, match_element_path, ele_path); @@ -1999,13 +2004,13 @@ static struct l_dbus_message *dev_key_send_call(struct l_dbus *dbus, src = node_get_primary(node) + ele->idx; if (!l_dbus_message_iter_get_fixed_array(&iter_data, &data, &len) || - !len || len > MAX_MSG_LEN) + !len || len > MAX_MSG_LEN) return dbus_error(msg, MESH_ERROR_INVALID_ARGS, "Incorrect data"); - /* TODO: use net_idx */ - if (!mesh_model_send(node, src, dst, APP_IDX_DEV_REMOTE, net_idx, - DEFAULT_TTL, data, len)) + app_idx = remote ? APP_IDX_DEV_REMOTE : APP_IDX_DEV_LOCAL; + if (!mesh_model_send(node, src, dst, app_idx, net_idx, DEFAULT_TTL, + data, len)) return dbus_error(msg, MESH_ERROR_NOT_FOUND, NULL); return l_dbus_message_new_method_return(msg); @@ -2226,9 +2231,9 @@ static void setup_node_interface(struct l_dbus_interface *iface) "element_path", "destination", "key_index", "data"); l_dbus_interface_method(iface, "DevKeySend", 0, dev_key_send_call, - "", "oqqay", "element_path", - "destination", "net_index", - "data"); + "", "oqbqay", "element_path", + "destination", "remote", + "net_index", "data"); l_dbus_interface_method(iface, "Publish", 0, publish_call, "", "oqay", "element_path", "model_id", "data"); l_dbus_interface_method(iface, "VendorPublish", 0, vendor_publish_call, -- 2.21.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH BlueZ v3 2/3] mesh: Explicit Remote/Local Device key usage 2019-09-26 18:14 ` [PATCH BlueZ v3 2/3] mesh: Explicit Remote/Local Device key usage Brian Gix @ 2019-09-26 20:40 ` Stotland, Inga 0 siblings, 0 replies; 10+ messages in thread From: Stotland, Inga @ 2019-09-26 20:40 UTC (permalink / raw) To: linux-bluetooth, Gix, Brian; +Cc: michal.lowas-rzechonek [-- Attachment #1: Type: text/plain, Size: 5433 bytes --] Hi Brian, On Thu, 2019-09-26 at 11:14 -0700, Brian Gix wrote: > When sending or receiving Device Key (privileged) mesh messages, the > remote vs local device key must be specified. This allows Apps to > specify Key Ring stored device keys, and sanity checks that the > correct > key exists before allowing the transmission. Loopback messages to > local > servers *must* use keys from the Key Ring to indicate privilege has > been > granted. > --- > mesh/model.c | 11 +++++++---- > mesh/node.c | 25 +++++++++++++++---------- > 2 files changed, 22 insertions(+), 14 deletions(-) > > diff --git a/mesh/model.c b/mesh/model.c > index a06b684a5..e9b346102 100644 > --- a/mesh/model.c > +++ b/mesh/model.c > @@ -735,14 +735,16 @@ static int add_sub(struct mesh_net *net, struct > mesh_model *mod, > } > > static void send_dev_key_msg_rcvd(struct mesh_node *node, uint8_t > ele_idx, > - uint16_t src, uint16_t net_idx, > - uint16_t size, const uint8_t > *data) > + uint16_t src, uint16_t app_idx, > + uint16_t net_idx, uint16_t > size, > + const uint8_t *data) > { > struct l_dbus *dbus = dbus_get_bus(); > struct l_dbus_message *msg; > struct l_dbus_message_builder *builder; > const char *owner; > const char *path; > + bool remote = (app_idx != APP_IDX_DEV_LOCAL); > > owner = node_get_owner(node); > path = node_get_element_path(node, ele_idx); > @@ -758,6 +760,7 @@ static void send_dev_key_msg_rcvd(struct > mesh_node *node, uint8_t ele_idx, > builder = l_dbus_message_builder_new(msg); > > l_dbus_message_builder_append_basic(builder, 'q', &src); > + l_dbus_message_builder_append_basic(builder, 'b', &remote); > l_dbus_message_builder_append_basic(builder, 'q', &net_idx); > dbus_append_byte_array(builder, data, size); > > @@ -936,8 +939,8 @@ bool mesh_model_rx(struct mesh_node *node, bool > szmict, uint32_t seq0, > else if (decrypt_idx == APP_IDX_DEV_REMOTE || > (decrypt_idx == APP_IDX_DEV_LOCAL && > mesh_net_is_local_address(net, src, > 1))) > - send_dev_key_msg_rcvd(node, i, src, 0, > - forward.size, > forward.data); > + send_dev_key_msg_rcvd(node, i, src, > decrypt_idx, > + 0, forward.size, > forward.data); > } > > /* > diff --git a/mesh/node.c b/mesh/node.c > index b6824f505..833377e99 100644 > --- a/mesh/node.c > +++ b/mesh/node.c > @@ -1976,7 +1976,8 @@ static struct l_dbus_message > *dev_key_send_call(struct l_dbus *dbus, > const char *sender, *ele_path; > struct l_dbus_message_iter iter_data; > struct node_element *ele; > - uint16_t dst, net_idx, src; > + uint16_t dst, app_idx, net_idx, src; > + bool remote; > uint8_t *data; > uint32_t len; > > @@ -1987,8 +1988,12 @@ static struct l_dbus_message > *dev_key_send_call(struct l_dbus *dbus, > if (strcmp(sender, node->owner)) > return dbus_error(msg, MESH_ERROR_NOT_AUTHORIZED, > NULL); > > - if (!l_dbus_message_get_arguments(msg, "oqqay", &ele_path, > &dst, > - &net_idx, > &iter_data)) > + if (!l_dbus_message_get_arguments(msg, "oqbqay", &ele_path, > &dst, > + &remote, &net_idx, > &iter_data)) > + return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL); > + > + /* Loopbacks to local servers must use *remote* addressing */ > + if (!remote && mesh_net_is_local_address(node->net, dst, 1)) > return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL); > > ele = l_queue_find(node->elements, match_element_path, > ele_path); > @@ -1999,13 +2004,13 @@ static struct l_dbus_message > *dev_key_send_call(struct l_dbus *dbus, > src = node_get_primary(node) + ele->idx; > > if (!l_dbus_message_iter_get_fixed_array(&iter_data, &data, > &len) || > - !len || len > MAX_MSG_LEN) > + !len || len > > MAX_MSG_LEN) > return dbus_error(msg, MESH_ERROR_INVALID_ARGS, > "Incorrect > data"); > > - /* TODO: use net_idx */ > - if (!mesh_model_send(node, src, dst, APP_IDX_DEV_REMOTE, > net_idx, > - DEFAULT_TTL, > data, len)) > + app_idx = remote ? APP_IDX_DEV_REMOTE : APP_IDX_DEV_LOCAL; > + if (!mesh_model_send(node, src, dst, app_idx, net_idx, > DEFAULT_TTL, > + data, > len)) > return dbus_error(msg, MESH_ERROR_NOT_FOUND, NULL); I think that mesh_model_send() should be modified to return an error code (int) instead of boolean. Otherwise, it may fail for a different reason than a mismatch in device key and the returned error is misleading. In fact, the Send() call returns D-Bus "Failed" error upon getting "false" from mesh_model_send() and this is not documented in the API doc. This probably should go a separate fix. > > return l_dbus_message_new_method_return(msg); > @@ -2226,9 +2231,9 @@ static void setup_node_interface(struct > l_dbus_interface *iface) > "element_path", > "destination", > "key_index", "data"); > l_dbus_interface_method(iface, "DevKeySend", 0, > dev_key_send_call, > - "", "oqqay", > "element_path", > - "destination", > "net_index", > - "data"); > + "", "oqbqay", > "element_path", > + "destination", > "remote", > + "net_index", "data"); > l_dbus_interface_method(iface, "Publish", 0, publish_call, "", > "oqay", > "element_path", "model_id", > "data"); > l_dbus_interface_method(iface, "VendorPublish", 0, > vendor_publish_call, [-- Attachment #2: smime.p7s --] [-- Type: application/x-pkcs7-signature, Size: 3265 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH BlueZ v3 3/3] mesh: Fix Key Ring permissions for local nodes 2019-09-26 18:14 [PATCH BlueZ v3 0/3] mesh: Fix Remote/Local dev key usage Brian Gix 2019-09-26 18:14 ` [PATCH BlueZ v3 1/3] mesh: Add remote boolean to DevKey transactions Brian Gix 2019-09-26 18:14 ` [PATCH BlueZ v3 2/3] mesh: Explicit Remote/Local Device key usage Brian Gix @ 2019-09-26 18:14 ` Brian Gix 2019-09-26 20:41 ` Stotland, Inga 2019-09-26 20:40 ` [PATCH BlueZ v3 0/3] mesh: Fix Remote/Local dev key usage Stotland, Inga 2019-10-01 18:07 ` Gix, Brian 4 siblings, 1 reply; 10+ messages in thread From: Brian Gix @ 2019-09-26 18:14 UTC (permalink / raw) To: linux-bluetooth; +Cc: brian.gix, inga.stotland, michal.lowas-rzechonek We do *not* automatically create populated key rings for imported or joined nodes, but we also do not *forbid* any node from adding a key in it's possesion to the local key ring. --- mesh/manager.c | 5 ----- mesh/node.c | 15 --------------- 2 files changed, 20 deletions(-) diff --git a/mesh/manager.c b/mesh/manager.c index 501ec10fe..633597659 100644 --- a/mesh/manager.c +++ b/mesh/manager.c @@ -282,7 +282,6 @@ static struct l_dbus_message *import_node_call(struct l_dbus *dbus, void *user_data) { struct mesh_node *node = user_data; - struct mesh_net *net = node_get_net(node); struct l_dbus_message_iter iter_key; uint16_t primary; uint8_t num_ele; @@ -298,10 +297,6 @@ static struct l_dbus_message *import_node_call(struct l_dbus *dbus, return dbus_error(msg, MESH_ERROR_INVALID_ARGS, "Bad device key"); - if (mesh_net_is_local_address(net, primary, num_ele)) - return dbus_error(msg, MESH_ERROR_INVALID_ARGS, - "Cannot overwrite local device key"); - if (!keyring_put_remote_dev_key(node, primary, num_ele, key)) return dbus_error(msg, MESH_ERROR_FAILED, NULL); diff --git a/mesh/node.c b/mesh/node.c index 833377e99..af45a6130 100644 --- a/mesh/node.c +++ b/mesh/node.c @@ -1681,7 +1681,6 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data) } else if (req->type == REQUEST_TYPE_IMPORT) { struct node_import *import = req->import; - struct keyring_net_key net_key; if (!create_node_config(node, node->uuid)) goto fail; @@ -1692,23 +1691,9 @@ static void get_managed_objects_cb(struct l_dbus_message *msg, void *user_data) import->net_idx, import->net_key)) goto fail; - memcpy(net_key.old_key, import->net_key, 16); - net_key.net_idx = import->net_idx; - if (import->flags.kr) - net_key.phase = KEY_REFRESH_PHASE_TWO; - else - net_key.phase = KEY_REFRESH_PHASE_NONE; - /* Initialize directory for storing keyring info */ init_storage_dir(node); - if (!keyring_put_remote_dev_key(node, import->unicast, - num_ele, import->dev_key)) - goto fail; - - if (!keyring_put_net_key(node, import->net_idx, &net_key)) - goto fail; - } else { /* Callback for create node request */ struct keyring_net_key net_key; -- 2.21.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH BlueZ v3 3/3] mesh: Fix Key Ring permissions for local nodes 2019-09-26 18:14 ` [PATCH BlueZ v3 3/3] mesh: Fix Key Ring permissions for local nodes Brian Gix @ 2019-09-26 20:41 ` Stotland, Inga 2019-09-26 23:27 ` Gix, Brian 0 siblings, 1 reply; 10+ messages in thread From: Stotland, Inga @ 2019-09-26 20:41 UTC (permalink / raw) To: linux-bluetooth, Gix, Brian; +Cc: michal.lowas-rzechonek [-- Attachment #1: Type: text/plain, Size: 2655 bytes --] Hi Brian, On Thu, 2019-09-26 at 11:14 -0700, Brian Gix wrote: > We do *not* automatically create populated key rings for imported or > joined nodes, Why not for Import()? Since both the DevKey and NetKey are in the possesion of the node... > but we also do not *forbid* any node from adding a key > in it's possesion to the local key ring. > --- > mesh/manager.c | 5 ----- > mesh/node.c | 15 --------------- > 2 files changed, 20 deletions(-) > > diff --git a/mesh/manager.c b/mesh/manager.c > index 501ec10fe..633597659 100644 > --- a/mesh/manager.c > +++ b/mesh/manager.c > @@ -282,7 +282,6 @@ static struct l_dbus_message > *import_node_call(struct l_dbus *dbus, > void *user_data) > { > struct mesh_node *node = user_data; > - struct mesh_net *net = node_get_net(node); > struct l_dbus_message_iter iter_key; > uint16_t primary; > uint8_t num_ele; > @@ -298,10 +297,6 @@ static struct l_dbus_message > *import_node_call(struct l_dbus *dbus, > return dbus_error(msg, MESH_ERROR_INVALID_ARGS, > "Bad device > key"); > > - if (mesh_net_is_local_address(net, primary, num_ele)) > - return dbus_error(msg, MESH_ERROR_INVALID_ARGS, > - "Cannot overwrite local device > key"); > - > if (!keyring_put_remote_dev_key(node, primary, num_ele, key)) > return dbus_error(msg, MESH_ERROR_FAILED, NULL); > > diff --git a/mesh/node.c b/mesh/node.c > index 833377e99..af45a6130 100644 > --- a/mesh/node.c > +++ b/mesh/node.c > @@ -1681,7 +1681,6 @@ static void get_managed_objects_cb(struct > l_dbus_message *msg, void *user_data) > > } else if (req->type == REQUEST_TYPE_IMPORT) { > struct node_import *import = req->import; > - struct keyring_net_key net_key; > > if (!create_node_config(node, node->uuid)) > goto fail; > @@ -1692,23 +1691,9 @@ static void get_managed_objects_cb(struct > l_dbus_message *msg, void *user_data) > import->net_idx, import- > >net_key)) > goto fail; > > - memcpy(net_key.old_key, import->net_key, 16); > - net_key.net_idx = import->net_idx; > - if (import->flags.kr) > - net_key.phase = KEY_REFRESH_PHASE_TWO; > - else > - net_key.phase = KEY_REFRESH_PHASE_NONE; > - > /* Initialize directory for storing keyring info */ > init_storage_dir(node); > > - if (!keyring_put_remote_dev_key(node, import->unicast, > - num_ele, import- > >dev_key)) > - goto fail; > - > - if (!keyring_put_net_key(node, import->net_idx, > &net_key)) > - goto fail; > - > } else { > /* Callback for create node request */ > struct keyring_net_key net_key; [-- Attachment #2: smime.p7s --] [-- Type: application/x-pkcs7-signature, Size: 3265 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH BlueZ v3 3/3] mesh: Fix Key Ring permissions for local nodes 2019-09-26 20:41 ` Stotland, Inga @ 2019-09-26 23:27 ` Gix, Brian 2019-09-27 2:50 ` Stotland, Inga 0 siblings, 1 reply; 10+ messages in thread From: Gix, Brian @ 2019-09-26 23:27 UTC (permalink / raw) To: linux-bluetooth, Stotland, Inga; +Cc: michal.lowas-rzechonek On Thu, 2019-09-26 at 20:41 +0000, Stotland, Inga wrote: > Hi Brian, > > On Thu, 2019-09-26 at 11:14 -0700, Brian Gix wrote: > > We do *not* automatically create populated key rings for imported or > > joined nodes, > > Why not for Import()? Since both the DevKey and NetKey are in the > possesion of the node... There are two (known) use cases for Import() 1. Node previously existed on a different physical piece of hardware, and is being migrated to this daemon. 2. Node was newly provisioned Out-Of-Band, and this is the net result of the provisioning. In *neither* case is it a given that the Node should be able to provision another node (the effect of adding the Net Key to the key ring). In neither case is it a given that the Node should be able to modify it's own Config Server states (the effect of adding it's Device Key to the key ring). However, if it *is* the case that one or more of these operations should be available to the Node, the Application that performed the Import may call the ImportSubnet() and/or the ImportRemoteNode() methods. In actuality, if this was the intent (particularily on a Node being migrated) a key-ring of some sort should have existed on the other physical piece of hardware. The point of this larger patch-set is to no longer assume that every Node has the ability to use it's own network keys to provision other nodes, or use it's own Device Key to change it's own Config Server states. Yes, that can still happen (as it can with non BlueZ nodes) but it should be by a deliberate mechanism, not as a "time saving default". > > but we also do not *forbid* any node from adding a key > > in it's possesion to the local key ring. > > --- > > mesh/manager.c | 5 ----- > > mesh/node.c | 15 --------------- > > 2 files changed, 20 deletions(-) > > > > diff --git a/mesh/manager.c b/mesh/manager.c > > index 501ec10fe..633597659 100644 > > --- a/mesh/manager.c > > +++ b/mesh/manager.c > > @@ -282,7 +282,6 @@ static struct l_dbus_message > > *import_node_call(struct l_dbus *dbus, > > void *user_data) > > { > > struct mesh_node *node = user_data; > > - struct mesh_net *net = node_get_net(node); > > struct l_dbus_message_iter iter_key; > > uint16_t primary; > > uint8_t num_ele; > > @@ -298,10 +297,6 @@ static struct l_dbus_message > > *import_node_call(struct l_dbus *dbus, > > return dbus_error(msg, MESH_ERROR_INVALID_ARGS, > > "Bad device > > key"); > > > > - if (mesh_net_is_local_address(net, primary, num_ele)) > > - return dbus_error(msg, MESH_ERROR_INVALID_ARGS, > > - "Cannot overwrite local device > > key"); > > - > > if (!keyring_put_remote_dev_key(node, primary, num_ele, key)) > > return dbus_error(msg, MESH_ERROR_FAILED, NULL); > > > > diff --git a/mesh/node.c b/mesh/node.c > > index 833377e99..af45a6130 100644 > > --- a/mesh/node.c > > +++ b/mesh/node.c > > @@ -1681,7 +1681,6 @@ static void get_managed_objects_cb(struct > > l_dbus_message *msg, void *user_data) > > > > } else if (req->type == REQUEST_TYPE_IMPORT) { > > struct node_import *import = req->import; > > - struct keyring_net_key net_key; > > > > if (!create_node_config(node, node->uuid)) > > goto fail; > > @@ -1692,23 +1691,9 @@ static void get_managed_objects_cb(struct > > l_dbus_message *msg, void *user_data) > > import->net_idx, import- > > > net_key)) > > goto fail; > > > > - memcpy(net_key.old_key, import->net_key, 16); > > - net_key.net_idx = import->net_idx; > > - if (import->flags.kr) > > - net_key.phase = KEY_REFRESH_PHASE_TWO; > > - else > > - net_key.phase = KEY_REFRESH_PHASE_NONE; > > - > > /* Initialize directory for storing keyring info */ > > init_storage_dir(node); > > > > - if (!keyring_put_remote_dev_key(node, import->unicast, > > - num_ele, import- > > > dev_key)) > > - goto fail; > > - > > - if (!keyring_put_net_key(node, import->net_idx, > > &net_key)) > > - goto fail; > > - > > } else { > > /* Callback for create node request */ > > struct keyring_net_key net_key; > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH BlueZ v3 3/3] mesh: Fix Key Ring permissions for local nodes 2019-09-26 23:27 ` Gix, Brian @ 2019-09-27 2:50 ` Stotland, Inga 0 siblings, 0 replies; 10+ messages in thread From: Stotland, Inga @ 2019-09-27 2:50 UTC (permalink / raw) To: linux-bluetooth, Gix, Brian; +Cc: michal.lowas-rzechonek [-- Attachment #1: Type: text/plain, Size: 4479 bytes --] Hi Brian, On Thu, 2019-09-26 at 23:27 +0000, Gix, Brian wrote: > On Thu, 2019-09-26 at 20:41 +0000, Stotland, Inga wrote: > > Hi Brian, > > > > On Thu, 2019-09-26 at 11:14 -0700, Brian Gix wrote: > > > We do *not* automatically create populated key rings for imported > > > or > > > joined nodes, > > > > Why not for Import()? Since both the DevKey and NetKey are in the > > possesion of the node... > > There are two (known) use cases for Import() > > 1. Node previously existed on a different physical piece of hardware, > and is being migrated to this daemon. > > 2. Node was newly provisioned Out-Of-Band, and this is the net result > of the provisioning. > > In *neither* case is it a given that the Node should be able to > provision another node (the effect of adding > the Net Key to the key ring). In neither case is it a given that the > Node should be able to modify it's own > Config Server states (the effect of adding it's Device Key to the key > ring). > > However, if it *is* the case that one or more of these operations > should be available to the Node, the > Application that performed the Import may call the ImportSubnet() > and/or the ImportRemoteNode() methods. In > actuality, if this was the intent (particularily on a Node being > migrated) a key-ring of some sort should have > existed on the other physical piece of hardware. > > > The point of this larger patch-set is to no longer assume that every > Node has the ability to use it's own > network keys to provision other nodes, or use it's own Device Key to > change it's own Config Server states. > > Yes, that can still happen (as it can with non BlueZ nodes) but it > should be by a deliberate mechanism, not as > a "time saving default". > > > Sounds good. Could you please add this description to the commit message? > > > but we also do not *forbid* any node from adding a key > > > in it's possesion to the local key ring. > > > --- > > > mesh/manager.c | 5 ----- > > > mesh/node.c | 15 --------------- > > > 2 files changed, 20 deletions(-) > > > > > > diff --git a/mesh/manager.c b/mesh/manager.c > > > index 501ec10fe..633597659 100644 > > > --- a/mesh/manager.c > > > +++ b/mesh/manager.c > > > @@ -282,7 +282,6 @@ static struct l_dbus_message > > > *import_node_call(struct l_dbus *dbus, > > > void *user_data) > > > { > > > struct mesh_node *node = user_data; > > > -struct mesh_net *net = node_get_net(node); > > > struct l_dbus_message_iter iter_key; > > > uint16_t primary; > > > uint8_t num_ele; > > > @@ -298,10 +297,6 @@ static struct l_dbus_message > > > *import_node_call(struct l_dbus *dbus, > > > return dbus_error(msg, MESH_ERROR_INVALID_ARGS, > > > "Bad device > > > key"); > > > > > > -if (mesh_net_is_local_address(net, primary, num_ele)) > > > -return dbus_error(msg, MESH_ERROR_INVALID_ARGS, > > > -"Cannot overwrite local device > > > key"); > > > - > > > if (!keyring_put_remote_dev_key(node, primary, num_ele, key)) > > > return dbus_error(msg, MESH_ERROR_FAILED, NULL); > > > > > > diff --git a/mesh/node.c b/mesh/node.c > > > index 833377e99..af45a6130 100644 > > > --- a/mesh/node.c > > > +++ b/mesh/node.c > > > @@ -1681,7 +1681,6 @@ static void get_managed_objects_cb(struct > > > l_dbus_message *msg, void *user_data) > > > > > > } else if (req->type == REQUEST_TYPE_IMPORT) { > > > struct node_import *import = req->import; > > > -struct keyring_net_key net_key; > > > > > > if (!create_node_config(node, node->uuid)) > > > goto fail; > > > @@ -1692,23 +1691,9 @@ static void get_managed_objects_cb(struct > > > l_dbus_message *msg, void *user_data) > > > import->net_idx, import- > > > > net_key)) > > > goto fail; > > > > > > -memcpy(net_key.old_key, import->net_key, 16); > > > -net_key.net_idx = import->net_idx; > > > -if (import->flags.kr) > > > -net_key.phase = KEY_REFRESH_PHASE_TWO; > > > -else > > > -net_key.phase = KEY_REFRESH_PHASE_NONE; > > > - > > > /* Initialize directory for storing keyring info */ > > > init_storage_dir(node); > > > > > > -if (!keyring_put_remote_dev_key(node, import->unicast, > > > -num_ele, import- > > > > dev_key)) > > > -goto fail; > > > - > > > -if (!keyring_put_net_key(node, import->net_idx, > > > &net_key)) > > > -goto fail; > > > - > > > } else { > > > /* Callback for create node request */ > > > struct keyring_net_key net_key; [-- Attachment #2: smime.p7s --] [-- Type: application/x-pkcs7-signature, Size: 3265 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH BlueZ v3 0/3] mesh: Fix Remote/Local dev key usage 2019-09-26 18:14 [PATCH BlueZ v3 0/3] mesh: Fix Remote/Local dev key usage Brian Gix ` (2 preceding siblings ...) 2019-09-26 18:14 ` [PATCH BlueZ v3 3/3] mesh: Fix Key Ring permissions for local nodes Brian Gix @ 2019-09-26 20:40 ` Stotland, Inga 2019-10-01 18:07 ` Gix, Brian 4 siblings, 0 replies; 10+ messages in thread From: Stotland, Inga @ 2019-09-26 20:40 UTC (permalink / raw) To: linux-bluetooth, Gix, Brian; +Cc: michal.lowas-rzechonek [-- Attachment #1: Type: text/plain, Size: 1474 bytes --] Hi Brian, On Thu, 2019-09-26 at 11:14 -0700, Brian Gix wrote: > V3: By popular demand, the name "remote" is now used for both > DevKeySend() > and DevKeyMessageReceived(). > > In DevKeySend(), setting remote == true means that the Key Ring > *must* be > used to encrypt the outgoing message, and a failure will be returned > if > the requested destination address does not include a device key in > the > local key ring. For remote == false requests, the request will be > rejected > if the destination is an element on the local node. > > In DevKeyMessageReceived(), the remote boolean will be set == true if > it > required the key ring to decrypot the message. If remote == false, > this > means that the local nodes Device Key successfully decrypted the > message, > and the message may be used to change or query privileged states. > > > Brian Gix (3): > mesh: Add local/remote bools to DevKey transactions > mesh: Use explicit Local vs Remote Device key usage > The two patches above are fine IMO (see some comments for #2, but these can be addressed in a separate patch) > mesh: Fix Key Ring permissions for local nodes This patch may require some explanation? > > doc/mesh-api.txt | 17 ++++++++++++++--- > mesh/manager.c | 5 ----- > mesh/model.c | 11 +++++++---- > mesh/node.c | 40 +++++++++++++++------------------------- > 4 files changed, 36 insertions(+), 37 deletions(-) > [-- Attachment #2: smime.p7s --] [-- Type: application/x-pkcs7-signature, Size: 3265 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH BlueZ v3 0/3] mesh: Fix Remote/Local dev key usage 2019-09-26 18:14 [PATCH BlueZ v3 0/3] mesh: Fix Remote/Local dev key usage Brian Gix ` (3 preceding siblings ...) 2019-09-26 20:40 ` [PATCH BlueZ v3 0/3] mesh: Fix Remote/Local dev key usage Stotland, Inga @ 2019-10-01 18:07 ` Gix, Brian 4 siblings, 0 replies; 10+ messages in thread From: Gix, Brian @ 2019-10-01 18:07 UTC (permalink / raw) To: linux-bluetooth; +Cc: michal.lowas-rzechonek, Stotland, Inga Applied patch set with commit message changes On Thu, 2019-09-26 at 11:14 -0700, Brian Gix wrote: > V3: By popular demand, the name "remote" is now used for both DevKeySend() > and DevKeyMessageReceived(). > > In DevKeySend(), setting remote == true means that the Key Ring *must* be > used to encrypt the outgoing message, and a failure will be returned if > the requested destination address does not include a device key in the > local key ring. For remote == false requests, the request will be rejected > if the destination is an element on the local node. > > In DevKeyMessageReceived(), the remote boolean will be set == true if it > required the key ring to decrypot the message. If remote == false, this > means that the local nodes Device Key successfully decrypted the message, > and the message may be used to change or query privileged states. > > > Brian Gix (3): > mesh: Add local/remote bools to DevKey transactions > mesh: Use explicit Local vs Remote Device key usage > mesh: Fix Key Ring permissions for local nodes > > doc/mesh-api.txt | 17 ++++++++++++++--- > mesh/manager.c | 5 ----- > mesh/model.c | 11 +++++++---- > mesh/node.c | 40 +++++++++++++++------------------------- > 4 files changed, 36 insertions(+), 37 deletions(-) > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-10-01 18:07 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-26 18:14 [PATCH BlueZ v3 0/3] mesh: Fix Remote/Local dev key usage Brian Gix 2019-09-26 18:14 ` [PATCH BlueZ v3 1/3] mesh: Add remote boolean to DevKey transactions Brian Gix 2019-09-26 18:14 ` [PATCH BlueZ v3 2/3] mesh: Explicit Remote/Local Device key usage Brian Gix 2019-09-26 20:40 ` Stotland, Inga 2019-09-26 18:14 ` [PATCH BlueZ v3 3/3] mesh: Fix Key Ring permissions for local nodes Brian Gix 2019-09-26 20:41 ` Stotland, Inga 2019-09-26 23:27 ` Gix, Brian 2019-09-27 2:50 ` Stotland, Inga 2019-09-26 20:40 ` [PATCH BlueZ v3 0/3] mesh: Fix Remote/Local dev key usage Stotland, Inga 2019-10-01 18:07 ` Gix, Brian
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).