feat: support cayo/snowdrift and root password #22
Conversation
Signed-off-by: Brian Ketelsen <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR adds support for two new server-focused Linux images (Cayo and Snowdrift) and implements root password collection functionality specifically for these server images. The changes span the UI layer, business logic, and installation script to enable conditional password entry and secure password handling during installation.
- Adds "Cayo Server" and "Snowdrift Server" image definitions to the available installation options
- Implements conditional password fields in the confirmation UI that appear only for server images
- Adds secure temporary file handling to pass root passwords to the installation script
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| snow_first_setup/images.json | Adds two new server image definitions (snowdrift and cayo) with display names and targets |
| snow_first_setup/gtk/install-confirm.ui | Adds password entry UI elements (PreferencesGroup with two PasswordEntryRow widgets) |
| snow_first_setup/views/install_confirm.py | Implements password validation logic, visibility control based on image selection, and stores password in window state |
| snow_first_setup/views/install_progress.py | Creates temporary file for password with restricted permissions, passes to install script, and cleans up afterwards |
| snow_first_setup/scripts/install-to-disk | Extends script parameters to accept root password file path and passes it to nbc installer |
| def __update_password_visibility(self): | ||
| """Show password fields only for cayo or snowdrift images""" | ||
| try: | ||
| # Check if the target image is cayo or snowdrift | ||
| needs_root_password = False | ||
| if self.__image_target: | ||
| # Check both the target string and the display text | ||
| target_lower = self.__image_target.lower() | ||
| text_lower = self.__image_text.lower() | ||
| needs_root_password = 'cayo' in target_lower or 'snowdrift' in target_lower or 'cayo' in text_lower or 'snowdrift' in text_lower |
There was a problem hiding this comment.
The list of images requiring root passwords ("cayo", "snowdrift") is hardcoded in the visibility check logic. If more server images are added in the future, this code will need to be updated. Consider defining this list as a constant at the module or class level, or better yet, adding a property to the image definitions in images.json to indicate whether root password is required.
| # Clean up root password file if created | ||
| if self.__root_password_file and os.path.exists(self.__root_password_file): | ||
| try: | ||
| os.unlink(self.__root_password_file) | ||
| print(f"[DEBUG] Cleaned up root password file: {self.__root_password_file}") | ||
| self.__root_password_file = None | ||
| except Exception as e: | ||
| print(f"[WARNING] Failed to clean up root password file: {e}") |
There was a problem hiding this comment.
The password file cleanup happens only in the success path after run_stream_script returns. If an exception occurs during run_stream_script or if the installation is interrupted, the temporary password file will not be cleaned up. Consider wrapping the script execution in a try-finally block to guarantee cleanup regardless of the outcome.
| passwords_valid = ( | ||
| self.__root_password and | ||
| self.__root_password_confirm and | ||
| self.__root_password == self.__root_password_confirm | ||
| ) | ||
| ok = ok and passwords_valid |
There was a problem hiding this comment.
The password validation only checks that passwords are non-empty and match, but doesn't enforce any password strength requirements. For a root password, consider adding minimum length requirements or warning users about weak passwords, especially since this is for server images where security is critical.
| except Exception: | ||
| self.__root_password = "" | ||
| self.__root_password_confirm = "" |
There was a problem hiding this comment.
The exception handler swallows all exceptions and prints a debug message, then continues with empty password strings. This could mask serious errors like GTK widget access failures. Consider logging the exception type and stack trace, or re-raising critical exceptions that indicate widget initialization problems.
| if self.root_password_group.get_visible() and self.__root_password: | ||
| self.__window.install_root_password = self.__root_password |
There was a problem hiding this comment.
The root password is stored directly in the window object without any cleanup mechanism. Once set, the password remains in memory as plain text until the application exits. Consider implementing a cleanup method that clears this sensitive data after the installation completes, or use a more secure approach like clearing it immediately after passing to the install script.
| self.root_password_confirm_entry.set_text("") | ||
| self.__root_password = "" | ||
| self.__root_password_confirm = "" | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Brian Ketelsen [email protected]