[Bluez PATCH 1/3] adapter: Use PeripheralLongTermKey to store LTK

[Bluez PATCH 1/3] adapter: Use PeripheralLongTermKey to store LTK

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

Introducing PeripheralLongTermKey group for storing LTK info to
replace the less inclusive term. Currently we still need to write/read
from both to ensure smooth transition, but later we should deprecate
the old term.

Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
---

 src/adapter.c | 41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index d0d38621b8..6b12c9e793 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3868,7 +3868,11 @@ static struct smp_ltk_info *get_peripheral_ltk_info(GKeyFile *key_file,
 
 	DBG("%s", peer);
 
-	ltk = get_ltk(key_file, peer, bdaddr_type, "SlaveLongTermKey");
+	/* Read from both entries. Later we should deprecate Slave. */
+	ltk = get_ltk(key_file, peer, bdaddr_type, "PeripheralLongTermKey");
+	if (!ltk)
+		ltk = get_ltk(key_file, peer, bdaddr_type, "SlaveLongTermKey");
+
 	if (ltk)
 		ltk->central = false;
 
@@ -8415,13 +8419,12 @@ static void new_link_key_callback(uint16_t index, uint16_t length,
 	bonding_complete(adapter, &addr->bdaddr, addr->type, 0);
 }
 
-static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
+static void store_ltk_group(struct btd_adapter *adapter, const bdaddr_t *peer,
 				uint8_t bdaddr_type, const unsigned char *key,
-				uint8_t central, uint8_t authenticated,
+				const char *group, uint8_t authenticated,
 				uint8_t enc_size, uint16_t ediv,
 				uint64_t rand)
 {
-	const char *group = central ? "LongTermKey" : "SlaveLongTermKey";
 	char device_addr[18];
 	char filename[PATH_MAX];
 	GKeyFile *key_file;
@@ -8431,11 +8434,6 @@ static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
 	char *str;
 	int i;
 
-	if (central != 0x00 && central != 0x01) {
-		error("Unsupported LTK type %u", central);
-		return;
-	}
-
 	ba2str(peer, device_addr);
 
 	snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s/info",
@@ -8475,6 +8473,31 @@ static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
 	g_key_file_free(key_file);
 }
 
+static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
+				uint8_t bdaddr_type, const unsigned char *key,
+				uint8_t central, uint8_t authenticated,
+				uint8_t enc_size, uint16_t ediv,
+				uint64_t rand)
+{
+	if (central != 0x00 && central != 0x01) {
+		error("Unsupported LTK type %u", central);
+		return;
+	}
+
+	if (central) {
+		store_ltk_group(adapter, peer, bdaddr_type, key, "LongTermKey",
+				authenticated, enc_size, ediv, rand);
+	} else {
+		/* Store duplicates for now. Later we should deprecate Slave. */
+		store_ltk_group(adapter, peer, bdaddr_type, key,
+				"PeripheralLongTermKey", authenticated,
+				enc_size, ediv, rand);
+		store_ltk_group(adapter, peer, bdaddr_type, key,
+				"SlaveLongTermKey", authenticated,
+				enc_size, ediv, rand);
+	}
+}
+
 static void new_long_term_key_callback(uint16_t index, uint16_t length,
 					const void *param, void *user_data)
 {
-- 
2.33.1.1089.g2158813163f-goog

[Bluez PATCH 2/3] doc: Add PeripheralLongTermKey for storing LTK

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

Update doc to reflect update in adapter.c.

Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
---

 doc/settings-storage.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/doc/settings-storage.txt b/doc/settings-storage.txt
index 1d96cd66d9..3c637c3521 100644
--- a/doc/settings-storage.txt
+++ b/doc/settings-storage.txt
@@ -314,9 +314,9 @@ Long term key) related to a remote device.
   Rand			Integer		Randomizer
 
 
-[SlaveLongTermKey] group contains:
+[PeripheralLongTermKey] group contains:
 
-  Same as the [LongTermKey] group, except for slave keys.
+  Same as the [LongTermKey] group, except for peripheral keys.
 
 
 [ConnectionParameters] group contains:
-- 
2.33.1.1089.g2158813163f-goog

[Bluez PATCH 3/3] adapter: Remove "Master" entry from LTK storage

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

