Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 30 Apr 2019 09:38:09 +0200
From:   Michal Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
To:     "Gix, Brian" <brian.gix@intel.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        "Stotland, Inga" <inga.stotland@intel.com>
Subject: Re: [PATCH BlueZ] mesh: Add remote dev key support and cleanup
Message-ID: <20190430073809.wdbhglfaiqsvscmc@scytale>
Mail-Followup-To: "Gix, Brian" <brian.gix@intel.com>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        "Stotland, Inga" <inga.stotland@intel.com>
References: <20190423231624.19302-1-brian.gix@intel.com>
 <20190424085945.zmniib4vpy2uiyql@kynes>
 <DEBB0CAA2616974FAE35E4B560B9A4376CB05944@ORSMSX103.amr.corp.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <DEBB0CAA2616974FAE35E4B560B9A4376CB05944@ORSMSX103.amr.corp.intel.com>
User-Agent: NeoMutt/20180716
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Brian,

On 04/24, Gix, Brian wrote:
> > From: Michał Lowas-Rzechonek [mailto:michal.lowas-rzechonek@silvair.com]
> > > +try_remote:
> > > +	/* Try remote device key if available */
> > > +	dev_key = node_get_remote_device_key(node, src);
> > > +	if (!dev_key)
> > > +		return -1;
> > > +
> > > +	if (mesh_crypto_payload_decrypt(NULL, 0, data, size, szmict, src,
> > > +					dst, key_id, seq, iv_idx, out,
> > dev_key))
> > > +		return APP_IDX_DEV_RMT;
> > > +
> > What's the purpose of APP_IDX_DEV_RMT?
> 
> App Index is an internal way to track which credentials were used to
> decrypt a message.  In theory, if the incoming message used the local
> device key, then the message is addressed to one of the local servers,
> and if the incoming message was decrypted by a *remote* device key it
> should be addressed to a local Client (as a response from a remote
> server).
>
> To enable that, we need to differentiate between the two possible
> device keys.
>
> However, also importantly, if a message that requires a response is
> received on *any* key (local dev, remote dev, or App Key) the response
> is supposed to be sent with the same key.
Right, but I think this is covered by the server/client model behaviour,
so I don't think we need to care if the message was encrypted using a
known remote device key, or our local device key.

This is indeed not explicitly mentioned in the spec, but I think the
consensus is that a server model shall always send responses using a
local device key.

I think this means that the API needs to allow using either local or
remote device key when *sending* a message (i.e. use remote key for
requests and local key for responses), but API for receiving messages
needs only to differentiate between device and application keys.

> When *sending* Dev Key message
> from the Application, we will try the remote nodes device key first,
> and if it does not exist we fall back to the local key.
I'm not sure the key selection should happen automaticall. I'd rather
have an explicit option to use local device key or remote one.

> In any case, these "Magic Numbers" are internal use only, and do not
> show up on the external interface.
Well, at the moment they are visible in Send() call: key_index argument
accepts 0x7fff as a way to indicate that local device key should be
used.

I guess you're refering to the updated doc/mesh-api.txt, so the plain Send()
method will accept only *real* app key indexes?

> > >  #define DEFAULT_CRPL 10
> > > +#define REMOTE_CACHE_LEN 5
> > By the way: why the limits here are set so low (and there doesn't seem to be
> > a way to change them...)?
> When being used as a Config Client, we are just trying to avoid
> reopening the file to fetch the device key to decode the expected
> response.
> (...)
> All the device keys are always available...  they
> just may need to be read into RAM from storage.
Ok, got it.

I was more concerned about the CRPL size. At the moment the limit means
that a node cannot simultaneously talk to more than 10 remote nodes
because of this (mesh/appkey.c:227):

	/* Replay Cache is fixed sized */
	if (l_queue_length(key->replay_cache) >= crpl) {
		int ret = l_queue_foreach_remove(key->replay_cache,
				clean_old_iv_index, L_UINT_TO_PTR(iv_index));

		if (!ret)
			return true;
	}

> > I don't see why the remote key storage should be kept separately (and in
> > binary files?) from the main node json. It makes the implementation much
> > more complicated, as you need to worry about directories etc.
> Again, this creates an open-ended RAM requirement that depends on the
> existence of external resources...
Not necessarily.

I understand that in the current implementation, the storage JSON is
kept in memory as a whole (mesh_node.jconfig), so adding stuff there
indeed increases the memory footprint of a node, but it doesn't need to
be like that.

What are your thoughts on replacing the JSON storage with something
more granular? The main bluetoothd seems to use a directory structure
and INI files, but a key-value store like gdbm might also be an option,
especially for frequently changing values like the sequence number.
A database would allow us to avoid explicit caching and rely on mmap().

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