Add storage to the example ACL implementation #14253

harimau-qirex · 2022-01-25T18:29:02Z

Problem

The ACL module needs to be able to store ACL entries between runs of the device.

Change overview

Adds support for ACL persistent storage. The ACL entries are saved to the persistent storage whenever an entry is created, updated, or deleted.
Sets the ACL storage on Linux, backed by the KVS.

Testing

Manually tested on Linux by mlepage-google. Procedure was to write an entry to ACL in chip-all-clusters-app, close the app, then re-open it and verify that the data loaded properly.

It uses a persistent storage delegate to store the data as a TLV-formatted blob. Platforms or applications that want content-aware storage can either implement a persistent storage delegate that reads the TLV-formatted blob or implement the AccessControl::Delegate interface.

* Don't specify buffer sizes for writers. * Use form of Next that includes both tag and type. * Don't wrap subjects and targets in an unnecessary structure layer.

…or deleted.

github-actions · 2022-01-25T19:25:28Z

PR #14253: Size comparison from 0902641 to fd85837

Increases above 0.2%:

platform	target	config	section	`0902641`	`fd85837`	change	% change
linux	thermostat-no-ble	arm64	(read only)	2043436	2063484	20048	1.0
			(read/write)	146625	150289	3664	2.5
			.bss	65649	68817	3168	4.8
			.data	904	960	56	6.2
			.data.rel.ro	73072	73480	408	0.6
			.got	4048	4064	16	0.4
			.init_array	320	336	16	5.0
			.rodata	130620	131068	448	0.3
			.text	`1698512`	1716912	18400	1.1
p6	all-clusters-app	default	.data	2584	2592	8	0.3

Increases (3 builds for linux, mbed, p6)

platform	target	config	section	`0902641`	`fd85837`	change	% change
linux	thermostat-no-ble	arm64	(read only)	2043436	2063484	20048	1.0
			(read/write)	146625	150289	3664	2.5
			.bss	65649	68817	3168	4.8
			.data	904	960	56	6.2
			.data.rel.ro	73072	73480	408	0.6
			.got	4048	4064	16	0.4
			.init_array	320	336	16	5.0
			.rodata	130620	131068	448	0.3
			.text	`1698512`	1716912	18400	1.1
mbed	all-clusters-app	CY8CPROTO_062_4343W+release	(read/write)	2349912	2351536	1624	0.1
			.text	1312512	1314136	1624	0.1
p6	all-clusters-app	default	(read/write)	2408048	`2409776`	1728	0.1
			.data	2584	2592	8	0.3
			.text	1366312	`1368040`	1728	0.1

Full report (31 builds for cyw30739, efr32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)

platform	target	config	section	`0902641`	`fd85837`	change	% change
cyw30739	light	cyw930739m2evb_01	(read/write)	572258	572258	0	0.0
			.app_xip_area	477320	477320	0	0.0
			.bss	77684	77684	0	0.0
			.data	596	596	0	0.0
			.rodata	0	0	0	0.0
			.text	0	0	0	0.0
efr32	lighting-app	BRD4161A	(read only)	832688	832688	0	0.0
			(read/write)	127216	127216	0	0.0
			.bss	125312	125312	0	0.0
			.data	1900	1900	0	0.0
			.text	832680	832680	0	0.0
		BRD4161A+rpc	(read only)	820068	820068	0	0.0
			(read/write)	143872	143872	0	0.0
			.bss	141872	141872	0	0.0
			.data	2000	2000	0	0.0
			.text	820060	820060	0	0.0
	window-app	BRD4161A	(read only)	808864	808864	0	0.0
			(read/write)	125872	125872	0	0.0
			.bss	124016	124016	0	0.0
			.data	1856	1856	0	0.0
			.text	808856	808856	0	0.0
k32w	light	k32w061+release	(read/write)	660496	660496	0	0.0
			.bss	77456	77456	0	0.0
			.data	1868	1868	0	0.0
			.text	575372	575372	0	0.0
	lock	k32w061+release	(read/write)	661392	661392	0	0.0
			.bss	77720	77720	0	0.0
			.data	1892	1892	0	0.0
			.text	575980	575980	0	0.0