The entry has been deprecated since 2014 and it's time to remove them
altogether.
---
Hi maintainers,
While cleaning this entry, I found that this entry is involved in some
kind of storage file conversion, probably when upgrading BlueZ 4 to 5.
Should we also remove the file conversion too, since it's dated to
2014 as well?


 src/adapter.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 6b12c9e793..3a3c957a6c 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3779,8 +3779,6 @@ static struct smp_ltk_info *get_ltk(GKeyFile *key_file, const char *peer,
 					uint8_t peer_type, const char *group)
 {
 	struct smp_ltk_info *ltk = NULL;
-	GError *gerr = NULL;
-	bool central;
 	char *key;
 	char *rand = NULL;
 
@@ -3836,12 +3834,6 @@ static struct smp_ltk_info *get_ltk(GKeyFile *key_file, const char *peer,
 									NULL);
 	ltk->ediv = g_key_file_get_integer(key_file, group, "EDiv", NULL);
 
-	central = g_key_file_get_boolean(key_file, group, "Master", &gerr);
-	if (gerr)
-		g_error_free(gerr);
-	else
-		ltk->central = central;
-
 	ltk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LTK,
 								ltk->val);
 
@@ -5904,7 +5896,6 @@ static void convert_ltk_entry(GKeyFile *key_file, void *value)
 	g_free(str);
 
 	g_key_file_set_integer(key_file, "LongTermKey", "Authenticated", auth);
-	g_key_file_set_integer(key_file, "LongTermKey", "Master", central);
 	g_key_file_set_integer(key_file, "LongTermKey", "EncSize", enc_size);
 	g_key_file_set_integer(key_file, "LongTermKey", "EDiv", ediv);
 
@@ -8445,9 +8436,6 @@ static void store_ltk_group(struct btd_adapter *adapter, const bdaddr_t *peer,
 		g_error_free(gerr);
 	}
 
-	/* Old files may contain this so remove it in case it exists */
-	g_key_file_remove_key(key_file, "LongTermKey", "Master", NULL);
-
 	for (i = 0; i < 16; i++)
 		sprintf(key_str + (i * 2), "%2.2X", key[i]);
 
-- 
2.33.1.1089.g2158813163f-goog

Re: [Bluez PATCH 3/3] adapter: Remove "Master" entry from LTK storage

[Luiz Augusto von Dentz]

Hi Archie,

On Sun, Oct 31, 2021 at 11:06 PM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> The entry has been deprecated since 2014 and it's time to remove them
> altogether.
> ---
> Hi maintainers,
> While cleaning this entry, I found that this entry is involved in some
> kind of storage file conversion, probably when upgrading BlueZ 4 to 5.
> Should we also remove the file conversion too, since it's dated to
> 2014 as well?

Perhaps we can but I'd had such cleanup as a separate patch then.

>
>  src/adapter.c | 12 ------------
>  1 file changed, 12 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 6b12c9e793..3a3c957a6c 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -3779,8 +3779,6 @@ static struct smp_ltk_info *get_ltk(GKeyFile *key_file, const char *peer,
>                                         uint8_t peer_type, const char *group)
>  {
>         struct smp_ltk_info *ltk = NULL;
> -       GError *gerr = NULL;
> -       bool central;
>         char *key;
>         char *rand = NULL;
>
> @@ -3836,12 +3834,6 @@ static struct smp_ltk_info *get_ltk(GKeyFile *key_file, const char *peer,
>                                                                         NULL);
>         ltk->ediv = g_key_file_get_integer(key_file, group, "EDiv", NULL);
>
> -       central = g_key_file_get_boolean(key_file, group, "Master", &gerr);
> -       if (gerr)
> -               g_error_free(gerr);
> -       else
> -               ltk->central = central;
> -
>         ltk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LTK,
>                                                                 ltk->val);
>
> @@ -5904,7 +5896,6 @@ static void convert_ltk_entry(GKeyFile *key_file, void *value)
>         g_free(str);
>
>         g_key_file_set_integer(key_file, "LongTermKey", "Authenticated", auth);
> -       g_key_file_set_integer(key_file, "LongTermKey", "Master", central);

Weird that it still was setting the "Master" even though this is meant
to convert the old format into the new one.

>         g_key_file_set_integer(key_file, "LongTermKey", "EncSize", enc_size);
>         g_key_file_set_integer(key_file, "LongTermKey", "EDiv", ediv);
>
> @@ -8445,9 +8436,6 @@ static void store_ltk_group(struct btd_adapter *adapter, const bdaddr_t *peer,
>                 g_error_free(gerr);
>         }
>
> -       /* Old files may contain this so remove it in case it exists */
> -       g_key_file_remove_key(key_file, "LongTermKey", "Master", NULL);
> -
>         for (i = 0; i < 16; i++)
>                 sprintf(key_str + (i * 2), "%2.2X", key[i]);
>
> --
> 2.33.1.1089.g2158813163f-goog
>


-- 
Luiz Augusto von Dentz

Re: [Bluez PATCH 3/3] adapter: Remove "Master" entry from LTK storage

[Archie Pusaka]

Hi Luiz,

On Tue, 2 Nov 2021 at 14:13, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Sun, Oct 31, 2021 at 11:06 PM Archie Pusaka <apusaka@google.com> wrote:
> >
> > From: Archie Pusaka <apusaka@chromium.org>
> >
> > The entry has been deprecated since 2014 and it's time to remove them
> > altogether.
> > ---
> > Hi maintainers,
> > While cleaning this entry, I found that this entry is involved in some
> > kind of storage file conversion, probably when upgrading BlueZ 4 to 5.
> > Should we also remove the file conversion too, since it's dated to
> > 2014 as well?
>
> Perhaps we can but I'd had such cleanup as a separate patch then.
>
Got it. Then I would prefer to wait until this series of patches is
merged before removing the file conversion.

> >
> >  src/adapter.c | 12 ------------
> >  1 file changed, 12 deletions(-)
> >
> > diff --git a/src/adapter.c b/src/adapter.c
> > index 6b12c9e793..3a3c957a6c 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -3779,8 +3779,6 @@ static struct smp_ltk_info *get_ltk(GKeyFile *key_file, const char *peer,
> >                                         uint8_t peer_type, const char *group)
> >  {
> >         struct smp_ltk_info *ltk = NULL;
> > -       GError *gerr = NULL;
> > -       bool central;
> >         char *key;
> >         char *rand = NULL;
> >
> > @@ -3836,12 +3834,6 @@ static struct smp_ltk_info *get_ltk(GKeyFile *key_file, const char *peer,
> >                                                                         NULL);
> >         ltk->ediv = g_key_file_get_integer(key_file, group, "EDiv", NULL);
> >
> > -       central = g_key_file_get_boolean(key_file, group, "Master", &gerr);
> > -       if (gerr)
> > -               g_error_free(gerr);
> > -       else
> > -               ltk->central = central;
> > -
> >         ltk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LTK,
> >                                                                 ltk->val);
> >
> > @@ -5904,7 +5896,6 @@ static void convert_ltk_entry(GKeyFile *key_file, void *value)
> >         g_free(str);
> >
> >         g_key_file_set_integer(key_file, "LongTermKey", "Authenticated", auth);
> > -       g_key_file_set_integer(key_file, "LongTermKey", "Master", central);
>
> Weird that it still was setting the "Master" even though this is meant
> to convert the old format into the new one.
>
Probably these "convert" methods precedes the "split LTK entries into
central and peripheral" decision, and this setting is unintentionally
left out when splitting LTKs.

> >         g_key_file_set_integer(key_file, "LongTermKey", "EncSize", enc_size);
> >         g_key_file_set_integer(key_file, "LongTermKey", "EDiv", ediv);
> >
> > @@ -8445,9 +8436,6 @@ static void store_ltk_group(struct btd_adapter *adapter, const bdaddr_t *peer,
> >                 g_error_free(gerr);
> >         }
> >
> > -       /* Old files may contain this so remove it in case it exists */
> > -       g_key_file_remove_key(key_file, "LongTermKey", "Master", NULL);
> > -
> >         for (i = 0; i < 16; i++)
> >                 sprintf(key_str + (i * 2), "%2.2X", key[i]);
> >
> > --
> > 2.33.1.1089.g2158813163f-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Thanks,
Archie

RE: [Bluez,1/3] adapter: Use PeripheralLongTermKey to store LTK

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 884 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=573377

---Test result---

Test Summary:
CheckPatch                    PASS      4.10 seconds
GitLint                       PASS      2.80 seconds
Prep - Setup ELL              PASS      42.57 seconds
Build - Prep                  PASS      0.50 seconds
Build - Configure             PASS      7.91 seconds
Build - Make                  PASS      182.22 seconds
Make Check                    PASS      9.23 seconds
Make Distcheck                PASS      218.49 seconds
Build w/ext ELL - Configure   PASS      8.01 seconds
Build w/ext ELL - Make        PASS      173.06 seconds



---
Regards,
Linux Bluetooth

Re: [Bluez PATCH 1/3] adapter: Use PeripheralLongTermKey to store LTK

[Luiz Augusto von Dentz]

Hi Archie,

On Sun, Oct 31, 2021 at 11:06 PM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> Introducing PeripheralLongTermKey group for storing LTK info to
> replace the less inclusive term. Currently we still need to write/read
> from both to ensure smooth transition, but later we should deprecate
> the old term.
>
> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> ---
>
>  src/adapter.c | 41 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index d0d38621b8..6b12c9e793 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -3868,7 +3868,11 @@ static struct smp_ltk_info *get_peripheral_ltk_info(GKeyFile *key_file,
>
>         DBG("%s", peer);
>
> -       ltk = get_ltk(key_file, peer, bdaddr_type, "SlaveLongTermKey");
> +       /* Read from both entries. Later we should deprecate Slave. */
> +       ltk = get_ltk(key_file, peer, bdaddr_type, "PeripheralLongTermKey");
> +       if (!ltk)
> +               ltk = get_ltk(key_file, peer, bdaddr_type, "SlaveLongTermKey");

If you find the old name being used you better replace it with the new
one and erase the old.

>         if (ltk)
>                 ltk->central = false;
>
> @@ -8415,13 +8419,12 @@ static void new_link_key_callback(uint16_t index, uint16_t length,
>         bonding_complete(adapter, &addr->bdaddr, addr->type, 0);
>  }
>
> -static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
> +static void store_ltk_group(struct btd_adapter *adapter, const bdaddr_t *peer,
>                                 uint8_t bdaddr_type, const unsigned char *key,
> -                               uint8_t central, uint8_t authenticated,
> +                               const char *group, uint8_t authenticated,
>                                 uint8_t enc_size, uint16_t ediv,
>                                 uint64_t rand)
>  {
> -       const char *group = central ? "LongTermKey" : "SlaveLongTermKey";
>         char device_addr[18];
>         char filename[PATH_MAX];
>         GKeyFile *key_file;
> @@ -8431,11 +8434,6 @@ static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
>         char *str;
>         int i;
>
> -       if (central != 0x00 && central != 0x01) {
> -               error("Unsupported LTK type %u", central);
> -               return;
> -       }
> -
>         ba2str(peer, device_addr);
>
>         snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s/info",
> @@ -8475,6 +8473,31 @@ static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
>         g_key_file_free(key_file);
>  }
>
> +static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
> +                               uint8_t bdaddr_type, const unsigned char *key,
> +                               uint8_t central, uint8_t authenticated,
> +                               uint8_t enc_size, uint16_t ediv,
> +                               uint64_t rand)
> +{
> +       if (central != 0x00 && central != 0x01) {
> +               error("Unsupported LTK type %u", central);
> +               return;
> +       }
> +
> +       if (central) {
> +               store_ltk_group(adapter, peer, bdaddr_type, key, "LongTermKey",
> +                               authenticated, enc_size, ediv, rand);
> +       } else {
> +               /* Store duplicates for now. Later we should deprecate Slave. */
> +               store_ltk_group(adapter, peer, bdaddr_type, key,
> +                               "PeripheralLongTermKey", authenticated,
> +                               enc_size, ediv, rand);
> +               store_ltk_group(adapter, peer, bdaddr_type, key,
> +                               "SlaveLongTermKey", authenticated,
> +                               enc_size, ediv, rand);

In not following why you want to keep duplicate entries?

> +       }
> +}
> +
>  static void new_long_term_key_callback(uint16_t index, uint16_t length,
>                                         const void *param, void *user_data)
>  {
> --
> 2.33.1.1089.g2158813163f-goog
>


-- 
Luiz Augusto von Dentz

Re: [Bluez PATCH 1/3] adapter: Use PeripheralLongTermKey to store LTK

[Archie Pusaka]

Hi Luiz,


On Tue, 2 Nov 2021 at 14:17, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Sun, Oct 31, 2021 at 11:06 PM Archie Pusaka <apusaka@google.com> wrote:
> >
> > From: Archie Pusaka <apusaka@chromium.org>
> >
> > Introducing PeripheralLongTermKey group for storing LTK info to
> > replace the less inclusive term. Currently we still need to write/read
> > from both to ensure smooth transition, but later we should deprecate
> > the old term.
> >
> > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> > ---
> >
> >  src/adapter.c | 41 ++++++++++++++++++++++++++++++++---------
> >  1 file changed, 32 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/adapter.c b/src/adapter.c
> > index d0d38621b8..6b12c9e793 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -3868,7 +3868,11 @@ static struct smp_ltk_info *get_peripheral_ltk_info(GKeyFile *key_file,
> >
> >         DBG("%s", peer);
> >
> > -       ltk = get_ltk(key_file, peer, bdaddr_type, "SlaveLongTermKey");
> > +       /* Read from both entries. Later we should deprecate Slave. */
> > +       ltk = get_ltk(key_file, peer, bdaddr_type, "PeripheralLongTermKey");
> > +       if (!ltk)
> > +               ltk = get_ltk(key_file, peer, bdaddr_type, "SlaveLongTermKey");
>
> If you find the old name being used you better replace it with the new
> one and erase the old.
>
> >         if (ltk)
> >                 ltk->central = false;
> >
> > @@ -8415,13 +8419,12 @@ static void new_link_key_callback(uint16_t index, uint16_t length,
> >         bonding_complete(adapter, &addr->bdaddr, addr->type, 0);
> >  }
> >
> > -static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
> > +static void store_ltk_group(struct btd_adapter *adapter, const bdaddr_t *peer,
> >                                 uint8_t bdaddr_type, const unsigned char *key,
> > -                               uint8_t central, uint8_t authenticated,
> > +                               const char *group, uint8_t authenticated,
> >                                 uint8_t enc_size, uint16_t ediv,
> >                                 uint64_t rand)
> >  {
> > -       const char *group = central ? "LongTermKey" : "SlaveLongTermKey";
> >         char device_addr[18];
> >         char filename[PATH_MAX];
> >         GKeyFile *key_file;
> > @@ -8431,11 +8434,6 @@ static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
> >         char *str;
> >         int i;
> >
> > -       if (central != 0x00 && central != 0x01) {
> > -               error("Unsupported LTK type %u", central);
> > -               return;
> > -       }
> > -
> >         ba2str(peer, device_addr);
> >
> >         snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s/info",
> > @@ -8475,6 +8473,31 @@ static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
> >         g_key_file_free(key_file);
> >  }
> >
> > +static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
> > +                               uint8_t bdaddr_type, const unsigned char *key,
> > +                               uint8_t central, uint8_t authenticated,
> > +                               uint8_t enc_size, uint16_t ediv,
> > +                               uint64_t rand)
> > +{
> > +       if (central != 0x00 && central != 0x01) {
> > +               error("Unsupported LTK type %u", central);
> > +               return;
> > +       }
> > +
> > +       if (central) {
> > +               store_ltk_group(adapter, peer, bdaddr_type, key, "LongTermKey",
> > +                               authenticated, enc_size, ediv, rand);
> > +       } else {
> > +               /* Store duplicates for now. Later we should deprecate Slave. */
> > +               store_ltk_group(adapter, peer, bdaddr_type, key,
> > +                               "PeripheralLongTermKey", authenticated,
> > +                               enc_size, ediv, rand);
> > +               store_ltk_group(adapter, peer, bdaddr_type, key,
> > +                               "SlaveLongTermKey", authenticated,
> > +                               enc_size, ediv, rand);
>
> In not following why you want to keep duplicate entries?
>
Based on the discussion in the other thread with Marcel, we don't want
to immediately remove the old entry since this would cause friction
when people are switching around between the newer and older version
of BlueZ.
Instead, for some time we would keep both entries alive, and only
after that we remove the old entry.

> > +       }
> > +}
> > +
> >  static void new_long_term_key_callback(uint16_t index, uint16_t length,
> >                                         const void *param, void *user_data)
> >  {
> > --
> > 2.33.1.1089.g2158813163f-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Thanks,
Archie

Re: [Bluez PATCH 1/3] adapter: Use PeripheralLongTermKey to store LTK

[Luiz Augusto von Dentz]

Hi Archie,

On Tue, Nov 2, 2021 at 2:10 AM Archie Pusaka <apusaka@google.com> wrote:
>
> Hi Luiz,
>
>
> On Tue, 2 Nov 2021 at 14:17, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Archie,
> >
> > On Sun, Oct 31, 2021 at 11:06 PM Archie Pusaka <apusaka@google.com> wrote:
> > >
> > > From: Archie Pusaka <apusaka@chromium.org>
> > >
> > > Introducing PeripheralLongTermKey group for storing LTK info to
> > > replace the less inclusive term. Currently we still need to write/read
> > > from both to ensure smooth transition, but later we should deprecate
> > > the old term.
> > >
> > > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> > > ---
> > >
> > >  src/adapter.c | 41 ++++++++++++++++++++++++++++++++---------
> > >  1 file changed, 32 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/src/adapter.c b/src/adapter.c
> > > index d0d38621b8..6b12c9e793 100644
> > > --- a/src/adapter.c
> > > +++ b/src/adapter.c
> > > @@ -3868,7 +3868,11 @@ static struct smp_ltk_info *get_peripheral_ltk_info(GKeyFile *key_file,
> > >
> > >         DBG("%s", peer);
> > >
> > > -       ltk = get_ltk(key_file, peer, bdaddr_type, "SlaveLongTermKey");
> > > +       /* Read from both entries. Later we should deprecate Slave. */
> > > +       ltk = get_ltk(key_file, peer, bdaddr_type, "PeripheralLongTermKey");
> > > +       if (!ltk)
> > > +               ltk = get_ltk(key_file, peer, bdaddr_type, "SlaveLongTermKey");
> >
> > If you find the old name being used you better replace it with the new
> > one and erase the old.
> >
> > >         if (ltk)
> > >                 ltk->central = false;
> > >
> > > @@ -8415,13 +8419,12 @@ static void new_link_key_callback(uint16_t index, uint16_t length,
> > >         bonding_complete(adapter, &addr->bdaddr, addr->type, 0);
> > >  }
> > >
> > > -static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
> > > +static void store_ltk_group(struct btd_adapter *adapter, const bdaddr_t *peer,
> > >                                 uint8_t bdaddr_type, const unsigned char *key,
> > > -                               uint8_t central, uint8_t authenticated,
> > > +                               const char *group, uint8_t authenticated,
> > >                                 uint8_t enc_size, uint16_t ediv,
> > >                                 uint64_t rand)
> > >  {
> > > -       const char *group = central ? "LongTermKey" : "SlaveLongTermKey";
> > >         char device_addr[18];
> > >         char filename[PATH_MAX];
> > >         GKeyFile *key_file;
> > > @@ -8431,11 +8434,6 @@ static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
> > >         char *str;
> > >         int i;
> > >
> > > -       if (central != 0x00 && central != 0x01) {
> > > -               error("Unsupported LTK type %u", central);
> > > -               return;
> > > -       }
> > > -
> > >         ba2str(peer, device_addr);
> > >
> > >         snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s/info",
> > > @@ -8475,6 +8473,31 @@ static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
> > >         g_key_file_free(key_file);
> > >  }
> > >
> > > +static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
> > > +                               uint8_t bdaddr_type, const unsigned char *key,
> > > +                               uint8_t central, uint8_t authenticated,
> > > +                               uint8_t enc_size, uint16_t ediv,
> > > +                               uint64_t rand)
> > > +{
> > > +       if (central != 0x00 && central != 0x01) {
> > > +               error("Unsupported LTK type %u", central);
> > > +               return;
> > > +       }
> > > +
> > > +       if (central) {
> > > +               store_ltk_group(adapter, peer, bdaddr_type, key, "LongTermKey",
> > > +                               authenticated, enc_size, ediv, rand);
> > > +       } else {
> > > +               /* Store duplicates for now. Later we should deprecate Slave. */
> > > +               store_ltk_group(adapter, peer, bdaddr_type, key,
> > > +                               "PeripheralLongTermKey", authenticated,
> > > +                               enc_size, ediv, rand);
> > > +               store_ltk_group(adapter, peer, bdaddr_type, key,
> > > +                               "SlaveLongTermKey", authenticated,
> > > +                               enc_size, ediv, rand);
> >
> > In not following why you want to keep duplicate entries?
> >
> Based on the discussion in the other thread with Marcel, we don't want
> to immediately remove the old entry since this would cause friction
> when people are switching around between the newer and older version
> of BlueZ.

I guess the problem would be downgrading wouldn't work since we would
have removed the old value once upgrading, in that case I would be
fine keeping it but Id leave a comment since we should probably remove
that as soon as we have a few releases.

> Instead, for some time we would keep both entries alive, and only
> after that we remove the old entry.
>
> > > +       }
> > > +}
> > > +
> > >  static void new_long_term_key_callback(uint16_t index, uint16_t length,
> > >                                         const void *param, void *user_data)
> > >  {
> > > --
> > > 2.33.1.1089.g2158813163f-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Thanks,
> Archie



-- 
Luiz Augusto von Dentz