united-manufacturing-hub / benthos-umh

Apache License 2.0
28 stars 7 forks source link

Add security mode and policy support #12

Closed devantler closed 10 months ago

devantler commented 10 months ago

This pull request adds support for specifying security mode and policy in the OPC-UA client options, and provides instructions for configuring benthos-umh in standalone mode with Docker. It also includes updates to the README.md file to reflect these changes.


I am not sure how you want the documentation and tests, so any guidance/help here is welcome :-)

CLAassistant commented 10 months ago

CLA assistant check
All committers have signed the CLA.

JeremyTheocharis commented 10 months ago

Firstly, thank you so much for your contribution to the project! We truly appreciate the effort you've put into this pull request. Your initiative in adding support for specifying security mode and policy in the OPC-UA client options is a significant step forward for the benthos-umh project.

Regarding your implementation, I wanted to share some insights based on our vision for benthos-umh. Our goal is to automate as much as possible to simplify the user experience. This stems from our observation that most users and companies find it challenging to correctly configure OPC-UA servers. Often, a single misconfigured security setting can undermine the entire security infrastructure. For example, without pre-validated server certificates or a trusted CA, the encryption becomes vulnerable to man-in-the-middle attacks.

Because we have never seen this actually being implemented, encryption in OPC-UA is not effective at all and just adds unnecessary configuration options. We therefore implemented the getReasonableEndpoint function, that just selects something for the user, that will definitely work. This aligns closely with the functionality you're introducing.

I suggest integrating your security mode and policy settings into the getReasonableEndpoint logic. This integration could involve adding parameters like overwriteSecurityPolicy and overwriteSecurityMode to enforce specific security configurations. For example: we just saw it that one OPC-UA server SSL was working faulty, so we introduced the insecure parameter, to enforce using None. You could adjust it to use overwriteSecurityPolicy and overwriteSecurityMode instead :)

For testing, try to test it on a OPC-UA server. If you are sure that it works, feel free to write us and we'll add it to our automated testing pipeline and test it against a WAGO PLC.

devantler commented 10 months ago

Firstly, thank you so much for your contribution to the project! We truly appreciate the effort you've put into this pull request. Your initiative in adding support for specifying security mode and policy in the OPC-UA client options is a significant step forward for the benthos-umh project.

Regarding your implementation, I wanted to share some insights based on our vision for benthos-umh. Our goal is to automate as much as possible to simplify the user experience. This stems from our observation that most users and companies find it challenging to correctly configure OPC-UA servers. Often, a single misconfigured security setting can undermine the entire security infrastructure. For example, without pre-validated server certificates or a trusted CA, the encryption becomes vulnerable to man-in-the-middle attacks.

Because we have never seen this actually being implemented, encryption in OPC-UA is not effective at all and just adds unnecessary configuration options. We therefore implemented the getReasonableEndpoint function, that just selects something for the user, that will definitely work. This aligns closely with the functionality you're introducing.

I suggest integrating your security mode and policy settings into the getReasonableEndpoint logic. This integration could involve adding parameters like overwriteSecurityPolicy and overwriteSecurityMode to enforce specific security configurations. For example: we just saw it that one OPC-UA server SSL was working faulty, so we introduced the insecure parameter, to enforce using None. You could adjust it to use overwriteSecurityPolicy and overwriteSecurityMode instead :)

For testing, try to test it on a OPC-UA server. If you are sure that it works, feel free to write us and we'll add it to our automated testing pipeline and test it against a WAGO PLC.

Thank you for the kind words! Bridging OT with IT is super important for IoT 4.0, and I feel relying on popular tools like Benthos or Kafka connectors is an important route to success. So being able to contribute to projects that share a similar goal is of great interest to me :-)

Your suggestions make sense to me, and I will gladly try implementing them. Moving the code such that it becomes a part of the getReasonableEndpoint logic should be fairly straightforward with your suggested approach :-)

I will hopefully find time to implement the changes this coming week :-)

devantler commented 10 months ago

@JeremyTheocharis I have implemented the requested changes along with a test and updated docs.

The changes to the README.md use the updated structure I have provided here #15. If you like this new structure, this PR should be merged after. This will minimize the number of merge conflicts after a rebase.

I am unfamiliar with the OPC UA test setup you run against, so if you are able to verify that the test works as expected on your end, that would be very helpful :-) I will try and test it locally with our OPC UA setup and update this comment if necessary.

JeremyTheocharis commented 10 months ago

I added some changes as I think an overwrite function did not make sense (sorry for proposing it). But I tested it now against our wago PLC and it works

devantler commented 10 months ago

Awesome! Happy that it worked 😃