Conversation
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
|
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.
| 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;
| 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.
Core
ENTRYGET_PUBKEYrequestGET_PUBKEYrequestGUI
Test