search

locka99 / opcua

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

fix server connexions from ignition #183

Closed lovasoa closed 2 years ago

lovasoa commented 2 years ago

Fixes https://github.com/locka99/opcua/issues/182

Reverts a change introduced in 5c20ed8f1d3b708b5c0441528eea792a9607fa07

svanharmelen commented 2 years ago

Reverting the change like this (so in full) will reintroduce the problems that were solved by this change. So I suggest to look for a solution that solves this particular issue, instead of rolling back the whole change...

lovasoa commented 2 years ago

@svanharmelen : Did you look at the pr ? This is not a simple git revert of the previous change. The previous change did not include any comment, but I suppose it introduced an all-zeroes nonce to force connections to clients that require a nonce during secure channel creation. This pr keeps the nonce, but makes it an actual random nonce, as required by the spec.

svanharmelen commented 2 years ago

Looks like I might have mixed things up... I was looking at this part which make me think of this change: https://github.com/locka99/opcua/pull/114/files

But it seems your change is not touching that code actually 👍🏻

lovasoa commented 2 years ago

@locka99 : is there something you want me to do before this can be merged ? We depend on this for our server.

locka99 commented 2 years ago

I'll take it since it uses a zero length array instead of null but we'll to monitor if it makes any servers unhappy. & thanks for your persistence in this.

locka99 commented 2 years ago

I just noticed SecurityPolicy::None is given a 32 byte array. Can you check please if 0 works?

lovasoa commented 2 years ago

I am no expert, but see https://github.com/eclipse/milo/pull/949 :

is it a standard-compliant behavior to send a null nonce ?

It is only compliant when the null nonce is in the OpenSecureChannelResponse and the channels are not using security.

There are other nonce exchanges, in the CreateSessionResponse and ActivateSessionResponse, and in these it is important in regards to the CVE that the nonce be non-empty and non-zero if they are going to be used in the subsequent encryption of credentials for transit on an otherwise unsecure channel.

locka99 commented 2 years ago

There is a underlying difference between a null bytestring and a zero length bytestring. It may be that using 0 satisfies everyone instead of null. I'll have to see if the specs say because this stuff has been a constant bane - some server complains when there is a nonce and others complain when there is a null. I suppose the client could try one way and then the other but it'd be great if it turns out to be a specific rule that is just implemented wrong.

lovasoa commented 2 years ago

@locka99 , would it be possible to publish a new release ? The latest release still cannot be connected to from java clients.

locka99 commented 2 years ago

I'll release a 0.9.1 with your changes cherry-picked into it