fix: Auto-detect local IPs for OPC-UA certificate to fix Docker hostname issues#86
Conversation
When running in Docker containers, the auto-generated OPC-UA server certificate only included the container hostname in the Subject Alternative Name (SAN). This caused BadCertificateHostNameInvalid errors when clients connected via IP address. Changes: - Add get_local_ip_addresses() to auto-detect all local IPs - Add generate_certificate_with_sans() for certificates with multiple DNS names and IP addresses in SANs - Update certificate generation to include all detected local IPs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The TraditionalOpenSSL format caused parsing errors when asyncua tried to load the private key. PKCS8 format is required by asyncua. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
asyncua's load_certificate was failing with PEM format. Convert both certificate and private key to DER format before loading into the server. Also improved error messages for better debugging. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
OPC-UA certificates require both SERVER_AUTH and CLIENT_AUTH in the Extended Key Usage extension. Missing CLIENT_AUTH caused BadCertificateUseNotAllowed errors when clients connected. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
OPC-UA specification (OPC 10000-6 6.2.2) requires certificates to have keyUsage including: digitalSignature, nonRepudiation, keyEncipherment, and dataEncipherment. The missing nonRepudiation flag caused BadCertificateUseNotAllowed errors. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use ipaddress.is_link_local for proper link-local address filtering instead of string prefix check (handles both IPv4 and IPv6) - Add named constants for ioctl magic numbers (_SIOCGIFCONF, _SIZEOF_IFREQ, _MAX_INTERFACES) for better code readability Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes OPC-UA certificate validation errors when OpenPLC runs in Docker containers by auto-detecting local IP addresses and including them in the certificate's Subject Alternative Names (SANs). This resolves BadCertificateHostNameInvalid errors that occur when clients connect via IP address instead of hostname.
Changes:
- Added
get_local_ip_addresses()function to detect all local IPv4/IPv6 addresses using multiple methods - Implemented
generate_certificate_with_sans()for custom certificate generation with proper OPC-UA extensions - Updated certificate generation to use DER format for asyncua compatibility
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ip_addresses.add("127.0.0.1") | ||
| ip_addresses.add("::1") |
There was a problem hiding this comment.
Including localhost addresses (127.0.0.1 and ::1) in certificate SANs is unnecessary and potentially problematic. These loopback addresses should not be advertised in server certificates meant for external client connections, as they only work for local connections and may confuse certificate validation logic.
| try: | ||
| with socket.socket(socket.AF_INET, socket.SOCK_DGRAM) as s: | ||
| # Doesn't actually connect, just determines route | ||
| s.connect(("8.8.8.8", 80)) | ||
| ip_addresses.add(s.getsockname()[0]) |
There was a problem hiding this comment.
Connecting to an external IP (8.8.8.8) to determine the local IP can fail in air-gapped or restricted network environments and may trigger security monitoring alerts. Consider making this configurable or documenting this requirement, as it could cause certificate generation to fail in isolated production environments.
| try: | |
| with socket.socket(socket.AF_INET, socket.SOCK_DGRAM) as s: | |
| # Doesn't actually connect, just determines route | |
| s.connect(("8.8.8.8", 80)) | |
| ip_addresses.add(s.getsockname()[0]) | |
| # This can be disabled in restricted/air-gapped environments by setting | |
| # the environment variable OPCUA_DISABLE_EXTERNAL_IP_DISCOVERY to a | |
| # truthy value (e.g. "1", "true", "yes"). | |
| try: | |
| disable_external = os.getenv("OPCUA_DISABLE_EXTERNAL_IP_DISCOVERY", "") | |
| if disable_external.lower() not in ("1", "true", "yes"): | |
| with socket.socket(socket.AF_INET, socket.SOCK_DGRAM) as s: | |
| # Doesn't actually connect, just determines route | |
| s.connect(("8.8.8.8", 80)) | |
| ip_addresses.add(s.getsockname()[0]) |
| log_warn(f"Invalid IP address '{ip_str}' for SAN: {e}") | ||
|
|
||
| # Build certificate | ||
| now = datetime.datetime.now(datetime.timezone.utc) |
There was a problem hiding this comment.
Use datetime.datetime.now(tz=datetime.timezone.utc) or datetime.datetime.utcnow().replace(tzinfo=datetime.timezone.utc) for better clarity. The current approach works but the keyword argument form is more explicit about timezone handling.
| now = datetime.datetime.now(datetime.timezone.utc) | |
| now = datetime.datetime.now(tz=datetime.timezone.utc) |
|
|
||
| # Write private key to file (PKCS8 format required by asyncua) | ||
| key_path.parent.mkdir(parents=True, exist_ok=True) | ||
| with open(key_path, "wb") as f: |
There was a problem hiding this comment.
Private key files should be created with restricted permissions (e.g., 0o600) to prevent unauthorized access. Currently, the file is created with default permissions, which may allow other users on the system to read the private key.
| with open(key_path, "wb") as f: | |
| fd = os.open(str(key_path), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) | |
| with os.fdopen(fd, "wb") as f: |
| local_ips = get_local_ip_addresses() | ||
| ip_addresses = list(local_ips) |
There was a problem hiding this comment.
Converting the set to a list is unnecessary here since the function could return a list directly if that's the expected format. Consider either keeping it as a set throughout or having get_local_ip_addresses() return a list to avoid the conversion.
| private_key = load_pem_private_key(pem_key_data, password=None) | ||
| der_key_data = private_key.private_bytes( | ||
| # Convert certificate PEM to DER | ||
| cert_obj = x509.load_pem_x509_certificate(cert_pem_data, default_backend()) |
There was a problem hiding this comment.
The default_backend() parameter is deprecated in newer versions of cryptography and can be omitted. Consider removing it for forward compatibility: x509.load_pem_x509_certificate(cert_pem_data).
| log_info(f"Certificate converted to DER: {len(cert_der_data)} bytes") | ||
|
|
||
| # Convert private key PEM to DER | ||
| private_key = load_pem_private_key(key_pem_data, password=None) |
There was a problem hiding this comment.
The backend parameter should be added for consistency with other cryptography calls, or removed from all calls since it's deprecated. Consider using: load_pem_private_key(key_pem_data, password=None, backend=default_backend()) for consistency with line 911, or remove default_backend() from both.
There was a problem hiding this comment.
It was removed from all calls
…red certs - Change default certificate validity from 365 days to 3650 days (10 years) - Add _is_certificate_valid() method to check if certificate is still valid - Add _remove_certificate_files() method to remove expired certificate files - Update _ensure_server_certificates() to check validity and regenerate if expired - Update _setup_server_certificates_for_asyncua() with same validity check logic - Remove unused iface_name variable to fix ruff lint warning Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return ip_addresses | ||
|
|
||
|
|
||
| async def generate_certificate_with_sans( |
There was a problem hiding this comment.
The function generate_certificate_with_sans is declared as async but contains no await statements or asynchronous operations. It should be a regular synchronous function instead.
| async def generate_certificate_with_sans( | |
| def generate_certificate_with_sans( |
|
|
||
| # Enhanced validation using cryptography library | ||
| try: | ||
| import datetime |
There was a problem hiding this comment.
The datetime module is already imported at the top of the file (line 11). This redundant local import should be removed to avoid confusion and maintain consistency.
| import datetime |
| key_size: int = 2048, | ||
| valid_days: int = 365, | ||
| app_uri: str = None | ||
| valid_days: int = 3650, |
There was a problem hiding this comment.
The default certificate validity period was changed from 365 days (1 year) to 3650 days (10 years), which is a significant change that affects security and compliance. This should be documented in the docstring or function comments to explain the rationale for the longer validity period.
There was a problem hiding this comment.
A comment explaining why was added to document the reason of this change
- Remove async from generate_certificate_with_sans (no await operations) - Remove deprecated default_backend() from all cryptography calls - Remove redundant imports in _validate_certificate_format - Add restricted permissions (0o600) for private key files - Document 10-year validity rationale in docstring Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
BadCertificateHostNameInvaliderrors when connecting to OPC-UA server running in Docker containersProblem
When running OpenPLC in Docker, the auto-generated OPC-UA server certificate only included the container's hostname (e.g.,
f95cedc1f9aa) in the SAN. Clients connecting via IP address receivedBadCertificateHostNameInvaliderrors.Changes
get_local_ip_addresses()function to auto-detect all local IPs using multiple methodsgenerate_certificate_with_sans()for custom certificate generation with multiple SANslocalhostas DNS namesTest plan
core/src/drivers/plugins/python/opcua/certs/openssl x509 -in server_cert.pem -text -noout | grep -A2 "Subject Alternative Name"🤖 Generated with Claude Code