Force inject basilica branding on rentals SSH and update with new cli…#378
Open
Xayaan wants to merge 4 commits intoone-covenant:mainfrom
Open
Force inject basilica branding on rentals SSH and update with new cli…#378Xayaan wants to merge 4 commits intoone-covenant:mainfrom
Xayaan wants to merge 4 commits intoone-covenant:mainfrom
Conversation
Contributor
WalkthroughIntroduces an internal Changes
Sequence DiagramsequenceDiagram
participant DM as DeploymentManager
participant FS as FileSystem
participant Shell as ShellStartup
participant SSH as SSHService
DM->>FS: write BASILICA_SSH_LOGIN_BANNER to /etc/issue.net and /etc/motd
DM->>FS: create /etc/profile.d/basilica-branding.sh
DM->>Shell: ensure shell startup files source branding script
DM->>FS: ensure /root/.ssh exists and add authorized_keys
DM->>SSH: edit sshd_config (Banner/PrintMotd) via sed/here-doc
DM->>SSH: start/restart (systemctl -> service -> direct sshd) with logging
SSH-->>DM: return success/failure (logged)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/basilica-validator/src/rental/deployment.rs (2)
613-623: Subtle interaction:.hushloginsuppresses/etc/motddisplay, making the/etc/motdwrite a no-op in most scenarios.With
PrintMotd noin sshd_config, PAM motd disabled, and.hushloginpresent, the/etc/motdcontent will never be displayed through any standard path. The branding actually reaches the user only via theBanner /etc/issue.netdirective and theprofile.dscript. Writing/etc/motdis harmless defense-in-depth, but worth documenting so future readers understand the layering.📝 Consider adding a brief comment
let set_login_banner = format!( r#"docker exec {container_id} sh -c "cat > /etc/issue.net <<'BASILICA_EOF' {banner} BASILICA_EOF +# /etc/motd is a fallback; primary branding uses Banner /etc/issue.net + profile.d script. cat /etc/issue.net > /etc/motd touch /root/.hushlogin""#, banner = BASILICA_SSH_LOGIN_BANNER );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/rental/deployment.rs` around lines 613 - 623, Add an inline comment above the set_login_banner block (where BASILICA_SSH_LOGIN_BANNER is written and client.execute_ssh_command is called) explaining that creating /root/.hushlogin suppresses PAM/sshd MOTD so writing /etc/motd will usually be a no-op; note that the visible branding is delivered via Banner /etc/issue.net and the profile.d script and that the /etc/motd write is kept as harmless defense‑in‑depth for future readers and maintainers.
655-671: PAMsedwill double-prefix on repeated provisioning runs.Line 663:
sed -i '/pam_motd\\.so/s/^/# basilica-disabled: /'matches any line containingpam_motd.so, including already-commented lines. On a second run it becomes# basilica-disabled: # basilica-disabled: …. Harmless but messy.🧹 Anchor to non-commented lines only
- sed -i '/pam_motd\\.so/s/^/# basilica-disabled: /' /etc/pam.d/sshd 2>/dev/null || true + sed -i '/^[^#]*pam_motd\\.so/s/^/# basilica-disabled: /' /etc/pam.d/sshd 2>/dev/null || trueThis limits the substitution to lines that aren't already commented out.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/rental/deployment.rs` around lines 655 - 671, The sed substitution in the enforce_shell_branding multi-line string currently matches any line containing pam_motd.so and will prepend the marker even for already-commented lines; update the sed expression inside the enforce_shell_branding string (the fragment used in client.execute_ssh_command) to only target non-commented lines (for example restrict the address to lines that do not start with optional whitespace followed by '#' and then contain pam_motd\.so, e.g. use an address like '/^[[:space:]]*[^#].*pam_motd\\.so/ s/^/# basilica-disabled: /') so repeated provisioning won't double-prefix commented lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/basilica-validator/src/rental/deployment.rs`:
- Around line 598-607: The appended Banner and PrintMotd lines in the config_ssh
command will be ignored if the base image already defines those directives, so
update the config_ssh construction (used by client.execute_ssh_command) to first
comment out any existing occurrences of Banner and PrintMotd in
/etc/ssh/sshd_config (and optionally PermitRootLogin, PubkeyAuthentication,
PasswordAuthentication) and then append the desired directives; locate the
config_ssh string in deployment.rs and replace the simple echo-append sequence
with a shell sequence that uses sed/awk to prefix matching lines with a comment
character before echoing the new settings, ensuring the new directives take
effect.
---
Nitpick comments:
In `@crates/basilica-validator/src/rental/deployment.rs`:
- Around line 613-623: Add an inline comment above the set_login_banner block
(where BASILICA_SSH_LOGIN_BANNER is written and client.execute_ssh_command is
called) explaining that creating /root/.hushlogin suppresses PAM/sshd MOTD so
writing /etc/motd will usually be a no-op; note that the visible branding is
delivered via Banner /etc/issue.net and the profile.d script and that the
/etc/motd write is kept as harmless defense‑in‑depth for future readers and
maintainers.
- Around line 655-671: The sed substitution in the enforce_shell_branding
multi-line string currently matches any line containing pam_motd.so and will
prepend the marker even for already-commented lines; update the sed expression
inside the enforce_shell_branding string (the fragment used in
client.execute_ssh_command) to only target non-commented lines (for example
restrict the address to lines that do not start with optional whitespace
followed by '#' and then contain pam_motd\.so, e.g. use an address like
'/^[[:space:]]*[^#].*pam_motd\\.so/ s/^/# basilica-disabled: /') so repeated
provisioning won't double-prefix commented lines.
epappas
requested changes
Feb 20, 2026
Member
There was a problem hiding this comment.
Going through this; the branding goal is fine but the implementation disables way more than it should. Requesting changes.
Remove
touch /root/.hushlogin-- Suppresses "Last login from X.X.X.X". That's a security indicator, not branding.clearin the profile.d script -- Don't wipe the terminal, just print the logo.- PAM sed (
sed ... pam_motd.so ...) --PrintMotd noalready handles MOTD suppression at the sshd level. No need to reach into PAM. Also double-prefixes on re-provisioning. chmod -x /etc/update-motd.d/*-- Blanket-killing dynamic MOTD scripts is overkill, same reason.- RC file injection loop -- Appending source lines into 7 shell startup files is unnecessary. Set the profile.d script to
0755instead of0644and let the standard profile.d mechanism handle it. Thezz-prefix gaming isn't needed either if we're not clearing the screen.
Keep
/etc/issue.net+Banner /etc/issue.net+PrintMotd no-- this is the actual branding- Error handling improvements -- good
- SSH restart fallback chain -- correct
Implementation
Two things, that's it:
- Write banner to
/etc/issue.net, configure sshd (comment out existing directives first since OpenSSH is first-match-wins):
sed -i 's/^\s*Banner\b/# basilica-override: &/; s/^\s*PrintMotd\b/# basilica-override: &/' /etc/ssh/sshd_config 2>/dev/null- Standard profile.d script at
0755for interactive shells:
cat > /etc/profile.d/basilica-branding.sh <<'EOF2'
case "$-" in *i*) ;; *) return 0 2>/dev/null || exit 0 ;; esac
[ -n "$BASILICA_LOGO_SHOWN" ] && return 0 2>/dev/null
export BASILICA_LOGO_SHOWN=1
cat /etc/issue.net
EOF2
chmod 0755 /etc/profile.d/basilica-branding.shUsers get the logo, and they still see last-login info and security notices.
Author
|
Done as requested. cargo fmt --all -- --check passes. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/basilica-validator/src/rental/deployment.rs (1)
625-634:PermitRootLogin,PubkeyAuthentication,PasswordAuthenticationstill subject to first-match-winsThe
sedat line 626 correctly comments out pre-existingBannerandPrintMotddirectives. The same first-match-wins semantics apply toPermitRootLogin,PubkeyAuthentication, andPasswordAuthentication: if the base image already setsPermitRootLogin yesorPasswordAuthentication yes, the appended values are silently ignored.♻️ Proposed fix: extend the sed to cover the remaining directives
let config_ssh = format!( r#"docker exec {container_id} sh -c "sed -i 's/^[[:space:]]*Banner[[:space:]].*/# basilica-override: &/; \ -s/^[[:space:]]*PrintMotd[[:space:]].*/# basilica-override: &/' /etc/ssh/sshd_config 2>/dev/null || true +s/^[[:space:]]*PrintMotd[[:space:]].*/# basilica-override: &/; \ +s/^[[:space:]]*PermitRootLogin[[:space:]].*/# basilica-override: &/; \ +s/^[[:space:]]*PubkeyAuthentication[[:space:]].*/# basilica-override: &/; \ +s/^[[:space:]]*PasswordAuthentication[[:space:]].*/# basilica-override: &/' /etc/ssh/sshd_config 2>/dev/null || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/rental/deployment.rs` around lines 625 - 634, The sed only comments out existing Banner and PrintMotd lines so appended sshd_config settings in the config_ssh string can be overridden; extend that sed to also match and comment out existing PermitRootLogin, PubkeyAuthentication, and PasswordAuthentication directives so the appended block takes effect. Locate the config_ssh string in deployment.rs (the variable config_ssh and its docker exec command using container_id) and add additional s/^[[:space:]]*PermitRootLogin[[:space:]].*/# basilica-override: &/; s/^[[:space:]]*PubkeyAuthentication[[:space:]].*/# basilica-override: &/; s/^[[:space:]]*PasswordAuthentication[[:space:]].*/# basilica-override: &/ to the sed invocation (keeping the existing 2>/dev/null || true and here-doc unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/basilica-validator/src/rental/deployment.rs`:
- Line 611: The here-doc creates a file named
/etc/profile.d/basilica-branding.sh but it must be
/etc/profile.d/zz-basilica-branding.sh so the branding script loads last; change
the cat target and any related references from "basilica-branding.sh" to
"zz-basilica-branding.sh" in the deployment script (the here-doc invocation that
currently reads cat > /etc/profile.d/basilica-branding.sh
<<'BASILICA_PROFILE_EOF') so the file is written with the zz- prefix.
- Around line 608-621: The heredoc string built in install_shell_branding is
being expanded by the outer double-quoted shell invocation, so escape dollar
signs so the literal variables reach the remote file; update the formatted
string in deployment.rs (the install_shell_branding value used with
client.execute_ssh_command) to replace occurrences of "$-" and
"$BASILICA_LOGO_SHOWN" with "\$-" and "\$BASILICA_LOGO_SHOWN" (and any other
$VAR instances in that outer double-quoted command) so the written
/etc/profile.d/basilica-branding.sh contains the unexpanded variables.
---
Nitpick comments:
In `@crates/basilica-validator/src/rental/deployment.rs`:
- Around line 625-634: The sed only comments out existing Banner and PrintMotd
lines so appended sshd_config settings in the config_ssh string can be
overridden; extend that sed to also match and comment out existing
PermitRootLogin, PubkeyAuthentication, and PasswordAuthentication directives so
the appended block takes effect. Locate the config_ssh string in deployment.rs
(the variable config_ssh and its docker exec command using container_id) and add
additional s/^[[:space:]]*PermitRootLogin[[:space:]].*/# basilica-override: &/;
s/^[[:space:]]*PubkeyAuthentication[[:space:]].*/# basilica-override: &/;
s/^[[:space:]]*PasswordAuthentication[[:space:]].*/# basilica-override: &/ to
the sed invocation (keeping the existing 2>/dev/null || true and here-doc
unchanged).
| // Standard profile.d script for interactive shells. | ||
| let install_shell_branding = format!( | ||
| r#"docker exec {container_id} sh -c "mkdir -p /etc/profile.d | ||
| cat > /etc/profile.d/basilica-branding.sh <<'BASILICA_PROFILE_EOF' |
Contributor
There was a problem hiding this comment.
Script name should be zz-basilica-branding.sh, not basilica-branding.sh
/etc/profile.d/ scripts are sourced in alphabetical order. The PR description explicitly specifies the zz- prefix so the branding script loads last — after provider/distro scripts that might otherwise clobber BASILICA_LOGO_SHOWN or print their own MOTD.
🔧 Proposed fix
-cat > /etc/profile.d/basilica-branding.sh <<'BASILICA_PROFILE_EOF'
+cat > /etc/profile.d/zz-basilica-branding.sh <<'BASILICA_PROFILE_EOF'
...
-chmod 0755 /etc/profile.d/basilica-branding.sh""#
+chmod 0755 /etc/profile.d/zz-basilica-branding.sh""#🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/basilica-validator/src/rental/deployment.rs` at line 611, The here-doc
creates a file named /etc/profile.d/basilica-branding.sh but it must be
/etc/profile.d/zz-basilica-branding.sh so the branding script loads last; change
the cat target and any related references from "basilica-branding.sh" to
"zz-basilica-branding.sh" in the deployment script (the here-doc invocation that
currently reads cat > /etc/profile.d/basilica-branding.sh
<<'BASILICA_PROFILE_EOF') so the file is written with the zz- prefix.
epappas
requested changes
Feb 21, 2026
Member
epappas
left a comment
epappas
left a comment
There was a problem hiding this comment.
Re-reviewed after the revision. The previous concerns (hushlogin, clear, PAM, update-motd, RC injection, 0644) are all resolved -- nice work. But there are three issues remaining before this can merge.
CRITICAL: Shell variable expansion bug -- branding script won't work
The install_shell_branding heredoc is inside sh -c "..." (double quotes). $- and $BASILICA_LOGO_SHOWN get expanded by the outer shell before the single-quoted heredoc delimiter is ever processed. Since the provisioning session is non-interactive, $- won't contain i, so the script immediately exits and the banner never displays. The dedup guard is also broken ($BASILICA_LOGO_SHOWN expands to empty).
Fix -- escape the dollar signs (backslashes pass through literally in Rust raw strings):
case \"\$-\" in *i*) ;; *) return 0 2>/dev/null || exit 0 ;; esac
[ -n \"\$BASILICA_LOGO_SHOWN\" ] && return 0 2>/dev/nullHIGH: sed only covers Banner/PrintMotd, not the security directives
The comment says "OpenSSH uses first-match-wins semantics, so comment existing directives first" but then only does it for 2 of 5 directives. If a base image has PasswordAuthentication yes or PermitRootLogin yes, the appended values are silently ignored.
Fix -- extend the sed:
sed -i 's/^[[:space:]]*\(Banner\|PrintMotd\|PermitRootLogin\|PubkeyAuthentication\|PasswordAuthentication\)[[:space:]].*/# basilica-override: &/' /etc/ssh/sshd_configMEDIUM: sshd_config failure logged at debug instead of warn
The codebase uses warn! for SSH config failures (e.g. line 141: warn!("Failed to configure SSH access...")). The new sshd_config block uses debug! which is inconsistent and underreports a security-relevant failure. Banner/profile.d are fine at debug! since they're cosmetic.
Author
|
@epappas Done, Please let me know if anything else, will fix. 👍 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/basilica-validator/src/rental/deployment.rs (1)
583-595:⚠️ Potential issue | 🟠 MajorShell injection risk:
ssh_public_keyis interpolated unsanitized into a shell command.The value of
ssh_public_keyis spliced directly into abash -c '…'string. If the key contains a single quote ('), it terminates the outer single-quoted argument tobash -c, allowing arbitrary command execution inside the container. Although well-formed SSH keys don't normally contain single quotes, this is user-supplied input flowing into a shell and should be sanitized.At minimum, strip or reject characters outside the expected SSH public key alphabet (
[A-Za-z0-9+/= @._-]), or usedocker exec … sh -cwith the key passed via stdin instead of interpolation.Suggested safer approach: pipe key via stdin
- let add_key_cmd = format!( - "docker exec {container_id} bash -c 'echo \"{ssh_public_key}\" > /root/.ssh/authorized_keys && \ - chmod 600 /root/.ssh/authorized_keys'" - ); - if let Err(e) = client.execute_ssh_command(&add_key_cmd).await { - debug!("Failed to add SSH key with bash: {}", e); - // Try without bash - let add_key_alt = format!( - "docker exec {container_id} sh -c 'echo \"{ssh_public_key}\" > /root/.ssh/authorized_keys && \ - chmod 600 /root/.ssh/authorized_keys'" - ); - client.execute_ssh_command(&add_key_alt).await?; - } + // Sanitize the public key: reject if it contains unexpected characters + if !ssh_public_key + .chars() + .all(|c| c.is_ascii_alphanumeric() || " +/=@._-\n".contains(c)) + { + return Err(anyhow::anyhow!( + "SSH public key contains unexpected characters" + )); + } + + let add_key_cmd = format!( + "docker exec {container_id} sh -c 'cat > /root/.ssh/authorized_keys && \ + chmod 600 /root/.ssh/authorized_keys' <<'BASILICA_KEY_EOF'\n{ssh_public_key}\nBASILICA_KEY_EOF" + ); + if let Err(e) = client.execute_ssh_command(&add_key_cmd).await { + debug!("Failed to add SSH key: {}", e); + return Err(e).context("Failed to install SSH public key"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/rental/deployment.rs` around lines 583 - 595, The current construction of add_key_cmd and add_key_alt interpolates ssh_public_key into a shell string (used by client.execute_ssh_command) and is vulnerable to shell injection; instead, send the key via stdin to docker (e.g., use "docker exec -i {container_id} sh -c 'cat > /root/.ssh/authorized_keys && chmod 600 /root/.ssh/authorized_keys'") and invoke client.execute_ssh_command with that command while supplying ssh_public_key on the command's stdin, and additionally validate/reject ssh_public_key against an allowed-character regex (e.g., only [A-Za-z0-9+/= @._-]) before attempting the operation; update references to add_key_cmd, add_key_alt, client.execute_ssh_command, ssh_public_key, and container_id accordingly.
🧹 Nitpick comments (1)
crates/basilica-validator/src/rental/deployment.rs (1)
642-658: All three SSH-start fallbacks can fail silently, yet Line 658 unconditionally logs success.If
systemctl,service, and directsshd -Dall fail, the function still logs"SSH access configured"and returnsOk(()). Consider logging a warning when no start method succeeds so operators can diagnose containers where SSH is unreachable.Suggested approach
if client.execute_ssh_command(&start_systemctl).await.is_err() { - // Try service command let start_service = format!( ... ); if client.execute_ssh_command(&start_service).await.is_err() { - // Try running sshd directly let start_direct = format!("docker exec -d {container_id} /usr/sbin/sshd -D"); - let _ = client.execute_ssh_command(&start_direct).await; + if let Err(e) = client.execute_ssh_command(&start_direct).await { + warn!("All SSH start methods failed for container {}: {}", container_id, e); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/rental/deployment.rs` around lines 642 - 658, The code currently attempts three fallbacks (start_systemctl, start_service, start_direct) but always logs success via info!("SSH access configured for container {}", container_id) regardless of whether any client.execute_ssh_command calls actually succeeded; change the flow to capture each execute_ssh_command result (or a boolean success flag) for start_systemctl, start_service, and start_direct (referencing the start_systemctl, start_service, start_direct variables and client.execute_ssh_command calls), and if all three fail log a warning (e.g., warn!("Failed to start SSH in container {}", container_id)) and return or propagate an Err/early return instead of the unconditional info call; otherwise only log the info message when at least one start method succeeded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/basilica-validator/src/rental/deployment.rs`:
- Around line 597-621: The current deployment only writes /etc/issue.net and a
profile.d script; extend the sequence (the blocks that build set_login_banner
and install_shell_branding and call client.execute_ssh_command) to also: write
the same BASILICA_SSH_LOGIN_BANNER content to /etc/motd, disable PAM MOTD output
(e.g., remove or comment pam_motd.so lines in /etc/pam.d/sshd or create a small
wrapper that disables PrintMotd), disable or make non-executable all
/etc/update-motd.d/* scripts (e.g., chmod -x or move them), and append small
sourcing hooks into common startup files (/etc/bash.bashrc, /etc/profile,
/etc/zsh/zshrc, and users’ ~/.bashrc/.profile) so
/etc/profile.d/basilica-branding.sh is sourced for zsh/non-interactive SSH
sessions; perform each change via client.execute_ssh_command and preserve the
existing debug/error logging pattern used for set_login_banner and
install_shell_branding.
- Line 626: The sed command that uses the BRE alternation
"\(Banner\|PrintMotd\|PermitRootLogin\|PubkeyAuthentication\|PasswordAuthentication\)"
is a GNU extension and fails on BusyBox/Alpine; edit the docker exec command
string that contains that sed invocation (the literal r#"docker exec
{container_id} sh -c "sed -i
's/^[[:space:]]*\(Banner\|PrintMotd\|PermitRootLogin\|PubkeyAuthentication\|PasswordAuthentication\)[[:space:]].*/#
basilica-override: &/' /etc/ssh/sshd_config 2>/dev/null || true) and replace the
single alternation-based substitution with multiple portable -e expressions (one
-e per directive) so each pattern is applied on BusyBox sed while preserving the
replacement '# basilica-override: &/' and the rest of the command semantics.
---
Outside diff comments:
In `@crates/basilica-validator/src/rental/deployment.rs`:
- Around line 583-595: The current construction of add_key_cmd and add_key_alt
interpolates ssh_public_key into a shell string (used by
client.execute_ssh_command) and is vulnerable to shell injection; instead, send
the key via stdin to docker (e.g., use "docker exec -i {container_id} sh -c 'cat
> /root/.ssh/authorized_keys && chmod 600 /root/.ssh/authorized_keys'") and
invoke client.execute_ssh_command with that command while supplying
ssh_public_key on the command's stdin, and additionally validate/reject
ssh_public_key against an allowed-character regex (e.g., only [A-Za-z0-9+/=
@._-]) before attempting the operation; update references to add_key_cmd,
add_key_alt, client.execute_ssh_command, ssh_public_key, and container_id
accordingly.
---
Duplicate comments:
In `@crates/basilica-validator/src/rental/deployment.rs`:
- Around line 611-617: The generated profile script is being written as
/etc/profile.d/basilica-branding.sh but must be named with a zz- prefix to
ensure last-in-order sourcing; update the writer in deployment.rs so the heredoc
target and subsequent chmod refer to /etc/profile.d/zz-basilica-branding.sh
(replace occurrences of "basilica-branding.sh" with "zz-basilica-branding.sh")
so the file is created and permissioned under the new name.
---
Nitpick comments:
In `@crates/basilica-validator/src/rental/deployment.rs`:
- Around line 642-658: The code currently attempts three fallbacks
(start_systemctl, start_service, start_direct) but always logs success via
info!("SSH access configured for container {}", container_id) regardless of
whether any client.execute_ssh_command calls actually succeeded; change the flow
to capture each execute_ssh_command result (or a boolean success flag) for
start_systemctl, start_service, and start_direct (referencing the
start_systemctl, start_service, start_direct variables and
client.execute_ssh_command calls), and if all three fail log a warning (e.g.,
warn!("Failed to start SSH in container {}", container_id)) and return or
propagate an Err/early return instead of the unconditional info call; otherwise
only log the info message when at least one start method succeeded.
Comment on lines
597
to
621
| // Install Basilica SSH banner content. | ||
| let set_login_banner = format!( | ||
| r#"docker exec {container_id} sh -c "cat > /etc/issue.net <<'BASILICA_EOF' | ||
| {banner} | ||
| BASILICA_EOF""#, | ||
| banner = BASILICA_SSH_LOGIN_BANNER | ||
| ); | ||
| if let Err(e) = client.execute_ssh_command(&set_login_banner).await { | ||
| debug!("Failed to install Basilica SSH daemon banner: {}", e); | ||
| } | ||
|
|
||
| // Standard profile.d script for interactive shells. | ||
| let install_shell_branding = format!( | ||
| r#"docker exec {container_id} sh -c "mkdir -p /etc/profile.d | ||
| cat > /etc/profile.d/basilica-branding.sh <<'BASILICA_PROFILE_EOF' | ||
| case "\$-" in *i*) ;; *) return 0 2>/dev/null || exit 0 ;; esac | ||
| [ -n "\$BASILICA_LOGO_SHOWN" ] && return 0 2>/dev/null | ||
| export BASILICA_LOGO_SHOWN=1 | ||
| cat /etc/issue.net | ||
| BASILICA_PROFILE_EOF | ||
| chmod 0755 /etc/profile.d/basilica-branding.sh""# | ||
| ); | ||
| if let Err(e) = client.execute_ssh_command(&install_shell_branding).await { | ||
| debug!("Failed to install Basilica shell branding script: {}", e); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any other files in the PR that implement the missing MOTD suppression
echo "=== Searching for pam_motd references ==="
rg -n "pam_motd" --type rust
echo "=== Searching for update-motd references ==="
rg -n "update-motd" --type rust
echo "=== Searching for /etc/motd references ==="
rg -n "/etc/motd" --type rust
echo "=== Searching for bashrc/zshrc hook references ==="
rg -n "bashrc\|zshrc\|bash_profile" --type rust
echo "=== Checking entire deployment.rs file around the provided lines for additional branding code ==="
sed -n '590,650p' crates/basilica-validator/src/rental/deployment.rsRepository: one-covenant/basilica
Length of output: 3282
Several branding enforcement steps described in the PR are missing from the implementation.
The implementation only writes to /etc/issue.net and creates a profile.d script, but lacks:
- No write to
/etc/motd— some shells/configs display/etc/motdon login instead of the banner - No
pam_motd.sosuppression — Ubuntu images commonly use PAM MOTD modules that print the default MOTD regardless ofPrintMotd noinsshd_config - No disabling of
/etc/update-motd.d/*— dynamic MOTD scripts will still run - No sourcing hooks appended to shell startup files —
zshon some distros doesn't source/etc/profile.d/, and.bashrc/.bash_profileare not sourced on non-interactive SSH sessions
Without these, the base-image MOTD will likely still appear on Ubuntu/provider images, partially defeating the branding goal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/basilica-validator/src/rental/deployment.rs` around lines 597 - 621,
The current deployment only writes /etc/issue.net and a profile.d script; extend
the sequence (the blocks that build set_login_banner and install_shell_branding
and call client.execute_ssh_command) to also: write the same
BASILICA_SSH_LOGIN_BANNER content to /etc/motd, disable PAM MOTD output (e.g.,
remove or comment pam_motd.so lines in /etc/pam.d/sshd or create a small wrapper
that disables PrintMotd), disable or make non-executable all
/etc/update-motd.d/* scripts (e.g., chmod -x or move them), and append small
sourcing hooks into common startup files (/etc/bash.bashrc, /etc/profile,
/etc/zsh/zshrc, and users’ ~/.bashrc/.profile) so
/etc/profile.d/basilica-branding.sh is sourced for zsh/non-interactive SSH
sessions; perform each change via client.execute_ssh_command and preserve the
existing debug/error logging pattern used for set_login_banner and
install_shell_branding.
| "docker exec {container_id} bash -c 'echo \"PermitRootLogin prohibit-password\" >> /etc/ssh/sshd_config && \ | ||
| echo \"PubkeyAuthentication yes\" >> /etc/ssh/sshd_config && \ | ||
| echo \"PasswordAuthentication no\" >> /etc/ssh/sshd_config'" | ||
| r#"docker exec {container_id} sh -c "sed -i 's/^[[:space:]]*\(Banner\|PrintMotd\|PermitRootLogin\|PubkeyAuthentication\|PasswordAuthentication\)[[:space:]].*/# basilica-override: &/' /etc/ssh/sshd_config 2>/dev/null || true |
Contributor
There was a problem hiding this comment.
sed BRE \| alternation is a GNU extension — fails silently on Alpine/BusyBox.
The \(Banner\|PrintMotd\|…\) alternation in basic regex is not POSIX and is unsupported by BusyBox sed (used in Alpine containers). The command will silently match nothing, leaving existing directives in place (defeating the first-match-wins override).
Use multiple -e expressions instead, which are portable:
Proposed fix
- r#"docker exec {container_id} sh -c "sed -i 's/^[[:space:]]*\(Banner\|PrintMotd\|PermitRootLogin\|PubkeyAuthentication\|PasswordAuthentication\)[[:space:]].*/# basilica-override: &/' /etc/ssh/sshd_config 2>/dev/null || true
+ r#"docker exec {container_id} sh -c "sed -i \
+-e 's/^[[:space:]]*Banner[[:space:]].*/# basilica-override: &/' \
+-e 's/^[[:space:]]*PrintMotd[[:space:]].*/# basilica-override: &/' \
+-e 's/^[[:space:]]*PermitRootLogin[[:space:]].*/# basilica-override: &/' \
+-e 's/^[[:space:]]*PubkeyAuthentication[[:space:]].*/# basilica-override: &/' \
+-e 's/^[[:space:]]*PasswordAuthentication[[:space:]].*/# basilica-override: &/' \
+/etc/ssh/sshd_config 2>/dev/null || true📝 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.
Suggested change
| r#"docker exec {container_id} sh -c "sed -i 's/^[[:space:]]*\(Banner\|PrintMotd\|PermitRootLogin\|PubkeyAuthentication\|PasswordAuthentication\)[[:space:]].*/# basilica-override: &/' /etc/ssh/sshd_config 2>/dev/null || true | |
| r#"docker exec {container_id} sh -c "sed -i \ | |
| -e 's/^[[:space:]]*Banner[[:space:]].*/# basilica-override: &/' \ | |
| -e 's/^[[:space:]]*PrintMotd[[:space:]].*/# basilica-override: &/' \ | |
| -e 's/^[[:space:]]*PermitRootLogin[[:space:]].*/# basilica-override: &/' \ | |
| -e 's/^[[:space:]]*PubkeyAuthentication[[:space:]].*/# basilica-override: &/' \ | |
| -e 's/^[[:space:]]*PasswordAuthentication[[:space:]].*/# basilica-override: &/' \ | |
| /etc/ssh/sshd_config 2>/dev/null || true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/basilica-validator/src/rental/deployment.rs` at line 626, The sed
command that uses the BRE alternation
"\(Banner\|PrintMotd\|PermitRootLogin\|PubkeyAuthentication\|PasswordAuthentication\)"
is a GNU extension and fails on BusyBox/Alpine; edit the docker exec command
string that contains that sed invocation (the literal r#"docker exec
{container_id} sh -c "sed -i
's/^[[:space:]]*\(Banner\|PrintMotd\|PermitRootLogin\|PubkeyAuthentication\|PasswordAuthentication\)[[:space:]].*/#
basilica-override: &/' /etc/ssh/sshd_config 2>/dev/null || true) and replace the
single alternation-based substitution with multiple portable -e expressions (one
-e per directive) so each pattern is applied on BusyBox sed while preserving the
replacement '# basilica-override: &/' and the rest of the command semantics.
epappas
approved these changes
Feb 23, 2026
epappas
requested changes
Feb 23, 2026
Member
epappas
left a comment
epappas
left a comment
There was a problem hiding this comment.
although code changes are accepted, we need to see a screenshot
I removed the post-login profile script printing path, so only sshd banner remains: Keeps: issue.net PrintMotd no first-match override logic for existing SSH directives Removed: basilica-branding.sh creation/printing Previous version was causing double logos to appear one on pre-login banner and one in post-login script.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/basilica-validator/src/rental/deployment.rs (2)
610-618:⚠️ Potential issue | 🟠 Major
sedBRE\|alternation is not POSIX-portable — still fails on Alpine/BusyBox containers.This was flagged in a prior review and remains unresolved. BusyBox
sed(Alpine) does not support\|in basic regex; the command will silently match nothing, leaving existing directives uncommented. Since OpenSSH uses first-match-wins, the appended lines will be ignored on those images.Use multiple
-eexpressions instead:Proposed portable fix
let config_ssh = format!( - r#"docker exec {container_id} sh -c "sed -i 's/^[[:space:]]*\(Banner\|PrintMotd\|PermitRootLogin\|PubkeyAuthentication\|PasswordAuthentication\)[[:space:]].*/# basilica-override: &/' /etc/ssh/sshd_config 2>/dev/null || true + r#"docker exec {container_id} sh -c "sed -i \ +-e 's/^[[:space:]]*Banner[[:space:]].*/# basilica-override: &/' \ +-e 's/^[[:space:]]*PrintMotd[[:space:]].*/# basilica-override: &/' \ +-e 's/^[[:space:]]*PermitRootLogin[[:space:]].*/# basilica-override: &/' \ +-e 's/^[[:space:]]*PubkeyAuthentication[[:space:]].*/# basilica-override: &/' \ +-e 's/^[[:space:]]*PasswordAuthentication[[:space:]].*/# basilica-override: &/' \ +/etc/ssh/sshd_config 2>/dev/null || true cat >> /etc/ssh/sshd_config <<'BASILICA_SSHD_EOF'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/rental/deployment.rs` around lines 610 - 618, The sed expression in the config_ssh string uses BRE alternation with '\|' which is not supported by BusyBox/Alpine sed and therefore may fail to comment out sshd directives; update the sed invocation inside the config_ssh formatted string (the multi-line shell passed to docker exec) to use separate -e expressions (one -e per pattern like 's/^[[:space:]]*Banner[[:space:]].*/# basilica-override: &/' etc.) or multiple simple sed commands so it is POSIX/BusyBox-safe, ensuring the same set of keys (Banner, PrintMotd, PermitRootLogin, PubkeyAuthentication, PasswordAuthentication) are reliably commented out before appending the BASILICA_SSHD_EOF block.
597-606:⚠️ Potential issue | 🟡 MinorBanner is only written to
/etc/issue.net;/etc/motdis not updated.The PR objectives state that branding is written to both
/etc/issue.netand/etc/motd. Some base images and shell configurations display/etc/motdon login (especially when PAM MOTD modules are active), so only writing/etc/issue.netmay leave the provider's default MOTD visible after the pre-login banner.Proposed fix: also write to /etc/motd
let set_login_banner = format!( - r#"docker exec {container_id} sh -c "cat > /etc/issue.net <<'BASILICA_EOF' + r#"docker exec {container_id} sh -c "cat > /etc/issue.net <<'BASILICA_EOF' +{banner} +BASILICA_EOF +cat > /etc/motd <<'BASILICA_EOF' {banner} BASILICA_EOF""#, banner = BASILICA_SSH_LOGIN_BANNER );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/rental/deployment.rs` around lines 597 - 606, The current code builds set_login_banner to write BASILICA_SSH_LOGIN_BANNER only to /etc/issue.net; update the deployment logic to also write the same banner to /etc/motd. Modify the command constructed for set_login_banner (or create a new command like set_login_motd) so it writes BASILICA_SSH_LOGIN_BANNER into both /etc/issue.net and /etc/motd (e.g., run two cat > ... <<'BASILICA_EOF' blocks or a single shell command that tees the content to both paths), then invoke client.execute_ssh_command(...) for the new/updated command and handle the Err case the same way as the existing debug log.
🧹 Nitpick comments (1)
crates/basilica-validator/src/rental/deployment.rs (1)
628-643: Unconditional "SSH access configured" log may be misleading when all start methods fail.If
systemctl,service, and directsshdall fail, line 643 still logs"SSH access configured"atinfo!level, which could mislead operators during debugging. Consider tracking whether any method succeeded and adjusting the log level accordingly.Suggested approach
- if client.execute_ssh_command(&start_systemctl).await.is_err() { + let ssh_started = if client.execute_ssh_command(&start_systemctl).await.is_ok() { + true + } else { // Try service command let start_service = format!( "docker exec {container_id} sh -c 'service ssh restart 2>/dev/null || service sshd restart 2>/dev/null || service ssh start 2>/dev/null || service sshd start 2>/dev/null'" ); - if client.execute_ssh_command(&start_service).await.is_err() { + if client.execute_ssh_command(&start_service).await.is_ok() { + true + } else { // Try running sshd directly let start_direct = format!("docker exec -d {container_id} /usr/sbin/sshd -D"); - let _ = client.execute_ssh_command(&start_direct).await; + client.execute_ssh_command(&start_direct).await.is_ok() } - } + }; - info!("SSH access configured for container {}", container_id); + if ssh_started { + info!("SSH access configured for container {}", container_id); + } else { + warn!("SSH access configured but service may not have started for container {}", container_id); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-validator/src/rental/deployment.rs` around lines 628 - 643, The info! log is unconditional even if all SSH start attempts fail; add a boolean (e.g., ssh_started) and set it to true when any of the attempts using start_systemctl, start_service or start_direct (executed via client.execute_ssh_command) succeed, then replace the unconditional info!("SSH access configured for container {}", container_id) with conditional logging: info! when ssh_started is true and warn! or error! when false, so operators can tell when SSH configuration actually failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/basilica-validator/src/rental/deployment.rs`:
- Around line 610-618: The sed expression in the config_ssh string uses BRE
alternation with '\|' which is not supported by BusyBox/Alpine sed and therefore
may fail to comment out sshd directives; update the sed invocation inside the
config_ssh formatted string (the multi-line shell passed to docker exec) to use
separate -e expressions (one -e per pattern like
's/^[[:space:]]*Banner[[:space:]].*/# basilica-override: &/' etc.) or multiple
simple sed commands so it is POSIX/BusyBox-safe, ensuring the same set of keys
(Banner, PrintMotd, PermitRootLogin, PubkeyAuthentication,
PasswordAuthentication) are reliably commented out before appending the
BASILICA_SSHD_EOF block.
- Around line 597-606: The current code builds set_login_banner to write
BASILICA_SSH_LOGIN_BANNER only to /etc/issue.net; update the deployment logic to
also write the same banner to /etc/motd. Modify the command constructed for
set_login_banner (or create a new command like set_login_motd) so it writes
BASILICA_SSH_LOGIN_BANNER into both /etc/issue.net and /etc/motd (e.g., run two
cat > ... <<'BASILICA_EOF' blocks or a single shell command that tees the
content to both paths), then invoke client.execute_ssh_command(...) for the
new/updated command and handle the Err case the same way as the existing debug
log.
---
Nitpick comments:
In `@crates/basilica-validator/src/rental/deployment.rs`:
- Around line 628-643: The info! log is unconditional even if all SSH start
attempts fail; add a boolean (e.g., ssh_started) and set it to true when any of
the attempts using start_systemctl, start_service or start_direct (executed via
client.execute_ssh_command) succeed, then replace the unconditional info!("SSH
access configured for container {}", container_id) with conditional logging:
info! when ssh_started is true and warn! or error! when false, so operators can
tell when SSH configuration actually failed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Force-inject Basilica branding into rental SSH login so users consistently see Basilica ASCII branding instead of base-image/provider MOTD branding.
Type of Change
Changes Made
BASILICA_SSH_LOGIN_BANNERconstant and used it for SSH login branding.Banner /etc/issue.netPrintMotd no/etc/issue.netand/etc/motd./etc/profile.d/zz-basilica-branding.shto:/root/.bashrc,/root/.profile,/root/.bash_profile,/root/.zshrc,/etc/profile,/etc/bash.bashrc,/etc/zsh/zshrc) when present.pam_motd.soentries in/etc/pam.d/sshd(best effort),/etc/update-motd.d/*executables (best effort).crates/basilica-validator/src/rental/deployment.rsTesting
How Has This Been Tested?
cargo test)Commands run:
cargo fmt --all -- --check✅Test Configuration
rustc 1.85.0-nightly (d117b7f21 2024-12-31)cargo 1.85.0-nightly (c86f4b3a1 2024-12-24)bittensor-rs 0.1.1(gitone-covenant/bittensor-rs, commitbd344ba6)Checklist
cargo fmtto format my codecargo clippyand addressed all warningsAdditional Context
Summary by CodeRabbit
New Features
Bug Fixes