linux	chip-tool-ipv6only	arm64	(read only)	`8697804`	`8697804`	0	0.0
			(read/write)	390817	390817	0	0.0
			.bss	56017	56017	0	0.0
			.data	1128	1128	0	0.0
			.data.rel.ro	260888	260888	0	0.0
			.dynamic	560	560	0	0.0
			.got	69040	69040	0	0.0
			.init	24	24	0	0.0
			.init_array	200	200	0	0.0
			.rodata	532532	532532	0	0.0
			.text	7326532	7326532	0	0.0
	thermostat-no-ble	arm64	(read only)	2043436	2063484	20048	1.0
			(read/write)	146625	150289	3664	2.5
			.bss	65649	68817	3168	4.8
			.data	904	960	56	6.2
			.data.rel.ro	73072	73480	408	0.6
			.dynamic	560	560	0	0.0
			.got	4048	4064	16	0.4
			.init	24	24	0	0.0
			.init_array	320	336	16	5.0
			.rodata	130620	131068	448	0.3
			.text	`1698512`	1716912	18400	1.1
mbed	all-clusters-app	CY8CPROTO_062_4343W+release	(read only)	6224	6224	0	0.0
			(read/write)	2349912	2351536	1624	0.1
			.bss	189404	189404	0	0.0
			.data	5296	5296	0	0.0
			.text	1312512	1314136	1624	0.1
	lighting-app	CY8CPROTO_062_4343W+release	(read only)	6224	6224	0	0.0
			(read/write)	2332232	2332232	0	0.0
			.bss	180920	180920	0	0.0
			.data	5576	5576	0	0.0
			.text	`1294832`	`1294832`	0	0.0
	lock-app	CY8CPROTO_062_4343W+release	(read only)	6224	6224	0	0.0
			(read/write)	2302816	2302816	0	0.0
			.bss	180824	180824	0	0.0
			.data	5568	5568	0	0.0
			.text	1265416	1265416	0	0.0
	pigweed-app	CY8CPROTO_062_4343W+release	(read only)	6224	6224	0	0.0
			(read/write)	1139712	1139712	0	0.0
			.bss	11756	11756	0	0.0
			.data	4368	4368	0	0.0
			.text	103096	103096	0	0.0
	shell	CY8CPROTO_062_4343W+release	(read only)	6224	6224	0	0.0
			(read/write)	2053896	2053896	0	0.0
			.bss	156876	156876	0	0.0
			.data	4864	4864	0	0.0
			.text	1016496	1016496	0	0.0
nrfconnect	lighting-app	nrf52840dk_nrf52840	(read/write)	941035	941035	0	0.0
			bss	119012	119012	0	0.0
			rodata	109072	109072	0	0.0
			text	635356	635356	0	0.0
		nrf52840dk_nrf52840+rpc	(read/write)	926519	926519	0	0.0
			bss	116056	116056	0	0.0
			rodata	101524	101524	0	0.0
			text	630748	630748	0	0.0
		nrf52840dongle_nrf52840	(read/write)	991679	991679	0	0.0
			bss	121852	121852	0	0.0
			rodata	113824	113824	0	0.0
			text	667552	667552	0	0.0
		nrf5340dk_nrf5340_cpuapp	(read/write)	850862	850862	0	0.0
			bss	115796	115796	0	0.0
			rodata	102248	102248	0	0.0
			text	552276	552276	0	0.0
	lock-app	nrf52840dk_nrf52840	(read/write)	911507	911507	0	0.0
			bss	119064	119064	0	0.0
			rodata	104128	104128	0	0.0
			text	610932	610932	0	0.0
		nrf5340dk_nrf5340_cpuapp	(read/write)	821578	821578	0	0.0
			bss	115880	115880	0	0.0
			rodata	97356	97356	0	0.0
			text	527888	527888	0	0.0
	pigweed-app	nrf52840dk_nrf52840	(read/write)	541835	541835	0	0.0
			bss	52588	52588	0	0.0
			rodata	50104	50104	0	0.0
			text	376940	376940	0	0.0
	pump-app	nrf52840dk_nrf52840	(read/write)	914183	914183	0	0.0
			bss	118808	118808	0	0.0
			rodata	104516	104516	0	0.0
			text	613396	613396	0	0.0
	pump-controller-app	nrf52840dk_nrf52840	(read/write)	909351	909351	0	0.0
			bss	118836	118836	0	0.0
			rodata	103620	103620	0	0.0
			text	609428	609428	0	0.0
	shell	nrf52840dk_nrf52840	(read/write)	798203	798203	0	0.0
			bss	109776	109776	0	0.0
			rodata	78288	78288	0	0.0
			text	533640	533640	0	0.0
		nrf5340dk_nrf5340_cpuapp	(read/write)	711022	711022	0	0.0
			bss	107664	107664	0	0.0
			rodata	72592	72592	0	0.0
			text	451316	451316	0	0.0
p6	all-clusters-app	default	(read/write)	2408048	`2409776`	1728	0.1
			.bss	117748	117748	0	0.0
			.data	2584	2592	8	0.3
			.text	1366312	`1368040`	1728	0.1
	light-app	default	(read/write)	2327768	2327768	0	0.0
			.bss	105512	105512	0	0.0
			.data	2408	2408	0	0.0
			.text	1286032	1286032	0	0.0
	lock-app	default	(read/write)	`2297208`	`2297208`	0	0.0
			.bss	105256	105256	0	0.0
			.data	2360	2360	0	0.0
			.text	1255472	1255472	0	0.0
qpg	lighting-app	qpg6105+debug	(read only)	565568	565568	0	0.0
			(read/write)	146936	146936	0	0.0
			.bss	89656	89656	0	0.0
			.data	1060	1060	0	0.0
			.text	560248	560248	0	0.0
	lock-app	qpg6105+debug	(read only)	514324	514324	0	0.0
			(read/write)	146940	146940	0	0.0
			.bss	89128	89128	0	0.0
			.data	992	992	0	0.0
			.text	509004	509004	0	0.0
	persistent-storage-app	qpg6105+debug	(read only)	107140	107140	0	0.0
			(read/write)	146940	146940	0	0.0
			.bss	38504	38504	0	0.0
			.data	288	288	0	0.0
			.text	101820	101820	0	0.0
telink	lighting-app	tlsr9518adk80d	(read/write)	839534	839534	0	0.0
			bss	87460	87460	0	0.0
			noinit	37160	37160	0	0.0
			text	586050	586050	0	0.0

src/access/examples/ExampleAccessControlDelegate.cpp

mlepage-google

Give me a couple hours and I'll pull it again and run it to verify it's working on Linux.

mlepage-google · 2022-01-26T20:55:12Z

OK I ran it on Linux and used REPL to make this ACL and it seemed to read back OK when running second time.

acl = [
  Clusters.AccessControl.Structs.AccessControlEntry(
    fabricIndex = 1,
    privilege = Clusters.AccessControl.Enums.Privilege.kAdminister,
    authMode = Clusters.AccessControl.Enums.AuthMode.kCase,
    subjects = [ 12344321 ] ),
  Clusters.AccessControl.Structs.AccessControlEntry(
    fabricIndex = 2,
    privilege = Clusters.AccessControl.Enums.Privilege.kOperate,
    authMode = Clusters.AccessControl.Enums.AuthMode.kCase,
    subjects = [ 55667788, 42424242, 53535353 ],
    targets = [
      Clusters.AccessControl.Structs.Target(
        cluster = 343,
      ),
      Clusters.AccessControl.Structs.Target(
        endpoint = 7,
      ),
      Clusters.AccessControl.Structs.Target(
        deviceType = 49,
      ),
    ]
  ),
]

src/access/examples/ExampleAccessControlDelegate.cpp

mlepage-google

Approve with the note that I don't like the serialize/deserialize code here, this is just to get something saving at all so when we turn ACL on, we don't lose access every time we restart. The plan is to move persistence out of this module to a higher level, in the cluster code, and use more of the generated code for encode/decode. This is just a stepping stone.

andy31415 · 2022-01-26T22:06:06Z

fast track: change created and approved by a domain owner, sufficient time passed for cross timezone review.

woody-apple · 2022-01-27T03:33:32Z

Removing fast track, domain owner not part of the policy, and this is < 3 days. However, this seems OK to merge.

woody-apple · 2022-01-27T03:34:57Z

Doh, needs one more per policy. @saurabhst @Damian-Nordic

tcarmelveilleux · 2022-01-28T21:17:00Z

src/lib/support/DefaultStorageKeyAllocator.h

+    // Access Control List
+
+    const char * AccessControlList() { return Format("acl"); }
+    const char * AccessControlEntry(size_t index) { return Format("acl/%zx", index); }


This will not work on some platforms due to missing "%z" specifier. Suggest:

const char * AccessControlEntry(size_t index) { return Format("acl/%x", static_cast(index)); }

I've created #14547 to fix this.

* Add a simple implementation of storage for ACLs. It uses a persistent storage delegate to store the data as a TLV-formatted blob. Platforms or applications that want content-aware storage can either implement a persistent storage delegate that reads the TLV-formatted blob or implement the AccessControl::Delegate interface. * Use tags correctly and fix a format string. * Applied improvements suggested by bzbarsky. * Don't specify buffer sizes for writers. * Use form of Next that includes both tag and type. * Don't wrap subjects and targets in an unnecessary structure layer. * Save the ACL entries to flash whenever an entry is created, updated, or deleted. * Add simple storage for ACL. * Style fixes. * Remove unneeded and potentially misleading comments. * Missed one of the comments. * Explain choice of buffer sizes. * Style fixes.

