Conversation
akremenetsky
commented
Mar 4, 2026
akremenetsky
commented
- Bootstrap process based on initial data
- Core manifest with core set
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust, data-driven bootstrapping mechanism for the system. By replacing static configuration files with Jinja2 templates and processing initial setup data from a Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
- Bootstrap process based on initial data - Core manifest with core set Signed-off-by: Anton Kremenetsky <anton.kremenetsky@gmail.com>
akremenetsky
force-pushed
the
feat/223/bootstrap-on-spec
branch
from
33fe593 to
1ec2003
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a bootstrap process based on initial data, including a core manifest with a core set, adding new configuration files, modifying existing scripts, and incorporating templating for dynamic configuration to streamline the initial setup and configuration of the Genesis Core environment. However, this implementation introduces several security vulnerabilities. The installation script handles passwords insecurely by using a predictable temporary file in /tmp, and the template rendering logic is susceptible to configuration injection because it lacks validation for external input from the spec.json file. Addressing these issues is critical to ensure the integrity and security of the bootstrap process.
I am having trouble creating individual review comments. Click here to see my feedback.
genesis_core/cmd/bootstrap_templates.py (135)
Data read from /mnt/cdrom/spec.json is rendered into system configuration files (e.g., dnsdist, netplan) using Jinja2 without any validation or escaping. This allows for configuration injection if the spec.json file is compromised or contains malicious values. For example, a malicious IP address could inject Lua code into the dnsdist configuration or manipulate the netplan YAML structure, potentially leading to unauthorized access or command execution.
Remediation:
Validate all inputs from spec.json against expected formats (e.g., using regular expressions for IP and MAC addresses) before passing them to the template engine.
genesis/images/install.sh (250-258)
The script uses a predictable temporary file /tmp/__passwd to set the user password. This is vulnerable to symlink attacks, where an attacker could create a symbolic link at this location to overwrite arbitrary files. Additionally, the password is briefly exposed in a world-readable directory. Furthermore, the script defaults to a weak password ('ubuntu') if GEN_USER_PASSWD is not set.
Remediation:
- Avoid using weak default passwords.
- Use
mktempto create a secure temporary file or, preferably, pipe the password directly tochpasswd:printf "%s\n" "ubuntu:$PASSWD" | sudo chpasswd.
genesis/images/bootstrap.sh (142-145)
The if statement checking for systemctl is no longer needed since systemctl is now assumed to be present. Removing this check simplifies the code and reduces unnecessary conditional logic. This can be simplified.
log "running: systemctl daemon-reload"
systemctl daemon-reload || truelog "running: systemctl daemon-reload"
systemctl daemon-reload || true
genesis/images/bootstrap.sh (156-159)
The if statement checking for systemctl is no longer needed since systemctl is now assumed to be present. Removing this check simplifies the code and reduces unnecessary conditional logic. This can be simplified.
log "running: systemctl daemon-reload"
systemctl daemon-reload || truelog "running: systemctl daemon-reload"
systemctl daemon-reload || true
genesis/images/bootstrap.sh (245-247)
The if statement checking for systemctl is no longer needed since systemctl is now assumed to be present. Removing this check simplifies the code and reduces unnecessary conditional logic. This can be simplified.
systemctl stop postgresql || truesystemctl stop postgresql || true
genesis/images/bootstrap.sh (254-257)
The if statement checking for command -v rsync is no longer needed since rsync is now assumed to be present. Instead of using cp, a retry mechanism is implemented to handle potential rsync failures, improving the robustness of the data copying process.
MAX_RETRIES=5
RETRY_COUNT=0
while [[ ${RETRY_COUNT} -lt ${MAX_RETRIES} ]]; do
if rsync -aHAX --numeric-ids "${OLD_PGDATA}/" "${NEW_PGDATA}/" ; then
log "rsync completed successfully"
break
else
RETRY_COUNT=$((RETRY_COUNT + 1))
if [[ ${RETRY_COUNT} -lt ${MAX_RETRIES} ]]; then
log "rsync failed, retrying in 0.5s (attempt ${RETRY_COUNT}/${MAX_RETRIES})"
sleep 0.5
else
log "ERROR: rsync failed after ${MAX_RETRIES} attempts"
exit 1
fi
fi
doneMAX_RETRIES=5
RETRY_COUNT=0
while [[ ${RETRY_COUNT} -lt ${MAX_RETRIES} ]]; do
if rsync -aHAX --numeric-ids "${OLD_PGDATA}/" "${NEW_PGDATA}/" ; then
log "rsync completed successfully"
break
else
RETRY_COUNT=$((RETRY_COUNT + 1))
if [[ ${RETRY_COUNT} -lt ${MAX_RETRIES} ]]; then
log "rsync failed, retrying in 0.5s (attempt ${RETRY_COUNT}/${MAX_RETRIES})"
sleep 0.5
else
log "ERROR: rsync failed after ${MAX_RETRIES} attempts"
exit 1
fi
fi
done
genesis/images/bootstrap.sh (270-273)
The if statement checking for systemctl is no longer needed since systemctl is now assumed to be present. Removing this check simplifies the code and reduces unnecessary conditional logic. This can be simplified.
systemctl daemon-reload || true
systemctl start postgresql || truesystemctl daemon-reload || true
systemctl start postgresql || true
genesis/images/bootstrap.sh (278-280)
The if statement checking for systemctl is no longer needed since systemctl is now assumed to be present. Removing this check simplifies the code and reduces unnecessary conditional logic. This can be simplified.
systemctl start postgresql || truesystemctl start postgresql || true
genesis/images/install.sh (165)
Moving the logging.yaml file is good, but the directory should be created first to avoid errors if it doesn't exist.
sudo mkdir -p /etc/genesis_universal_agent
sudo cp "$GC_PATH/etc/genesis_universal_agent/logging.yaml" /etc/genesis_universal_agent/sudo mkdir -p /etc/genesis_universal_agent
sudo cp "$GC_PATH/etc/genesis_universal_agent/logging.yaml" /etc/genesis_universal_agent/
1 participant
