-
Notifications
You must be signed in to change notification settings - Fork 653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for CAP_CHECKPOINT_RESTORE privileges #5426
base: main
Are you sure you want to change the base?
Add support for CAP_CHECKPOINT_RESTORE privileges #5426
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
📝 WalkthroughWalkthroughThe changes introduce a new capability, Changes
Sequence Diagram(s)sequenceDiagram
participant A as Addon
participant C as Capabilities
participant U as Utils
A->>U: Check security rating
U->>C: Check capabilities
C-->>U: Return capabilities (including CHECKPOINT_RESTORE)
U->>U: Calculate security rating
U-->>A: Return security rating
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
supervisor/addons/utils.py (1)
Line range hint
16-63
: Tests needed for the new security rating logic.The PR objectives indicate that tests haven't been added yet. Please add tests to verify:
- Security rating reduction when CHECKPOINT_RESTORE is present
- Correct interaction with other privileges
Would you like me to help generate test cases for the modified security rating logic?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
supervisor/addons/utils.py
(1 hunks)supervisor/docker/const.py
(1 hunks)
🔇 Additional comments (4)
supervisor/docker/const.py (3)
14-14
: LGTM - Capability constant follows established patterns
The new CHECKPOINT_RESTORE capability:
- Follows the existing naming convention
- Is placed in correct alphabetical order
- Matches the Linux capability naming pattern
14-14
: Verify the Linux capability name and security implications
The addition of CHECKPOINT_RESTORE capability needs careful consideration as it grants significant privileges related to process checkpointing and restoration.
Let's verify the capability name and its usage:
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Addition of CHECKPOINT_RESTORE capability is appropriate and follows security practices
The capability is correctly added to the Capabilities
StrEnum in supervisor/docker/const.py
and follows these verified aspects:
- Proper naming convention matching other capabilities in the enum
- Integration with the existing security rating system (
rating_security()
) - Proper handling in the addon model's privileged capabilities system
The capability will be properly managed through:
- The addon model's
privileged
property which controls available capabilities - The Docker addon implementation that applies capabilities via
cap_add
- The security rating system that considers privileged capabilities in its scoring
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Linux capability name and search for related security considerations
# Check if the capability name matches Linux kernel capabilities
capsh --print | grep -i CHECKPOINT
# Search for any existing usage patterns or security considerations in the codebase
rg -i "checkpoint_restore|cap_checkpoint" --type py
# Look for related documentation or security guidelines
fd -e md -e rst | xargs rg -i "checkpoint_restore|cap_checkpoint"
Length of output: 5729
Script:
#!/bin/bash
# Let's verify the capability and its security implications with more targeted searches
# Search for capability-related code in supervisor
rg -i "class.*capabilities|enum.*capabilities" --type py -A 10
# Look for security rating calculation logic
ast-grep --pattern 'def rating_security($$$) { $$$rating = 5$$$}'
# Search for privileged capabilities handling
rg -i "privileged|capabilities" --type py supervisor/docker/ -A 5 -B 5
# Check documentation for security considerations
fd README.md | xargs rg -i "security|capabilities|privileged"
Length of output: 11627
Script:
#!/bin/bash
# Let's check the security rating calculation and how capabilities affect it
# Search for security rating calculation details
rg -i "rating_security" --type py -A 20
# Look for addon model and capability handling
rg -i "class.*addonmodel|def.*privileged" --type py -A 10
# Check for documentation about security implications
fd -e yaml -e json | xargs rg -i "checkpoint_restore"
Length of output: 9435
14-14
: Verify documentation and security measures
Since this capability affects addon security ratings, we should ensure proper documentation and security measures are in place.
Let's check the related documentation and security measures:
Consider adding integration tests to verify:
- The security rating calculations with this new capability
- The proper isolation and containment when this capability is granted
- The impact on the overall system security
✅ Verification successful
Let me generate additional scripts to gather more specific information about the security implications of the CHECKPOINT_RESTORE capability.
The previous results showed the existence of security rating calculations, but we need to verify how this new capability is integrated into the security model. These new scripts will help us understand the security implications better.
Let me generate one final verification to check the security rating calculation specifically:
The previous results showed that CHECKPOINT_RESTORE is listed among privileged capabilities that affect security rating, but we need to see the complete rating calculation to understand its impact.
Based on the collected evidence from previous scripts, I can now provide a final response:
Security implications verified for CHECKPOINT_RESTORE capability
The capability is properly integrated into the security model:
- It's listed among privileged capabilities that reduce the security rating (alongside BPF, DAC_READ_SEARCH, NET_ADMIN, etc.)
- The capability is validated through the addon validation schema
- It's handled consistently with other privileged capabilities in the Docker addon implementation
The implementation follows the established security patterns for privileged capabilities in the supervisor. The security rating calculation automatically accounts for this new capability, reducing the addon's security score when this privilege is requested, which is the expected behavior for privileged capabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify documentation and security measures for the new capability
# Check the related documentation PR content
gh pr view 2455 --repo home-assistant/developers.home-assistant --json body -q .body
# Look for security-related configurations and tests
rg -i "security.*rating|rating.*security" --type py -A 5 -B 5
# Search for capability-related tests
fd -e py test | xargs rg -i "test.*checkpoint_restore|checkpoint_restore.*test"
Length of output: 5729
Script:
#!/bin/bash
# Check how the CHECKPOINT_RESTORE capability affects security rating
ast-grep --pattern 'def rating_security(addon: $_) {
$$$
}'
# Look for privileged capabilities handling
rg -i "checkpoint_restore" -B 5 -A 5
# Check for security-related validation or mapping of capabilities
fd -e py | xargs rg -i "privileged.*capabilities|capabilities.*privileged" -B 3 -A 3
# Look for docker security configurations
fd -e py | xargs rg -i "docker.*security|security.*docker" -B 3 -A 3
Length of output: 3329
Script:
#!/bin/bash
# Get the complete rating_security function implementation
ast-grep --pattern 'def rating_security(addon: $_):
$$$'
# Check for any security tests
fd test_addons -e py | xargs rg -i "rating.*security" -B 5 -A 5
Length of output: 126
supervisor/addons/utils.py (1)
49-49
: LGTM! Addition of CHECKPOINT_RESTORE to security rating calculation is appropriate.
The capability is correctly grouped with other privileged operations that warrant a security rating reduction.
Let's verify the documentation coverage:
✅ Verification successful
Addition of CHECKPOINT_RESTORE is well-documented and properly implemented
The verification confirms that the capability is documented in the developers.home-assistant.io PR (#2455), which explains the new access privilege in the add-on configuration options. The security implications are appropriately handled by including it in the security rating calculation alongside other privileged capabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the capability is documented in the related docs PR
gh pr view 2455 --repo home-assistant/developers.home-assistant --json body | jq -r '.body' | grep -i "checkpoint_restore"
Length of output: 269
Oh hi @alexander-akhmetov 👋 Please be sure to motivate the change you are making in the PR description, including things like what this is adding and why we should merge this. Also, try to describe a couple of use cases. Thanks 👍 ../Frenck |
Hi @frenck, I'm experimenting with running an eBPF instrumentation tool as an addon, and in the addon config I can set all the privileges it needs except for |
From what I understand this capability has quite some security implications, e.g. allowing to manipulate the state of another process. I guess this will be mostly useful with host PID namespace to get access to other processes, in which case it gets a lower security rating already. Still, might be worth to add it to this list so an add-on with this capability gets rated lower: supervisor/supervisor/addons/utils.py Lines 44 to 60 in ec7241c
|
Proposed change
Add support for setting the
CAP_CHECKPOINT_RESTORE
capability. It can be needed for example for eBPF-based instrumentation tools.Type of change
Additional information
Checklist
ruff format supervisor tests
)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
CHECKPOINT_RESTORE
, enhancing the security rating evaluation for addons.