BLE: Manual BLE security manager db synchronisation #14824

paul-szczepanek-arm · 2021-06-23T21:17:53Z

Summary of changes

Adds new BLE API call to manually attempt to synchronise the security manager DB.

Impact of changes

Migration actions required

Documentation

none

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

ciarmcom · 2021-06-23T21:30:16Z

@paul-szczepanek-arm, thank you for your changes.
@ARMmbed/mbed-os-connectivity @ARMmbed/mbed-os-test @ARMmbed/mbed-os-maintainers please review.

chrisswinchatt-arm · 2021-06-24T12:51:07Z

connectivity/FEATURE_BLE/include/ble/SecurityManager.h

@@ -501,11 +501,29 @@ class SecurityManager
     * Normally all bonding information is lost when device is reset, this requests that the stack
     * attempts to save the information and reload it during initialisation. This is not guaranteed.
     *
+     * @note This option is itself saved together with bonding data. When data is read after reset,
+     * the state of this option decides if data should be restored. If this option has not been saved


My read of this is that writeBondingStateToPersistentStorage is effectively a no-op unless preserveBondingStateOnReset is set to true before the reset. What's the rationale behind this?

preserveBondingStateOnReset is an option that affects behaviour after the reset. Maybe I should've called it loadBondingDataFromPersistentStorageAfterReset

Could writeBondingStateToPersistentStorage implicitly set preserveBondingStateOnReset? Or is there a reason someone would write the data but not automatically load it after a reset?

That is a good point. If I can't find a good reason by tomorrow I'll add the call.

pan-

How do we test this ?

Should we add a command for sync and reset the MCU during the test ?

It is a draft, what is expected to be added to this PR ?

pan- · 2021-06-28T13:52:56Z

connectivity/FEATURE_BLE/include/ble/SecurityManager.h

+     * @note You still need to call preserveBondingStateOnReset(true) before reset happens for data to be
+     * loaded when it's read.
+     *
+     * @note Depending on the driver used to implement the storage solution used this may be a disruptive


Maybe we could use a less ominous tone and instruct the user to prefer calling this function outside of connection otherwise the connection can drop if storage is the internal flash.

Well, this is what it means. Other operations may be affected not just connections. That's why I used generic phrasing.

pan- · 2021-06-28T13:55:03Z

connectivity/FEATURE_BLE/source/generic/FileSecurityDb.h

