[WIP]feat: OPC UA Role Permissions #6149

AsishGanesh1 · 2023-11-29T08:20:00Z

Attach well known roles to session

Add 8 well known role details to session instance
UA_Server_getSessionAttribute API can be made use of for accessing the role data
Remove redundant API's (like hasAccessToNode, hasAccessToMethod)
Remove read user AccessLevel/Executable/RolePermissions related methods
To avoid read outside the context of a session

TODO:

Add CMake flag for usage of role based permission
Add test application
Address remaining review comments
Rebase to current master head

- Register callback while client tries to connect to server using username Change-Id: Ic85121614c84ab70f93f4572b0024679ea30627d

- This commit adds the default roles in address space and handles the add and remove role functionality Change-Id: Icecd4e197d9b1071390b207828a9b7c723483050

- This commit handles different roles based on session Signed-off-by: Keerthivasan.AS<keerthivasan.as@kalycito.com> Change-Id: I2e588fef4019ddb06c8928200158b30362c16cb9

…tone 4 - Adds the AddIdentity and RemoveIdentity method nodes in the address space - Fixes the userExecutable permission rights - Fixes the logger function used instead of printf() - Add IdentityCriteriaMappingType in the Identity nodes created - AddIdentity and RemoveIdentity method calls handling - Fix memory leaks and remove value change in AddIdentity() method Change-Id: Ic6663f724e2a5756bb2ac8b6b154d67c4f11aa3e

- This commit adds the role and user role node along with the updated permission for a single node - This commit fixes the segementation fault for wrong user name and password - This commit adds client code for server validation Change-Id: I27a318782100a94335cec56d6c3ff8fd76b73591

- This commit adds test cases for role based implementation - Fixes the logging mechanism - Added unit test cases Change-Id: I55ff9810115d8c82d0b0b5cf07c9d3149b9fcfd9

-> renamed test_role_permissions.c to check_role_permissions.c -> removed "Experimental" message when enabling A&C. Signed-off-by: Keerthivasan Alagarsamy Senthilkumaran <keerthivasan.as@kalycito.com> Change-Id: Ifb625c11b0734a72de08c2dc35b5b85405bbb6f3

Signed-off-by: Keerthivasan Alagarsamy Senthilkumaran <keerthivasan.as@kalycito.com> Change-Id: I5a9874705be0b6c4bef4ca922c1a643e78fdd742

Signed-off-by: Keerthivasan Alagarsamy Senthilkumaran <keerthivasan.as@kalycito.com> Change-Id: Id0caf384196a5db534736ee1e8376a689da153f0

Signed-off-by: Keerthivasan Alagarsamy Senthilkumaran <keerthivasan.as@kalycito.com> Change-Id: I479445f41e6f81ac5825176fe9e4e77c701a0388

Signed-off-by: Keerthivasan Alagarsamy Senthilkumaran <keerthivasan.as@kalycito.com> Change-Id: Ib75d6ac5e7f99e6fd6e8a4e6068aaa0ab53e4eb6

- Addressed following comments from Julius: 1. Trailing "s" missing for RolePermissions 2. Include all well known roles 3. Use prefered naming scheme 4. Rename AccessControlGroup Signed-off-by: Asish Ganesh <asish@eclatron.com>

- [X] Add 8 well known role details to session instance UA_Server_getSessionAttribute API can be made use of for accessing the role data - [X] Remove redundant API's (like hasAccessToNode, hasAccessToMethod) - [X] Remove read user AccessLevel/Executable/RolePermissions related methods To avoid read outside the context of a session - TODO: - [ ] Add CMake flag for usage of role based permission - [ ] Add test application - [ ] Address remaining review comments Signed-off-by: Asish Ganesh <asish@eclatron.com>

AsishGanesh1 · 2023-11-29T08:36:28Z

This pull request is build on top of #5636.
Track it here for further updates.

jpfr

Nice! Good to see a contribution from your end.
You have to help us a bit getting our head around this part of the spec.
Comments inline.

jpfr · 2023-11-29T12:31:41Z

src/server/ua_session.h

@@ -57,6 +57,9 @@ typedef struct {
    size_t localeIdsSize;
    UA_String *localeIds;

+    /* Role information */
+    UA_NodeId role;


Does a session have only a single role?
I would guess that for fine-grained roles we would like to have several roles per session.

I've incorporated managing multiple roles per session.

Nice.
Since every role gets a power of two 2,4,8,16,32,… we can use a 64Bit integer as bitmap for all the roles. That is more efficient also for comparing inside the nodes..

jpfr · 2023-11-29T12:32:23Z

CMakeLists.txt

@@ -186,7 +186,7 @@ endif()
 set(UA_MULTITHREADING ${MULTITHREADING_DEFAULT} CACHE STRING
    "Multithreading support (<100: No multithreading support, >=100: Thread-safety enabled (internal mutexes)")

-option(UA_ENABLE_SUBSCRIPTIONS_ALARMS_CONDITIONS "Enable the use of A&C. (EXPERIMENTAL)" OFF)


A&C doesn't have much to do with the roles.
Let's look at the status of A&C in a second step.

jpfr · 2023-11-29T12:34:43Z

include/open62541/client_highlevel.h

+                                       size_t newRolePermissionSize,
+                                       const UA_RolePermissionType *rolePermission);
+
+UA_StatusCode UA_EXPORT


