2019-07-17 08:37:37

by Michał Lowas-Rzechonek

[permalink] [raw]
Subject: [PATCH BlueZ v5 1/4] mesh: Add ImportLocalNode API documentation

From: Jakub Witowski <[email protected]>

This updates the mesh-api.txt with new ImportLocalNode() API.
---
doc/mesh-api.txt | 52 ++++++++++++++++++++++++++++++++++++++----------
1 file changed, 41 insertions(+), 11 deletions(-)

diff --git a/doc/mesh-api.txt b/doc/mesh-api.txt
index 0ac2fdfd1..7c2a1fafa 100644
--- a/doc/mesh-api.txt
+++ b/doc/mesh-api.txt
@@ -151,16 +151,31 @@ Methods:
org.bluez.mesh.Error.InvalidArguments
org.bluez.mesh.Error.AlreadyExists,

- uint64 token ImportLocalNode(string json_data)
+ uint64 token ImportLocalNode(object app_root, array{byte}[16] uuid,
+ string data_type, array{byte} import_data)

This method creates a local mesh node based on node
configuration that has been generated outside bluetooth-meshd.

- The json_data parameter is a full JSON representation of a node
- configuration file. The format must conform to the schema
- defined in "Mesh Node Configuration Schema" section. Any
- included token will be ignored in favor of a locally generated
- token value.
+ The app_root parameter is a D-Bus object root path of the
+ application that implements org.bluez.mesh.Application1
+ interface.
+
+ The import_data parameter contains a representation of a
+ provisioned node. Format of this representation depends on
+ value of data_type parameter.
+
+ Allowed data_type values are: "json".
+
+ When data_type is "json", bluetooth-meshd daemon treats
+ import_data is a JSON document following <TBD> schema. See the
+ examples at the end of this document.
+
+ The import_data parameter can contain either minimal, or
+ complete representation of a provisioned node.
+
+ When a complete representation is provided, it is validated
+ against composition data provided by the application.

The returned token must be preserved by the application in
order to authenticate itself to the mesh daemon and attach to
@@ -173,8 +188,8 @@ Methods:

PossibleErrors:
org.bluez.mesh.Error.InvalidArguments,
- org.bluez.mesh.Error.AlreadyExists
- org.bluez.mesh.Error.NotFound,
+ org.bluez.mesh.Error.AlreadyExists,
+ org.bluez.mesh.Error.NotSupported,
org.bluez.mesh.Error.Failed

Mesh Node Hierarchy
@@ -1064,6 +1079,21 @@ Properties:
Uniform Resource Identifier points to out-of-band (OOB)
information (e.g., a public key)

-Mesh Node Configuration Schema
-==============================
-<TBD>
+Mesh Node Configuration Examples
+================================
+Minimal JSON representation for ImportLocalNode():
+
+{
+ "IVindex":0,
+ "IVupdate":0,
+ "unicastAddress":"0012",
+ "deviceKey":"7daa45cd1e9e11a4b86eeef7d01efa11",
+ "netKeys":[
+ {
+ "index":"0000",
+ "key":"2ddfef86d67144c394428ea3078f86f9",
+ "keyRefresh":0
+ }
+ ],
+ "sequenceNumber":15 /* optional */
+}
--
2.19.1


2019-07-17 19:37:11

by Stotland, Inga

[permalink] [raw]
Subject: Re: [PATCH BlueZ v5 1/4] mesh: Add ImportLocalNode API documentation

Hi Mical, Jakub, Brian...


On Wed, 2019-07-17 at 10:36 +0200, Michał Lowas-Rzechonek wrote:
> From: Jakub Witowski <[email protected]>
>
> This updates the mesh-api.txt with new ImportLocalNode() API.
> ---
> doc/mesh-api.txt | 52 ++++++++++++++++++++++++++++++++++++++------
> ----
> 1 file changed, 41 insertions(+), 11 deletions(-)
>
> diff --git a/doc/mesh-api.txt b/doc/mesh-api.txt
> index 0ac2fdfd1..7c2a1fafa 100644
> --- a/doc/mesh-api.txt
> +++ b/doc/mesh-api.txt
> @@ -151,16 +151,31 @@ Methods:
> org.bluez.mesh.Error.InvalidArguments
> org.bluez.mesh.Error.AlreadyExists,
>
> - uint64 token ImportLocalNode(string json_data)
> + uint64 token ImportLocalNode(object app_root, array{byte}[16]
> uuid,
> + string
> data_type, array{byte} import_data)


I apologize for the backtracking, but I would like to revisit this API.

I feel like having "object app_root" is unnecessary and also, creates
some gnarly pathways within the code.

What exactly is the problem for requiring the composition data to be
part of the import_data? It's just weird to say "oh, it may be there or
it may be not".

Getting rid of the app_root and mandating the composition to be part of
the import_data allows:

1) Avoid checking whether this is a "full" configuration or the
"minimal" one

2) Efficiently re-use the existing code:
Adding an API call like this one may be sufficient

mesh_config_import(const char *cfg_dir,
const uint8_t uuid[16],
const uint8 *import_data, <import__len>?,
mesh_config_node_func_t cb,
void *user_data)

We can just re-factor the code that parses and populates a single node
from the stored configuration. user_data may contain whatever we need
to preserve in order to respond to d-bus call.

>
> This method creates a local mesh node based on node
> configuration that has been generated outside
> bluetooth-meshd.
>
> - The json_data parameter is a full JSON representation
> of a node
> - configuration file. The format must conform to the
> schema
> - defined in "Mesh Node Configuration Schema" section.
> Any
> - included token will be ignored in favor of a locally
> generated
> - token value.
> + The app_root parameter is a D-Bus object root path of
> the
> + application that implements org.bluez.mesh.Application1
> + interface.
> +
> + The import_data parameter contains a representation of
> a
> + provisioned node. Format of this representation depends
> on
> + value of data_type parameter.
> +
> + Allowed data_type values are: "json".
> +
> + When data_type is "json", bluetooth-meshd daemon treats
> + import_data is a JSON document following <TBD> schema.
> See the
> + examples at the end of this document.
> +
> + The import_data parameter can contain either minimal,
> or
> + complete representation of a provisioned node.
> +
> + When a complete representation is provided, it is
> validated
> + against composition data provided by the application.
>
> The returned token must be preserved by the application
> in
> order to authenticate itself to the mesh daemon and
> attach to
> @@ -173,8 +188,8 @@ Methods:
>
> PossibleErrors:
> org.bluez.mesh.Error.InvalidArguments,
> - org.bluez.mesh.Error.AlreadyExists
> - org.bluez.mesh.Error.NotFound,
> + org.bluez.mesh.Error.AlreadyExists,
> + org.bluez.mesh.Error.NotSupported,
> org.bluez.mesh.Error.Failed
>
> Mesh Node Hierarchy
> @@ -1064,6 +1079,21 @@ Properties:
> Uniform Resource Identifier points to out-of-band (OOB)
> information (e.g., a public key)
>
> -Mesh Node Configuration Schema
> -==============================
> -<TBD>
> +Mesh Node Configuration Examples
> +================================
> +Minimal JSON representation for ImportLocalNode():
> +
> +{
> + "IVindex":0,
> + "IVupdate":0,
> + "unicastAddress":"0012",
> + "deviceKey":"7daa45cd1e9e11a4b86eeef7d01efa11",
> + "netKeys":[
> + {
> + "index":"0000",
> + "key":"2ddfef86d67144c394428ea3078f86f9",
> + "keyRefresh":0
> + }
> + ],
> + "sequenceNumber":15 /* optional */
> +}

Regards, Inga


Attachments:
smime.p7s (3.19 kB)

2019-07-17 19:48:00

by Michał Lowas-Rzechonek

[permalink] [raw]
Subject: Re: [PATCH BlueZ v5 1/4] mesh: Add ImportLocalNode API documentation

Hello,

On 07/17, Stotland, Inga wrote:
> I feel like having "object app_root" is unnecessary and also, creates
> some gnarly pathways within the code.
>
> What exactly is the problem for requiring the composition data to be
> part of the import_data? It's just weird to say "oh, it may be there or
> it may be not".

The main issue lies on the application side. In order to properly
Attach(), the application must expose element structure via D-Bus.

If we say that it must also do the same via JSON, to call
ImportLocalNode, it leads to code duplication on the application side.

Moreover, the app still needs to be queried via D-Bus to check that the
passed JSON matches the D-Bus structure - otherwise the app would then
fail to Attach() and the user would be in deep trouble.

> Getting rid of the app_root and mandating the composition to be part of
> the import_data allows:
>
> 1) Avoid checking whether this is a "full" configuration or the
> "minimal" one