@@ -136,7 +136,7 @@ class FileSecurityDb : public SecurityDb {

    virtual void restore();

-    virtual void sync(entry_handle_t db_handle);
+    virtual void sync(entry_handle_t db_handle = invalid_entry_handle);


What do we gain with the default parameter ? It is an implementation and it won't be called directly by the BLE framework.

So that I don't have to modify existing code.

pan- · 2021-06-28T13:55:37Z

connectivity/FEATURE_BLE/source/generic/KVStoreSecurityDb.h

@@ -200,7 +200,7 @@ class KVStoreSecurityDb : public SecurityDb {

    virtual void restore();

-    virtual void sync(entry_handle_t db_handle);
+    virtual void sync(entry_handle_t db_handle = invalid_entry_handle);


Same as above

paul-szczepanek-arm · 2021-06-28T17:56:53Z

It will stop being a draft when I verify this :P

paul-szczepanek-arm · 2021-06-29T08:21:18Z

@AGlass0fMilk does this solution look acceptable to you?

paul-szczepanek-arm · 2021-06-29T15:08:01Z

Tested with persistent storage, flushing works so you can sync storage before reset.

AGlass0fMilk · 2021-06-30T18:32:25Z

So this essentially adds a new Security Manager API method to manually flush the security DB to non-volatile storage?

Does the SM still need to be reset afterwards? I see that as an arbitrary requirement -- why does the SM state need to be reset to simply save the current security database?

If this works as you say, I think it's fine. The main goal is to make it clear to users how to save security credentials to NV memory to save them the headache of debugging caching/filesystem-related issues.

paul-szczepanek-arm · 2021-07-01T10:05:52Z

no, you don't need to reset it anymore, the new call causes buffers to flush so your db is good to be restored

ciarmcom · 2021-07-06T11:50:24Z

This pull request has automatically been marked as stale because it has had no recent activity. @AGlass0fMilk, @pan-, @ARMmbed/mbed-os-maintainers, @ARMmbed/mbed-os-test, please complete review of the changes to move the PR forward. Thank you for your contributions.

paul-szczepanek-arm · 2021-07-06T11:58:32Z

@AGlass0fMilk does it solve your problem? Please accept in review or let me know what you don't like.

0xc0170 · 2021-07-09T10:37:08Z

CI started

mbed-ci · 2021-07-09T11:11:12Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

paul-szczepanek-arm requested a review from pan- June 23, 2021 21:17

paul-szczepanek-arm added 2 commits June 23, 2021 22:25

add new BLE API call to sync security db with persisten storage

7b4d1d5

ble security db complete sync implementation

c9d0ff8

paul-szczepanek-arm force-pushed the persistence-fix branch from dcb361f to c9d0ff8 Compare June 23, 2021 21:25

ciarmcom added the release-type: feature label Jun 23, 2021

ciarmcom requested review from a team June 23, 2021 21:30

ciarmcom added needs: review component: connectivity component: test labels Jun 23, 2021

paul-szczepanek-arm mentioned this pull request Jun 23, 2021

[BLE] Bonding information is not saved with FileSecurityDb unless SecurityManager::reset() is called #14685

Closed

chrisswinchatt-arm approved these changes Jun 24, 2021

View reviewed changes

chrisswinchatt-arm reviewed Jun 24, 2021

View reviewed changes

pan- reviewed Jun 28, 2021

View reviewed changes

add implicit call to preserve state on reset

470917f

paul-szczepanek-arm requested a review from AGlass0fMilk June 29, 2021 08:20

paul-szczepanek-arm marked this pull request as ready for review June 29, 2021 15:06

paul-szczepanek-arm requested a review from pan- July 5, 2021 08:07

ciarmcom added the stale Stale Pull Request label Jul 6, 2021

ciarmcom removed the stale Stale Pull Request label Jul 6, 2021

0xc0170 approved these changes Jul 9, 2021

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Jul 9, 2021

mergify bot added ready for merge and removed needs: CI labels Jul 9, 2021

0xc0170 merged commit 3c2c5be into ARMmbed:master Jul 12, 2021

mergify bot removed the ready for merge label Jul 12, 2021

mbedmain added release-version: 6.13.0 Release-pending and removed release-type: feature Release-pending labels Jul 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLE: Manual BLE security manager db synchronisation #14824

BLE: Manual BLE security manager db synchronisation #14824

paul-szczepanek-arm commented Jun 23, 2021

ciarmcom commented Jun 23, 2021

chrisswinchatt-arm Jun 24, 2021 •

edited

Loading

paul-szczepanek-arm Jun 24, 2021

chrisswinchatt-arm Jun 24, 2021

paul-szczepanek-arm Jun 24, 2021

pan- left a comment •

edited

Loading

pan- Jun 28, 2021

paul-szczepanek-arm Jun 28, 2021

pan- Jun 28, 2021

paul-szczepanek-arm Jun 28, 2021

pan- Jun 28, 2021

paul-szczepanek-arm Jun 28, 2021

paul-szczepanek-arm commented Jun 28, 2021

paul-szczepanek-arm commented Jun 29, 2021

paul-szczepanek-arm commented Jun 29, 2021

AGlass0fMilk commented Jun 30, 2021

paul-szczepanek-arm commented Jul 1, 2021

ciarmcom commented Jul 6, 2021

paul-szczepanek-arm commented Jul 6, 2021

0xc0170 commented Jul 9, 2021

mbed-ci commented Jul 9, 2021

BLE: Manual BLE security manager db synchronisation #14824

BLE: Manual BLE security manager db synchronisation #14824

Conversation

paul-szczepanek-arm commented Jun 23, 2021

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

ciarmcom commented Jun 23, 2021

chrisswinchatt-arm Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan- left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-szczepanek-arm commented Jun 28, 2021

paul-szczepanek-arm commented Jun 29, 2021

paul-szczepanek-arm commented Jun 29, 2021

AGlass0fMilk commented Jun 30, 2021

paul-szczepanek-arm commented Jul 1, 2021

ciarmcom commented Jul 6, 2021

paul-szczepanek-arm commented Jul 6, 2021

0xc0170 commented Jul 9, 2021

mbed-ci commented Jul 9, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

chrisswinchatt-arm Jun 24, 2021 •

edited

Loading

pan- left a comment •

edited

Loading