-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[WIP]feat: OPC UA Role Permissions #6149
base: master
Are you sure you want to change the base?
[WIP]feat: OPC UA Role Permissions #6149
Conversation
- 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<[email protected]> 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 <[email protected]> Change-Id: Ifb625c11b0734a72de08c2dc35b5b85405bbb6f3
Signed-off-by: Keerthivasan Alagarsamy Senthilkumaran <[email protected]> Change-Id: I5a9874705be0b6c4bef4ca922c1a643e78fdd742
Signed-off-by: Keerthivasan Alagarsamy Senthilkumaran <[email protected]> Change-Id: Id0caf384196a5db534736ee1e8376a689da153f0
Signed-off-by: Keerthivasan Alagarsamy Senthilkumaran <[email protected]> Change-Id: I479445f41e6f81ac5825176fe9e4e77c701a0388
Signed-off-by: Keerthivasan Alagarsamy Senthilkumaran <[email protected]> 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 <[email protected]>
- [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 <[email protected]>
This pull request is build on top of #5636. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -57,6 +57,9 @@ typedef struct { | |||
size_t localeIdsSize; | |||
UA_String *localeIds; | |||
|
|||
/* Role information */ | |||
UA_NodeId role; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've incorporated managing multiple roles per session.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A&C doesn't have much to do with the roles.
Let's look at the status of A&C in a second step.
size_t newRolePermissionSize, | ||
const UA_RolePermissionType *rolePermission); | ||
|
||
UA_StatusCode UA_EXPORT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
writeUserRolePermissionsAttribute
is removed
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
const UA_NodeId *nodeId, void *nodeContext); | ||
const UA_NodeId *nodeId, void *nodeContext, UA_RolePermissionType *userRolePermission,size_t userRoleSize); | ||
|
||
/* Check access to Node */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add more documentation.
How is access
different from allowRead and allowBrowse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These API's are removed.
@@ -127,6 +137,8 @@ struct UA_AccessControl { | |||
UA_DateTime endTimestamp, | |||
bool isDeleteModified); | |||
#endif | |||
UA_Boolean (*checkUserDatabase)(const UA_UserNameIdentityToken *userToken, UA_String *roleName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
Validate that the user has a certain role?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checkUserDatabase
is not required and is removed.
@@ -411,6 +412,11 @@ struct UA_NodeHead { | |||
UA_MonitoredItem *monitoredItems; /* MonitoredItems for Events and immediate | |||
* DataChanges (no sampling interval). */ | |||
#endif | |||
|
|||
size_t rolePermissionsSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
size_t rolePermissionsSize; | ||
UA_RolePermissionType *rolePermissions; | ||
size_t userRolePermissionsSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
include/open62541/server.h
Outdated
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -7,6 +7,7 @@ | |||
*/ | |||
|
|||
#include <open62541/plugin/accesscontrol_default.h> | |||
#include <server/ua_server_internal.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The server_internal.h is not public for the plugins to use…
@@ -57,6 +57,9 @@ typedef struct { | |||
size_t localeIdsSize; | |||
UA_String *localeIds; | |||
|
|||
/* Role information */ | |||
UA_NodeId role; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
af52a89
to
a276017
Compare
Attach well known roles to session
UA_Server_getSessionAttribute API can be made use of for accessing the role data
To avoid read outside the context of a session
TODO:
[email protected]