-
Notifications
You must be signed in to change notification settings - Fork 180
Refactor diskencrypt #3279
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
Refactor diskencrypt #3279
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.
Sorry @Johnson-zs, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Johnson-zs The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Warning
详情 {
"export": {
"src/plugins/filemanager/dfmplugin-disk-encrypt-entry/menu/diskencryptmenuscene.cpp": {
"a": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
],
"b": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
]
}
}
} |
7a73706 to
8f80189
Compare
|
Warning
详情 {
"export": {
"src/plugins/filemanager/dfmplugin-disk-encrypt-entry/menu/diskencryptmenuscene.cpp": {
"a": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
],
"b": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
]
}
}
} |
|
Warning
详情 {
"export": {
"src/plugins/filemanager/dfmplugin-disk-encrypt-entry/menu/diskencryptmenuscene.cpp": {
"a": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
],
"b": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
]
}
}
} |
4f4a59a to
cd0f801
Compare
|
Warning
详情 {
"export": {
"src/plugins/filemanager/dfmplugin-disk-encrypt-entry/menu/diskencryptmenuscene.cpp": {
"a": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
],
"b": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
]
}
}
} |
cd0f801 to
7b0dddd
Compare
|
Warning
详情 {
"export": {
"src/plugins/filemanager/dfmplugin-disk-encrypt-entry/menu/diskencryptmenuscene.cpp": {
"a": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
],
"b": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
]
}
}
} |
|
TAG Bot New tag: 6.5.100 |
7b0dddd to
46ad658
Compare
|
Warning
详情 {
"export": {
"src/plugins/filemanager/dfmplugin-disk-encrypt-entry/menu/diskencryptmenuscene.cpp": {
"a": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
],
"b": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
]
}
}
} |
|
TAG Bot New tag: 6.5.101 |
46ad658 to
2180087
Compare
|
Warning
详情 {
"export": {
"src/plugins/filemanager/dfmplugin-disk-encrypt-entry/menu/diskencryptmenuscene.cpp": {
"a": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
],
"b": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
]
}
}
} |
2180087 to
7e48f26
Compare
|
Warning
详情 {
"export": {
"src/plugins/filemanager/dfmplugin-disk-encrypt-entry/menu/diskencryptmenuscene.cpp": {
"a": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
],
"b": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
]
}
}
} |
7e48f26 to
50a24ca
Compare
|
Warning
详情 {
"export": {
"src/plugins/filemanager/dfmplugin-disk-encrypt-entry/menu/diskencryptmenuscene.cpp": {
"a": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
],
"b": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
]
}
}
} |
Changed disk encryption D-Bus methods to use file descriptors instead of direct variant maps for credential transmission Added secure pipe-based credential transmission mechanism between client and service Implemented QDataStream serialization for reliable credential data transfer Ensured immediate pipe closure after use to minimize credential exposure Modified DBus interface definitions to reflect new credential transmission method The changes improve security by ensuring sensitive credentials like passphrases are transmitted securely over D-Bus through file descriptors rather than potentially exposed in memory via QVariantMap. This reduces the risk of credential sniffing attacks during transmission. Influence: 1. Test all disk encryption operations (encrypt/decrypt/change passphrase) 2. Verify credentials are properly transmitted and received 3. Check error handling when pipes fail 4. Test with multiple simultaneous encryption operations 5. Verify compatibility with existing encrypted disks refactor: 增强磁盘加密凭证传输安全性 1. 修改磁盘加密D-Bus方法,使用文件描述符而非直接传输variant map来传递 凭证 2. 添加基于管道的安全凭证传输机制 3. 实现QDataStream序列化确保可靠的凭证数据传输 4. 确保使用后立即关闭管道最小化凭证暴露风险 5. 修改DBus接口定义以反映新的凭证传输方式 这些改动通过文件描述符安全传输敏感凭证而非使用QVariantMap内存传输,降低 了传输过程中凭证被窃取的风险。 Influence: 1. 测试所有磁盘加密操作(加密/解密/修改密码) 2. 验证凭证是否正确传输和接收 3. 检查管道失败时的错误处理 4. 测试多个并发加密操作 5. 验证与现有加密磁盘的兼容性
The changes refactor the credential transmission logic for disk encryption/decryption operations to use a common helper function. Previously each operation (re-encryption and decryption) had duplicate pipe creation and credential serialization code. Now this logic is centralized in sendCredentialsViaFd() which handles: 1. Pipe creation for secure credentials transfer 2. Credentials serialization using QDataStream 3. FD creation and cleanup 4. Synchronous/asynchronous D-Bus calls Benefits include: 1. Reduced code duplication 2. More consistent error handling 3. Improved log messages with method context 4. Proper pipe cleanup in all cases 5. Better separation of concerns Log: Improved security message when transmitting encryption credentials Influence: 1. Verify disk encryption still works with valid credentials 2. Test with invalid credentials to check error handling 3. Verify decryption works for encrypted devices 4. Check logs for credential transmission errors 5. Test cancellation during credential transmission refactor: 改进凭据传输安全性 该提交重构了磁盘加密/解密操作的凭据传输逻辑,改用通用辅助函数。之前每个 操作(重新加密和解密)都有重复的管道创建和凭据序列化代码。现在这些逻辑集 中在sendCredentialsViaFd()中,处理: 1. 用于安全凭据传输的管道创建 2. 使用QDataStream进行凭据序列化 3. 文件描述符创建和清理 4. 同步/异步D-Bus调用 优点包括: 1. 减少代码重复 2. 更一致的错误处理 3. 改进的带方法上下文的日志消息 4. 所有情况下正确的管道清理 5. 更好的关注点分离 Log: 改进传输加密凭据时的安全消息显示 Influence: 1. 验证使用有效凭据时磁盘加密仍能工作 2. 使用无效凭据测试错误处理 3. 验证对加密设备的解密功能 4. 检查凭据传输错误的日志 5. 测试凭据传输期间的取消操作
Changed disk encryption authentication from deprecated PKLA format to modern Polkit JavaScript rules format. Updated CMake installation configuration to install rules in new standard location (/usr/ share/polkit-1/rules.d) instead of old location (/etc/polkit-1/ localauthority/10-vendor.d). Removed old pkla file completely. This change was made because PKLA format has been deprecated in favor of JavaScript rules format which provides more flexibility and is the current standard for Polkit authentication rules. The new format follows modern Linux security practices and maintains better compatibility with newer Polkit versions. Influence: 1. Verify disk encryption functionality still works properly 2. Test authentication prompts appear/disappear as expected 3. Check system logs for any Polkit-related errors 4. Confirm rules file exists in correct location (/usr/share/polkit- 1/rules.d) 5. Test with different user permissions scenarios refactor: 从 pkla 迁移到 polkit 规则格式 将磁盘加密认证从已废弃的 PKLA 格式改为现代的 Polkit JavaScript 规则格 式。更新了 CMake 安装配置,将规则安装到新的标准位置(/usr/share/polkit-1/ rules.d)而非旧位置(/etc/polkit-1/localauthority/10-vendor.d)。完全移除了 旧的 pkla 文件。 进行此更改是因为 PKLA 格式已被废弃,转而支持 JavaScript 规则格式,后者提 供了更大的灵活性且是当前 Polkit 认证规则的标准。新格式遵循现代 Linux 安 全实践,并与较新的 Polkit 版本保持更好的兼容性。 Influence: 1. 验证磁盘加密功能仍正常工作 2. 测试认证提示是否按预期出现/消失 3. 检查系统日志是否有 Polkit 相关错误 4. 确认规则文件存在于正确位置(/usr/share/polkit-1/rules.d) 5. 使用不同用户权限场景进行测试
50a24ca to
ad79863
Compare
|
Warning
详情 {
"export": {
"src/plugins/filemanager/dfmplugin-disk-encrypt-entry/menu/diskencryptmenuscene.cpp": {
"a": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
],
"b": [
" { encrypt_param_keys::kKeyExportToPath, param.exportPath },"
]
}
}
} |
deepin pr auto review我来对这次代码修改进行审查:
建议改进:
static constexpr size_t kMaxCredentialsSize = 1024 * 1024; // 1MB
if (credentials.size() > kMaxCredentialsSize) {
fmCritical() << "Credentials data too large:" << credentials.size();
return false;
}
struct timeval tv;
tv.tv_sec = 5; // 5 seconds timeout
tv.tv_usec = 0;
setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
ssize_t written = 0;
const char *data = credentials.constData();
size_t remaining = credentials.size();
while (remaining > 0) {
ssize_t result = write(pipefd[1], data + written, remaining);
if (result < 0) {
if (errno == EINTR) continue;
fmCritical() << "Failed to write credentials to pipe:" << strerror(errno);
break;
}
written += result;
remaining -= result;
}
if (fd < 0) {
fmCritical() << "[parseCredentialsFromFd] Invalid file descriptor value:" << fd
<< "Error:" << strerror(errno);
return false;
}这些改进建议主要是为了增强代码的健壮性和安全性,特别是在处理系统资源和错误情况时。 |
|
/forcemerge |
Reviewer's GuideRefactored D-Bus credential passing to use anonymous pipes and QDBusUnixFileDescriptor for secure transmission, centralizing repeated code in sendCredentialsViaFd and parseCredentialsFromFd, updated server methods to parse credentials via FD, and revised D-Bus interfaces and polkit installation to support the new mechanism. Sequence diagram for secure credential transmission via file descriptor (FD)sequenceDiagram
participant Client as "DiskEncryptMenuScene (Client)"
participant D-Bus as "QDBusInterface (D-Bus)"
participant Service as "DiskEncryptSetup (Service)"
participant Private as "DiskEncryptSetupPrivate"
Client->>D-Bus: sendCredentialsViaFd(method, params, asyncCall)
D-Bus->>Service: D-Bus method call with QDBusUnixFileDescriptor
Service->>Private: parseCredentialsFromFd(credentialsFd, &args)
Private-->>Service: Parsed args
Service->>Service: Validate args, start worker
Service-->>Client: D-Bus reply (success/failure)
ER diagram for updated D-Bus method signatureserDiagram
DISK_ENCRYPT_MENU_SCENE ||--o{ QDBUS_INTERFACE : calls
QDBUS_INTERFACE ||--o{ DISK_ENCRYPT_SETUP : invokes
DISK_ENCRYPT_SETUP ||--o{ DISK_ENCRYPT_SETUP_PRIVATE : uses
DISK_ENCRYPT_SETUP }|--|{ QDBUS_UNIX_FILE_DESCRIPTOR : receives
DISK_ENCRYPT_SETUP_PRIVATE }|--|{ QDBUS_UNIX_FILE_DESCRIPTOR : parses
DISK_ENCRYPT_SETUP_PRIVATE }|--|{ QVARIANT_MAP : outputs
QDBUS_UNIX_FILE_DESCRIPTOR }|--|{ QVARIANT_MAP : contains credentials
Class diagram for DiskEncryptMenuScene and DiskEncryptSetup credential handlingclassDiagram
class DiskEncryptMenuScene {
+doReencryptDevice(param)
+doDecryptDevice(param)
+doChangePassphrase(param)
+sendCredentialsViaFd(iface, method, params, asyncCall)
}
class DiskEncryptSetup {
+InitEncryption(args)
+ResumeEncryption(args)
+Decryption(credentialsFd)
+ChangePassphrase(credentialsFd)
+SetupAuthArgs(credentialsFd)
}
class DiskEncryptSetupPrivate {
+parseCredentialsFromFd(credentialsFd, args)
}
DiskEncryptMenuScene --|> sendCredentialsViaFd
DiskEncryptSetup --|> DiskEncryptSetupPrivate
DiskEncryptSetupPrivate --|> parseCredentialsFromFd
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/plugins/filemanager/dfmplugin-disk-encrypt-entry/menu/diskencryptmenuscene.cpp:502-510` </location>
<code_context>
+ stream << params;
+
+ // Write credentials to pipe and close write end immediately
+ ssize_t written = write(pipefd[1], credentials.constData(), credentials.size());
+ close(pipefd[1]); // Close write end immediately after writing
+
+ if (written != credentials.size()) {
</code_context>
<issue_to_address>
**suggestion:** No retry or partial write handling for pipe write.
write() may not write all bytes at once, especially with large buffers. Loop until all bytes are written to guarantee complete transmission.
```suggestion
// Write credentials to pipe and close write end immediately
const char* buf = credentials.constData();
qint64 totalWritten = 0;
qint64 toWrite = credentials.size();
while (totalWritten < toWrite) {
ssize_t written = write(pipefd[1], buf + totalWritten, toWrite - totalWritten);
if (written < 0) {
fmCritical() << "Error writing credentials to pipe:" << strerror(errno);
close(pipefd[1]);
close(pipefd[0]);
return;
}
totalWritten += written;
}
close(pipefd[1]); // Close write end immediately after writing
if (totalWritten != credentials.size()) {
fmCritical() << "Failed to write all credentials to pipe, written:" << totalWritten << "expected:" << credentials.size();
close(pipefd[0]);
return;
}
```
</issue_to_address>
### Comment 2
<location> `src/services/diskencrypt/dbus/diskencryptsetup.cpp:424-429` </location>
<code_context>
+ char readBuffer[1024];
+ ssize_t bytesRead;
+
+ while ((bytesRead = read(fd, readBuffer, sizeof(readBuffer))) > 0) {
+ buffer.append(readBuffer, bytesRead);
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** No error handling for interrupted or failed read from pipe.
Handle EINTR and other recoverable errors in the read loop, or log errors to aid diagnosis.
```suggestion
char readBuffer[1024];
ssize_t bytesRead;
while (true) {
bytesRead = read(fd, readBuffer, sizeof(readBuffer));
if (bytesRead > 0) {
buffer.append(readBuffer, bytesRead);
} else if (bytesRead == 0) {
// EOF
break;
} else {
if (errno == EINTR) {
// Interrupted, retry
continue;
} else {
qWarning() << "[DiskEncryptSetupPrivate::parseCredentialsFromFd] Read error from pipe:" << strerror(errno);
break;
}
}
}
```
</issue_to_address>
### Comment 3
<location> `src/services/diskencrypt/dbus/diskencryptsetup.cpp:411-412` </location>
<code_context>
+{
+ Q_ASSERT(args);
+ // Validate file descriptor
+ if (!credentialsFd.isValid()) {
+ qWarning() << "[DiskEncryptSetupPrivate::parseCredentialsFromFd] Invalid file descriptor provided";
+ return false;
+ }
</code_context>
<issue_to_address>
**nitpick:** No logging of file descriptor value on invalidity.
Consider including the invalid file descriptor value in the log message to aid debugging.
</issue_to_address>
### Comment 4
<location> `src/services/diskencrypt/dbus/diskencryptsetup.cpp:407` </location>
<code_context>
return nullptr;
}
+bool DiskEncryptSetupPrivate::parseCredentialsFromFd(const QDBusUnixFileDescriptor &credentialsFd, QVariantMap *args)
+{
+ Q_ASSERT(args);
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring repeated FD-parsing and validation logic into a shared helper function and a template method to simplify your D-Bus stub implementations.
Here’s one way to cut down on the repeated logic in your D-Bus stubs and move the heavy FD-parsing out of DiskEncryptSetupPrivate into a small helper:
1) pull `parseCredentialsFromFd` out into a free function in, say, `helpers/dbus_utils.h/.cpp`:
```cpp
// helpers/dbus_utils.h
#pragma once
#include <QDBusUnixFileDescriptor>
#include <QVariantMap>
namespace dbus_utils {
static inline bool parseCredentials(const QDBusUnixFileDescriptor& ufd,
QVariantMap& out) {
if (!ufd.isValid()) return false;
int fd = ufd.fileDescriptor();
if (fd < 0) return false;
QByteArray buf;
char tmp[1024];
while (auto n = ::read(fd, tmp, sizeof(tmp)) > 0)
buf.append(tmp, n);
if (buf.isEmpty()) return false;
QDataStream ds(&buf, QIODevice::ReadOnly);
ds >> out;
return (ds.status() == QDataStream::Ok);
}
}
```
2) add a tiny wrapper in `DiskEncryptSetup` to factor out the common “check-auth → parse → validate → start” pattern:
```cpp
// in DiskEncryptSetup.h/.cpp
template<typename ValidateFn, typename StartFn>
bool DiskEncryptSetup::handleViaFd(
const QDBusUnixFileDescriptor& fd,
const char* action,
ValidateFn validate,
StartFn startWorker)
{
if (m_dptr->jobRunning) {
qWarning() << "[DiskEncryptSetup] job already running";
return false;
}
if (!m_dptr->checkAuth(action)) {
qWarning() << "[DiskEncryptSetup]" << action << "auth failed";
return false;
}
QVariantMap args;
if (!dbus_utils::parseCredentials(fd, args)) {
qCritical() << "[DiskEncryptSetup]" << action << "failed to parse credentials";
return false;
}
if (!validate(args)) {
qCritical() << "[DiskEncryptSetup]" << action << "invalid args:" << args;
return false;
}
return startWorker(args);
}
```
3) now your three stubs shrink to just:
```cpp
bool DiskEncryptSetup::Decryption(const QDBusUnixFileDescriptor& fd)
{
return handleViaFd(
fd,
kActionDecrypt,
[this](auto& args){ return m_dptr->validateDecryptArgs(args); },
[this](auto& args){
auto w = m_dptr->createDecryptWorker(args.value(...).toString(), args);
if (!w) return false;
m_dptr->initThreadConnection(w);
connect(w, &QThread::finished, m_dptr, &DiskEncryptSetupPrivate::onDecryptFinished);
w->start();
return true;
});
}
bool DiskEncryptSetup::ChangePassphrase(const QDBusUnixFileDescriptor& fd)
{
return handleViaFd(
fd,
kActionChgPwd,
[this](auto& args){ return m_dptr->validateChgPwdArgs(args); },
[this](auto& args){
auto w = new PassphraseChangeWorker(args);
connect(w, &QThread::finished, m_dptr, &DiskEncryptSetupPrivate::onPassphraseChanged);
w->start();
return true;
});
}
void DiskEncryptSetup::SetupAuthArgs(const QDBusUnixFileDescriptor& fd)
{
handleViaFd(
fd,
/* no auth check */ nullptr,
[](auto&){ return true; },
[this](auto& args){
Q_EMIT NotificationHelper::instance()->replyAuthArgs(args);
return true;
});
}
```
That:
• Removes the big `parseCredentialsFromFd` method from your private class
• Centralizes FD-burst parsing in one tiny helper
• Consolidates the repeated “auth→parse→validate→spawn” steps into a single template
All existing behavior stays the same, but each stub is now a dozen lines instead of 40+.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| if (written != credentials.size()) { | ||
| fmCritical() << "Failed to write credentials to pipe, written:" << written << "expected:" << credentials.size(); | ||
| close(pipefd[0]); | ||
| return; | ||
| } | ||
|
|
||
| // Create file descriptor for D-Bus transmission | ||
| QDBusUnixFileDescriptor fd(pipefd[0]); |
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.
suggestion: No retry or partial write handling for pipe write.
write() may not write all bytes at once, especially with large buffers. Loop until all bytes are written to guarantee complete transmission.
| // Write credentials to pipe and close write end immediately | |
| ssize_t written = write(pipefd[1], credentials.constData(), credentials.size()); | |
| close(pipefd[1]); // Close write end immediately after writing | |
| if (written != credentials.size()) { | |
| fmCritical() << "Failed to write credentials to pipe, written:" << written << "expected:" << credentials.size(); | |
| close(pipefd[0]); | |
| return; | |
| } | |
| // Write credentials to pipe and close write end immediately | |
| const char* buf = credentials.constData(); | |
| qint64 totalWritten = 0; | |
| qint64 toWrite = credentials.size(); | |
| while (totalWritten < toWrite) { | |
| ssize_t written = write(pipefd[1], buf + totalWritten, toWrite - totalWritten); | |
| if (written < 0) { | |
| fmCritical() << "Error writing credentials to pipe:" << strerror(errno); | |
| close(pipefd[1]); | |
| close(pipefd[0]); | |
| return; | |
| } | |
| totalWritten += written; | |
| } | |
| close(pipefd[1]); // Close write end immediately after writing | |
| if (totalWritten != credentials.size()) { | |
| fmCritical() << "Failed to write all credentials to pipe, written:" << totalWritten << "expected:" << credentials.size(); | |
| close(pipefd[0]); | |
| return; | |
| } |
| if (!credentialsFd.isValid()) { | ||
| qWarning() << "[DiskEncryptSetupPrivate::parseCredentialsFromFd] Invalid file descriptor provided"; |
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.
nitpick: No logging of file descriptor value on invalidity.
Consider including the invalid file descriptor value in the log message to aid debugging.
|
This pr force merged! (status: blocked) |
Summary by Sourcery
Refactor disk encryption plugin and service to securely transmit credentials over D-Bus using file descriptors instead of QVariantMap arguments
Enhancements:
Build: