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

Add security mode and policy support #12

Merged
merged 10 commits into from
Nov 14, 2023
Merged

Add security mode and policy support #12

merged 10 commits into from
Nov 14, 2023

Conversation

devantler
Copy link
Contributor

@devantler devantler commented Nov 10, 2023

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
Copy link

CLAassistant commented Nov 10, 2023

CLA assistant check
All committers have signed the CLA.

@JeremyTheocharis
Copy link
Member

JeremyTheocharis commented Nov 12, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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 JeremyTheocharis self-requested a review November 14, 2023 10:28
@JeremyTheocharis
Copy link
Member

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

@JeremyTheocharis JeremyTheocharis merged commit cfcc1f9 into united-manufacturing-hub:master Nov 14, 2023
@devantler
Copy link
Contributor Author

Awesome! Happy that it worked 😃

JeremyTheocharis pushed a commit that referenced this pull request Nov 16, 2023
* Update global workflows (#3)

* Update global workflows (#7)

* Update global workflows (#8)

* Update global workflows (#10)

* Update global workflows (#9)

* Update global workflows (#12)

* Update global workflows (#11)

* Update global workflows (#13)

* Update global workflows (#14)

* Fix error handling and data conversion in
OPCUAInput ReadBatch function

* Delete .github/PULL_REQUEST_TEMPLATE.md

* Delete .github/dependabot.yaml

* Delete .github/CODEOWNERS

* Delete .github/release-drafter.yml

* Delete .github/renovate.json

* Delete .github/workflows/auto-merge.yaml

* Delete .github/workflows/fork-sync.yaml

* Delete .github/workflows/github-readme-tree.yaml

* Delete .github/workflows/todos-to-issues.yaml

* Delete .github/workflows/sync-labels.yaml

* Delete .github/workflows/require-labels.yaml

* Delete .github/workflows/release-drafter.yaml

* Final fix

* Fix nil value handling in OPCUAInput ReadBatchPull
method

* Fix error message format in OPCUA plugin
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.

3 participants