I'm not convinced that the "full" configuration is even needed. We
certaintly don't use it in our use case, but it might be required in the
future.

> 2) Efficiently re-use the existing code:
> Adding an API call like this one may be sufficient
>
> mesh_config_import(const char *cfg_dir,
> const uint8_t uuid[16],
> const uint8 *import_data, <import__len>?,
> mesh_config_node_func_t cb,
> void *user_data)
>
> We can just re-factor the code that parses and populates a single node
> from the stored configuration. user_data may contain whatever we need
> to preserve in order to respond to d-bus call.

After refactoring node validation to byte-compare composition data, the
code also becomes significantly simpler, and execution paths for Join(),
Attach(), CreateNetwork() and ImportLocalNode() converge.

I've implemented this validation method on top of this patch-set. I'll
send it as RFC shortly.

regards
--
Michał Lowas-Rzechonek <[email protected]>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

2019-07-17 21:16:24

by Gix, Brian

[permalink] [raw]
Subject: RE: [PATCH BlueZ v5 1/4] mesh: Add ImportLocalNode API documentation

Hi Inga & Michal,

> On 07/17, Stotland, Inga wrote:
> > I feel like having "object app_root" is unnecessary and also, creates
> > some gnarly pathways within the code.
> >
> > What exactly is the problem for requiring the composition data to be
> > part of the import_data? It's just weird to say "oh, it may be there
> > or it may be not".
>
> The main issue lies on the application side. In order to properly Attach(), the
> application must expose element structure via D-Bus.

I understand this concern, but I think I am agreeing with Inga on this one.

At the time of inception, Not only the Prov Data (net key, net idx, iv index, flags, unicast, dev key) must be present, but also so must the "Composition Data"... all of the data which is returned by the "GET COMPOSITION DATA" opcode. This data needs to be present as soon as the Provisioner starts querying and setting states on the (new or migrated) fresh node.

An important point to remember here is that *the application does not need to be attached" while the remote Provisioner/ConfigClient is interacting with the nodes Config Server. One of our Design Goals, in fact, was that the node always exists on the network, regardless of whether the App is attached... This is why the Config Server model is part of the daemon.... It is always "OnLine".

For this reason, I am supporting Inga here, that the JSON input file should contain everything Jakub has included:
Dev Key
Net Key
Net Index
IV Index
Flags
Unicast

*And* it should include the pure Composition Data including
CID/VID/PID (optional .. Can be set with BlueZ defaults)
CRPL (optional ... Can be set with internal defaults)
Features (optional ... missing are created as either "Unsupported" or "Disabled")
array of Elements, containing array of Models. (Mandatory ... Needed for Cfg Server to function)

The application has no need to even exist at this point, as long as it can attach to the token at some point in the future.
But this *does* enable the ability to have a *generic* application that can inject nodes (fully configured, or "New") into the daemon.

But worries that the App might not immediately be able to "Attach" go away.



> If we say that it must also do the same via JSON, to call ImportLocalNode, it
> leads to code duplication on the application side.
>
> Moreover, the app still needs to be queried via D-Bus to check that the passed
> JSON matches the D-Bus structure - otherwise the app would then fail to
> Attach() and the user would be in deep trouble.

The composition as reflected by the GetManagedObjects() call is "sanity checked" against the internal storage *every* time the App attaches... I think Inga is concerned with code complexity and bloat to repeat this during ImportLocalNode(), simply as a "second way" attain the composition data. This is different from the Join() case, in my opinion, where the JSON (or other storage) is being created *totally* from scratch, via the provisioning interaction with the remote Provisioner. In that case, yes... the "owning application" needs to be present on the D-Bus.


> > Getting rid of the app_root and mandating the composition to be part
> > of the import_data allows:
> >
> > 1) Avoid checking whether this is a "full" configuration or the
> > "minimal" one
>
> I'm not convinced that the "full" configuration is even needed. We certaintly
> don't use it in our use case, but it might be required in the future.

We *definitely* need an option for importing/migrating a fully configured node. Phones are retired and replaced... Workstations are retired and replaced... Some nodes publications will inevitably need to pick up the "current conversation" with the migrated node, where the old conversation left off. And this will almost certainly be a rare (but important) operation, and a "Utility" application to perform the operation (that does not itself need to have the model/element arrays implemented) will be easier to write and maintain.


