Fix restoring unencrypted backup in corner case #5600
Conversation
If a backup has a encrypted and unencrypted location, and the encrypted location is beeing restored first, the encryption key is still cached. When the user restores the unencrypted backup next, it will fail because the Supervisor tries to use encryption key still.
|
Warning Rate limit exceeded@agners has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes update the backup restoration process and its associated test coverage. In the backup manager module, the internal password handling method has been renamed from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BackupManager
participant Backup
Client->>BackupManager: Initiate do_restore_full/partial(backup, password, location)
BackupManager->>BackupManager: _set_location_password(backup, password, location)
alt Location is protected
BackupManager->>Backup: Validate and set provided password
else Not Protected
BackupManager->>Backup: Set password to None
end
BackupManager->>Backup: Execute restore process (restore folders, Home Assistant, add-ons)
BackupManager-->>Client: Return restoration result
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
supervisor/backups/manager.py (1)
730-731: Log ignored password for unprotected backupsCurrently, the code discards any user-supplied password by unconditionally setting it to
None. This is correct for an unprotected backup. However, to improve clarity and user awareness, you may consider logging a warning or debug message if the user provided a password that’s being ignored:else: + if password: + _LOGGER.warning( + "Ignoring user-provided password for unprotected backup %s", + backup.slug + ) backup.set_password(None)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supervisor/backups/manager.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.1
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/api/test_backups.py (1)
958-1011: LGTM! Test thoroughly validates the bugfix.The test effectively validates the fix for restoring unencrypted backups after encrypted ones by:
- Testing encrypted backup restore with password
- Testing unencrypted backup restore without password
- Testing encrypted backup restore with password again
Consider adding assertions to verify the backup's encryption state before each restore to make the test more robust:
# Restore encrypted backup (test_file := coresys.config.path_ssl / "test.txt").touch() + assert backup.all_locations[None]["protected"] is True resp = await api_client.post( f"/backups/{backup.slug}/restore/partial", json={"location": None, "password": "test", "folders": ["ssl"]}, ) # Restore unencrypted backup test_file.touch() + assert backup.all_locations[".cloud_backup"]["protected"] is False resp = await api_client.post( f"/backups/{backup.slug}/restore/partial", json={"location": ".cloud_backup", "folders": ["ssl"]}, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/api/test_backups.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.1
| @@ -727,6 +727,8 @@ async def _validate_location_password( | |||
| raise BackupInvalidError( | |||
| f"Invalid password for backup {backup.slug}", _LOGGER.error | |||
| ) | |||
| else: | |||
There was a problem hiding this comment.
Shouldn't this be a finally? Otherwise we leave the password around in cache if validate_password raises
There was a problem hiding this comment.
We should not trust whatever is left around.
This else relates to if backup.all_locations[location][ATTR_PROTECTED]:: This way we set/clear the password on every invocation of _validate_location_password. We probably should rename _validate_location_password to _set_location_password or something, since it does set the password...
There was a problem hiding this comment.
oh I get it now. The collapsed view just showed the raise and then else, I thought it was a try - else, not an if - else. My bad, looks good 👍
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
supervisor/backups/backup.py (1)
300-327: Consider documenting the Docker configuration precedence.The
consolidatemethod now gives precedence to the Docker configuration from the newly reloaded backup. While this change is correct and aligns with the PR objectives, it would be helpful to document this behavior in the method's docstring to make the precedence rules clear for future maintainers.Apply this diff to add documentation:
def consolidate(self, backup: Self) -> None: - """Consolidate two backups with same slug in different locations.""" + """Consolidate two backups with same slug in different locations. + + When consolidating backups, the Docker configuration from the newly reloaded + backup takes precedence over the existing configuration. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
supervisor/backups/backup.py(1 hunks)supervisor/backups/manager.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- supervisor/backups/manager.py
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build i386 supervisor
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.1
🔇 Additional comments (1)
supervisor/backups/backup.py (1)
325-326: LGTM! Docker configuration update aligns with PR objectives.The added code ensures that the Docker configuration from the newly reloaded backup is used during consolidation, which helps fix the issue with restoring unencrypted backups after encrypted ones by ensuring the correct Docker configuration is maintained.
This reverts commit 9b47a1c.
| f"/backups/{backup.slug}/restore/partial", | ||
| json={"location": None, "password": "test", "folders": ["ssl"]}, | ||
| ) | ||
| assert resp.status == 200 |
There was a problem hiding this comment.
So this actually exposes another issue: The Docker configuration (registry credentials) are stored as part of the metadata. Since we consolidate metadata, we might use the "wrong" metadata on restore. E.g. if a encrypted and unencrypted backup is consolidated, we might try to decrypt a unencrypted password (which usually fails, and as the reason for the test failure), or use the encrypted password without decrypting it (which actually goes through unnoticed).
I propose to simply drop the Docker configuration feature from the backup for now. We've intend to move the config to a supervisor.tar.gz specific file anyways.
There was a problem hiding this comment.
Actually, this likely will lead to a bigger discussion. Let's separate the bugfix for the Docker config in a separate PR. I made this PR to be about the original issue only.
agners
force-pushed
the
fix-restoring-unencrypted-backup
branch
from
a39c085 to
315f667
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/api/test_backups.py (1)
982-1016: Enhance test coverage with additional assertions.While the test effectively verifies the basic restore functionality, consider adding assertions to:
- Verify the backup's content after each restore
- Validate the encryption state remains consistent
- Check that the password cache is properly cleared between restores
Example additions:
assert resp.status == 200 body = await resp.json() assert body["result"] == "ok" assert not test_file.is_file() +# Verify backup content and state +assert backup.homeassistant is not None +assert backup.protected is True +assert backup.supervisor_version is not None +# Verify password cache is cleared +assert backup._password is None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/api/test_backups.py(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Check pylint
tests/api/test_backups.py
[warning] 976-976:
W0511: TODO: There is a bug in the restore code that causes the restore to fail (fixme)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.1
🔇 Additional comments (2)
tests/api/test_backups.py (2)
958-974: LGTM! Well-structured test setup.The test setup effectively creates a mixed encryption scenario by copying encrypted and unencrypted backup fixtures to different locations and verifies the initial state.
976-980: Track and address the Docker registry configuration bug.The workaround using
MagicMockallows the test to proceed, but the underlying issue should be tracked and fixed:
- Bug: Docker registry configuration restore fails when backup locations have mixed encryption states
- Current workaround: Mock the restore_dockerconfig
- Proposed solution: Move Docker configuration to a separate
supervisor.tar.gzfileWould you like me to:
- Create an issue to track this bug?
- Help implement the solution to move Docker configuration to a separate file?
🧰 Tools
🪛 GitHub Check: Check pylint
[warning] 976-976:
W0511: TODO: There is a bug in the restore code that causes the restore to fail (fixme)
Proposed change
If a backup has a encrypted and unencrypted location, and the encrypted location is beeing restored first, the encryption key is still cached. When the user restores the unencrypted backup next, it will fail because the Supervisor tries to use encryption key still.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
New Features
Tests