Role-based Access Control #6521
Conversation
| @@ -546,6 +546,9 @@ UA_Node_copy(const UA_Node *src, UA_Node *dst) { | |||
| UA_StatusCode retval = UA_NodeId_copy(&srchead->nodeId, &dsthead->nodeId); | |||
| retval |= UA_QualifiedName_copy(&srchead->browseName, &dsthead->browseName); | |||
|
|
|||
| /* Copy the RolePermissions */ | |||
| memcpy(dsthead->rolePermissions, srchead->rolePermissions, sizeof(UA_RoleSet) * 16); | |||
There was a problem hiding this comment.
Why not using sizeof(dsthead->rolePermissions) for the size or the named constant "UA_ROLEPERMISSIONS_COUNT" instead of "16" here?
There was a problem hiding this comment.
Good find. Will change that.
| /* If a second server is started later it can "steal" the port. | ||
| * Having port reuse enabled is important for development. | ||
| * Otherwise a long TCP TIME_WAIT is required before the port can be used again. */ | ||
| conf->tcpReuseAddr = true; |
There was a problem hiding this comment.
This change seems to be off-topic here.
There was a problem hiding this comment.
Yes. Will be moved out of this PR.
| #define UA_ROLEINDEX_SECURITYADMIN 7 | ||
|
|
||
| typedef struct { | ||
| UA_UInt16 bits[UA_ROLESET_MAX / 16]; |
There was a problem hiding this comment.
Using "(UA_ROLESET_MAX + 15) / 16" here might relax the current restriction of UA_ROLESET_MAX being a multiple of 16.
| UA_RoleSet delNodeRoles = node->head.rolePermissions[UA_ROLEPERMISSIONINDEX_DELETENODE]; | ||
| if(session != &server->adminSession && | ||
| !UA_RoleSet_intersects(delNodeRoles, session->roles)) { | ||
| UA_LOG_NODEID_INFO(&node->head.nodeId, | ||
| UA_LOG_INFO_SESSION(server->config.logging, session, "DeleteNode (%.*s): " | ||
| "Cannot delete the node because of missing RolePermisions", | ||
| (int)nodeIdStr.length, nodeIdStr.data)); | ||
| UA_NODESTORE_RELEASE(server, node); | ||
| *result = UA_STATUSCODE_BADUSERACCESSDENIED; | ||
| return; | ||
| } |
There was a problem hiding this comment.
If I understand this correctly, this checks, if the session has a role assigned which is required to gain the DELETENODE permission. What is the difference of this check and the one in the AccessControl Plugin (allowDeleteNode)?
There was a problem hiding this comment.
The AccessControl plugin checks go on top of the RolePermissions.
Everything the RolePermissions do can also be done in the pure AccessControl plugin.
But it will be a lot easier to move most checks into the Role-Based Access Control.
Because you don't to build all the machinery to track node access permissions individually.
|
Hi, It seems, that the new permission checks are now hard-wired into several places instead of being encapsulated in the AccessControl plugin. Is this desired? What purpose does the AccessControl plugin have then if the permission checks are not done there? |
| * OPC UA uses role-based access control (RBAC). Every session is assigned a set | ||
| * of roles it represents. Every node in the information contains | ||
| * RolePermissions that define the rights of each role. |
There was a problem hiding this comment.
This does not seem to be correct. When looking into the specification it states:
If not specified, the value of DefaultRolePermissions Property from the NamespaceMetadata Object associated with the Node shall be used instead.
My understanding is, that not every node in the information model contains RolePermissions.
There was a problem hiding this comment.
Every node can have it (optional) in the spec.
We would chose to have the overhead for every node.
| * the node keeps a role-set of all the roles which have this permission. | ||
| * When a RolePermission is changed manually (added or removed) the changes | ||
| * propagate recursively to all child nodes (hierarchical reference). */ | ||
| UA_RoleSet rolePermissions[UA_ROLEPERMISSIONS_COUNT]; |
There was a problem hiding this comment.
This looks like quite some overhead here. Beside that not every node actually has RolePermissions assigned (see comment below) it also poses an upper limit of roles that may be added at runtime.
Wouldn't it be better to keep the RolePermissions outside of the node and accessing them (or only check for access) through an abstraction? That way it would reduce the overhead here and it would also allow to deviate from the standard role mapping which is permitted by the OPC UA specification?
There was a problem hiding this comment.
These are 32byte overhead per node.
16 permission types and 2 byte per type (assuming up to 16 roles).
There was a problem hiding this comment.
Fair enough, the current size of UA_Node is already at around 400 Bytes so this increase is only about 8%.
And also from what I see, Namespace 0 contains around 5000 nodes. So yes, this overhead seems to be negligible.
Nevertheless I want to point two things out here, that might be worth considering:
- I would expect the amount of permutations for these RolePermissions to be strictly smaller than the overall number of nodes.
Why? Because first, it seems to be a maintenance burden to assign different role permissions to each node.
And second, doing this does not make sense at all because of the inter-dependencies between the nodes. - It does not feel right to have this matrix embedded inside the UA_Node structure. ;-)
Having a pointer here for example allows to easily detect that no RolePermissions have been assigned (e.g. to fall back to the namespace defaults). And it would also allow to add roles at runtime without any limitations.
We will have both. |
Well that sounds good. |
No description provided.