ENG-2389: add trusted server option via fingerprint #118
Conversation
WalkthroughThe pull request enhances the OPC UA connection security by implementing a server certificate fingerprint validation mechanism. Key changes include the addition of environment variable assignments for device fingerprints in the workflow file, the introduction of a Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
opcua_plugin/connect.go (2)
6-6: Consider adopting a stronger hashing algorithm for fingerprint validation.
Although SHA-1 is frequently used for certificate fingerprints, it is widely known to be more vulnerable than newer hashing algorithms. If the server allows stronger options (e.g. SHA-256), consider supporting them to enhance security.
188-196: Doc comment can be shortened or adapted to standard Go style.
Currently, the doc string is lengthy. While thorough, consider reformatting to match Go conventions by starting the comment with the function name and summarizing briefly, e.g.:
“// filterEndpointsByFingerprint filters the endpoints by certificate fingerprint, ensuring that only trusted endpoints are included.”opcua_plugin/opcua_plc_test.go (1)
75-75: Possibility for a global test configuration approach.
Loadingvar fingerprint stringfrom environment variables is fine; consider wrapping all test configuration (endpoints, credentials, fingerprints) into a single struct or helper for tidier testing if these tests expand further.README.md (1)
191-203: Consider enhancing the fingerprint documentation.While the documentation provides a good overview of the fingerprint feature, it could be improved by:
- Explaining the expected format of the SHA1 hash (length, character set).
- Adding instructions on how to obtain the fingerprint from a server's certificate.
- Including security considerations and best practices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/opcua-plc.yml(4 hunks)README.md(2 hunks)opcua_plugin/connect.go(2 hunks)opcua_plugin/opcua.go(4 hunks)opcua_plugin/opcua_plc_test.go(4 hunks)
🔇 Additional comments (17)
opcua_plugin/connect.go (5)
9-9: New hex encoding import looks good.
No issues with importingencoding/hex; it is standard and suitable for this use case.
177-186: Error handling of fingerprint filtering is appropriate.
Filtering endpoints after adjusting hosts is clear and promptly reports errors if filtering fails. This flow cleanly separates network-layer concerns (host replacement) from certificate verification (fingerprint matching).
197-203: Silent bypass if fingerprint is empty.
When no fingerprint is provided, all endpoints are effectively trusted. Verify that this behavior aligns with your security requirements. In some scenarios, it might be beneficial to log a warning if the user expected an encrypted/trusted connection but did not provide a fingerprint.
204-247: Implementation correctly skips endpoints with invalid certificates or mismatched fingerprints.
This is sensible defensive programming. Each step logs a clear warning before continuing, preventing the entire operation from failing prematurely.
248-249: Helpful error message for missing matching endpoints.
Returning an explicit error when no endpoints match the fingerprint is user-friendly and clearly indicates the issue.opcua_plugin/opcua_plc_test.go (5)
158-178: Good validation of a successful connection with a valid fingerprint.
Ensuring the test fails if the fingerprint is absent or incorrect helps confirm that the feature works as intended. The skipping logic is also sensible for controlled environments.
180-192: Appropriate negative test case.
Verifying certificate mismatch properly ensures that an incorrect fingerprint triggers a connection failure, bolstering confidence in security checks.
227-227: Same pattern of environment variable usage for WAGO tests.
Maintaining consistency in environment-based configuration across tests is good practice.
291-311: Robust test for WAGO PLC with a valid fingerprint.
Similar to the Siemens S7 approach, verifying the correctness of the fingerprint is consistent and ensures coverage for multiple PLC types.
313-324: Well-defined failure scenario for WAGO PLC with invalid fingerprint.
Keeps the test logic consistent and ensures coverage for negative paths.opcua_plugin/opcua.go (4)
49-50: Adding fingerprint to the config spec is clear and well-labeled.
Users can readily understand the role of the field, and setting defaults helps them get started.
141-145: Reading the fingerprint from config.
The error handling here is consistent with the rest of the fields. No immediate issues noted.
161-161: Assigning fingerprint to struct is straightforward.
No concerns with performance or maintainability.
202-202: New struct field for fingerprint.
Properly placed and consistent with the existing fields, keeping the input structure cohesive..github/workflows/opcua-plc.yml (2)
59-59: LGTM: S7 device fingerprint configuration.The fingerprint environment variables for S7 devices (both main and fallback) are correctly set from GitHub secrets and only when the respective device is available.
Also applies to: 75-75
90-90: LGTM: Wago device fingerprint configuration.The fingerprint environment variables for Wago devices (both main and fallback) are correctly set from GitHub secrets and only when the respective device is available.
Also applies to: 106-106
README.md (1)
128-128: LGTM: Configuration option documentation.The new
fingerprintoption is correctly documented in the configuration example with appropriate formatting and indication that it's optional.
opcua_plugin/connect.go
Outdated
| // return an error if there are no endpoints with matching fingerprints - to see failure | ||
| } | ||
| if len(filteredEP) == 0 { | ||
| return nil, fmt.Errorf("no endpoints with matching certificate fingerprint found: '%s' ", g.Fingerprint) |
There was a problem hiding this comment.
a good error message has the following components:
- What did happen (no endpoints with matching certificate fingerprint found. expected A, got B)
- Why does this need to happen and why is it important (because the user configured a fingerprint to ensure that nobody intercepts the communication between client and server. This means either the server certificate or the server was changed, e.g., because the server was exchanged or the certificate was re-created. If you didn't do any of this, it could mean that someone else is trying to intercept the communication. )
- What does the user need to do (if you know that the server has changed its certificate, apply the new certificate fungerprint by changing the line XXX to YYY. if not, please double check and contact your IT security)
There was a problem hiding this comment.
g.Log.Warnf(
"Endpoint %s certificate fingerprint mismatch. Expected: '%s', got: '%s'. "+
"Either the server’s certificate was intentionally updated, or you are connecting to the wrong server. "+
"If intentional, please set the new 'serverCertificateFingerprint' in your configuration. "+
"Otherwise, double-check your security settings.",
ep.EndpointURL, g.ServerCertificateFingerprint, epFingerprint,
)
| return adjustedEndpoints, nil | ||
| return filteredEndpoints, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
during initial connect (without fingerprint ser):
1. When the serverCertificateFingerprint is Empty
Step 1: Log an Info Message
(Initial approach, does not block deployment)
// Info-level log when user did not specify `serverCertificateFingerprint`
g.Log.Infof(
"[OPC UA Plugin] No 'serverCertificateFingerprint' was provided. " +
"We strongly recommend specifying 'serverCertificateFingerprint=xxxx' to verify the server's identity " +
"and avoid potential security risks. Future releases may escalate this to a warning that prevents deployment."
)There was a problem hiding this comment.
// Warning-level log in future release
g.Log.Warnf(
"[OPC UA Plugin] Missing 'serverCertificateFingerprint'. " +
"Connections without a trusted certificate fingerprint can be insecure. " +
"Please set 'serverCertificateFingerprint=xxxx' to ensure a trusted connection. " +
"Continuing without a fingerprint may be blocked in future versions."
)
There was a problem hiding this comment.
// Warning-level log in future release
g.Log.Warnf(
"[OPC UA Plugin] Missing 'serverCertificateFingerprint'. " +
"Connections without a trusted certificate fingerprint can be insecure. " +
"Please set 'serverCertificateFingerprint=xxxx' to ensure a trusted connection. " +
"Continuing without a fingerprint may be blocked in future versions."
)
There was a problem hiding this comment.
// Warning-level log in future release
g.Log.Warnf(
"[OPC UA Plugin] Missing 'serverCertificateFingerprint'. " +
"Connections without a trusted certificate fingerprint can be insecure. " +
"Please set 'serverCertificateFingerprint=xxxx' to ensure a trusted connection. " +
"Continuing without a fingerprint may be blocked in future versions."
)
There was a problem hiding this comment.
So i refactored this a bit, since i looked up the opcua.GetEndpoints and it didn't make sense before to check on each endpoint and filter the endpoints. So what we do now is:
- check if fingerprint is set
- if fingerprint isn't set -> provide some info message that this is potentially unsafe and may lead to a failure of the deployment in future releases
- if fingerprint is set -> check from the first endpoint (since the first one should always be available, or we will fail anyways)
- if fingerprint matches continue with connecting attempt
- if fingerprint doesn't match provide some Information regarding the mismatch (including the server's fingerprint -> so the user could copy/paste the fingerprint (sha3)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
opcua_plugin/connect.go (1)
259-261: Enhance error message with actual vs expected fingerprint.The error message when no matching endpoints are found could be more helpful by including both the expected and actual fingerprints.
- return nil, fmt.Errorf("No endpoints with matching certificate fingerprint '%s' found. ", g.ServerCertificateFingerprint) + return nil, fmt.Errorf("No endpoints with matching certificate fingerprint found. Expected: '%s', but none of the endpoints matched. This could indicate a server certificate change or a potential security issue.", g.ServerCertificateFingerprint)README.md (1)
201-201: Add comma after 'Otherwise'.Minor grammatical improvement needed.
-> - Future releases may escalate this to a **warning** that blocks deployment in certain environments. +> - Future releases may escalate this to a **warning** that blocks deployment in certain environments. -> - If your server's certificate changes (e.g. renewal, new server) update the `serverCertificateFingerprint` accordingly. Otherwise the connection will be rejected, signaling a potential security issue or misconfiguration. +> - If your server's certificate changes (e.g. renewal, new server) update the `serverCertificateFingerprint` accordingly. Otherwise, the connection will be rejected, signaling a potential security issue or misconfiguration.🧰 Tools
🪛 LanguageTool
[uncategorized] ~201-~201: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...verCertificateFingerprint` accordingly. Otherwise the connection will be rejected, signal...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
README.md(2 hunks)go.mod(2 hunks)opcua_plugin/connect.go(2 hunks)opcua_plugin/opcua.go(4 hunks)opcua_plugin/opcua_plc_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- opcua_plugin/opcua.go
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~201-~201: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...verCertificateFingerprint` accordingly. Otherwise the connection will be rejected, signal...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: go-test-opcua-plc
- GitHub Check: go-test-nodered-js
- GitHub Check: build-docker (arm/v7)
- GitHub Check: build-docker (arm64)
🔇 Additional comments (12)
opcua_plugin/connect.go (5)
189-194: LGTM! Well-documented function header.The function header clearly explains the purpose and importance of fingerprint validation for server trust.
198-207: LGTM! Good handling of unset fingerprint case.The implementation correctly handles the case when no fingerprint is provided, with appropriate logging and future-proofing through the info message.
211-237: LGTM! Robust certificate parsing and validation.The implementation properly handles certificate validation with appropriate error checks and logging:
- Skips endpoints without certificates
- Properly decodes PEM format
- Validates certificate parsing
239-253: LGTM! Secure fingerprint calculation and user-friendly error messages.The implementation:
- Uses SHA3-512 for secure fingerprint calculation
- Provides clear error messages with actionable guidance
178-186: LGTM! Well-integrated fingerprint filtering.The changes to FetchAllEndpoints properly integrate the new fingerprint filtering functionality while maintaining error handling.
opcua_plugin/opcua_plc_test.go (3)
158-176: LGTM! Good test coverage for successful fingerprint validation.The test properly verifies that connection succeeds with a matching fingerprint.
178-188: LGTM! Good negative test case.The test properly verifies that connection fails with an incorrect fingerprint.
288-319: LGTM! Duplicate test coverage for WAGO PLC.The tests are appropriately duplicated for the WAGO PLC to ensure consistent behavior across different PLC types.
README.md (2)
191-202: LGTM! Clear and comprehensive documentation.The documentation effectively explains:
- The purpose of the fingerprint validation
- Current behavior and future changes
- Impact of certificate changes
🧰 Tools
🪛 LanguageTool
[uncategorized] ~201-~201: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...verCertificateFingerprint` accordingly. Otherwise the connection will be rejected, signal...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
203-209: LGTM! Good configuration example.The YAML example clearly shows how to configure the fingerprint option.
go.mod (2)
Line range hint
8-15: Well-documented x509 certificate handling configuration.The godebug directive is properly documented with a clear explanation of its purpose for handling certificates with negative serial numbers, which is crucial for backwards compatibility with certain OPC UA servers.
317-317: Security-related dependency updates look good.The coordinated updates to golang.org/x/crypto, golang.org/x/term, and golang.org/x/sys are appropriate for implementing the server certificate fingerprint validation feature.
Let's verify these versions are the latest stable releases:
Also applies to: 373-373, 377-377
✅ Verification successful
All security-related dependencies are at their latest stable versions
- golang.org/x/crypto v0.32.0
- golang.org/x/term v0.28.0
- golang.org/x/sys v0.29.0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the specified versions are the latest stable releases # Skip cloning as we're only checking external package versions curl -s "https://proxy.golang.org/golang.org/x/crypto/@v/list" | sort -V | tail -n 1 curl -s "https://proxy.golang.org/golang.org/x/term/@v/list" | sort -V | tail -n 1 curl -s "https://proxy.golang.org/golang.org/x/sys/@v/list" | sort -V | tail -n 1Length of output: 273
Script:
#!/bin/bash # Check the actual versions in go.mod grep -E "golang.org/x/(crypto|sys|term)" go.modLength of output: 155
add test add test to s7&wago + workflow env workflow/env
refactored checkServerCertificateFingerprint comment
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
201-201: Consider adding a comma after 'Otherwise'.For better readability, consider adding a comma after 'Otherwise' in the sentence.
-> If your server's certificate changes (e.g. renewal, new server) update the `serverCertificateFingerprint` accordingly. Otherwise, the connection will be rejected, signaling a potential security issue or misconfiguration.🧰 Tools
🪛 LanguageTool
[uncategorized] ~201-~201: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...verCertificateFingerprint` accordingly. Otherwise the connection will be rejected, signal...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
.github/workflows/opcua-plc.yml(4 hunks)README.md(2 hunks)go.mod(2 hunks)opcua_plugin/connect.go(3 hunks)opcua_plugin/opcua.go(4 hunks)opcua_plugin/opcua_plc_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- opcua_plugin/opcua_plc_test.go
- go.mod
- .github/workflows/opcua-plc.yml
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~201-~201: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...verCertificateFingerprint` accordingly. Otherwise the connection will be rejected, signal...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: go-test-opcua-plc
- GitHub Check: go-test-opcua-plc
🔇 Additional comments (9)
opcua_plugin/opcua.go (3)
49-50: LGTM! Configuration field added correctly.The new
serverCertificateFingerprintfield is properly integrated into the configuration specification with a clear description and default value.
141-144: LGTM! Field retrieval implemented correctly.The field value is properly retrieved from the configuration and error handling is consistent with other fields.
161-161: LGTM! Struct field added with clear comment.The
ServerCertificateFingerprintfield is properly added to theOPCUAInputstruct with a descriptive comment.opcua_plugin/connect.go (4)
182-191: LGTM! Well-documented function with clear purpose.The function header clearly explains the purpose and importance of fingerprint verification for security.
193-201: LGTM! Proper certificate validation.The code correctly handles the case where a server certificate is missing, with a clear error message explaining the security implications.
221-225: LGTM! Secure fingerprint calculation.The implementation correctly uses SHA3-512 for fingerprint calculation, which is a cryptographically secure hash function.
566-586: LGTM! Well-integrated into connection flow.The fingerprint verification is properly integrated into the connection process with:
- Clear logging of security implications when fingerprint is not provided
- Proper error handling when fingerprint verification fails
README.md (2)
191-202: LGTM! Clear and comprehensive documentation.The documentation effectively:
- Explains the purpose and security benefits of the fingerprint verification
- Warns about future changes to the security policy
- Provides guidance for certificate changes
🧰 Tools
🪛 LanguageTool
[uncategorized] ~201-~201: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...verCertificateFingerprint` accordingly. Otherwise the connection will be rejected, signal...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
203-209: LGTM! Clear configuration example.The YAML configuration example is clear and matches the implementation.
opcua_plugin/connect.go
Outdated
| // may be escalated to Warning-Level in future releases | ||
| g.Log.Infof( | ||
| "No 'serverCertificateFingerprint' was provided. " + | ||
| "We strongly recommend specifying 'serverCertificateFingerprint=xxxx' to verify the server's identity " + |
There was a problem hiding this comment.
this here should then print the actual serverCertificateFingerprint, to make it easier for the user to copy it
| func (g *OPCUAInput) checkServerCertificateFingerprint(endpoint *ua.EndpointDescription) error { | ||
|
|
||
| // If the endpoint doesn't provide a server certificate, we return here | ||
| // This is not expected to happen, since an OPC-UA server should always |
There was a problem hiding this comment.
it should only have a certificate when using encrypted connection. probably makes sense to include a check here, that once a fingerprint is selected, we only accept encrypted connections
There was a problem hiding this comment.
with this we should then be able to prevent a downgrade attack, that accidently benthos selects an unencrypted certificate even though the fingerprint is set
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
opcua_plugin/connect.go (2)
182-222: Consider enforcing fingerprint verification for encrypted connections.The current implementation only warns when no fingerprint is provided. Since this is a security feature, consider making it mandatory for encrypted connections in future releases.
func (g *OPCUAInput) checkServerCertificateFingerprint(endpoint *ua.EndpointDescription) error { + // Only proceed with fingerprint checks for encrypted connections + if !isNoSecurityEndpoint(endpoint) { endpointFingerprint, err := g.getServerCertificateFingerprint(endpoint) if err != nil { return err } switch { case g.ServerCertificateFingerprint != "": // Return error with information about the mismatch of the endpoints // certificate fingerprint. if endpointFingerprint != g.ServerCertificateFingerprint { return fmt.Errorf("fingerprint mismatch for endpoint %s\n"+ "Expected: '%s'\n"+ "Got: '%s'\n\n"+ "Either the server's certificate was intentionally updated, or you are connecting to the wrong server.\n"+ "If intentional, please set the new 'serverCertificateFingerprint' in your configuration.\n"+ "Otherwise, double-check your security settings.", endpoint.EndpointURL, g.ServerCertificateFingerprint, endpointFingerprint) } default: - g.Log.Infof( + g.Log.Warnf( "No 'serverCertificateFingerprint' was provided. "+ "We strongly recommend specifying 'serverCertificateFingerprint=%s' to verify the server's identity "+ "and avoid potential security risks. Future releases may escalate this to a warning that prevents deployment.", endpointFingerprint, ) + // Return error for encrypted connections without fingerprint + return fmt.Errorf("fingerprint verification is required for encrypted connections") } + } return nil }
432-441: Fix typo and standardize error handling across connection methods.
- Fix the typo "checknig" in error messages
- Consider standardizing error handling across all three connection methods for consistency
- g.Log.Infof("Error while checknig on the server certificates fingerprint: %s", err) + g.Log.Infof("Error while checking the server certificate fingerprint: %s", err)Also applies to: 508-517, 567-576
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
opcua_plugin/connect.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-docker (arm/v7)
🔇 Additional comments (2)
opcua_plugin/connect.go (2)
8-8: LGTM! Good choice of cryptographic hash function.The use of SHA3 for fingerprint generation is a secure choice, providing strong cryptographic properties.
Also applies to: 15-15
224-268: LGTM! Robust certificate handling with proper error checks.The implementation properly handles certificate parsing, validation, and fingerprint generation with comprehensive error messages.
|
|
refactor strict/unencrypted
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
opcua_plugin/connect.go (2)
198-225: Consider enhancing error handling for getServerCertificateFingerprint.The function correctly verifies the server certificate fingerprint and provides informative messages. However, the error from
getServerCertificateFingerprintcould be wrapped with additional context.Apply this diff to improve error handling:
endpointFingerprint, err := g.getServerCertificateFingerprint(endpoint) if err != nil { - return err + return fmt.Errorf("failed to get server certificate fingerprint: %w", err) }
236-271: Consider extracting certificate type as a constant.The function correctly calculates the SHA3-512 fingerprint. Consider extracting the certificate type string to improve maintainability.
Apply this diff to extract the constant:
+const ( + certificateType = "CERTIFICATE" +) func (g *OPCUAInput) getServerCertificateFingerprint(endpoint *ua.EndpointDescription) (string, error) { // ... pemCert := pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", + Type: certificateType, Bytes: endpoint.ServerCertificate, }) // ... }README.md (1)
202-202: Add missing comma after "Otherwise".Add a comma to improve readability.
-Otherwise the connection will be rejected, signaling a potential security issue or misconfiguration. +Otherwise, the connection will be rejected, signaling a potential security issue or misconfiguration.🧰 Tools
🪛 LanguageTool
[uncategorized] ~202-~202: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...verCertificateFingerprint` accordingly. Otherwise the connection will be rejected, signal...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)opcua_plugin/authentication.go(2 hunks)opcua_plugin/connect.go(10 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~202-~202: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...verCertificateFingerprint` accordingly. Otherwise the connection will be rejected, signal...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-docker (arm/v7)
- GitHub Check: build-docker (arm64)
🔇 Additional comments (5)
opcua_plugin/authentication.go (1)
51-51: LGTM! Improved maintainability by using ua.FormatSecurityPolicyURI.The change from hardcoded security policy URIs to using
ua.FormatSecurityPolicyURIis a good improvement that reduces the risk of typos and makes the code more maintainable.Also applies to: 57-57
opcua_plugin/connect.go (3)
8-8: LGTM! Appropriate imports for fingerprint functionality.The addition of
encoding/hexandgolang.org/x/crypto/sha3is appropriate for implementing secure certificate fingerprint verification.Also applies to: 15-15
435-444: LGTM! Improved user guidance for encrypted endpoints.The addition of informative messages about encrypted endpoints and required security settings helps users configure their connections correctly.
515-522: LGTM! Enhanced security validation.The addition of security mode and policy validation ensures that strict connections are properly configured.
README.md (1)
191-213: LGTM! Comprehensive documentation for serverCertificateFingerprint.The documentation clearly explains:
- Purpose and benefits of using certificate fingerprints
- Configuration requirements and dependencies
- Future compatibility considerations
- Troubleshooting guidance
🧰 Tools
🪛 LanguageTool
[uncategorized] ~202-~202: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...verCertificateFingerprint` accordingly. Otherwise the connection will be rejected, signal...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
| if currentEndpoint.SecurityMode == ua.MessageSecurityModeSignAndEncrypt && | ||
| !isNoSecurityEndpoint(currentEndpoint) { | ||
| serverCertFingerprint, _ := g.getServerCertificateFingerprint(currentEndpoint) | ||
| g.Log.Infof("Encrypted endpoint was found, please provide the following "+ |
There was a problem hiding this comment.
Update the error message to show the exact securityPolicy that the user should select. Also include why the user should provide this information (to enable encryption)
| @@ -205,6 +206,8 @@ input: | |||
| opcua: | |||
| endpoint: 'opc.tcp://localhost:46010' | |||
| nodeIDs: ['ns=2;s=IoTSensors'] | |||
| securityMode: SignAndEncrypt | |||
There was a problem hiding this comment.
maybe have two examples here? one example without encryption that shows that it is really easy to work with benthos-umh (will negiotiate everything automatically). and then one example showing "strict security enforcement" with policy, fingerprint and mode?
There was a problem hiding this comment.
Ok, I helped revise the info messages and a little bit the flow, you can find them here:
https://chatgpt.com/share/67a0ff51-011c-8010-b733-992d80eaa8a4
(please double check it all :) )
| //TODO: specify | ||
| g.Log.Infof("Establishing an unencrypted connection...") | ||
|
|
||
| if g.DirectConnect || len(endpoints) == 0 { |
There was a problem hiding this comment.
i think it was originally the other way around. attemptBestENdpoint followed by direct conncet right?
There was a problem hiding this comment.
No it was like this:
- if the user provided the DirectConnect flag or if we didn't get any endpoints
- otherwise
attemptBestEndpointConnection
benthos-umh/opcua_plugin/connect.go
Lines 486 to 560 in 4db3728
Fixes ENG-2389
Description:
fingerprint: <servers-fingerprint>in input.yamlThe fingerprint will get checked while setting up a connection. The filtering happens while fetching the endpoints since we don't have to worry about implementing it into
connectWithSecurityas well asattemptBestEndpointConnection. Thererfore it will always be used if the user set's thefingerprint.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Chores