Remove TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP from kv_config #14490

harmut01 · 2021-03-30T14:48:45Z

Summary of changes

NO_RBP (no rollback protection) is intended to not require an internal
TDB, however, DeviceKey, which we use to derive SecureStore's
encryption key, still does. Currently, no internal TDB is created with
these two configurations, meaning there's no way to store the DeviceKey
and SecureStore doesn't work.

Impact of changes

Migration actions required

Documentation

ARMmbed/mbed-os-5-docs#1440 raised to remove references to TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP from the online documentation.

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[x] 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

NO_RBP (no rollback protection) is intended to not require an internal TDB, however, DeviceKey, which we use to derive SecureStore's encryption key, still does. Currently, no internal TDB is created with these two configurations, meaning there's no way to store the DeviceKey and SecureStore doesn't work.

ciarmcom · 2021-03-30T15:00:20Z

@harmut01, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

LDong-Arm · 2021-03-30T15:37:11Z

@harmut01 Thanks for the change. Please add "Fixes: #11788" to the PR description - this also automatically closes the issue upon merging of this PR.

@ARMmbed/mbed-os-core FYI, the complete removal of **_NO_RBP was advised by @davidsaada who is the author of SecureStore, see #11788 (comment)

LDong-Arm · 2021-03-30T15:49:23Z

@harmut01 Please also update the README.md of https://github.com/ARMmbed/mbed-os-example-kvstore, thanks.

harmut01 · 2021-03-31T12:04:31Z

Thanks @LDong-Arm, I’ve raised ARMmbed/mbed-os-example-kvstore#75 to update the KVStore example README but can’t add you as a reviewer. Could you take a look?

LDong-Arm · 2021-03-31T12:09:49Z

Thanks @LDong-Arm, I’ve raised #75 to update the KVStore example README but can’t add you as a reviewer. Could you take a look?

Thanks, I'll have a look. It's ARMmbed/mbed-os-example-kvstore#75 (full URL for a different repo).

0xc0170 · 2021-04-06T14:27:21Z

CI started

mbed-ci · 2021-04-06T18:40:14Z

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_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-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-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	✔️

0xc0170 · 2021-04-07T07:45:48Z

Major update

vs

Functionality change

@harmut01 @LDong-Arm is this breaking change? We can't merge this now if that is the case (the next major release should contain more changes to have justification to release a major version).

LDong-Arm · 2021-04-07T08:37:52Z

@0xc0170 It's removal of a feature that has been broken ever since its introduction. For this reason I don't think there can be any existing users of it. But we need to decide if this qualifies as a major change or not.

harmut01 · 2021-04-07T08:43:11Z

Thanks @LDong-Arm, also important to note that support for it has since been dropped, although the source code hasn't been removed, see #11788 (comment).

The documentation previously referred to a weakly defined function `storage_configuration`, however, this function was replaced at some stage by `kv_init_storage_config`. Refactor the explanation on how to override the default configurations to reflect this. Also, remove the snippet which was used to show the implentation of `storage_configuration`.

Pull request has been modified.

0xc0170 · 2021-04-15T10:51:33Z

CI restarted

mbed-ci · 2021-04-15T13:35:56Z

Jenkins CI Test : ✔️ SUCCESS

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

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-example-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-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
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_cmake-example-test	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

0xc0170 · 2021-04-28T11:13:30Z

@0xc0170 It's removal of a feature that has been broken ever since its introduction. For this reason I don't think there can be any existing users of it. But we need to decide if this qualifies as a major change or not.

@Patater @donatieng If this is major change (even removing broken functionality still is breaking in terms of definition), we should park this until we get master ready for breaking changes like this (something to agree on, as this is one of the many).

How shall we proceed? Should we keep PRs with major tag opened to monitor them or create tracking issue for major changes and maintain a list of all parked major changes to renew once we get the last release of 6.x out - to avoid stale/conflicts pokes).

I'll think about this and we shall discuss soon.

0xc0170 · 2021-05-06T14:14:04Z

Or one way to explore if we can deprecate this immediately (docs PR could remove the references and left mentions if anyone is looking at them, they are deprecated and docs were removed) and we will do this clean up with the rest of deprecated functionality.

Open for suggestions how to tackle this one.

LDong-Arm · 2021-05-06T15:00:42Z

Or one way to explore if we can deprecate this immediately (docs PR could remove the references and left mentions if anyone is looking at them, they are deprecated and docs were removed) and we will do this clean up with the rest of deprecated functionality.

Open for suggestions how to tackle this one.

I just doubled checked, storage.storage_type is a string, and C/C++ macros don't support checking/comparing string values. Not sure if a deprecation check is doable here. 😟

LDong-Arm · 2021-05-06T15:05:51Z

storage/kvstore/kv_config/global/mbed_lib.json