My guess is that the UserRolePermissions are auto-computed from the roles of the user and the permissions of these roles.
Similar to AccessRights vs UserAccessRights.
So this is not something that can be written, only read.
Check if writeUserRolePermissionsAttribute makes sense or should be removed.

writeUserRolePermissionsAttribute is removed

jpfr · 2023-11-29T12:35:48Z

include/open62541/plugin/accesscontrol.h

@@ -80,27 +81,36 @@ struct UA_AccessControl {
    /* Allow adding a node */
    UA_Boolean (*allowAddNode)(UA_Server *server, UA_AccessControl *ac,
                               const UA_NodeId *sessionId, void *sessionContext,
-                               const UA_AddNodesItem *item);
+                               const UA_AddNodesItem *item, UA_RolePermissionType *userRolePermission, size_t userRoleSize);


I think we can keep the current API intact.
By adding a lightweight method where we can look up the userRolePermissions from the session id.
So allowNode can read the permissions only when needed.

jpfr · 2023-11-29T12:36:17Z

include/open62541/plugin/accesscontrol.h

-                                  const UA_NodeId *nodeId, void *nodeContext);
+                                  const UA_NodeId *nodeId, void *nodeContext, UA_RolePermissionType *userRolePermission,size_t userRoleSize);
+
+    /* Check access to Node */


Add more documentation.
How is access different from allowRead and allowBrowse?

These API's are removed.

jpfr · 2023-11-29T12:36:55Z

include/open62541/plugin/accesscontrol.h

@@ -127,6 +137,8 @@ struct UA_AccessControl {
                                                      UA_DateTime endTimestamp,
                                                      bool isDeleteModified);
 #endif
+    UA_Boolean (*checkUserDatabase)(const UA_UserNameIdentityToken *userToken, UA_String *roleName);


What does this do?
Validate that the user has a certain role?

checkUserDatabase is not required and is removed.

jpfr · 2023-11-29T12:39:44Z

include/open62541/plugin/nodestore.h

@@ -411,6 +412,11 @@ struct UA_NodeHead {
    UA_MonitoredItem *monitoredItems; /* MonitoredItems for Events and immediate
                                       * DataChanges (no sampling interval). */
 #endif
+
+    size_t rolePermissionsSize;


It seems to be a lot of overhead to manage all role-permissions for each node.
Does the standard define a way how permissions propagate along the hierarchical references?

The best solution would be to inherit standard role-permissions for every new node based on the parents.
And require manual role definitions only as override to the inherited permissions.

jpfr · 2023-11-29T12:40:41Z

include/open62541/plugin/nodestore.h

+
+    size_t rolePermissionsSize;
+    UA_RolePermissionType *rolePermissions;
+    size_t userRolePermissionsSize;


The user-role-permissions are the result of compating the userroles and the role-permissions.
They are different for every user.
So it doesn't make sense to store them in the node for all users at once.

jpfr · 2023-11-29T12:42:26Z

include/open62541/server.h

@@ -711,6 +713,13 @@ UA_Server_readAccessLevel(UA_Server *server, const UA_NodeId nodeId,
                            outAccessLevel);
 }

+static UA_INLINE UA_THREADSAFE UA_StatusCode
+UA_Server_readUserAccessLevel(UA_Server *server, const UA_NodeId nodeId,


_readUserAccessLevel and _readUserExecutable is redundant for a local lookup.
Because the local user always has admin rights.
So he can do everything and userAccessLevel == AccessLevel and also executable == userExecutable.
The methods can be removed.

This method is removed.

jpfr

Very nice.
I am still missing how the access control plugin can access the roles via the API.
That should be possible as well.

i would like to split this into two PR.
The first adds roles to the sessions and exposes them to accesscontrol.
The second one adds roles to the nodes as well to match.

jpfr · 2024-04-10T06:03:11Z

plugins/ua_accesscontrol_default.c

@@ -7,6 +7,7 @@
 */

 #include <open62541/plugin/accesscontrol_default.h>
+#include <server/ua_server_internal.h>


The server_internal.h is not public for the plugins to use…

jpfr · 2024-04-10T06:08:53Z

src/server/ua_session.h

@@ -57,6 +57,9 @@ typedef struct {
    size_t localeIdsSize;
    UA_String *localeIds;

+    /* Role information */
+    UA_NodeId role;


Nice.
Since every role gets a power of two 2,4,8,16,32,… we can use a 64Bit integer as bitmap for all the roles. That is more efficient also for comparing inside the nodes..

jpfr · 2024-04-10T06:11:00Z

src/server/ua_services.h

@@ -50,6 +50,27 @@ typedef void (*UA_Service)(UA_Server*, UA_Session*,
 typedef void (*UA_ChannelService)(UA_Server*, UA_SecureChannel*,
                                  const void *request, void *response);

+typedef enum {


Where are custom roles assigned to a number?

- Remove unused API's & redundant code sections - Each session can have a user logged in who can have multiples roles Signed-off-by: Asish Ganesh <asish@eclatron.com>

opcua-tsn-team-kalycito and others added 13 commits February 7, 2023 11:21

[TASK] Implement check database

11119ea

- Register callback while client tries to connect to server using username Change-Id: Ic85121614c84ab70f93f4572b0024679ea30627d

[TASK] Implement add and remove role functionality

bbed8f4

- This commit adds the default roles in address space and handles the add and remove role functionality Change-Id: Icecd4e197d9b1071390b207828a9b7c723483050

[TASK] Implementation of role permission access for milestone 3

4f5cfbe

- This commit handles different roles based on session Signed-off-by: Keerthivasan.AS<keerthivasan.as@kalycito.com> Change-Id: I2e588fef4019ddb06c8928200158b30362c16cb9

[TASK] Implementation to test Role permission for milestone 6

101e75c

- This commit adds test cases for role based implementation - Fixes the logging mechanism - Added unit test cases Change-Id: I55ff9810115d8c82d0b0b5cf07c9d3149b9fcfd9

[refactor]: Revert the client examples changes

2089667

Signed-off-by: Keerthivasan Alagarsamy Senthilkumaran <keerthivasan.as@kalycito.com> Change-Id: I5a9874705be0b6c4bef4ca922c1a643e78fdd742

[refactor]: Revert the sample application of user database

7b324b9

Signed-off-by: Keerthivasan Alagarsamy Senthilkumaran <keerthivasan.as@kalycito.com> Change-Id: Id0caf384196a5db534736ee1e8376a689da153f0

[refactor]: Remove unwanted files

07ab4eb

Signed-off-by: Keerthivasan Alagarsamy Senthilkumaran <keerthivasan.as@kalycito.com> Change-Id: I479445f41e6f81ac5825176fe9e4e77c701a0388

[refactor]: Enable conditional checks for role permission

1ba6b9d

Signed-off-by: Keerthivasan Alagarsamy Senthilkumaran <keerthivasan.as@kalycito.com> Change-Id: Ib75d6ac5e7f99e6fd6e8a4e6068aaa0ab53e4eb6

Add sample application for role based access control

a4b9c53

- Addressed following comments from Julius: 1. Trailing "s" missing for RolePermissions 2. Include all well known roles 3. Use prefered naming scheme 4. Rename AccessControlGroup Signed-off-by: Asish Ganesh <asish@eclatron.com>

jpfr reviewed Nov 29, 2023

View reviewed changes

jpfr reviewed Apr 10, 2024

View reviewed changes

Handle several roles per session

a276017

- Remove unused API's & redundant code sections - Each session can have a user logged in who can have multiples roles Signed-off-by: Asish Ganesh <asish@eclatron.com>

AsishGanesh1 force-pushed the open62541_Role_Permissions branch from af52a89 to a276017 Compare April 10, 2024 06:16

jpfr mentioned this pull request Jun 3, 2024

Ongoing Work, Feature Wishlist and Prioritization #5075

Open

33 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]feat: OPC UA Role Permissions #6149

[WIP]feat: OPC UA Role Permissions #6149

AsishGanesh1 commented Nov 29, 2023 •

edited

AsishGanesh1 commented Nov 29, 2023

jpfr left a comment

jpfr Nov 29, 2023

AsishGanesh1 Apr 10, 2024

jpfr Apr 10, 2024

jpfr Nov 29, 2023

jpfr Nov 29, 2023

AsishGanesh1 Apr 10, 2024

jpfr Nov 29, 2023

jpfr Nov 29, 2023

AsishGanesh1 Apr 10, 2024

jpfr Nov 29, 2023

AsishGanesh1 Apr 10, 2024

jpfr Nov 29, 2023

jpfr Nov 29, 2023

jpfr Nov 29, 2023

AsishGanesh1 Apr 10, 2024

jpfr left a comment

jpfr Apr 10, 2024

jpfr Apr 10, 2024

jpfr Apr 10, 2024

[WIP]feat: OPC UA Role Permissions #6149

Are you sure you want to change the base?

[WIP]feat: OPC UA Role Permissions #6149

Conversation

AsishGanesh1 commented Nov 29, 2023 • edited

AsishGanesh1 commented Nov 29, 2023

jpfr left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpfr left a 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

AsishGanesh1 commented Nov 29, 2023 •

edited