Merged
Conversation
WalkthroughAdds ESP32 Secure Boot v2: gitignore entries for keys, new secure-boot SDK defaults, signing/verification script, SECURITY.md updates, and detailed secure-boot docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as "Developer (CLI)"
participant Script as "scripts/sign_firmware.sh"
participant ESP as "espsecure.py"
participant FS as "Filesystem / Build Artifacts"
Dev->>Script: run command (generate-key / sign-app / sign-bootloader / sign-all / verify)
Script->>FS: check `keys/`, `build/keep.bin`, `build/bootloader/bootloader.bin`
alt generate-key
Script->>ESP: espsecure.py generate_signing_key (RSA-3072)
ESP-->>FS: write `keys/secure_boot_signing_key.pem` (chmod set)
Script-->>Dev: print success and path
else sign-app / sign-bootloader / sign-all
Script->>ESP: espsecure.py sign binary -> `*-signed.bin`
ESP-->>FS: write signed artifact(s)
Script-->>Dev: print signed artifact paths
else verify
Script->>ESP: espsecure.py verify `*-signed.bin`
ESP-->>Script: verification result
Script-->>Dev: display verification status/warnings
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
kwsantiago
approved these changes
Jan 22, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@sdkconfig.defaults.secureboot`:
- Around line 26-27: The config currently sets
CONFIG_SECURE_DISABLE_ROM_DL_MODE=y which permanently disables ROM Download Mode
via eFuse; confirm this is intentional for production and, if you need
recoverability for development, change the setting to one of the alternatives
(use CONFIG_SECURE_INSECURE_ALLOW_DL_MODE=y for full esptool/ROM download access
during development, or CONFIG_SECURE_ENABLE_SECURE_ROM_DL_MODE=y to enable
secure/controlled download mode if supported) and remove or set
CONFIG_SECURE_DISABLE_ROM_DL_MODE to n accordingly; update the
sdkconfig.defaults.secureboot entries so only the chosen download-mode option is
enabled and add a short inline comment documenting the risk/reason for the
chosen option.
🧹 Nitpick comments (3)
scripts/sign_firmware.sh (2)
97-120: Addcheck_signing_keycall toverify_signaturesfor consistent error handling.The verification function uses
$SIGNING_KEY(line 107, 114) but doesn't callcheck_signing_key()first. If the key is missing,espsecure.pywill fail with a less informative error.Proposed fix
verify_signatures() { check_espsecure + check_signing_key local app_bin="${BUILD_DIR}/keep-signed.bin"
105-117: Consider non-zero exit when no signed binaries exist.Currently, if both signed binaries are missing, the function prints warnings but exits successfully. This could mask issues in CI pipelines.
Proposed fix
local app_bin="${BUILD_DIR}/keep-signed.bin" local bootloader_bin="${BUILD_DIR}/bootloader/bootloader-signed.bin" + local found=0 echo "Verifying signatures..." if [ -f "$app_bin" ]; then echo "Verifying application..." espsecure.py verify_signature --version 2 --keyfile "$SIGNING_KEY" "$app_bin" + found=1 else echo "Warning: Signed application not found" fi if [ -f "$bootloader_bin" ]; then echo "Verifying bootloader..." espsecure.py verify_signature --version 2 --keyfile "$SIGNING_KEY" "$bootloader_bin" + found=1 else echo "Warning: Signed bootloader not found" fi + if [ "$found" -eq 0 ]; then + echo "Error: No signed binaries found to verify" + exit 1 + fi + echo "Verification complete." }docs/SECURE_BOOT.md (1)
76-78: Add language specifiers to fenced code blocks.Per static analysis hints, these configuration snippets should specify a language for proper rendering.
Proposed fix
To increment version for a security-critical update: -``` +```ini CONFIG_BOOTLOADER_APP_SECURE_VERSION=2```diff For additional protection of firmware at rest, enable flash encryption in `sdkconfig.defaults.secureboot`: -``` +```ini CONFIG_SECURE_FLASH_ENC_ENABLED=y CONFIG_SECURE_FLASH_ENCRYPTION_MODE_RELEASE=y</details> Also applies to: 86-89 </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
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 by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.