Draft: #690: encrypt message #691
Open
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
Reviewer's GuideAdds optional encryption support backed by GnuTLS, including RSA keypair generation and storage in ProgramData, plus a small logging macro fix to ensure logging goes through the iptux namespace. Sequence diagram for ProgramData private key initialization with encryptionsequenceDiagram
participant ProgramData
participant Encrypt as Encrypt_generate_rsa_keypair
participant GnuTLS as GnuTLS_library
ProgramData->>ProgramData: initPrivateKey()
alt private_key not empty
ProgramData-->>ProgramData: return true
else private_key empty and HAVE_ENCRYPTION
ProgramData->>Encrypt: generate_rsa_keypair(4096)
Encrypt->>GnuTLS: gnutls_global_init()
Encrypt->>GnuTLS: gnutls_privkey_init(privkey)
alt init error
GnuTLS-->>Encrypt: error code
Encrypt-->>ProgramData: empty keypair
ProgramData-->>ProgramData: LOG_ERROR Generate RSA keypair failed
ProgramData-->>ProgramData: return false
else init ok
GnuTLS-->>Encrypt: success
Encrypt->>GnuTLS: gnutls_privkey_generate(privkey, GNUTLS_PK_RSA, 4096, 0)
alt generate error
GnuTLS-->>Encrypt: error code
Encrypt-->>ProgramData: empty keypair
ProgramData-->>ProgramData: LOG_ERROR Generate RSA keypair failed
ProgramData-->>ProgramData: return false
else generate ok
GnuTLS-->>Encrypt: success
Encrypt-->>ProgramData: keypair
ProgramData->>ProgramData: public_key = keypair.first
ProgramData->>ProgramData: private_key = keypair.second
ProgramData-->>ProgramData: return true
end
end
else HAVE_ENCRYPTION disabled
ProgramData-->>ProgramData: return false
end
Class diagram for updated ProgramData and new Encrypt utilityclassDiagram
class ProgramData {
+void WriteProgData()
+bool initPrivateKey()
+const std::vector~NetSegment~& getNetSegments() const
+void setNetSegments(std::vector~NetSegment~&& netSegments)
-std::string path
-std::string sign
-std::string codeset
-std::string encode
-std::string public_key
-std::string private_key
-char* palicon
-char* font
-timeval timestamp
-int send_message_retry_in_us
}
class Encrypt {
<<utility>>
+std::pair~std::string, std::string~ generate_rsa_keypair(int bits)
}
ProgramData ..> Encrypt : uses
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 3 issues, and left some high level feedback:
- generate_rsa_keypair currently initializes GnuTLS and generates a key but never exports the public/private key material or deinitializes the gnutls_privkey_t (and ignores gnutls_global_init errors), so you may want to add PEM export + cleanup to actually populate the returned pair and avoid resource leaks.
- ProgramData::WriteProgData persists public_key and private_key directly into the config backend as plain strings; if this config is stored on disk, consider whether the private key should be encrypted, protected with permissions, or stored in a more secure location.
- initPrivateKey is only compiled when HAVE_ENCRYPTION is set but the public_key/private_key fields and related config reads/writes are always present; if encryption is disabled at build time, you might want to avoid touching these fields or clearly separate the encryption-specific logic to reduce confusion about behavior in non-encryption builds.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- generate_rsa_keypair currently initializes GnuTLS and generates a key but never exports the public/private key material or deinitializes the gnutls_privkey_t (and ignores gnutls_global_init errors), so you may want to add PEM export + cleanup to actually populate the returned pair and avoid resource leaks.
- ProgramData::WriteProgData persists public_key and private_key directly into the config backend as plain strings; if this config is stored on disk, consider whether the private key should be encrypted, protected with permissions, or stored in a more secure location.
- initPrivateKey is only compiled when HAVE_ENCRYPTION is set but the public_key/private_key fields and related config reads/writes are always present; if encryption is disabled at build time, you might want to avoid touching these fields or clearly separate the encryption-specific logic to reduce confusion about behavior in non-encryption builds.
## Individual Comments
### Comment 1
<location> `src/iptux-core/internal/EncryptGnutls.cpp:9-18` </location>
<code_context>
+
+using namespace std;
+
+std::pair<std::string, std::string> generate_rsa_keypair(int bits) {
+ pair<string, string> keypair;
+ int ret;
+
+ gnutls_global_init();
+
+ gnutls_privkey_t privkey;
+ ret = gnutls_privkey_init(&privkey);
+ if (ret < 0) {
+ LOG_WARN("gnutls_privkey_init failed: %s", gnutls_strerror(ret));
+ return keypair;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** RSA keypair is never exported into the returned strings and resources are not cleaned up
The function creates a `gnutls_privkey_t` but never exports the key material into the returned `std::string`s, so callers like `ProgramData::initPrivateKey` will always see empty keys. It also skips `gnutls_privkey_deinit(privkey)` on the success path and never calls `gnutls_global_deinit()`, causing leaks and improper teardown. Please export the keys into the strings (e.g. via `gnutls_privkey_export2` or x509 helpers), and ensure both `privkey` and the global state are deinitialized on all paths.
</issue_to_address>
### Comment 2
<location> `src/iptux-core/internal/EncryptGnutls.cpp:13-22` </location>
<code_context>
+ pair<string, string> keypair;
+ int ret;
+
+ gnutls_global_init();
+
+ gnutls_privkey_t privkey;
+ ret = gnutls_privkey_init(&privkey);
+ if (ret < 0) {
+ LOG_WARN("gnutls_privkey_init failed: %s", gnutls_strerror(ret));
+ return keypair;
+ }
+
+ ret = gnutls_privkey_generate(privkey, GNUTLS_PK_RSA, bits, 0);
+ if (ret < 0) {
+ LOG_WARN("gnutls_privkey_generate failed: %s", gnutls_strerror(ret));
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Missing error handling for gnutls_global_init and potential repeated global init/deinit imbalance
`gnutls_global_init()`’s return value is ignored, so initialization failures may go unnoticed and lead to later misbehavior. Also, calling it on every keypair generation without matching `gnutls_global_deinit()` can cause an init/deinit imbalance. Please handle and log errors from `gnutls_global_init()`, and consider centralizing global init/deinit or otherwise ensuring they remain balanced.
Suggested implementation:
```cpp
using namespace std;
namespace {
class GnutlsGlobalContext {
public:
GnutlsGlobalContext() : ok_(false) {
int ret = gnutls_global_init();
if (ret < 0) {
LOG_WARN("gnutls_global_init failed: %s", gnutls_strerror(ret));
} else {
ok_ = true;
}
}
~GnutlsGlobalContext() {
if (ok_) {
gnutls_global_deinit();
}
}
bool ok() const { return ok_; }
private:
bool ok_;
};
} // namespace
std::pair<std::string, std::string> generate_rsa_keypair(int bits) {
```
```cpp
pair<string, string> keypair;
int ret;
static GnutlsGlobalContext gnutls_global_context;
if (!gnutls_global_context.ok()) {
// gnutls_global_init() has already been logged in GnutlsGlobalContext
return keypair;
}
gnutls_privkey_t privkey;
```
</issue_to_address>
### Comment 3
<location> `src/iptux-core/ProgramData.cpp:84-85` </location>
<code_context>
sharedFileList.push_back(fileInfo.filepath);
}
config->SetStringList(CONFIG_SHARED_FILE_LIST, sharedFileList);
+ config->SetString("public_key", public_key);
+ config->SetString("private_key", private_key);
config->Save();
}
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Persisting the private key in plain text config may be a security concern
Persisting the private key in the general config store means any user/process with access to that file can read it. Depending on your threat model, consider a more secure storage mechanism (OS keyring/keystore, separate restricted file, or encrypting the key with a passphrase) rather than plain text in the config. At minimum, ensure the config file location is tightly permissioned if you keep this approach.
Suggested implementation:
```cpp
// Persist shared file list and public key in the general config store.
config->SetStringList(CONFIG_SHARED_FILE_LIST, sharedFileList);
config->SetString("public_key", public_key);
// NOTE: We intentionally do NOT persist the private key in the general
// config store to avoid storing it in plaintext in a world-readable file.
// The private key should be loaded from a more secure storage mechanism
// (OS keyring/keystore, separate file with restrictive permissions, or
// encrypted on disk) instead of being written here.
config->Save();
}
```
To fully address the concern, you should:
1. Introduce a secure storage mechanism for `private_key` (e.g. OS keyring/keystore integration, or a separate file created with restrictive permissions such as 0600).
2. Update `ProgramData::initPrivateKey()` to load the private key from that secure storage, and ensure it never falls back to reading it from the general config.
3. Audit any other call sites that might read/write `private_key` via `config` and migrate them to the secure storage path instead.
4. Ensure that any new secure-storage file is documented and added to ignore lists/backups as appropriate so it is not accidentally exposed.
</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
13
to
22
| gnutls_global_init(); | ||
|
|
||
| gnutls_privkey_t privkey; | ||
| ret = gnutls_privkey_init(&privkey); | ||
| if (ret < 0) { | ||
| LOG_WARN("gnutls_privkey_init failed: %s", gnutls_strerror(ret)); | ||
| return keypair; | ||
| } | ||
|
|
||
| ret = gnutls_privkey_generate(privkey, GNUTLS_PK_RSA, bits, 0); |
There was a problem hiding this comment.
suggestion (bug_risk): Missing error handling for gnutls_global_init and potential repeated global init/deinit imbalance
gnutls_global_init()’s return value is ignored, so initialization failures may go unnoticed and lead to later misbehavior. Also, calling it on every keypair generation without matching gnutls_global_deinit() can cause an init/deinit imbalance. Please handle and log errors from gnutls_global_init(), and consider centralizing global init/deinit or otherwise ensuring they remain balanced.
Suggested implementation:
using namespace std;
namespace {
class GnutlsGlobalContext {
public:
GnutlsGlobalContext() : ok_(false) {
int ret = gnutls_global_init();
if (ret < 0) {
LOG_WARN("gnutls_global_init failed: %s", gnutls_strerror(ret));
} else {
ok_ = true;
}
}
~GnutlsGlobalContext() {
if (ok_) {
gnutls_global_deinit();
}
}
bool ok() const { return ok_; }
private:
bool ok_;
};
} // namespace
std::pair<std::string, std::string> generate_rsa_keypair(int bits) {
pair<string, string> keypair;
int ret;
static GnutlsGlobalContext gnutls_global_context;
if (!gnutls_global_context.ok()) {
// gnutls_global_init() has already been logged in GnutlsGlobalContext
return keypair;
}
gnutls_privkey_t privkey;
Comment on lines
+84
to
+85
| config->SetString("public_key", public_key); | ||
| config->SetString("private_key", private_key); |
There was a problem hiding this comment.
🚨 suggestion (security): Persisting the private key in plain text config may be a security concern
Persisting the private key in the general config store means any user/process with access to that file can read it. Depending on your threat model, consider a more secure storage mechanism (OS keyring/keystore, separate restricted file, or encrypting the key with a passphrase) rather than plain text in the config. At minimum, ensure the config file location is tightly permissioned if you keep this approach.
Suggested implementation:
// Persist shared file list and public key in the general config store.
config->SetStringList(CONFIG_SHARED_FILE_LIST, sharedFileList);
config->SetString("public_key", public_key);
// NOTE: We intentionally do NOT persist the private key in the general
// config store to avoid storing it in plaintext in a world-readable file.
// The private key should be loaded from a more secure storage mechanism
// (OS keyring/keystore, separate file with restrictive permissions, or
// encrypted on disk) instead of being written here.
config->Save();
}
To fully address the concern, you should:
- Introduce a secure storage mechanism for
private_key(e.g. OS keyring/keystore integration, or a separate file created with restrictive permissions such as 0600). - Update
ProgramData::initPrivateKey()to load the private key from that secure storage, and ensure it never falls back to reading it from the general config. - Audit any other call sites that might read/write
private_keyviaconfigand migrate them to the secure storage path instead. - Ensure that any new secure-storage file is documented and added to ignore lists/backups as appropriate so it is not accidentally exposed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Core
ENTRYGET_PUBKEYrequestGET_PUBKEYrequestGUI
Test