ENG-2389: add trusted server option via fingerprint #118
Conversation
WalkthroughThis pull request enhances the OPC UA plugin and its testing infrastructure. It introduces a new job for Kepware device testing that depends on a preceding job, while also updating the workflow with environment variable assignments for device fingerprints. The README has been revised to reflect changes in configuration options, including security mode and certificate fingerprint settings. The OPC UA plugin code now incorporates certificate fingerprint validation, refined connection methods, and user authentication checks. Additionally, tests have been updated to verify these new security features, and several dependency versions in Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant A as OPCUAInput
participant E as Endpoint Server
U->>A: Initiate OPC UA Connection
A->>A: getUserAuthenticationType()
A->>A: isSecuritySelected()?
alt Security Selected
A->>A: encryptedConnect()
A->>A: checkServerCertificateFingerprint()
A->>A: storeServerCertificateFingerprint()
else No Security
A->>A: unencryptedConnect()
end
A->>E: openConnection()
E-->>A: Connection Response
A-->>U: Connection Established
sequenceDiagram
participant W as Workflow Engine
participant O as go-test-opcua-plc Job
participant K as go-test-kepware-plc Job
W->>O: Start go-test-opcua-plc
O-->>W: Job Complete
W->>K: Trigger go-test-kepware-plc (needs: go-test-opcua-plc)
K-->>W: Job Complete
Possibly related PRs
Suggested reviewers
Poem
🪧 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.
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.
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)
|
ignore those commits for now, i just started resolving |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
opcua_plugin/browse_frontend.go (1)
109-143:⚠️ Potential issue❓ Verification inconclusive
Avoid using BrowseName as map keys.
Based on previous feedback, using BrowseName as map keys isn't reliable.
Run the following script to check for other instances of BrowseName usage as keys:
🏁 Script executed:
#!/bin/bash # Description: Check for BrowseName usage as map keys ast-grep --pattern 'map[$_]string { $$$BrowseName$$$: $_ }'Length of output: 61
Attention: Manual Verification Needed for BrowseName Key Usage
The current implementation in
opcua_plugin/browse_frontend.go(lines 109–143) constructs theChildIDMapusing string segments (derived from splittingnode.Path) as keys. Although the review comment cautions against using BrowseName for map keys due to reliability concerns, our automated check using the provided AST grep pattern returned no output. This inconclusive result suggests that either no direct map entries keyed on "BrowseName" are present or the detection pattern may need refinement.
- Action: Please manually verify whether the components used as keys (i.e., the
partstrings) represent BrowseName values and confirm that no fragile reliance on BrowseName exists elsewhere in the codebase. A more robust key may be required if BrowseName values are indeed being implicitly used.
♻️ Duplicate comments (1)
opcua_plugin/connect.go (1)
286-312:⚠️ Potential issueFix missing return value usage in
storeServerCertificateFingerprint.The function calculates a fingerprint but doesn't store it.
Apply this diff to fix the issue:
- g.ServerCertificates[endpoint] = hex.EncodeToString(shaFingerprint[:]) + fingerprint := hex.EncodeToString(shaFingerprint[:]) + g.ServerCertificates[endpoint] = fingerprint + return nil
🧹 Nitpick comments (15)
opcua_plugin/browse_workers.go (5)
18-25: Consider adding a rationale for the chosen sample size and latency target.The
ServerMetricsstruct uses fixed values (SampleSize = 5,TargetLatency = 250ms). While these defaults may work in many cases, performance tuning often benefits from parameterization or at least an explanatory comment for why these exact values are used. Converting them to configurable parameters might increase flexibility in production environments.
27-34: Add unit tests for NewServerMetrics.The constructor function initializes default values that are critical for worker management. Ensure these values operate correctly under tests (e.g., verifying
currentWorkers == InitialWorkers,targetLatency == TargetLatency).Would you like me to stub out a basic unit test to verify these assumptions?
56-90: Revisit single-worker removal per iteration.Currently, each
toRemoveiteration removes exactly one worker (due to thebreak), causing potentially slow down-scaling. If your server is under high load, more aggressive removal might be beneficial. You could remove multiple workers in a single iteration if needed.- for i := 0; i < toRemove; i++ { - for id := range workerID { - metrics.removeWorker(id) - break - } - } +removeCount := 0 +for id := range workerID { + metrics.removeWorker(id) + removeCount++ + if removeCount == toRemove { + break + } +}
58-63: Consider partial history retention.
if len(sm.responseTimes) < SampleSize { return 0, 0 }short-circuits adjustments until SampleSize is met. This logic ensures stable initial observations but can cause the system to ignore short bursts of high latency early on. You might consider partial calculations before reaching the full sample.
72-78: Check for overlap in worker scaling conditions.Your logic uses
if avgResponse > sm.targetLatencyfollowed by a separateif avgResponse < sm.targetLatency. If the response is very close to the target, both blocks could be skipped, effectively leaving the pool size unchanged. That might be acceptable, but if you want to handle equality as well, considerelse if.opcua_plugin/browse.go (6)
66-68: Clarify usage note for Browse.Lines 66–68 wrap an internal
browsecall in a public function. Provide a comment describing when and why a developer should useBrowsedirectly (or not). Maintaining consistent usage guidance reduces confusion for future maintainers.
70-76: Explain the rationale behind the “level” field in NodeTask.Although the code references a max depth of 25, the introduction of
NodeTask.levelmight need more explanation in comments, indicating how you envisioned controlling or bounding recursion.
283-293: Improve error messages with next steps.Currently,
sendErrorlogs or warns if the channel is blocked or the context is done, but it does not guide the user on what to do next. Per coding guidelines, consider providing a short actionable suggestion, such as retrying or checking logs.
294-305: Add cycle detection comment to browseChildren.
visited.LoadOrStore(...)effectively prevents infinite loops in deeply or circularly referenced node graphs. A small clarifying comment near line 306 or line 194 would help make that logic more explicit, avoiding confusion about why we skip previously seen nodes.
356-359: Examine lengthy browse timeout.Increasing the timeout to 1 hour (lines 358–359) helps handle large node sets but can also tie up resources for a long time if a server is unresponsive. Ensure logs or progress output help the user see that the system is still actively browsing.
396-398: Name the goroutine for easier debugging.When spawning
go browse(...)around line 397, consider naming or logging the invocation (e.g., “Starting browse for NodeID X…”) in a more explicit manner. This can simplify debugging if any goroutine hangs or stalls.opcua_plugin/connect.go (2)
245-274: Enhance error handling for certificate fingerprint validation.The error messages are well-structured and provide clear next steps. However, consider adding more context about the security implications.
Apply this diff to enhance the error messages:
- if g.ServerCertificates[endpoint] == "" { - return fmt.Errorf("The servers endpoint '%s' doesn't provide any server certificate. "+ - "This is crucial in order to get a reliable OPC-UA connection, please "+ - "check your server's certificates and make sure it exists.", endpoint.EndpointURL) + if g.ServerCertificates[endpoint] == "" { + return fmt.Errorf("Security Error: The server endpoint '%s' does not provide a certificate. "+ + "A valid server certificate is required for secure OPC-UA connections to prevent "+ + "man-in-the-middle attacks. Please verify your server's certificate configuration.", + endpoint.EndpointURL) }
705-708: Improve security endpoint information.The function provides good information about available security endpoints, but it could be more proactive in preventing insecure connections.
Consider implementing a configuration option to enforce secure connections by rejecting connections to endpoints without proper security settings. This would help prevent accidental use of insecure endpoints.
opcua_plugin/browse_frontend.go (1)
162-175: Improve node ID normalization logic.The
normalizeNodeIDfunction handles edge cases well but could be more robust.Apply this diff to enhance the function:
func normalizeNodeID(nodeID *ua.NodeID) string { + if nodeID == nil { + return "" + } id := nodeID.String() if strings.HasPrefix(id, "s=i=") { return strings.TrimPrefix(id, "s=") } if strings.HasPrefix(id, "s=ns=") { return strings.TrimPrefix(id, "s=") } return id }README.md (1)
191-213: LGTM! Well-documented feature with minor grammar fixes needed.The documentation clearly explains the server certificate fingerprint feature, its purpose, and important considerations.
Consider these minor grammar improvements:
- > - **Otherwise the `serverCertificateFingerprint` does not provide any value since it's only used for encryption. + > - **Otherwise, the `serverCertificateFingerprint` does not provide any value since it's only used for encryption. - > - **You will need to set `securityMode` to `SignAndEncrypt` and the `securityPolicy` to one of the available policies. + > - **You will need to set `securityMode` to `SignAndEncrypt` and the `securityPolicy` to one of the available policies.**🧰 Tools
🪛 LanguageTool
[uncategorized] ~200-~200: Possible missing comma found.
Context: ...to one of the available policies. > - **Otherwise theserverCertificateFingerprintdoes...(AI_HYDRA_LEO_MISSING_COMMA)
[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 ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
.github/actions/setup-go/action.yml(1 hunks).github/workflows/kepware-plc.yml(0 hunks).github/workflows/main.yml(4 hunks).github/workflows/modbus-plc.yml(0 hunks).github/workflows/nodered-js.yml(0 hunks).github/workflows/opcua-plc.yml(5 hunks).github/workflows/s7-plc.yml(0 hunks).github/workflows/sensorconnect.yml(0 hunks).github/workflows/tag-processor.yml(0 hunks)README.md(5 hunks)go.mod(3 hunks)opcua_plugin/browse.go(5 hunks)opcua_plugin/browse_frontend.go(4 hunks)opcua_plugin/browse_workers.go(1 hunks)opcua_plugin/connect.go(12 hunks)opcua_plugin/opcua_kepware_test.go(1 hunks)opcua_plugin/opcua_opc-plc_test.go(1 hunks)opcua_plugin/opcua_unittest_test.go(8 hunks)opcua_plugin/server_info.go(1 hunks)
💤 Files with no reviewable changes (6)
- .github/workflows/s7-plc.yml
- .github/workflows/sensorconnect.yml
- .github/workflows/nodered-js.yml
- .github/workflows/modbus-plc.yml
- .github/workflows/kepware-plc.yml
- .github/workflows/tag-processor.yml
✅ Files skipped from review due to trivial changes (1)
- opcua_plugin/opcua_opc-plc_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: When reviewing an error message, ensure it meets the...
**/*: When reviewing an error message, ensure it meets these guidelines. If a message is lacking (e.g., missing clear next steps or context), provide an example of the correct format to help improve clarity and usability.Quick Guidelines for Writing Effective Error Messages
- Structure Your Message with Three Key Parts
• Context:
Explain what the system was doing.
Example: “Couldn’t parse config file: /etc/sample-config.properties.”
• Error Description:
Clearly state what failed without generic wording.
Example: “Invalid snapshot mode ‘nevr’ provided.”
• Mitigation/Next Steps:
Tell the user what to do next. For complex issues, include an error code or documentation link.
Example: “Please update the mode; refer to our docs for valid options.”- Clarity & Conciseness
• Use plain, simple language and avoid unnecessary jargon (unless targeting technical users).
• Aim for short sentences (ideally 8–14 words) for better understanding.- Consistent Tone & Style
• Uniformity: Use the same voice, terminology, and punctuation across all messages.
• Empathy: Maintain a neutral, supportive tone; avoid blaming the user.
• Avoid overly casual language (skip “oops”/“whoops” and excessive exclamation points).- Visibility & Placement (User-Facing Messages)
• Proximity: Place error messages close to the related UI element to reduce cognitive load.
• Design: Use high-contrast text, icons, or subtle animations; follow accessibility guidelines (don’t rely solely on color).- Actionability & Efficiency
• Clearly indicate the next steps (e.g., “Retry later”, “Log in again”, “Check your input”).
• Preserve user input when possible to minimize frustration.
• Where relevant, provide error codes or links for further troubleshooting.- Security & Privacy Considerations
• User-Facing: Avoid exposing internal details or sensitive data.
• Developer/Logging: Include detailed context (like file paths or exception chains) for debugging, but be cautious about sensitive information.- Logging & Exception Best Practices (for Developer/Operations Messages)
• Log the full context—including exception chains—to aid troubleshooting.
• Use structured logging (e.g., JSON) for filtering and analysis.
• Choose a consistent method: either raise an exception or log an error (not both) to avoid redundancy.
• Don’t make error messages an API contract; expose machine-readable error codes or types instead.- Additional Best Practices
• Fail Early: Report errors as soon as possible (e.g., during input validation or startup).
• One Concept, One Term: Use consistent terminology across messages and documentation.
• Localization: For internal logs, use English; if localizing for users, follow a vetted process.
opcua_plugin/opcua_kepware_test.goopcua_plugin/server_info.goopcua_plugin/opcua_unittest_test.goopcua_plugin/browse_workers.goopcua_plugin/browse_frontend.goopcua_plugin/connect.goopcua_plugin/browse.goREADME.md
`**/*`: Whenever in a new PR a new function or code is intro...
**/*: Whenever in a new PR a new function or code is introduced, ensure that there is at least somewhere as a code comment, in the README or wherer ever explained the "why" behind the decision. So e.g., why something was preferred over other options, or why a certain loop needs to be executed.
opcua_plugin/opcua_kepware_test.goopcua_plugin/server_info.goopcua_plugin/opcua_unittest_test.goopcua_plugin/browse_workers.goopcua_plugin/browse_frontend.goopcua_plugin/connect.goopcua_plugin/browse.goREADME.md
🧠 Learnings (1)
opcua_plugin/browse_frontend.go (1)
Learnt from: JeremyTheocharis
PR: united-manufacturing-hub/benthos-umh#98
File: opcua_plugin/browse_frontend.go:110-110
Timestamp: 2025-01-08T17:15:13.882Z
Learning: In OPCUA node handling (Go), avoid using `BrowseName` as map keys since they aren't guaranteed to be unique across different nodes. Instead, use unique identifiers like `NodeID` to prevent potential key collisions and data loss in node maps.
🪛 LanguageTool
README.md
[uncategorized] ~200-~200: Possible missing comma found.
Context: ...to one of the available policies. > - **Otherwise the serverCertificateFingerprint does...
(AI_HYDRA_LEO_MISSING_COMMA)
[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 (4)
- GitHub Check: go-test-modbus-plc
- GitHub Check: go-test-tag-processor
- GitHub Check: go-test-opcua-plc
- GitHub Check: go-test-s7-plc
🔇 Additional comments (18)
opcua_plugin/opcua_kepware_test.go (2)
113-115: LGTM! Great documentation of the "why".The comment effectively explains the rationale behind adding
FlakeAttempts, providing valuable context for future maintainers about the unreliability of PLC clients.
116-116: LGTM! Good solution for handling flaky tests.Adding
FlakeAttempts(3)is an appropriate way to handle the unreliability of PLC clients in the test environment.opcua_plugin/browse_workers.go (1)
36-44: Validate uniqueness of worker IDs.While collisions in UUIDs are highly unlikely, consider logging or gracefully handling any unexpected scenario if an ID were somehow reused or re-added, potentially overwriting channels in
workerControls.opcua_plugin/browse.go (1)
157-281: Validate concurrency cancellation paths in worker function.Workers handle multiple select cases for
stopChan,ctx.Done(), andtaskChan. Verify thoroughly that partial context cancellations or rapid removal signals won’t accidentally deadlock or leave tasks unprocessed. Consider more explicit finalization steps or logs to confirm all in-flight tasks are resolved.opcua_plugin/server_info.go (1)
40-42: Confirm concurrency handling with three parallel Browse calls.Lines 40–42 spawn three separate goroutines to browse manufacturer, product, and software versions. Although this is convenient, ensure that partial errors are handled gracefully—currently, the first error from
errChanhalts the entire process, even if the other calls succeed. This may be acceptable, but be aware that partial success is discarded.opcua_plugin/opcua_unittest_test.go (1)
106-107: LGTM! Function signature updates are consistent.The Browse function calls have been correctly updated across all test cases.
Also applies to: 133-134, 171-172, 211-212, 251-252, 294-295
.github/actions/setup-go/action.yml (1)
23-23:✅ Verification successful
Verify compatibility with Go 1.23.x.
The upgrade to Go 1.23.x and newer action versions is good, but ensure compatibility.
Also applies to: 29-30, 34-35, 37-37
🌐 Web query:
What is the latest stable version of Go?💡 Result:
The latest stable version of Go as of February 2025 is Go 1.23.6, released on February 4, 2025[3][4]. This is part of the Go 1.23 release family which follows the updated version numbering scheme introduced in Go 1.21[2].
Key details:
- Current supported releases:
1.23.x (latest: v1.23.6)[3]
1.22.x (latest: v1.22.12)[3]- Support policy: Each major release receives security updates and bug fixes until two newer major releases exist[3][4]
- Recent update highlights in Go 1.23.x:
- Security fixes for cryptographic packages (
crypto/elliptic)[4]- Compiler and toolchain improvements[4]
- Runtime and standard library enhancements[4]
To install or upgrade using Go's automatic toolchain management (available since Go 1.21):
go install golang.org/dl/go1.23.6@latest go1.23.6 downloadCitations:
- 1: https://github.com/golang/tools/releases
- 2: https://tip.golang.org/doc/go1.21
- 3: https://endoflife.date/go
- 4: https://go.dev/doc/devel/release
- 5: https://tip.golang.org/doc/go1.23
- 6: https://go.dev/dl
- 7: https://groups.google.com/g/golang-nuts/c/_ROx_UGCCCg
- 8: https://cmake.org/download/
Ensure compatibility with the latest patch release in the Go 1.23 series.
The default value (
'1.23.x') in.github/actions/setup-go/action.ymlis appropriately set because it will pick up the latest stable release (currently Go 1.23.6 as of February 2025). Please verify that all related action steps (lines 23, 29–30, 34–35, and 37) work correctly with this release family, ensuring no unintended side effects from the update..github/workflows/opcua-plc.yml (6)
56-56: LGTM!The fingerprint environment variable is correctly set alongside the endpoint URI for the S7 device.
72-72: LGTM!The fallback fingerprint environment variable is correctly set alongside the fallback endpoint URI for the S7 device.
87-87: LGTM!The fingerprint environment variable is correctly set alongside the endpoint URI for the Wago device.
103-103: LGTM!The fallback fingerprint environment variable is correctly set alongside the fallback endpoint URI for the Wago device.
149-153: LGTM! Smart workaround for Kepware runtime restarts.The time-based check ensures that tests don't fail unnecessarily during Kepware runtime's scheduled restarts by waiting for a minute when the restart is expected.
124-174: LGTM! Well-structured test job.The Kepware PLC test job:
- Correctly depends on the OPC UA PLC test job
- Uses consistent runner group and permissions
- Follows the same pattern as other PLC test jobs
.github/workflows/main.yml (3)
21-21: LGTM!Using double quotes for the branch pattern improves YAML consistency.
36-36: LGTM!Using double quotes for architecture values improves YAML consistency.
306-306: LGTM! Good upgrade to latest artifact actions.Upgrading to v4 of the artifact actions brings performance improvements and new features.
Also applies to: 320-320
README.md (2)
126-126: LGTM! Simplified security options.Removing the 'Sign' option simplifies the choices to either no security or full security (SignAndEncrypt), making it clearer for users.
204-212: LGTM! Clear and complete example configuration.The example effectively demonstrates how to configure the server certificate fingerprint along with the required security mode and policy settings.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
opcua_plugin/authentication.go (2)
153-192: Consider renaming the function for clarity.The current name
isSecuritySelecteddoesn't fully convey that this function validates security settings and requires a fingerprint. Consider a more descriptive name likeisSecureConnectionConfiguredorvalidateSecuritySettings.
164-167: Enhance error messages for better user guidance.The error messages could be more actionable by including:
- The impact of using insecure settings
- Clear next steps for the user
Apply this diff to improve the messages:
- g.Log.Infof("Invalid securityMode '%s'. For secure (encrypted) connections "+ - ", please set securityMode to 'SignAndEncrypt' or 'Sign'. This setting is "+ - "required to enable encryption and verify the server's certificate.", g.SecurityMode) + g.Log.Infof("Invalid securityMode '%s'. To ensure secure communication:"+ + "\n1. Set securityMode to 'SignAndEncrypt' or 'Sign'"+ + "\n2. This enables encryption and server certificate verification"+ + "\n3. Without encryption, your data may be exposed to security risks", g.SecurityMode) - g.Log.Infof("Invalid securityPolicy '%s'. For encrypted communication, "+ - "please choose a valid policy (e.g., 'Basic256Sha256', 'Basic256', or "+ - "'Basic128Rsa15') that your server supports.", g.SecurityPolicy) + g.Log.Infof("Invalid securityPolicy '%s'. To enable encryption:"+ + "\n1. Choose a valid policy: 'Basic256Sha256' (recommended), 'Basic256', or 'Basic128Rsa15'"+ + "\n2. Verify that your server supports the selected policy"+ + "\n3. Note: Basic128Rsa15 and Basic256 are deprecated due to security concerns", g.SecurityPolicy)Also applies to: 180-183
opcua_plugin/opcua_plc_test.go (2)
164-165: Improve skip message clarity.The current skip message "does not work right now, since we have to stop cpu for encryption" is unclear. Consider providing more context about why the test is skipped and what conditions are needed to run it.
Apply this diff:
- Skip("does not work right now, since we have to stop cpu for encryption") + Skip("Test requires CPU to be stopped for encryption. This test verifies secure connection with " + + "valid fingerprint but cannot run in automated testing due to hardware requirements.")
183-196: Add test case for invalid security configuration.The current tests verify fingerprint mismatch but don't test invalid security mode/policy combinations.
Add a test case like this:
It("should fail with invalid security configuration", func() { input = &OPCUAInput{ Endpoint: endpoint, NodeIDs: nil, SecurityMode: "Invalid", SecurityPolicy: "Invalid", ServerCertificates: make(map[*ua.EndpointDescription]string), ServerCertificateFingerprint: "test123", } err := input.Connect(ctx) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("invalid security configuration")) })README.md (1)
180-182: Fix formatting in security documentation.Address these formatting issues:
- Add missing commas in list items
- Break up repeated sentence beginnings
- Improve readability of long sentences
Apply these changes:
- Messages are signed for integrity and authenticity but not encrypted. + Messages are signed for integrity and authenticity, but not encrypted. - If omitted while still using encryption the client will attempt to connect + If omitted while using encryption, the client will attempt to connect - The auto‐generated certificate is logged in Base64 so you can copy/paste + The auto‐generated certificate is logged in Base64, so you can copy/paste - By default benthos-umh will now connect via SignAndEncrypt and Basic256Sha256 and if this fails + By default, benthos-umh will now connect via SignAndEncrypt and Basic256Sha256. If this fails,Also applies to: 195-197, 202-203, 220-221
🧰 Tools
🪛 LanguageTool
[uncategorized] ~180-~180: Possible missing comma found.
Context: ...re signed for integrity and authenticity but not encrypted. - SignAndEncrypt...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(3 hunks)opcua_plugin/authentication.go(1 hunks)opcua_plugin/connect.go(13 hunks)opcua_plugin/opcua.go(4 hunks)opcua_plugin/opcua_kepware_test.go(5 hunks)opcua_plugin/opcua_plc_test.go(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- opcua_plugin/opcua_kepware_test.go
- opcua_plugin/opcua.go
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: When reviewing an error message, ensure it meets the...
**/*: When reviewing an error message, ensure it meets these guidelines. If a message is lacking (e.g., missing clear next steps or context), provide an example of the correct format to help improve clarity and usability.Quick Guidelines for Writing Effective Error Messages
- Structure Your Message with Three Key Parts
• Context:
Explain what the system was doing.
Example: “Couldn’t parse config file: /etc/sample-config.properties.”
• Error Description:
Clearly state what failed without generic wording.
Example: “Invalid snapshot mode ‘nevr’ provided.”
• Mitigation/Next Steps:
Tell the user what to do next. For complex issues, include an error code or documentation link.
Example: “Please update the mode; refer to our docs for valid options.”- Clarity & Conciseness
• Use plain, simple language and avoid unnecessary jargon (unless targeting technical users).
• Aim for short sentences (ideally 8–14 words) for better understanding.- Consistent Tone & Style
• Uniformity: Use the same voice, terminology, and punctuation across all messages.
• Empathy: Maintain a neutral, supportive tone; avoid blaming the user.
• Avoid overly casual language (skip “oops”/“whoops” and excessive exclamation points).- Visibility & Placement (User-Facing Messages)
• Proximity: Place error messages close to the related UI element to reduce cognitive load.
• Design: Use high-contrast text, icons, or subtle animations; follow accessibility guidelines (don’t rely solely on color).- Actionability & Efficiency
• Clearly indicate the next steps (e.g., “Retry later”, “Log in again”, “Check your input”).
• Preserve user input when possible to minimize frustration.
• Where relevant, provide error codes or links for further troubleshooting.- Security & Privacy Considerations
• User-Facing: Avoid exposing internal details or sensitive data.
• Developer/Logging: Include detailed context (like file paths or exception chains) for debugging, but be cautious about sensitive information.- Logging & Exception Best Practices (for Developer/Operations Messages)
• Log the full context—including exception chains—to aid troubleshooting.
• Use structured logging (e.g., JSON) for filtering and analysis.
• Choose a consistent method: either raise an exception or log an error (not both) to avoid redundancy.
• Don’t make error messages an API contract; expose machine-readable error codes or types instead.- Additional Best Practices
• Fail Early: Report errors as soon as possible (e.g., during input validation or startup).
• One Concept, One Term: Use consistent terminology across messages and documentation.
• Localization: For internal logs, use English; if localizing for users, follow a vetted process.
opcua_plugin/opcua_plc_test.goopcua_plugin/authentication.goopcua_plugin/connect.goREADME.md
`**/*`: Whenever in a new PR a new function or code is intro...
**/*: Whenever in a new PR a new function or code is introduced, ensure that there is at least somewhere as a code comment, in the README or wherer ever explained the "why" behind the decision. So e.g., why something was preferred over other options, or why a certain loop needs to be executed.
opcua_plugin/opcua_plc_test.goopcua_plugin/authentication.goopcua_plugin/connect.goREADME.md
🪛 LanguageTool
README.md
[uncategorized] ~180-~180: Possible missing comma found.
Context: ...re signed for integrity and authenticity but not encrypted. - SignAndEncrypt...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~195-~195: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...smatch, the connection is rejected. - If omitted while *still using encryption...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~202-~202: Possible missing comma found.
Context: ...auto‐generated certificate is logged in Base64 so you can copy/paste it into your conf...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~220-~220: Possible missing comma found.
Context: ...will now connect via SignAndEncrypt and Basic256Sha256 and if this fails it will fall back to ...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test-opcua-plc
🔇 Additional comments (2)
opcua_plugin/authentication.go (1)
142-150: LGTM! Clean implementation of user authentication type determination.The function follows the single responsibility principle and includes a helpful comment about future extensibility.
opcua_plugin/connect.go (1)
331-360: LGTM! Well-structured certificate validation with clear error messages.The function:
- Properly validates server certificates
- Provides clear error messages with context and next steps
- Uses secure SHA3-512 for fingerprint generation
|
So now we have 2 different starting points here:
Since the "encrypted"-connection doesn't work well with Siemens right now, I just prepared tests / workflow for the S7-Fingerprint, but the test isn't included here, so it should get skipped. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
opcua_plugin/authentication.go (2)
142-150: Enhance method documentation with return value descriptions.The method logic is correct, but the documentation could be more comprehensive.
Add return value descriptions to clarify the behavior:
-// Here we can add UserTokenTypeCertificate and UserTokenTypeIssuedToken later +// getUserAuthenticationType determines the appropriate user token type based on credentials. +// +// Returns: +// - ua.UserTokenTypeUserName: When both username and password are provided +// - ua.UserTokenTypeAnonymous: When no credentials are provided +// +// Note: Future support planned for UserTokenTypeCertificate and UserTokenTypeIssuedToken
152-152: Fix typo in comment.The comment contains a spelling error.
-// explicitely check if security is selected + only allow specified settings +// explicitly check if security is selected + only allow specified settingsopcua_plugin/connect.go (3)
374-400: Improve error handling and messages.The error messages could be more descriptive and use error wrapping.
Consider this refactoring:
func (g *OPCUAInput) storeServerCertificateFingerprint(endpoint *ua.EndpointDescription) error { pemCert := pem.EncodeToMemory(&pem.Block{ Type: "CERTIFICATE", Bytes: endpoint.ServerCertificate, }) // decode the certificate from base64 to DER format block, _ := pem.Decode(pemCert) if block == nil { - return fmt.Errorf("Could not decode server certificate.") + return fmt.Errorf("failed to decode server certificate: invalid PEM format") } // parse the DER-format certificate cert, err := x509.ParseCertificate(block.Bytes) if err != nil { - return fmt.Errorf("Error while parsing server certificate.") + return fmt.Errorf("failed to parse server certificate: %w", err) } // calculating the checksum of the certificate (sha3 is needed here) // and convert the array into a slice for encoding shaFingerprint := sha3.Sum512(cert.Raw) g.ServerCertificates[endpoint] = hex.EncodeToString(shaFingerprint[:]) return nil }
812-839: Enhance error handling and logging.The method could benefit from more detailed error handling and logging.
Consider this refactoring:
func (g *OPCUAInput) openConnection( ctx context.Context, endpoint *ua.EndpointDescription, authType ua.UserTokenType, discoveryOnly bool, ) (*opcua.Client, error) { + g.Log.Debugf("Attempting to connect to endpoint %s with auth type %v", endpoint.EndpointURL, authType) opts, err := g.GetOPCUAClientOptions(endpoint, authType, discoveryOnly) if err != nil { - g.Log.Errorf("Failed to get OPC UA client options: %s", err) + return nil, fmt.Errorf("failed to get OPC UA client options: %w", err) } c, err := opcua.NewClient(endpoint.EndpointURL, opts...) if err != nil { - g.Log.Errorf("Failed to create a new client") + return nil, fmt.Errorf("failed to create new client for endpoint %s: %w", endpoint.EndpointURL, err) } - // Connect to the endpoint - // If connection fails, then continue to the next endpoint - // Connect to the selected endpoint + g.Log.Debugf("Attempting to establish connection to endpoint %s", endpoint.EndpointURL) err = c.Connect(ctx) if err != nil { + g.Log.Debugf("Connection failed to endpoint %s: %v", endpoint.EndpointURL, err) return nil, err } + g.Log.Infof("Successfully connected to endpoint %s", endpoint.EndpointURL) return c, nil }
764-789: Enhance connection logic with detailed logging and constants.The connection logic could benefit from more detailed logging and message constants.
Consider this refactoring:
+const ( + msgAttemptingEncryptedConnection = "Attempting encrypted connection with mode '%s', policy '%s', and fingerprint '%s'" + msgConnectionFailed = "Encrypted connection failed: %v. Security settings: mode='%s', policy='%s', fingerprint='%s'" +) if g.isSecuritySelected() { + g.Log.Infof(msgAttemptingEncryptedConnection, g.SecurityMode, g.SecurityPolicy, g.ServerCertificateFingerprint) c, err = g.encryptedConnect(ctx, endpoints, selectedAuthentication) if err != nil { - g.Log.Infof("Error while connecting using securityMode: '%s',"+ - "securityPolicy: '%s', serverCertificateFingerprint: '%s'. err:%v", - g.SecurityMode, g.SecurityPolicy, g.ServerCertificateFingerprint, err) + g.Log.Errorf(msgConnectionFailed, err, g.SecurityMode, g.SecurityPolicy, g.ServerCertificateFingerprint) return err } g.Client = c return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
opcua_plugin/authentication.go(1 hunks)opcua_plugin/connect.go(13 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: When reviewing an error message, ensure it meets the...
**/*: When reviewing an error message, ensure it meets these guidelines. If a message is lacking (e.g., missing clear next steps or context), provide an example of the correct format to help improve clarity and usability.Quick Guidelines for Writing Effective Error Messages
- Structure Your Message with Three Key Parts
• Context:
Explain what the system was doing.
Example: “Couldn’t parse config file: /etc/sample-config.properties.”
• Error Description:
Clearly state what failed without generic wording.
Example: “Invalid snapshot mode ‘nevr’ provided.”
• Mitigation/Next Steps:
Tell the user what to do next. For complex issues, include an error code or documentation link.
Example: “Please update the mode; refer to our docs for valid options.”- Clarity & Conciseness
• Use plain, simple language and avoid unnecessary jargon (unless targeting technical users).
• Aim for short sentences (ideally 8–14 words) for better understanding.- Consistent Tone & Style
• Uniformity: Use the same voice, terminology, and punctuation across all messages.
• Empathy: Maintain a neutral, supportive tone; avoid blaming the user.
• Avoid overly casual language (skip “oops”/“whoops” and excessive exclamation points).- Visibility & Placement (User-Facing Messages)
• Proximity: Place error messages close to the related UI element to reduce cognitive load.
• Design: Use high-contrast text, icons, or subtle animations; follow accessibility guidelines (don’t rely solely on color).- Actionability & Efficiency
• Clearly indicate the next steps (e.g., “Retry later”, “Log in again”, “Check your input”).
• Preserve user input when possible to minimize frustration.
• Where relevant, provide error codes or links for further troubleshooting.- Security & Privacy Considerations
• User-Facing: Avoid exposing internal details or sensitive data.
• Developer/Logging: Include detailed context (like file paths or exception chains) for debugging, but be cautious about sensitive information.- Logging & Exception Best Practices (for Developer/Operations Messages)
• Log the full context—including exception chains—to aid troubleshooting.
• Use structured logging (e.g., JSON) for filtering and analysis.
• Choose a consistent method: either raise an exception or log an error (not both) to avoid redundancy.
• Don’t make error messages an API contract; expose machine-readable error codes or types instead.- Additional Best Practices
• Fail Early: Report errors as soon as possible (e.g., during input validation or startup).
• One Concept, One Term: Use consistent terminology across messages and documentation.
• Localization: For internal logs, use English; if localizing for users, follow a vetted process.
opcua_plugin/authentication.goopcua_plugin/connect.go
`**/*`: Whenever in a new PR a new function or code is intro...
**/*: Whenever in a new PR a new function or code is introduced, ensure that there is at least somewhere as a code comment, in the README or wherer ever explained the "why" behind the decision. So e.g., why something was preferred over other options, or why a certain loop needs to be executed.
opcua_plugin/authentication.goopcua_plugin/connect.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: create-manifests
- GitHub Check: go-test-opcua-plc
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
174-204: Enhance security documentation with best practices.Consider adding:
- Security implications of each mode
- Recommendations for production environments
- Examples of obtaining and verifying fingerprints
- Warning about security risks of deprecated policies
🧰 Tools
🪛 LanguageTool
[style] ~195-~195: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...smatch, the connection is rejected. - If omitted while *still using encryption...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
219-222: Add migration guide for deprecated insecure mode.Add a section explaining:
- Why insecure mode is deprecated
- How to transition from insecure to secure mode
- Steps to handle certificate trust during transition
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/opcua-plc.yml(5 hunks)README.md(3 hunks)opcua_plugin/opcua_kepware_test.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: When reviewing an error message, ensure it meets the...
**/*: When reviewing an error message, ensure it meets these guidelines. If a message is lacking (e.g., missing clear next steps or context), provide an example of the correct format to help improve clarity and usability.Quick Guidelines for Writing Effective Error Messages
- Structure Your Message with Three Key Parts
• Context:
Explain what the system was doing.
Example: “Couldn’t parse config file: /etc/sample-config.properties.”
• Error Description:
Clearly state what failed without generic wording.
Example: “Invalid snapshot mode ‘nevr’ provided.”
• Mitigation/Next Steps:
Tell the user what to do next. For complex issues, include an error code or documentation link.
Example: “Please update the mode; refer to our docs for valid options.”- Clarity & Conciseness
• Use plain, simple language and avoid unnecessary jargon (unless targeting technical users).
• Aim for short sentences (ideally 8–14 words) for better understanding.- Consistent Tone & Style
• Uniformity: Use the same voice, terminology, and punctuation across all messages.
• Empathy: Maintain a neutral, supportive tone; avoid blaming the user.
• Avoid overly casual language (skip “oops”/“whoops” and excessive exclamation points).- Visibility & Placement (User-Facing Messages)
• Proximity: Place error messages close to the related UI element to reduce cognitive load.
• Design: Use high-contrast text, icons, or subtle animations; follow accessibility guidelines (don’t rely solely on color).- Actionability & Efficiency
• Clearly indicate the next steps (e.g., “Retry later”, “Log in again”, “Check your input”).
• Preserve user input when possible to minimize frustration.
• Where relevant, provide error codes or links for further troubleshooting.- Security & Privacy Considerations
• User-Facing: Avoid exposing internal details or sensitive data.
• Developer/Logging: Include detailed context (like file paths or exception chains) for debugging, but be cautious about sensitive information.- Logging & Exception Best Practices (for Developer/Operations Messages)
• Log the full context—including exception chains—to aid troubleshooting.
• Use structured logging (e.g., JSON) for filtering and analysis.
• Choose a consistent method: either raise an exception or log an error (not both) to avoid redundancy.
• Don’t make error messages an API contract; expose machine-readable error codes or types instead.- Additional Best Practices
• Fail Early: Report errors as soon as possible (e.g., during input validation or startup).
• One Concept, One Term: Use consistent terminology across messages and documentation.
• Localization: For internal logs, use English; if localizing for users, follow a vetted process.
opcua_plugin/opcua_kepware_test.goREADME.md
`**/*`: Whenever in a new PR a new function or code is intro...
**/*: Whenever in a new PR a new function or code is introduced, ensure that there is at least somewhere as a code comment, in the README or wherer ever explained the "why" behind the decision. So e.g., why something was preferred over other options, or why a certain loop needs to be executed.
opcua_plugin/opcua_kepware_test.goREADME.md
🪛 LanguageTool
README.md
[style] ~195-~195: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...smatch, the connection is rejected. - If omitted while *still using encryption...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test-opcua-plc
🔇 Additional comments (6)
opcua_plugin/opcua_kepware_test.go (4)
22-29: LGTM!The addition of the
fingerprintvariable is appropriate for storing the server certificate fingerprint.
58-60: LGTM!The consistent initialization of
ServerCertificatesas an empty map across all test cases is appropriate for storing server certificates during connection attempts.Also applies to: 114-116, 213-214, 275-276
116-117: LGTM!The assignment of
ServerCertificateFingerprintfrom the environment variable is correct and aligns with the PR objectives for server certificate validation.
170-172: LGTM!The addition of
FlakeAttempts(3)is well-documented and appropriately addresses the unreliability of PLC clients connected via OPC-UA to the Kepware server..github/workflows/opcua-plc.yml (2)
56-57: LGTM!The fingerprint environment variables are consistently set from GitHub secrets for all devices (S7, Wago, and Kepware), addressing the previous inconsistency concern.
Also applies to: 72-73, 87-88, 103-104, 160-161
142-164: LGTM!The Kepware port availability check includes well-documented time-based wait logic to handle runtime restarts, and the port check implementation is consistent with other devices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
125-128: Configuration Update: New Security Options for OPC UA
These lines introduce the updated configuration fields: deprecating theinsecureflag, updatingsecurityModeto only acceptNoneorSignAndEncrypt, and adding the newserverCertificateFingerprintfield with a SHA3-based fingerprint. The changes are consistent with the PR objectives.
174-205: Documentation Note: Security Options Description
The expanded “Security Options” section now provides comprehensive details about the security mode, policy, fingerprint, and client certificate. However, the note block is quite wordy—especially around lines 174 and 197, where successive sentences begin similarly. For clarity and conciseness, consider rephrasing those parts. For example, you might revise the fingerprint subsection as follows:- - If you specify this field, the client will verify that the server’s certificate matches the given fingerprint. If there’s a mismatch, the connection is rejected. If omitted while **still using encryption** (`Sign` or `SignAndEncrypt`), the client will attempt to connect and then **log** the server’s actual fingerprint. You can copy that fingerprint into your config to be certain you’re connecting to the intended server. + - When specified, the client verifies that the server’s certificate matches the provided fingerprint and rejects the connection if they differ. If omitted while using encryption (`Sign` or `SignAndEncrypt`), the client will connect and log the actual fingerprint for your review.This refactoring will improve readability and avoid repetitive sentence openings.
🧰 Tools
🪛 LanguageTool
[style] ~174-~174: Consider a shorter alternative to avoid wordiness.
Context: ...`` ##### Security Options > Note: In order to ensure a fully secure connection, we st...(IN_ORDER_TO_PREMIUM)
[style] ~197-~197: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...smatch, the connection is rejected. - If omitted while *still using encryption...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: When reviewing an error message, ensure it meets the...
**/*: When reviewing an error message, ensure it meets these guidelines. If a message is lacking (e.g., missing clear next steps or context), provide an example of the correct format to help improve clarity and usability.Quick Guidelines for Writing Effective Error Messages
- Structure Your Message with Three Key Parts
• Context:
Explain what the system was doing.
Example: “Couldn’t parse config file: /etc/sample-config.properties.”
• Error Description:
Clearly state what failed without generic wording.
Example: “Invalid snapshot mode ‘nevr’ provided.”
• Mitigation/Next Steps:
Tell the user what to do next. For complex issues, include an error code or documentation link.
Example: “Please update the mode; refer to our docs for valid options.”- Clarity & Conciseness
• Use plain, simple language and avoid unnecessary jargon (unless targeting technical users).
• Aim for short sentences (ideally 8–14 words) for better understanding.- Consistent Tone & Style
• Uniformity: Use the same voice, terminology, and punctuation across all messages.
• Empathy: Maintain a neutral, supportive tone; avoid blaming the user.
• Avoid overly casual language (skip “oops”/“whoops” and excessive exclamation points).- Visibility & Placement (User-Facing Messages)
• Proximity: Place error messages close to the related UI element to reduce cognitive load.
• Design: Use high-contrast text, icons, or subtle animations; follow accessibility guidelines (don’t rely solely on color).- Actionability & Efficiency
• Clearly indicate the next steps (e.g., “Retry later”, “Log in again”, “Check your input”).
• Preserve user input when possible to minimize frustration.
• Where relevant, provide error codes or links for further troubleshooting.- Security & Privacy Considerations
• User-Facing: Avoid exposing internal details or sensitive data.
• Developer/Logging: Include detailed context (like file paths or exception chains) for debugging, but be cautious about sensitive information.- Logging & Exception Best Practices (for Developer/Operations Messages)
• Log the full context—including exception chains—to aid troubleshooting.
• Use structured logging (e.g., JSON) for filtering and analysis.
• Choose a consistent method: either raise an exception or log an error (not both) to avoid redundancy.
• Don’t make error messages an API contract; expose machine-readable error codes or types instead.- Additional Best Practices
• Fail Early: Report errors as soon as possible (e.g., during input validation or startup).
• One Concept, One Term: Use consistent terminology across messages and documentation.
• Localization: For internal logs, use English; if localizing for users, follow a vetted process.
README.md
`**/*`: Whenever in a new PR a new function or code is intro...
**/*: Whenever in a new PR a new function or code is introduced, ensure that there is at least somewhere as a code comment, in the README or wherer ever explained the "why" behind the decision. So e.g., why something was preferred over other options, or why a certain loop needs to be executed.
README.md
🪛 LanguageTool
README.md
[style] ~174-~174: Consider a shorter alternative to avoid wordiness.
Context: ...`` ##### Security Options > Note: In order to ensure a fully secure connection, we st...
(IN_ORDER_TO_PREMIUM)
[style] ~197-~197: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...smatch, the connection is rejected. - If omitted while *still using encryption...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-docker (arm/v7)
- GitHub Check: build-docker (arm64)
- GitHub Check: go-test-opcua-plc
🔇 Additional comments (1)
README.md (1)
217-218: Security Sample: Updated Credential Example
The sample configuration now correctly includes the updatedserverCertificateFingerprintandclientCertificatefields as per the new security requirements. Ensure that users replace the placeholder values with their actual fingerprint (generated using SHA3-512) and certificate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
174-198: Enhance Clarity in the Security Options Description.
The detailed description of security options (including the fingerprint verification process) is excellent; however, static analysis suggests that a few successive sentences—especially around line 197—begin with similar phrasing. Consider reworking the sentence starting at line 197 to vary the structure (for example, mixing conditional phrases or reordering the clauses) and to remove any superfluous words such as "of" where possible. These minor stylistic adjustments will improve readability and maintain a consistent tone throughout the document.🧰 Tools
🪛 LanguageTool
[style] ~174-~174: Consider removing “of” to be more concise
Context: ...nnection, you must explicitly configure all of the following security options. However, if...(ALL_OF_THE)
[style] ~197-~197: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...smatch, the connection is rejected. - If omitted while *still using encryption...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: When reviewing an error message, ensure it meets the...
**/*: When reviewing an error message, ensure it meets these guidelines. If a message is lacking (e.g., missing clear next steps or context), provide an example of the correct format to help improve clarity and usability.Quick Guidelines for Writing Effective Error Messages
- Structure Your Message with Three Key Parts
• Context:
Explain what the system was doing.
Example: “Couldn’t parse config file: /etc/sample-config.properties.”
• Error Description:
Clearly state what failed without generic wording.
Example: “Invalid snapshot mode ‘nevr’ provided.”
• Mitigation/Next Steps:
Tell the user what to do next. For complex issues, include an error code or documentation link.
Example: “Please update the mode; refer to our docs for valid options.”- Clarity & Conciseness
• Use plain, simple language and avoid unnecessary jargon (unless targeting technical users).
• Aim for short sentences (ideally 8–14 words) for better understanding.- Consistent Tone & Style
• Uniformity: Use the same voice, terminology, and punctuation across all messages.
• Empathy: Maintain a neutral, supportive tone; avoid blaming the user.
• Avoid overly casual language (skip “oops”/“whoops” and excessive exclamation points).- Visibility & Placement (User-Facing Messages)
• Proximity: Place error messages close to the related UI element to reduce cognitive load.
• Design: Use high-contrast text, icons, or subtle animations; follow accessibility guidelines (don’t rely solely on color).- Actionability & Efficiency
• Clearly indicate the next steps (e.g., “Retry later”, “Log in again”, “Check your input”).
• Preserve user input when possible to minimize frustration.
• Where relevant, provide error codes or links for further troubleshooting.- Security & Privacy Considerations
• User-Facing: Avoid exposing internal details or sensitive data.
• Developer/Logging: Include detailed context (like file paths or exception chains) for debugging, but be cautious about sensitive information.- Logging & Exception Best Practices (for Developer/Operations Messages)
• Log the full context—including exception chains—to aid troubleshooting.
• Use structured logging (e.g., JSON) for filtering and analysis.
• Choose a consistent method: either raise an exception or log an error (not both) to avoid redundancy.
• Don’t make error messages an API contract; expose machine-readable error codes or types instead.- Additional Best Practices
• Fail Early: Report errors as soon as possible (e.g., during input validation or startup).
• One Concept, One Term: Use consistent terminology across messages and documentation.
• Localization: For internal logs, use English; if localizing for users, follow a vetted process.
README.md
`**/*`: Whenever in a new PR a new function or code is intro...
**/*: Whenever in a new PR a new function or code is introduced, ensure that there is at least somewhere as a code comment, in the README or wherer ever explained the "why" behind the decision. So e.g., why something was preferred over other options, or why a certain loop needs to be executed.
README.md
🪛 LanguageTool
README.md
[style] ~174-~174: Consider removing “of” to be more concise
Context: ...nnection, you must explicitly configure all of the following security options. However, if...
(ALL_OF_THE)
[style] ~197-~197: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...smatch, the connection is rejected. - If omitted while *still using encryption...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-docker (arm/v7)
- GitHub Check: build-docker (arm64)
- GitHub Check: go-test-tag-processor
- GitHub Check: go-test-modbus-plc
- GitHub Check: build-docker (amd64)
- GitHub Check: go-test-s7-plc
- GitHub Check: go-test-nodered-js
- GitHub Check: go-test-sensorconnect
- GitHub Check: go-test-opcua-plc
🔇 Additional comments (2)
README.md (2)
125-128: Updated Security Configuration Options and Deprecation Notice.
The configuration block now clearly marks theinsecureoption as deprecated while restrictingsecurityModetoNoneorSignAndEncrypt. In addition, the newserverCertificateFingerprintparameter (displayed as'sha3-fingerprint-of-cert') is introduced to support SHA3-based fingerprint validation. Please verify that any other references in the documentation (and related configuration examples) consistently reflect this updated security posture.
217-218: Consistent Configuration Example for Fingerprint Validation.
The configuration example (lines 217–218) correctly demonstrates the addition of both theserverCertificateFingerprintandclientCertificatefields. Ensure that the placeholder'sha3-fingerprint-of-cert'is clearly documented as an example value and that users are directed to procure their actual server certificate fingerprint from the appropriate server logs or connection output.
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