Replace the 'z' format specifier added in #14253 with one that's supported in all platforms.

harimau-qirex added 6 commits January 25, 2022 13:18

Use tags correctly and fix a format string.

3d13822

Applied improvements suggested by bzbarsky.

6e70749

* Don't specify buffer sizes for writers. * Use form of Next that includes both tag and type. * Don't wrap subjects and targets in an unnecessary structure layer.

Save the ACL entries to flash whenever an entry is created, updated, …

05f6099

…or deleted.

Add simple storage for ACL.

e124814

Style fixes.

fd85837

harimau-qirex requested a review from mlepage-google January 25, 2022 18:29

boring-cyborg bot added examples lib labels Jan 25, 2022

pullapprove bot requested review from xylophone21 and yufengwangca January 25, 2022 18:29

pullapprove bot added the review - pending label Jan 25, 2022

woody-apple approved these changes Jan 26, 2022

View reviewed changes

andy31415 reviewed Jan 26, 2022

View reviewed changes

src/access/examples/ExampleAccessControlDelegate.cpp Outdated Show resolved Hide resolved

andy31415 reviewed Jan 26, 2022

View reviewed changes

src/access/examples/ExampleAccessControlDelegate.cpp Outdated Show resolved Hide resolved

andy31415 reviewed Jan 26, 2022

View reviewed changes

src/access/examples/ExampleAccessControlDelegate.cpp Outdated Show resolved Hide resolved

andy31415 approved these changes Jan 26, 2022

View reviewed changes

mlepage-google suggested changes Jan 26, 2022

View reviewed changes

Remove unneeded and potentially misleading comments.

d8a7999

mlepage-google reviewed Jan 26, 2022

View reviewed changes

src/access/examples/ExampleAccessControlDelegate.cpp Outdated Show resolved Hide resolved

mlepage-google approved these changes Jan 26, 2022

View reviewed changes

Missed one of the comments.

7b91cc9

andy31415 added the fast track label Jan 26, 2022

harimau-qirex added 2 commits January 26, 2022 17:12

Explain choice of buffer sizes.

eda3a44

Style fixes.

3e9d734

woody-apple removed the fast track label Jan 27, 2022

andy31415 merged commit 812f820 into project-chip:master Jan 27, 2022

harimau-qirex deleted the simple-acl-storage branch January 27, 2022 16:24

mlepage-google mentioned this pull request Jan 27, 2022

Implement simple access control cluster persistence/storage #10251

Closed

tcarmelveilleux reviewed Jan 28, 2022

View reviewed changes

harimau-qirex mentioned this pull request Jan 28, 2022

Remove the 'z' format specifier in the storage key names. #14547

Merged

harimau-qirex added a commit that referenced this pull request Feb 1, 2022

Remove the 'z' format specifier in the storage key names. (#14547)

9de9890

Replace the 'z' format specifier added in #14253 with one that's supported in all platforms.

harimau-qirex mentioned this pull request Feb 1, 2022

Move the access control ownership and initialization to Server. #14680

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add storage to the example ACL implementation #14253

Add storage to the example ACL implementation #14253

harimau-qirex commented Jan 25, 2022

github-actions bot commented Jan 25, 2022 •

edited

Loading

mlepage-google left a comment

mlepage-google commented Jan 26, 2022

mlepage-google left a comment

andy31415 commented Jan 26, 2022

woody-apple commented Jan 27, 2022

woody-apple commented Jan 27, 2022

tcarmelveilleux Jan 28, 2022

harimau-qirex Jan 28, 2022

Add storage to the example ACL implementation #14253

Add storage to the example ACL implementation #14253

Conversation

harimau-qirex commented Jan 25, 2022

Problem

Change overview

Testing

github-actions bot commented Jan 25, 2022 • edited Loading

mlepage-google left a comment

Choose a reason for hiding this comment

mlepage-google commented Jan 26, 2022

mlepage-google left a comment

Choose a reason for hiding this comment

andy31415 commented Jan 26, 2022

woody-apple commented Jan 27, 2022

woody-apple commented Jan 27, 2022

tcarmelveilleux Jan 28, 2022

Choose a reason for hiding this comment

harimau-qirex Jan 28, 2022

Choose a reason for hiding this comment

github-actions bot commented Jan 25, 2022 •

edited

Loading