-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle access control events from non-cluster operations #14326
Comments
There is a snag with the "any code can change ACLs approach" if we try to fire events from Access Control module up to cluster to report. The report has metadata: who (which admin) is doing the change. This comes from the subject descriptor. The subject descriptor is not provided to the Access Control module when entries are being created/read/updated/deleted. So if we wish to fire events from the module, we need a way to get that metadata to the listener (mainly the Access Control cluster) so it can use it when generating the event report. |
This is done now by PR #17357 Because CRUD APIs now take an optional subject descriptor, and fire notify callbacks to registered listeners with the details of the CRUD operation (including the subject descriptor). The cluster registers itself as a listener, and uses these notification callbacks to trigger events, if a subject descriptor was provided. It's up to calling code to provide a correct subject descriptor if one is available; IM operations always do so, but we have other cases (e.g. during boot we are creating entries but not in the same sense). The reason subject descriptor is optional is that CRUD operations can happen internal to the system, and therefore unattributed. E.g. when loading entries from persistent storage into the system module, we are creating entries, but "not really" in the sense that events care about, and this is being done by the "self" not another node. We can debate whether we want the subject descriptor to always be required, and have special values for "self" or otherwise unattributed, but right now this is the easiest way to get correct behaviour (events when we want them, and not when we don't). Importantly, the AddNOC command provides an appropriate subject descriptor when creating the initial ACL entry. If you commission a device then read the AccessControl cluster events, you'll see the event for that CRUD operation. |
As discussed here:
#13838 (comment)
Currently, all ACL management is performed by the cluster, which generates events, except for one added ACL in the AddNOC command, which has a special mechanism to fire a notification up to the cluster so its event can be generated.
The question is, what about other non-cluster non-AddNOC ACL management, how does it generate events? Generally the DM working group frowned upon this, but we're not sure it's totally forbidden right now in the spec.
So look that up, and if it's not forbidden, decide whether to forbid it (in which case the current approach is good), or if it's not going to be forbidden, change the current approach.
If we keep the approach ("random code shouldn't change ACLs"), we just have to hook up that special mechanism, probably by making a "create admin ACL entry" API right on AccessControl, that does the creation but also fires the listener (which is present but not fully implemented). Then, have the cluster register as a listener, and handle that one notification. (The cluster won't have to handle other notifications via the listener, because it does it directly as it performs operations.)
If we go with the "any code can change ACLs approach," we need to make that listener interface fleshed out and able to handle all the operations (create, read, update, delete). The cluster will register a listener and fire events for all of them. Except, when the cluster is populating data at startup (from flash into AccessControl), it will need to ignore those notifications as it interacts with the AccessControl API.
The text was updated successfully, but these errors were encountered: