feat: add polkit authorization for system services #1003
Conversation
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
Added Polkit authorization checks to backlight helper service and created policy files for multiple system services. Modified SetBrightness method to accept sender parameter and perform authorization before executing privileged operations. Added 10 new policy files for various DDE services including bluetooth, display, power, timedate, etc. Removed unused Apps1 service configuration. The changes enhance system security by requiring proper authorization for sensitive operations. The backlight helper now checks if the caller has permission to adjust brightness settings through Polkit. Multiple policy files define the authorization requirements for different system services, ensuring only authorized users can perform privileged actions. Log: Added security authorization for system service operations Influence: 1. Test brightness adjustment functionality with authorized and unauthorized users 2. Verify Polkit authorization prompts appear when required 3. Check that privileged operations fail without proper authorization 4. Test different service operations with various user privilege levels 5. Verify policy files are correctly installed and recognized by the system 6. Test backward compatibility with existing applications feat: 为系统服务添加 Polkit 授权机制 为背光助手服务添加 Polkit 授权检查，并为多个系统服务创建策略文件。修改 SetBrightness 方法以接受发送者参数，并在执行特权操作前进行授权验证。新增 10 个 DDE 服务的策略文件，包括蓝牙、显示、电源、时间日期等。移除未使用的 Apps1 服务配置。 这些更改通过要求对敏感操作进行适当授权来增强系统安全性。背光助手现在会检 查调用者是否具有通过 Polkit 调整亮度设置的权限。多个策略文件定义了不同系 统服务的授权要求，确保只有授权用户才能执行特权操作。 Log: 新增系统服务操作的安全授权机制 Influence: 1. 测试授权和未授权用户的亮度调节功能 2. 验证需要时 Polkit 授权提示是否正确显示 3. 检查无适当授权时特权操作是否失败 4. 测试不同用户权限级别下的各种服务操作 5. 验证策略文件是否正确安装并被系统识别 6. 测试与现有应用程序的向后兼容性
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fly602 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's GuideAdds Polkit-based authorization checks to the backlight helper service and introduces dedicated Polkit policy files for multiple DDE system services while removing an unused system service configuration. Sequence diagram for Polkit-guarded SetBrightness callsequenceDiagram
actor UserApp
participant SystemBus
participant BacklightHelper
participant PolkitAuthority
participant SysFsBacklight
UserApp->>SystemBus: Call SetBrightness(sender, type0, name, value)
SystemBus->>BacklightHelper: SetBrightness(sender, type0, name, value)
BacklightHelper->>BacklightHelper: DelayAutoQuit()
BacklightHelper->>BacklightHelper: checkAuthorization(actionId, sender)
BacklightHelper->>SystemBus: Connect SystemBus()
SystemBus-->>BacklightHelper: SystemBus connection
BacklightHelper->>PolkitAuthority: CheckAuthorization(subject SystemBusName, actionId, flags AllowUserInteraction)
PolkitAuthority-->>BacklightHelper: AuthorizationResult(IsAuthorized)
alt Authorized
BacklightHelper->>BacklightHelper: getBrightnessFilename(type0, name)
BacklightHelper->>SysFsBacklight: Write brightness value
SysFsBacklight-->>BacklightHelper: Write result
BacklightHelper-->>SystemBus: Return success
SystemBus-->>UserApp: Success
else Not authorized
BacklightHelper-->>SystemBus: Return dbus.Error(not authorized)
SystemBus-->>UserApp: Error not authorized
end
Class diagram for Manager and Polkit authorization helperclassDiagram
class Manager {
+dbusutilService service
+GetInterfaceName() string
+SetBrightness(sender dbus.Sender, type0 byte, name string, value int32) *dbus.Error
-getBacklightGs(name string) bool
}
class AuthHelper {
<<utility>>
+checkAuthorization(actionId string, sysBusName string) error
}
class PolkitAuthority {
+CheckAuthorization(cookie uint32, subject Subject, actionId string, details map, flags uint32, cancellationId string) AuthorizationResult
}
class Subject {
+SetDetail(key string, value string)
}
class AuthorizationResult {
+IsAuthorized bool
}
Manager ..> AuthHelper : uses
AuthHelper ..> PolkitAuthority : creates and calls
AuthHelper ..> Subject : configures
PolkitAuthority ..> AuthorizationResult : returns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new SetBrightness signature now requires a dbus.Sender; please ensure all D-Bus registrations and callers are updated consistently so the method remains callable and introspection stays in sync with the implementation.
- checkAuthorization creates a new SystemBus connection on each call; consider reusing the existing service connection or caching the bus/authority to avoid repeated connection setup on every brightness change.
- The Polkit action ID string used in SetBrightness is hard-coded; extracting it into a named constant (shared with the corresponding .policy file if possible) would reduce the risk of mismatches and make future changes safer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new SetBrightness signature now requires a dbus.Sender; please ensure all D-Bus registrations and callers are updated consistently so the method remains callable and introspection stays in sync with the implementation.
- checkAuthorization creates a new SystemBus connection on each call; consider reusing the existing service connection or caching the bus/authority to avoid repeated connection setup on every brightness change.
- The Polkit action ID string used in SetBrightness is hard-coded; extracting it into a named constant (shared with the corresponding .policy file if possible) would reduce the risk of mismatches and make future changes safer.
## Individual Comments
### Comment 1
<location> `bin/backlight_helper/main.go:105-110` </location>
<code_context>
return filepath.Join("/sys/class", subsystem, name, "brightness"), nil
}
+func checkAuthorization(actionId string, sysBusName string) error {
+ systemBus, err := dbus.SystemBus()
+ if err != nil {
+ return err
+ }
+ authority := polkit.NewAuthority(systemBus)
+ subject := polkit.MakeSubject(polkit.SubjectKindSystemBusName)
+ subject.SetDetail("name", sysBusName)
</code_context>
<issue_to_address>
**suggestion (performance):** Reuse the existing D-Bus connection (or cache the Authority) instead of creating a new system bus and Authority on each call.
Each `SetBrightness` call now creates a new `dbus.SystemBus()` connection and `polkit.Authority`, which is relatively expensive and can stress the bus under frequent brightness updates. Since this helper already runs on the system bus, consider reusing `m.service.Conn()` (or passing the system bus into `checkAuthorization`) and using a singleton or cached `Authority` instance to keep the Polkit check cheap and avoid potential connection leaks.
Suggested implementation:
```golang
func checkAuthorization(actionId string, sysBusName string, authority *polkit.Authority) error {
if authority == nil {
return errors.New("polkit authority is not initialized")
}
subject := polkit.MakeSubject(polkit.SubjectKindSystemBusName)
subject.SetDetail("name", sysBusName)
ret, err := authority.CheckAuthorization(
0,
subject,
actionId,
nil,
polkit.CheckAuthorizationFlagsAllowUserInteraction,
"",
)
if err != nil {
return err
}
if !ret.IsAuthorized {
return errors.New("not authorized")
}
return nil
}
```
To fully implement the optimization and reuse the existing D-Bus connection, you should also:
1. **Cache a single `*polkit.Authority` instance**, initialized from the existing system bus connection, likely on your main helper/service struct:
- Add a field, e.g.:
```go
type Manager struct {
// existing fields...
polkitAuthority *polkit.Authority
}
```
- Initialize it once where you construct `Manager`, using the existing D-Bus connection:
```go
m.polkitAuthority = polkit.NewAuthority(m.service.Conn())
if m.polkitAuthority == nil {
// handle error as appropriate (NewAuthority may return nil + error in your version)
}
```
2. **Update all callers of `checkAuthorization`** to pass the cached authority instead of relying on the internal `dbus.SystemBus()` call. For example, change:
```go
if err := checkAuthorization(actionId, sender); err != nil {
// ...
}
```
to:
```go
if err := checkAuthorization(actionId, sender, m.polkitAuthority); err != nil {
// ...
}
```
3. **Ensure imports are correct**:
- `polkit` is already imported; no new imports are required for the change above.
- If your `polkit.NewAuthority` returns `(*polkit.Authority, error)` in this codebase instead of just `*polkit.Authority`, adjust the initialization accordingly and handle the error.
These additional changes will ensure that `SetBrightness` and any other authorization-using operations reuse the same system bus connection and `polkit.Authority`, avoiding per-call overhead and potential connection leaks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+105
to
+110
| func checkAuthorization(actionId string, sysBusName string) error { | ||
| systemBus, err := dbus.SystemBus() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| authority := polkit.NewAuthority(systemBus) |
There was a problem hiding this comment.
suggestion (performance): Reuse the existing D-Bus connection (or cache the Authority) instead of creating a new system bus and Authority on each call.
Each SetBrightness call now creates a new dbus.SystemBus() connection and polkit.Authority, which is relatively expensive and can stress the bus under frequent brightness updates. Since this helper already runs on the system bus, consider reusing m.service.Conn() (or passing the system bus into checkAuthorization) and using a singleton or cached Authority instance to keep the Polkit check cheap and avoid potential connection leaks.
Suggested implementation:
func checkAuthorization(actionId string, sysBusName string, authority *polkit.Authority) error {
if authority == nil {
return errors.New("polkit authority is not initialized")
}
subject := polkit.MakeSubject(polkit.SubjectKindSystemBusName)
subject.SetDetail("name", sysBusName)
ret, err := authority.CheckAuthorization(
0,
subject,
actionId,
nil,
polkit.CheckAuthorizationFlagsAllowUserInteraction,
"",
)
if err != nil {
return err
}
if !ret.IsAuthorized {
return errors.New("not authorized")
}
return nil
}To fully implement the optimization and reuse the existing D-Bus connection, you should also:
-
Cache a single
*polkit.Authorityinstance, initialized from the existing system bus connection, likely on your main helper/service struct:- Add a field, e.g.:
type Manager struct { // existing fields... polkitAuthority *polkit.Authority }
- Initialize it once where you construct
Manager, using the existing D-Bus connection:m.polkitAuthority = polkit.NewAuthority(m.service.Conn()) if m.polkitAuthority == nil { // handle error as appropriate (NewAuthority may return nil + error in your version) }
- Add a field, e.g.:
-
Update all callers of
checkAuthorizationto pass the cached authority instead of relying on the internaldbus.SystemBus()call. For example, change:if err := checkAuthorization(actionId, sender); err != nil { // ... }
to:
if err := checkAuthorization(actionId, sender, m.polkitAuthority); err != nil { // ... }
-
Ensure imports are correct:
polkitis already imported; no new imports are required for the change above.- If your
polkit.NewAuthorityreturns(*polkit.Authority, error)in this codebase instead of just*polkit.Authority, adjust the initialization accordingly and handle the error.
These additional changes will ensure that SetBrightness and any other authorization-using operations reuse the same system bus connection and polkit.Authority, avoiding per-call overhead and potential connection leaks.
deepin pr auto review这份代码diff主要涉及对DDE（Deepin Desktop Environment）系统服务的安全增强，通过引入Polkit（PolicyKit）来控制对系统资源的访问。以下是对代码的详细审查和改进建议： 1. 语法和逻辑审查主要修改点：
潜在问题：
2. 代码质量改进
3. 性能考虑
4. 安全建议
5. 其他建议
这些改进建议旨在提高代码的安全性、可维护性和可靠性，同时保持良好的性能。建议根据实际项目需求和优先级逐步实施这些改进。 |
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.
Added Polkit authorization checks to backlight helper service and created policy files for multiple system services. Modified SetBrightness method to accept sender parameter and perform authorization before executing privileged operations. Added 10 new policy files for various DDE services including bluetooth, display, power, timedate, etc. Removed unused Apps1 service configuration.
The changes enhance system security by requiring proper authorization for sensitive operations. The backlight helper now checks if the caller has permission to adjust brightness settings through Polkit. Multiple policy files define the authorization requirements for different system services, ensuring only authorized users can perform privileged actions.
Log: Added security authorization for system service operations
Influence:
feat: 为系统服务添加 Polkit 授权机制
为背光助手服务添加 Polkit 授权检查，并为多个系统服务创建策略文件。修改
SetBrightness 方法以接受发送者参数，并在执行特权操作前进行授权验证。新增
10 个 DDE 服务的策略文件，包括蓝牙、显示、电源、时间日期等。移除未使用的
Apps1 服务配置。
这些更改通过要求对敏感操作进行适当授权来增强系统安全性。背光助手现在会检
查调用者是否具有通过 Polkit 调整亮度设置的权限。多个策略文件定义了不同系
统服务的授权要求，确保只有授权用户才能执行特权操作。
Log: 新增系统服务操作的安全授权机制
https://bugzilla.opensuse.org/show_bug.cgi?id=1257149
Influence:
Summary by Sourcery
Add Polkit-based authorization for system services and backlight brightness changes to harden privileged operations.
New Features:
Enhancements:
Chores: