fix(security): audit and batch sudo calls in brev-setup.sh#1147
fix(security): audit and batch sudo calls in brev-setup.sh#1147fdzdev wants to merge 3 commits intoNVIDIA:mainfrom
Conversation
Consolidate 3 separate sudo apt-get update calls into a single upfront index refresh, reducing privilege escalation surface. Add audit header documenting all sudo calls by category: [pkg], [bin], [svc], [cfg], [run] (NVBUG 6002888). No sudo calls removed — all are required for system package management on Brev VMs. The dangerous [run] category (NodeSource setup) is addressed separately in PR NVIDIA#869. Made-with: Cursor Signed-off-by: Facundo Fernandez <facu.tha@gmail.com> Made-with: Cursor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe setup script consolidates apt package index updates into a single upfront Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/brev-setup.sh (1)
85-92:⚠️ Potential issue | 🔴 CriticalMissing
apt-get updateafter adding NVIDIA repository will cause installation failure.Lines 85-89 add the NVIDIA Container Toolkit repository to the system, but the package index was refreshed at line 52 before this repository existed. When line 90 attempts to install
nvidia-container-toolkit, apt will fail with "Unable to locate package" because the index doesn't include packages from the newly-added repository.The
apt-get updatecall that was removed from this block was not redundant—it was required to refresh the index after dynamically adding a new source.🐛 Proposed fix: Add apt-get update after repository setup
curl -s -L https://nvidia.github.io/libnvidia-container/stable/deb/nvidia-container-toolkit.list \ | sed 's#deb https://#deb [signed-by=/usr/share/keyrings/nvidia-container-toolkit-keyring.gpg] https://#g' \ | sudo tee /etc/apt/sources.list.d/nvidia-container-toolkit.list >/dev/null + sudo apt-get update -qq >/dev/null 2>&1 sudo apt-get install -y -qq nvidia-container-toolkit >/dev/null 2>&1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/brev-setup.sh` around lines 85 - 92, The script adds the NVIDIA Container Toolkit APT repository but then immediately runs sudo apt-get install nvidia-container-toolkit; insert an apt-get update after the repository is written (after the curl/sed/tee block that creates /etc/apt/sources.list.d/nvidia-container-toolkit.list and before sudo apt-get install -y -qq nvidia-container-toolkit) so the package index includes the newly-added source and the installation of nvidia-container-toolkit and subsequent sudo nvidia-ctk runtime configure will succeed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/brev-setup.sh`:
- Around line 85-92: The script adds the NVIDIA Container Toolkit APT repository
but then immediately runs sudo apt-get install nvidia-container-toolkit; insert
an apt-get update after the repository is written (after the curl/sed/tee block
that creates /etc/apt/sources.list.d/nvidia-container-toolkit.list and before
sudo apt-get install -y -qq nvidia-container-toolkit) so the package index
includes the newly-added source and the installation of nvidia-container-toolkit
and subsequent sudo nvidia-ctk runtime configure will succeed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f6af5ef-3b42-4af2-947f-1af1fd0e9726
📒 Files selected for processing (1)
scripts/brev-setup.sh
cv
left a comment
There was a problem hiding this comment.
Thanks for rebasing this onto main — that preserved the checksum-verification hardening from #869, which was my main concern on the previous pass.
I still think we should keep one targeted apt-get update in the NVIDIA Container Toolkit block, though.
Right now the PR removes the update after writing:
/etc/apt/sources.list.d/nvidia-container-toolkit.list
That update is not redundant, because the repository is being added dynamically during the script run. The upfront apt-get update happens before that source exists, so the package index may not include nvidia-container-toolkit yet. In that case the subsequent install can fail with Unable to locate package nvidia-container-toolkit.
So I think the right shape here is:
- keep the new single upfront update for the base distro packages
- restore a second update immediately after adding the NVIDIA apt source
- keep the
ghinstall on the upfront index refresh only
Once that NVIDIA repo refresh is back, I think this is in good shape.
|
I like it. Sounds good Carlos. |
Summary
sudo apt-get updatecalls into a single upfront index refresh (NVBUG 6002888)[pkg],[bin],[svc],[cfg],[run][run]category (NodeSource setup as root) is addressed separately in PR fix(security): stop piping curl output directly to sudo bash #869Test plan
brev-setup.shon fresh VM — all packages install correctly with single upfrontapt-get updateghCLI installs — works without separate updateSigned-off-by: Facundo Fernandez facu.tha@gmail.com
Summary by CodeRabbit