locka99 / opcua

A client and server implementation of the OPC UA specification written in Rust
Mozilla Public License 2.0
476 stars 129 forks source link

requestedMaxReferencesPerNode being hard-coded to 1000 may make the server abort the connection #223

Open laumann opened 1 year ago

laumann commented 1 year ago

I've been running into an issue with using the browse functionality, and managed to reduce it to a reasonable test case.

Steps to reproduce

Spin up the OPC-UA PLC simulator:

docker run --rm -d --net host --name opcplccomplex mcr.microsoft.com/iotedge/opc-plc:latest --pn=50000 --autoaccept --sph --sn=1000 --ph=localhost --ctb --ut

(link)

Clone the sample repository and run:

$ git clone https://github.com/omnioiot/opcua-chunked-browse  # Bad repo name, but I thought it was about chunking to 
$ cd opcua-chunked-browse
$ RUST_OPCUA_LOG=debug cargo run

Observe the error:

2022-07-19 11:39:01.870 - INFO - opcua::client::comms::tcp_transport - Discarding chunk marked in as final error

From which I conclude that the server has decided to abort the connection, I don't think the client did anything incorrect here.

Capturing with wireshark:

Screenshot from 2022-07-19 13-30-24

I adjusted the hard-coded 1000 in ViewService::browse() down to 500 which allowed the browse to complete successfully.

https://github.com/locka99/opcua/blob/2858154ed5ebe894ba0e86fb51bc546b116fdb87/lib/src/client/session/session.rs#L2123-L2132

I don't really know how to address this though: If the server decides to abort the connection, there's not much the client can do. The way I read the specification Part 4, 5.8.2.2 also suggests that "requestedMaxReferencesPerNode" is just a hint, not a requirement.

For now, I've hacked ViewService::browse() to accept a requested_max_references_per_node to make it possible for the client to control this parameter, and we might have to leave it at that. See https://github.com/omnioiot/opcua/commit/6dbe8785d3756a7af58c234e79e16525ab74eef6

laumann commented 1 year ago

I forgot to mention that I also tried setting it to zero, but that also made the server abort the connection.

laumann commented 1 year ago

And just to clarify: I completely understand if this is a kind of "not our problem" issue :-) There will always be bad server implementations, I just wanted to raise some awareness now that I managed to reduce it to something I could share with you.

milgner commented 1 year ago

there's a patch here - I'll try to find some time to create a MR for it.

milgner commented 1 year ago

And just to clarify: I completely understand if this is a kind of "not our problem" issue :-) There will always be bad server implementations, I just wanted to raise some awareness now that I managed to reduce it to something I could share with you.

Note that this is not a bad server implementation, only a side effect of OPC UA being mind-boggingly complex. A full-fledged and semantics-aware OPC UA client is supposed to query the servers OperationLimits after connecting to it (hoping that it's there and has some meaningful values filled in) and then use the MaxNodesPerBrowse and MaxNodesPerRead attributes it finds there for its calls to the Browse and Read services.

Edit: I should probably mention that I have yet to see such a semantics-aware OPCUA client library. So far I haven't come across any in any programming language. Even using fully-qualified node ids (with nsu instead of ns as their namespace qualifier) seems to be very very rare, even though the namespace index could possibly change at any time.

laumann commented 1 year ago

there's a patch here - I'll try to find some time to create a MR for it.

That's basically verbatim the same patch I ended up writing :-) Only I didn't bother making it optional, I figured that one could provide a pub const DEFAULT_REQUESTED_MAX_REFERENCES_PER_NODE: u32 = 1000 and let users use that. But maybe Option<u32> is more ergonomic here.

Note that this is not a bad server implementation, only a side effect of OPC UA being mind-boggingly complex. A full-fledged and semantics-aware OPC UA client is supposed to query the servers OperationLimits after connecting to it (hoping that it's there and has some meaningful values filled in) and then use the MaxNodesPerBrowse and MaxNodesPerRead attributes it finds there for its calls to the Browse and Read services.

TIL thanks for that! Now I wonder if I could get the client to do this...

milgner commented 1 year ago

Note that this is not a bad server implementation, only a side effect of OPC UA being mind-boggingly complex. A full-fledged and semantics-aware OPC UA client is supposed to query the servers OperationLimits after connecting to it (hoping that it's there and has some meaningful values filled in) and then use the MaxNodesPerBrowse and MaxNodesPerRead attributes it finds there for its calls to the Browse and Read services.

TIL thanks for that! Now I wonder if I could get the client to do this...

Unfortunately it's very hard to do in actuality: I've seen servers that don't provide OperationLimits as well as those that do have it but report either 0 or 65536 for these attribute values. And if one then tries to work with 65536 as the limit and has too many values - usually in the context of having using long, string-based node identifiers - it can very well violate the maximum message size, leaving client or server unable to encode the request or response. (Browse responses can work around it by using continuation points but there are edge cases in other scenarios that will break)

In my experience there's a huge gap between OPC UA's claim to being a protocol which can do everything in a completely dynamic manner and the fact that one has to carefully fine-tune how a client works with each individual server. :grimacing:

MightyPork commented 3 weeks ago

This broke communication with a PhoenixContact PLCnext and caused quite some head scratching for us.

I now forked the library and changed requested_max_references_per_node to 100, which fixed the issue. This parameter must be configurable if it can't be auto-set (from the server configuration?)

version is 0.9.1