mesh: Handling application failures on Create/Join/Import

mesh: Handling application failures on Create/Join/Import

[Michał Lowas-Rzechonek]

Hi,

I've noticed an issue with Create/Join/Import calls: when application
performs one of these calls and fails to save the resulting token (for
whatever reason), the node created on the daemon side is pretty much
unusable. It's also rather hard to get rid of, as the daemon won't even
expose them over D-Bus when application is not Attach()ed.

I would like to discuss possible solutions to this. 

One of the ideas is to give the application some time to successfully
Attach() itself to the new node, otherwise it gets removed.

Another possibility would be to remove "created but never attached" nodes on
daemon restart.

Thoughts?

regards
-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

Re: mesh: Handling application failures on Create/Join/Import

[Michał Lowas-Rzechonek]

On 12/17, Michał Lowas-Rzechonek wrote:
> I would like to discuss possible solutions to this. 
> 
> One of the ideas is to give the application some time to successfully
> Attach() itself to the new node, otherwise it gets removed.
> 
> Another possibility would be to remove "created but never attached" nodes on
> daemon restart.

Or maybe change the token flow a bit and instead of straight return,
make the daemon call JoinComplete in all 3 cases, expecting a call
return from the application?

If JoinComplete call fails, node could be dropped.

-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

Re: mesh: Handling application failures on Create/Join/Import

[Gix, Brian]

Hi Michał,

On Tue, 2019-12-17 at 19:47 +0100, Michał Lowas-Rzechonek wrote:
> On 12/17, Michał Lowas-Rzechonek wrote:
> > I would like to discuss possible solutions to this. 
> > 
> > One of the ideas is to give the application some time to successfully
> > Attach() itself to the new node, otherwise it gets removed.

Not all nodes need to be attachable...  For instance, a node that is only used for friendship, relaying or
beaconing can exist without ever being attached to...  so requiring an Attach() shouldn't be a requirement.

> > 
> > Another possibility would be to remove "created but never attached" nodes on
> > daemon restart.
> 
> Or maybe change the token flow a bit and instead of straight return,
> make the daemon call JoinComplete in all 3 cases, expecting a call
> return from the application?
> 
> If JoinComplete call fails, node could be dropped.
> 

I think the Application does need to take responsibility for the token, once it receives it...  If the call (or
response) that delivers the token to the App fails, the node should be deleted, but the token is considered
sensitive enough that we lock down access to it as tight as possible.  If it is inadvertantly leaked, then
whoever gets it has all the abilities of the node, so we minimize who sees it.

Re: mesh: Handling application failures on Create/Join/Import

[Gix, Brian]

On Tue, 2019-12-17 at 18:56 +0000, Gix, Brian wrote:
> Hi Michał,
> 
> On Tue, 2019-12-17 at 19:47 +0100, Michał Lowas-Rzechonek wrote:
> > On 12/17, Michał Lowas-Rzechonek wrote:
> > > I would like to discuss possible solutions to this. 
> > > 
> > > One of the ideas is to give the application some time to successfully
> > > Attach() itself to the new node, otherwise it gets removed.
> 
> Not all nodes need to be attachable...  For instance, a node that is only used for friendship, relaying or
> beaconing can exist without ever being attached to...  so requiring an Attach() shouldn't be a requirement.

I think one piece of functionality that we have *not* yet tested is Node Reset.  If a Config Client sends a
Node Reset to an "Orphaned Node", using that nodes Device Key, the daemon should be cleaning up all of it's
storage.

> 
> > > Another possibility would be to remove "created but never attached" nodes on
> > > daemon restart.
> > 
> > Or maybe change the token flow a bit and instead of straight return,
> > make the daemon call JoinComplete in all 3 cases, expecting a call
> > return from the application?
> > 
> > If JoinComplete call fails, node could be dropped.
> > 
> 
> I think the Application does need to take responsibility for the token, once it receives it...  If the call
> (or
> response) that delivers the token to the App fails, the node should be deleted, but the token is considered
> sensitive enough that we lock down access to it as tight as possible.  If it is inadvertantly leaked, then
> whoever gets it has all the abilities of the node, so we minimize who sees it.
> 
> 

Re: mesh: Handling application failures on Create/Join/Import

[michal.lowas-rzechonek]

On 12/17, Gix, Brian wrote:
> I think one piece of functionality that we have *not* yet tested is
> Node Reset.  If a Config Client sends a Node Reset to an "Orphaned
> Node", using that nodes Device Key, the daemon should be cleaning up
> all of it's storage.

I meant 'get rid of it from the application side'. After Create(), noone
else knows how to talk to the newly created config server...

> > I think the Application does need to take responsibility for the
> > token, once it receives it...  If the call (or response) that
> > delivers the token to the App fails, the node should be deleted

Indeed. Would you be willing to accept a patch that changes token
delivery to use JoinComplete call in all cases, and checks that the call
returns successfully?

-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

Re: mesh: Handling application failures on Create/Join/Import

[Gix, Brian]

On Tue, 2019-12-17 at 20:11 +0100, michal.lowas-rzechonek@silvair.com wrote:
> On 12/17, Gix, Brian wrote:
> > I think one piece of functionality that we have *not* yet tested is
> > Node Reset.  If a Config Client sends a Node Reset to an "Orphaned
> > Node", using that nodes Device Key, the daemon should be cleaning up
> > all of it's storage.
> 
> I meant 'get rid of it from the application side'. After Create(), noone
> else knows how to talk to the newly created config server...
> 
> > > I think the Application does need to take responsibility for the
> > > token, once it receives it...  If the call (or response) that
> > > delivers the token to the App fails, the node should be deleted
> 
> Indeed. Would you be willing to accept a patch that changes token
> delivery to use JoinComplete call in all cases, and checks that the call
> returns successfully?


Maybe...  I need to discuss it with Inga.  How does this relate to Create() though?  There is no JoinComplete()
call currently axs a result of Create()...  just the return value from the Create() method. Is your problem
that the daemon cannot be sure that the return value (token) from Create() was received?

Re: mesh: Handling application failures on Create/Join/Import

[michal.lowas-rzechonek]

On 12/17, Gix, Brian wrote:
> > Indeed. Would you be willing to accept a patch that changes token
> > delivery to use JoinComplete call in all cases, and checks that the call
> > returns successfully?
> 
> Maybe...  I need to discuss it with Inga.  How does this relate to
> Create() though?  There is no JoinComplete() call currently axs a
> result of Create()...  just the return value from the Create() method.
> Is your problem that the daemon cannot be sure that the return value
> (token) from Create() was received?

Yes, exactly.

-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

Re: mesh: Handling application failures on Create/Join/Import

[Stotland, Inga]

Hi Michal, Brian,

On Tue, 2019-12-17 at 21:06 +0100, michal.lowas-rzechonek@silvair.com
wrote:
> On 12/17, Gix, Brian wrote:
> > > Indeed. Would you be willing to accept a patch that changes token
> > > delivery to use JoinComplete call in all cases, and checks that the call
> > > returns successfully?
> > 
> > Maybe...  I need to discuss it with Inga.  How does this relate to
> > Create() though?  There is no JoinComplete() call currently axs a
> > result of Create()...  just the return value from the Create() method.
> > Is your problem that the daemon cannot be sure that the return value
> > (token) from Create() was received?
> 
> Yes, exactly.
> 

As much as I don't like API changes in a released code, this seems to
be a legitimate case for doing so.

So yes, let's change the logic so that all three methods (Join, Create,
and Import) have void return values and the token is delivered in
JoinComplete(). A node is "finalized" only upon successful return of
JoinComplete().

Best regards,
Inga

Re: mesh: Handling application failures on Create/Join/Import

[Gix, Brian]

On Tue, 2019-12-17 at 21:11 +0000, Stotland, Inga wrote:
> Hi Michal, Brian,
> 
> On Tue, 2019-12-17 at 21:06 +0100, michal.lowas-rzechonek@silvair.com
> wrote:
> > On 12/17, Gix, Brian wrote:
> > > > Indeed. Would you be willing to accept a patch that changes token
> > > > delivery to use JoinComplete call in all cases, and checks that the call
> > > > returns successfully?
> > > 
> > > Maybe...  I need to discuss it with Inga.  How does this relate to
> > > Create() though?  There is no JoinComplete() call currently axs a
> > > result of Create()...  just the return value from the Create() method.
> > > Is your problem that the daemon cannot be sure that the return value
> > > (token) from Create() was received?
> > 
> > Yes, exactly.
> > 
> 
> As much as I don't like API changes in a released code, this seems to
> be a legitimate case for doing so.
> 
> So yes, let's change the logic so that all three methods (Join, Create,
> and Import) have void return values and the token is delivered in
> JoinComplete(). A node is "finalized" only upon successful return of
> JoinComplete().

And if the call to JoinComplete() fails, the daemon should delete the node storage.


> Best regards,
> Inga