VM: make reference to interface instead of vhost#2013
Conversation
This requires users to update their VM settings and use the interface name instead of the vhost name as network source. This brings VMs inline with Docker which also shows the interface name for custom networks
WalkthroughThe changes enhance the detection and handling of network interfaces within the VM manager plugin. In Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)
1247-1258: Consider removing commented out code.This section is commented out to prevent breaking VMSettings.page when libvirt is not running. If this code is no longer needed, consider removing it entirely rather than leaving it commented out.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php(1 hunks)emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php(1 hunks)
🔇 Additional comments (2)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (1)
634-634: LGTM! The regex pattern update aligns with the PR objectives.The change extends network interface detection to include
bondandethinterfaces, which aligns with the goal of using interface names instead of vhost names for VM network configuration.Let's verify the impact of this change by checking for any network interface references in the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for network interface references in the codebase. # Search for network interface patterns in the codebase echo "Searching for network interface patterns..." rg -A 5 "((vir)?br|bond|eth|wlan)[0-9]+(\.[0-9]+)?"Length of output: 126650
Regex Update Verified: The pattern now correctly includes bond and eth interfaces.
The search results confirm that in the file
emhttp/plugins/dynamix.vm.manager/include/libvirt.phpthe new regex:'^((vir)?br|bond|eth|wlan)[0-9]+(\.[0-9]+)?'is in place to correctly detect interfaces named with the prefixes
bondandeth(in addition tovirbrandwlan). Other parts of the codebase (like Docker and network configuration files) also consistently reference interfaces such asbond0,eth0, etc., aligning with the PR objectives.
- The change extends detection to include
bondandethinterfaces for virtual machine network configuration.- It aligns with the project's goal of using interface names rather than vhost names.
- No inconsistencies were found in related files where this regex might impact behavior.
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)
1230-1246: Improved network interface detection and filtering logic.The changes enhance the network interface detection by:
- Including bond and eth interfaces in addition to bridges
- Filtering out redundant interfaces that are part of bonds or bridges
- Providing a cleaner, more accurate list of available network interfaces
This aligns well with the PR objective of using interface names instead of vhost names.
When user name and password are stored in plain text, these will be encrypted at start up.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
emhttp/plugins/dynamix/scripts/open_ssl (1)
1-29: 🛠️ Refactor suggestionConsider adding input validation and rate limiting.
To prevent potential abuse, consider adding:
- Input length limits
- Rate limiting
- Input character validation
Apply this diff to add these security measures:
+// Add at the beginning of the file +define('MAX_INPUT_LENGTH', 1024); +define('RATE_LIMIT_FILE', '/tmp/ssl_operations'); +define('RATE_LIMIT_SECONDS', 60); +define('RATE_LIMIT_OPERATIONS', 100); + +function checkRateLimit() { + $operations = @file_get_contents(RATE_LIMIT_FILE) ?: '{}'; + $operations = json_decode($operations, true) ?: []; + $now = time(); + + // Clean old entries + $operations = array_filter($operations, fn($time) => $time > $now - RATE_LIMIT_SECONDS); + + // Check limit + if (count($operations) >= RATE_LIMIT_OPERATIONS) { + error_log("Rate limit exceeded"); + exit(1); + } + + // Add new operation + $operations[] = $now; + file_put_contents(RATE_LIMIT_FILE, json_encode($operations)); +} + +function validateInput($input) { + if (strlen($input) > MAX_INPUT_LENGTH) { + error_log("Input exceeds maximum length"); + return false; + } + if (!preg_match('/^[\w\s@.-]+$/', $input)) { + error_log("Input contains invalid characters"); + return false; + } + return true; +} + +// Add before the switch statement +checkRateLimit(); +if (!empty($argv[2]) && !validateInput($argv[2])) { + exit(1); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/include/update.wireless.php(1 hunks)emhttp/plugins/dynamix/scripts/open_ssl(1 hunks)etc/rc.d/rc.wireless(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- etc/rc.d/rc.wireless
| if (!empty($_POST['USERNAME'])) $_POST['USERNAME'] = exec("$open_ssl encrypt \"{$_POST['USERNAME']}\""); | ||
| if (!empty($_POST['PASSWORD'])) $_POST['PASSWORD'] = exec("$open_ssl encrypt \"{$_POST['PASSWORD']}\""); |
There was a problem hiding this comment.
Add input validation and error handling for encryption.
The current implementation has several security and reliability concerns:
- Using
execwith user input without proper validation is potentially dangerous. - No error handling for failed encryption attempts.
- No validation of the encryption results.
Apply this diff to improve security and reliability:
-if (!empty($_POST['USERNAME'])) $_POST['USERNAME'] = exec("$open_ssl encrypt \"{$_POST['USERNAME']}\"");
-if (!empty($_POST['PASSWORD'])) $_POST['PASSWORD'] = exec("$open_ssl encrypt \"{$_POST['PASSWORD']}\"");
+function safeEncrypt($value) {
+ if (empty($value)) return '';
+ // Validate input contains only allowed characters
+ if (!preg_match('/^[\w\s@.-]+$/', $value)) {
+ error_log("Invalid characters in input");
+ return '';
+ }
+ // Escape shell arguments properly
+ $escaped = escapeshellarg($value);
+ // Execute with error checking
+ $result = exec("$GLOBALS[open_ssl] encrypt $escaped 2>&1", $output, $returnCode);
+ if ($returnCode !== 0) {
+ error_log("Encryption failed: " . implode("\n", $output));
+ return '';
+ }
+ return $result;
+}
+
+$_POST['USERNAME'] = safeEncrypt($_POST['USERNAME']);
+$_POST['PASSWORD'] = safeEncrypt($_POST['PASSWORD']);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!empty($_POST['USERNAME'])) $_POST['USERNAME'] = exec("$open_ssl encrypt \"{$_POST['USERNAME']}\""); | |
| if (!empty($_POST['PASSWORD'])) $_POST['PASSWORD'] = exec("$open_ssl encrypt \"{$_POST['PASSWORD']}\""); | |
| function safeEncrypt($value) { | |
| if (empty($value)) return ''; | |
| // Validate input contains only allowed characters | |
| if (!preg_match('/^[\w\s@.-]+$/', $value)) { | |
| error_log("Invalid characters in input"); | |
| return ''; | |
| } | |
| // Escape shell arguments properly | |
| $escaped = escapeshellarg($value); | |
| // Execute with error checking | |
| $result = exec("$GLOBALS[open_ssl] encrypt $escaped 2>&1", $output, $returnCode); | |
| if ($returnCode !== 0) { | |
| error_log("Encryption failed: " . implode("\n", $output)); | |
| return ''; | |
| } | |
| return $result; | |
| } | |
| $_POST['USERNAME'] = safeEncrypt($_POST['USERNAME']); | |
| $_POST['PASSWORD'] = safeEncrypt($_POST['PASSWORD']); |
| switch ($argv[1]) { | ||
| case 'encrypt': | ||
| if (!empty($argv[2]) && isset($cipher,$key,$iv)) $encrypt = openssl_encrypt($argv[2],$cipher,$key,0,$iv); | ||
| if (!empty($encrypt)) echo "$encrypt"; | ||
| break; | ||
| case 'decrypt': | ||
| if (!empty($argv[2]) && isset($cipher,$key,$iv)) $decrypt = openssl_decrypt($argv[2],$cipher,$key,0,$iv); | ||
| if (!empty($decrypt)) echo "$decrypt"; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Improve error handling and add logging.
The current implementation silently fails and doesn't provide feedback for troubleshooting.
Apply this diff to add proper error handling:
switch ($argv[1]) {
case 'encrypt':
- if (!empty($argv[2]) && isset($cipher,$key,$iv)) $encrypt = openssl_encrypt($argv[2],$cipher,$key,0,$iv);
- if (!empty($encrypt)) echo "$encrypt";
+ if (empty($argv[2])) {
+ error_log("No input provided for encryption");
+ exit(1);
+ }
+ $encrypt = openssl_encrypt($argv[2], $cipher, $key, 0, $iv);
+ if ($encrypt === false) {
+ error_log("Encryption failed: " . openssl_error_string());
+ exit(1);
+ }
+ echo $encrypt;
break;
case 'decrypt':
- if (!empty($argv[2]) && isset($cipher,$key,$iv)) $decrypt = openssl_decrypt($argv[2],$cipher,$key,0,$iv);
- if (!empty($decrypt)) echo "$decrypt";
+ if (empty($argv[2])) {
+ error_log("No input provided for decryption");
+ exit(1);
+ }
+ $decrypt = openssl_decrypt($argv[2], $cipher, $key, 0, $iv);
+ if ($decrypt === false) {
+ error_log("Decryption failed: " . openssl_error_string());
+ exit(1);
+ }
+ echo $decrypt;
break;
+default:
+ error_log("Invalid command. Use 'encrypt' or 'decrypt'");
+ exit(1);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch ($argv[1]) { | |
| case 'encrypt': | |
| if (!empty($argv[2]) && isset($cipher,$key,$iv)) $encrypt = openssl_encrypt($argv[2],$cipher,$key,0,$iv); | |
| if (!empty($encrypt)) echo "$encrypt"; | |
| break; | |
| case 'decrypt': | |
| if (!empty($argv[2]) && isset($cipher,$key,$iv)) $decrypt = openssl_decrypt($argv[2],$cipher,$key,0,$iv); | |
| if (!empty($decrypt)) echo "$decrypt"; | |
| break; | |
| } | |
| switch ($argv[1]) { | |
| case 'encrypt': | |
| if (empty($argv[2])) { | |
| error_log("No input provided for encryption"); | |
| exit(1); | |
| } | |
| $encrypt = openssl_encrypt($argv[2], $cipher, $key, 0, $iv); | |
| if ($encrypt === false) { | |
| error_log("Encryption failed: " . openssl_error_string()); | |
| exit(1); | |
| } | |
| echo $encrypt; | |
| break; | |
| case 'decrypt': | |
| if (empty($argv[2])) { | |
| error_log("No input provided for decryption"); | |
| exit(1); | |
| } | |
| $decrypt = openssl_decrypt($argv[2], $cipher, $key, 0, $iv); | |
| if ($decrypt === false) { | |
| error_log("Decryption failed: " . openssl_error_string()); | |
| exit(1); | |
| } | |
| echo $decrypt; | |
| break; | |
| default: | |
| error_log("Invalid command. Use 'encrypt' or 'decrypt'"); | |
| exit(1); | |
| } |
| $ssl_input = '/etc/rc.d/rc.ssl.input'; | ||
| if (is_readable($ssl_input)) extract(parse_ini_file($ssl_input)); |
There was a problem hiding this comment.
Validate SSL configuration file contents.
The current implementation doesn't validate the INI file contents, which could lead to security issues if the file is corrupted or tampered with.
Apply this diff to add validation:
-if (is_readable($ssl_input)) extract(parse_ini_file($ssl_input));
+function validateSSLConfig($config) {
+ $required = ['cipher', 'key', 'iv'];
+ foreach ($required as $param) {
+ if (empty($config[$param])) {
+ error_log("Missing required SSL parameter: $param");
+ return false;
+ }
+ }
+ // Validate cipher is supported
+ if (!in_array($config['cipher'], openssl_get_cipher_methods())) {
+ error_log("Unsupported cipher method: {$config['cipher']}");
+ return false;
+ }
+ return true;
+}
+
+$config = [];
+if (is_readable($ssl_input)) {
+ $config = parse_ini_file($ssl_input);
+ if (!$config || !validateSSLConfig($config)) {
+ error_log("Invalid SSL configuration");
+ exit(1);
+ }
+ extract($config);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $ssl_input = '/etc/rc.d/rc.ssl.input'; | |
| if (is_readable($ssl_input)) extract(parse_ini_file($ssl_input)); | |
| $ssl_input = '/etc/rc.d/rc.ssl.input'; | |
| function validateSSLConfig($config) { | |
| $required = ['cipher', 'key', 'iv']; | |
| foreach ($required as $param) { | |
| if (empty($config[$param])) { | |
| error_log("Missing required SSL parameter: $param"); | |
| return false; | |
| } | |
| } | |
| // Validate cipher is supported | |
| if (!in_array($config['cipher'], openssl_get_cipher_methods())) { | |
| error_log("Unsupported cipher method: {$config['cipher']}"); | |
| return false; | |
| } | |
| return true; | |
| } | |
| $config = []; | |
| if (is_readable($ssl_input)) { | |
| $config = parse_ini_file($ssl_input); | |
| if (!$config || !validateSSLConfig($config)) { | |
| error_log("Invalid SSL configuration"); | |
| exit(1); | |
| } | |
| extract($config); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
emhttp/plugins/dynamix/scripts/open_ssl (2)
15-16:⚠️ Potential issueImprove SSL configuration handling.
The current implementation doesn't validate the INI file contents, which could lead to security issues if the file is corrupted or tampered with.
Apply this diff to add validation:
-if (is_readable($ssl_input)) extract(parse_ini_file($ssl_input)); +function validateSSLConfig($config) { + $required = ['cipher', 'key', 'iv']; + foreach ($required as $param) { + if (empty($config[$param])) { + error_log("Missing required SSL parameter: $param"); + return false; + } + } + // Validate cipher is supported + if (!in_array($config['cipher'], openssl_get_cipher_methods())) { + error_log("Unsupported cipher method: {$config['cipher']}"); + return false; + } + return true; +} + +$config = []; +if (is_readable($ssl_input)) { + $config = parse_ini_file($ssl_input); + if (!$config || !validateSSLConfig($config)) { + error_log("Invalid SSL configuration"); + exit(1); + } + extract($config); +}
27-33:⚠️ Potential issueImprove error handling in encryption/decryption operations.
The current implementation silently fails and doesn't provide feedback for troubleshooting.
Apply this diff to add proper error handling:
case 'encrypt': - if (!empty($argv[2]) && isset($cipher,$key,$iv)) $encrypt = openssl_encrypt($argv[2],$cipher,$key,0,$iv); - if (!empty($encrypt)) echo "$encrypt"; + if (empty($argv[2])) { + error_log("No input provided for encryption"); + exit(1); + } + $encrypt = openssl_encrypt($argv[2], $cipher, $key, 0, $iv); + if ($encrypt === false) { + error_log("Encryption failed: " . openssl_error_string()); + exit(1); + } + echo $encrypt; break; case 'decrypt': - if (!empty($argv[2]) && isset($cipher,$key,$iv)) $decrypt = openssl_decrypt($argv[2],$cipher,$key,0,$iv); - if (!empty($decrypt)) echo "$decrypt"; + if (empty($argv[2])) { + error_log("No input provided for decryption"); + exit(1); + } + $decrypt = openssl_decrypt($argv[2], $cipher, $key, 0, $iv); + if ($decrypt === false) { + error_log("Decryption failed: " . openssl_error_string()); + exit(1); + } + echo $decrypt;
🧹 Nitpick comments (1)
etc/rc.d/rc.wireless (1)
13-13: Use a configurable path for the OpenSSL script.The path to the OpenSSL script is hardcoded, which could cause issues if the installation directory changes.
Apply this diff to make the path configurable:
-OPENSSL="/usr/local/emhttp/webGui/scripts/open_ssl" +: ${OPENSSL:="/usr/local/emhttp/webGui/scripts/open_ssl"}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/scripts/open_ssl(1 hunks)etc/rc.d/rc.wireless(4 hunks)
🔇 Additional comments (2)
etc/rc.d/rc.wireless (2)
210-210: Remove redundant IP address flush.The IP address flush is redundant since
ipaddr_downis already called earlier to handle IP address cleanup for both IPv4 and IPv6.Remove the redundant line:
- run ip addr flush dev $PORT
226-240: LGTM! Improved credential handling.The changes improve security by:
- Encrypting plain credentials before storing
- Using the OpenSSL script for encryption/decryption
- Properly handling both encrypted and plain credentials
| $key = exec("dmidecode -qt1 | grep -Pom1 'Manufacturer: \K.+' | sed 's/ /_/g'")."ABCDEFGH"; | ||
| $iv = "12".exec("cat /sys/class/net/wlan0/address | sed 's/://g'")."34"; | ||
| file_put_contents($ssl_input, "cipher=aes-256-cbc\nkey=".substr($key,0,63)."\niv=$iv\n"); |
There was a problem hiding this comment.
Improve key generation security.
The current key generation has several security concerns:
- Uses predictable system information for key generation
- Hardcoded values in the key and IV
- Fixed interface name 'wlan0' which may not exist
Apply this diff to improve security:
- $key = exec("dmidecode -qt1 | grep -Pom1 'Manufacturer: \K.+' | sed 's/ /_/g'")."ABCDEFGH";
- $iv = "12".exec("cat /sys/class/net/wlan0/address | sed 's/://g'")."34";
- file_put_contents($ssl_input, "cipher=aes-256-cbc\nkey=".substr($key,0,63)."\niv=$iv\n");
+ $key = bin2hex(random_bytes(32)); // Generate a secure random key
+ $iv = bin2hex(random_bytes(16)); // Generate a secure random IV
+ file_put_contents($ssl_input, "cipher=aes-256-gcm\nkey=$key\niv=$iv\n", LOCK_EX);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $key = exec("dmidecode -qt1 | grep -Pom1 'Manufacturer: \K.+' | sed 's/ /_/g'")."ABCDEFGH"; | |
| $iv = "12".exec("cat /sys/class/net/wlan0/address | sed 's/://g'")."34"; | |
| file_put_contents($ssl_input, "cipher=aes-256-cbc\nkey=".substr($key,0,63)."\niv=$iv\n"); | |
| $key = bin2hex(random_bytes(32)); // Generate a secure random key | |
| $iv = bin2hex(random_bytes(16)); // Generate a secure random IV | |
| file_put_contents($ssl_input, "cipher=aes-256-gcm\nkey=$key\niv=$iv\n", LOCK_EX); |
2 participants
This requires users to update their VM settings and use the interface name instead of the vhost name as network source.
This brings VMs inline with Docker which also shows the interface name for custom networks
Summary by CodeRabbit
New Features
Bug Fixes