feat: Add brute-force protection and asyncua User class integration#85
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds brute-force protection to OPC-UA authentication and improves integration with the asyncua library by returning native User objects. It includes comprehensive unit tests and documentation analyzing authentication patterns and security modes.
Changes:
- Implemented
RateLimiterclass with configurable lockout settings (5 attempts, 5-minute lockout by default) - Updated
OpenPLCUserManagerto return asyncua's nativeUserclass instead ofSimpleNamespace - Added 32 unit tests covering rate limiting, authentication methods, and role mappings
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/pytest/plugins/opcua/test_user_manager.py | New comprehensive test suite for user manager functionality including rate limiting, authentication methods, and role mappings |
| docs/opcua/OPCUA_SECURITY_MODE_INSUFFICIENT_ANALYSIS.md | Documentation explaining BadSecurityModeInsufficient error and security configuration options |
| docs/opcua/OPCUA_AUTHENTICATION_REVIEW.md | Analysis comparing implementation with asyncua 1.1.8 patterns |
| core/src/drivers/plugins/python/opcua/user_manager.py | Added rate limiting infrastructure and updated to return asyncua User objects |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _authenticate_password( | ||
| self, username: str, password: str | ||
| ) -> tuple[Optional[User], Optional[str]]: |
There was a problem hiding this comment.
The return type uses the modern tuple[...] syntax which requires Python 3.9+. For better compatibility with older Python versions, consider using Tuple[Optional[User], Optional[str]] from the typing module (already imported on line 15) or verify that Python 3.9+ is the minimum supported version.
| return User(role=asyncua_role, name=username), openplc_role | ||
|
|
||
| def _authenticate_certificate(self, certificate: Any) -> Optional[Any]: | ||
| def _authenticate_certificate(self, certificate: Any) -> tuple[Optional[User], Optional[str]]: |
There was a problem hiding this comment.
The return type uses the modern tuple[...] syntax which requires Python 3.9+. For better compatibility with older Python versions, consider using Tuple[Optional[User], Optional[str]] from the typing module (already imported on line 15) or verify that Python 3.9+ is the minimum supported version.
| return User(role=asyncua_role, name=f"cert:{cert_id}"), openplc_role | ||
|
|
||
| def _authenticate_anonymous(self, profile: Any) -> Optional[Any]: | ||
| def _authenticate_anonymous(self, profile: Any) -> tuple[Optional[User], Optional[str]]: |
There was a problem hiding this comment.
The return type uses the modern tuple[...] syntax which requires Python 3.9+. For better compatibility with older Python versions, consider using Tuple[Optional[User], Optional[str]] from the typing module (already imported on line 15) or verify that Python 3.9+ is the minimum supported version.
marconetsf
left a comment
There was a problem hiding this comment.
PR Review Summary
This PR implements a well-designed brute-force protection mechanism for OPC-UA authentication with the following highlights:
Strengths:
- Clean separation of concerns (
RateLimiter,RateLimitConfig,AuthAttemptTrackerclasses) - Proper asyncua integration (returns native
Userclass instead ofSimpleNamespace) - Comprehensive test coverage (32 tests, all passing)
- Good documentation added for authentication patterns and security analysis
Test Results: 32/32 tests pass
Pre-commit: Black, isort, ruff pass. Minor pylint warnings (see issues comment).
Overall: ✅ APPROVED
See full review: docs/pr-reviews/PR_85_REVIEW.md
Minor Issues Found (Non-blocking)These are suggestions for improvement - not blocking merge. 1. Unnecessary parentheses (pylint C0325)
self.rate_limiter.record_attempt(rate_limit_id, success=user is not None)2. Constant naming style (pylint C0103)
Suggestions (Optional Improvements)3. Thread safety consideration
4. Memory cleanup scheduling
Full review: |
Summary
Userclass for better library integrationChanges
Brute-Force Protection (
user_manager.py)RateLimiterclass with configurable settings:max_attempts: 5 (default) failed attempts before lockoutlockout_duration_seconds: 300 (5 minutes) lockout periodattempt_window_seconds: 60 (1 minute) window for counting attemptsuser:{username}) or certificate fingerprint (cert:{fingerprint})asyncua User Class Integration
asyncua.crypto.permission_rules.Userobjects instead ofSimpleNamespaceopenplc_roleattribute for permission callbacksUnit Tests (
test_user_manager.py)Documentation
OPCUA_AUTHENTICATION_REVIEW.md: Comparison with asyncua 1.1.8 patternsOPCUA_SECURITY_MODE_INSUFFICIENT_ANALYSIS.md: Analysis of BadSecurityModeInsufficient errorTest plan
pytest tests/pytest/plugins/opcua/test_user_manager.py)🤖 Generated with Claude Code