@@ -2,7 +2,7 @@
 "name": "storage",
    "config": {
        "storage_type": {
-            "help": "Options are TDB_INTERNAL, TDB_EXTERNAL, TDB_EXTERNAL_NO_RBP, FILESYSTEM, FILESYSTEM_NO_RBP or default. If default, the storage type will be chosen according to the component defined in targets.json",
+            "help": "Options are TDB_INTERNAL, TDB_EXTERNAL, FILESYSTEM or default. If default, the storage type will be chosen according to the component defined in targets.json",


@0xc0170
If we don't want a breaking change (removal of TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP) for now, the most we can do is say

`TDB_EXTERNAL_NO_RBP` and `FILESYSTEM_NO_RBP` do not work and are thus deprecated

in this help text and above functions that define them. We may not benefit from compile-time deprecation warnings due to preprocessor being unable to compare macros with strings, but I don't have any best ideas.

ciarmcom · 2021-05-11T15:52:08Z

This pull request has automatically been marked as stale because it has had no recent activity. @davidsaada, @donatieng, please complete review of the changes to move the PR forward. Thank you for your contributions.

harmut01 · 2021-05-11T16:05:57Z

Closing this since NO_RBP will be deprecated by @LDong-Arm in a separate PR.

LDong-Arm · 2021-05-11T16:26:33Z

Or one way to explore if we can deprecate this immediately (docs PR could remove the references and left mentions if anyone is looking at them, they are deprecated and docs were removed) and we will do this clean up with the rest of deprecated functionality.

Open for suggestions how to tackle this one.

@0xc0170 #14657 created to change removal to deprecation.

harmut01 requested review from davidsaada and LDong-Arm and removed request for davidsaada March 30, 2021 14:48

ciarmcom added the release-type: major label Mar 30, 2021

ciarmcom requested a review from a team March 30, 2021 15:00

ciarmcom added needs: review component: core component: docs labels Mar 30, 2021

harmut01 requested a review from Patater March 30, 2021 15:04

LDong-Arm approved these changes Mar 31, 2021

View reviewed changes

harmut01 mentioned this pull request Mar 31, 2021

Remove TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP from kv configurations ARMmbed/mbed-os-example-kvstore#75

Open

harmut01 mentioned this pull request Mar 31, 2021

Remove TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP from KVStore docs ARMmbed/mbed-os-5-docs#1440

Merged

LDong-Arm approved these changes Apr 1, 2021

View reviewed changes

Patater previously approved these changes Apr 1, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Apr 1, 2021

0xc0170 previously approved these changes Apr 6, 2021

View reviewed changes

mergify bot added the ready for merge label Apr 6, 2021

mergify bot removed the needs: CI label Apr 6, 2021

0xc0170 added needs: review and removed ready for merge labels Apr 7, 2021

0xc0170 requested a review from donatieng April 28, 2021 11:13

LDong-Arm reviewed May 6, 2021

View reviewed changes

ciarmcom added the stale Stale Pull Request label May 11, 2021

harmut01 closed this May 11, 2021

mergify bot removed component: core component: docs needs: review release-type: major stale Stale Pull Request labels May 11, 2021

LDong-Arm mentioned this pull request May 11, 2021

Deprecate broken TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP in kv_config #14657

Merged

Patater mentioned this pull request Oct 5, 2021

No internal kv store for storing Root Of Trust key when using TDB_EXTERNAL_NO_RBP #11788

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP from kv_config #14490

Remove TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP from kv_config #14490

harmut01 commented Mar 30, 2021 •

edited

Loading

ciarmcom commented Mar 30, 2021

LDong-Arm commented Mar 30, 2021

LDong-Arm commented Mar 30, 2021

harmut01 commented Mar 31, 2021 •

edited

Loading

LDong-Arm commented Mar 31, 2021

0xc0170 commented Apr 6, 2021

mbed-ci commented Apr 6, 2021

0xc0170 commented Apr 7, 2021

LDong-Arm commented Apr 7, 2021

harmut01 commented Apr 7, 2021

0xc0170 commented Apr 15, 2021

mbed-ci commented Apr 15, 2021

0xc0170 commented Apr 28, 2021 •

edited

Loading

0xc0170 commented May 6, 2021

LDong-Arm commented May 6, 2021

LDong-Arm May 6, 2021

ciarmcom commented May 11, 2021

harmut01 commented May 11, 2021

LDong-Arm commented May 11, 2021

Remove TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP from kv_config #14490

Remove TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP from kv_config #14490

Conversation

harmut01 commented Mar 30, 2021 • edited Loading

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

ciarmcom commented Mar 30, 2021

LDong-Arm commented Mar 30, 2021

LDong-Arm commented Mar 30, 2021

harmut01 commented Mar 31, 2021 • edited Loading

LDong-Arm commented Mar 31, 2021

0xc0170 commented Apr 6, 2021

mbed-ci commented Apr 6, 2021

Jenkins CI Test : ✔️ SUCCESS

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

0xc0170 commented Apr 7, 2021

LDong-Arm commented Apr 7, 2021

harmut01 commented Apr 7, 2021

0xc0170 commented Apr 15, 2021

mbed-ci commented Apr 15, 2021

Jenkins CI Test : ✔️ SUCCESS

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

0xc0170 commented Apr 28, 2021 • edited Loading

0xc0170 commented May 6, 2021

LDong-Arm commented May 6, 2021

LDong-Arm May 6, 2021

Choose a reason for hiding this comment

ciarmcom commented May 11, 2021

harmut01 commented May 11, 2021

LDong-Arm commented May 11, 2021

harmut01 commented Mar 30, 2021 •

edited

Loading

harmut01 commented Mar 31, 2021 •

edited

Loading

0xc0170 commented Apr 28, 2021 •

edited

Loading