>
> > 2) Efficiently re-use the existing code:
> > Adding an API call like this one may be sufficient
> >
> > mesh_config_import(const char *cfg_dir,
> > const uint8_t uuid[16],
> > const uint8 *import_data, <import__len>?,
> > mesh_config_node_func_t cb,
> > void *user_data)
> >
> > We can just re-factor the code that parses and populates a single node
> > from the stored configuration. user_data may contain whatever we need
> > to preserve in order to respond to d-bus call.
>
> After refactoring node validation to byte-compare composition data, the code
> also becomes significantly simpler, and execution paths for Join(), Attach(),
> CreateNetwork() and ImportLocalNode() converge.

Again, I cannot think of any situations where Join/Attach/Create would ever exist in the absence of the Application.

This is an easy and obvious use case with Import.

--Brian

2019-07-18 08:18:57

by Michał Lowas-Rzechonek

[permalink] [raw]
Subject: Re: [PATCH BlueZ v5 1/4] mesh: Add ImportLocalNode API documentation

Brian, Inga,

On 07/17, Gix, Brian wrote:
> The application has no need to even exist at this point, as long as it
> can attach to the token at some point in the future. But this *does*
> enable the ability to have a *generic* application that can inject
> nodes (fully configured, or "New") into the daemon.

The same thing can be said about CreateNetwork() call - you don't really
*need* to have an attached (of even "attachable") application in order
to create a new node.

In fact, CreateNetwork() and ImportLocalNode() calls are very similar in
this regard, it's just that CreateNetwork generates keys, addresses and
starting sequence number on the daemon side, while ImportLocalNode()
allows passing them from the application.

Since CreateNetwork() does query the app via D-Bus, I think it's more
consistent to query the app during ImportLocalNode() as well.

That, or replace both calls with a more generic CreateNode() call that
would accept create/import data as an agument, and not query the app at
all.

> > If we say that it must also do the same via JSON, to call
> > ImportLocalNode, it leads to code duplication on the application
> > side.
> >
> > Moreover, the app still needs to be queried via D-Bus to check that
> > the passed JSON matches the D-Bus structure - otherwise the app
> > would then fail to Attach() and the user would be in deep trouble.
>
> The composition as reflected by the GetManagedObjects() call is
> "sanity checked" against the internal storage *every* time the App
> attaches... I think Inga is concerned with code complexity and bloat
> to repeat this during ImportLocalNode(),

This is covered by the same code path as other calls. I think it's
cleaner to always perform GetManagedObjects() dance for all 4 calls.

And, as I mentioned before, this code path becomes even simpler when we
refactor "sanity checking" to compare serialized composition data.

> This is different from the Join() case, in my opinion, where the JSON
> (or other storage) is being created *totally* from scratch, via the
> provisioning interaction with the remote Provisioner. In that case,
> yes... the "owning application" needs to be present on the D-Bus.

Not necessarily. The application needs to provide ProvisionAgent
interface only, and only in cases where provisioner selects
authentication method that requires the app to perform some OOB action.

It is entirely possible for a joining node to say it doesn't support any
OOB actions, and the mesh daemon can accept provisioning wihout any
interaction with the app.

> > I'm not convinced that the "full" configuration is even needed. We
> > certaintly don't use it in our use case, but it might be required in
> > the future.
>
> We *definitely* need an option for importing/migrating a fully
> configured node. Phones are retired and replaced... Workstations are
> retired and replaced... Some nodes publications will inevitably need
> to pick up the "current conversation" with the migrated node, where
> the old conversation left off. And this will almost certainly be a
> rare (but important) operation, and a "Utility" application to perform
> the operation (that does not itself need to have the model/element
> arrays implemented) will be easier to write and maintain.
(...)
> Again, I cannot think of any situations where Join/Attach/Create would
> ever exist in the absence of the Application.
>
> This is an easy and obvious use case with Import.

I don't think it makes sense to have a utility application that would
migrate the node without having anything that would Attach() to it.

IMO the migration operation should be a part of application logic,
because this operation requires talking to external Provisioner, and
Provisioner API is vendor specific. So I don't think you can't really
create a 'generic migration tool'.

In case of replaced device, the use case I see is re-installing the same
application on a new device, and having it re-create the node either
from data received from the Provisioner, or exported from the previous
instance of the application. I don't see a need to instantiate the node
on the replaces device without reinstalling the application - such a
node would not be operational anyway.

--
Michał Lowas-Rzechonek <[email protected]>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND