Skip to content
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

Fix SimpleRoleRuleset #1718

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cziebuhr
Copy link
Contributor

@cziebuhr cziebuhr commented Oct 7, 2024

I had issues using a Siemens S7 PLC as OPC UA client reading values out of an asyncua server. It turned out that it used RegisterNodes, which is currently only allowed with write permission.

According to https://reference.opcfoundation.org/Core/Part4/v105/docs/5.8.5, the RegisterNodes Service can be used by Clients to register the Nodes that they know they will access repeatedly (e.g. Write, Call).

The service itself neither reads or writes data, it's just an optional optimization which is implemented as noop by asyncua.

@cziebuhr cziebuhr changed the title RegisterNodes doesn't need write permission Fix SimpleRoleRuleset Oct 11, 2024
@cziebuhr
Copy link
Contributor Author

I found another related issue:
When using SecurityPolicy None, SimpleRoleRuleset has not been used, allowing all requests even for anonymous users. Unfortunately, SecurityPolicy None (class SecurityPolicy) is implemented in ua/uaprotocol_hand.py, all other polices are implemented in crypto/security_policies.py. I now moved the usage of SimpleRoleRuleset to server/server.py, to only have it in one place. Also adding it to ua/uaprotocol_hand.py would have led to a circular import. SecurityPolicy() is also used both in server and client code, and shouldn't pull in server code via UserRole if used from client code. Personally, I would have also moved classes CryptographyNone, SecurityPolicy, and SecurityPolicyFactory to crypto/security_policies.py (not addressed in this PR), because they are internal classes and not defined by OPC UA spec.

As a consequence, I also allowed writing values for regular users:

  • It was possible before SimpleRoleRuleset was implemented
  • It was possible when using SecurityPolicy None
  • It is still not possible for regular users if writable flag is not set
  • It would otherwise be only possible for admins, for which writable flag isn't needed anyway
  • Tests have been failing after the fix above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant