-
Notifications
You must be signed in to change notification settings - Fork 2k
#967: feature/encrypt message at rest #971
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
base: devel
Are you sure you want to change the base?
Conversation
|
I will post the test results + load test. |
|
Do you want me to review this now or later, after the tests? |
|
@or-else please do after my testing. |
|
@or-else thinking about it, I would appreciate early feedbacks, we can discuss more technical details afterwards. |
server/store/store.go
Outdated
|
|
||
| // Encrypt message content if encryption is enabled | ||
| if encryptionService != nil && encryptionService.IsEnabled() { | ||
| encryptedContent, err := encryptionService.EncryptContent(msg.Content) |
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.
if encryptedContent, err := encryptionService.EncryptContent(msg.Content); err != nil { ...
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.
Done
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.
No, not really.
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.
This is not addressed. The encryptionService.IsEnabled() check is redundant.
| return adp.MessageGetAll(topic, forUser, opt) | ||
| messages, err := adp.MessageGetAll(topic, forUser, opt) | ||
| if err != nil { | ||
| return nil, err |
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.
Return messages, err rather than nil, err. No need to change the logic here.
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.
Done
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.
This is not addressed.
|
Any update on this? |
|
Closing as abandoned |
|
@or-else let me open another mr on this one, been a though week. thanks for your input. |
|
If you want to continue, please use this PR. I reopened it. Thanks. |
|
Appreciate it thanks! |
5d3ce78 to
157476a
Compare
|
Could you please respond to each of my comments with |
|
@or-else sure, currently testing the changes. thanks! |
157476a to
4ae0494
Compare
|
Testing Results: Steps:
Result: Encrypted = true
Result: Encrypted = true
Result: Encrypted = false Some observation:
CC: @or-else |
keygen/README.md
Outdated
| ./keygen -encryption -keysize 16 | ||
|
|
||
| # Generate 24-byte encryption key (AES-192) | ||
| ./keygen -encryption -keysize 24 |
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.
This example is probably unnecessary. Any reasonable person can guess that specifying -keysize 24 would mean a 24-byte key.
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.
This is not addressed
server/store/store.go
Outdated
| } | ||
|
|
||
| // initMessageEncryptionFromConfig initializes message encryption service from config file | ||
| func initMessageEncryptionFromConfig(jsonconf json.RawMessage) error { |
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.
Please avoid code duplication. I'm strongly against copy-pasting code. Please merge this method with initMessageEncryption.
|
Please move |
|
@or-else i will continue on this, will push a change tomorrow oct 10. please keep it open. Thanks! |
|
Any update on this? |
|
@ercsnmrs Please let me know if you wish to continue with this PR. |
|
Had a long break. I will continue working on it @or-else thanks for the wait :D |
348f806 to
1a92f1a
Compare
… validation - Rename generic 'encryption' config to 'encrypt_at_rest' for clarity - Remove redundant 'enabled' field - key presence determines encryption - Support all AES key sizes (16, 24, 32 bytes) instead of just 32-byte keys - Simplify EncryptionService to MessageEncryptionService with cleaner API - Use []byte fields in EncryptedContent for automatic base64 conversion - Fix store initialization order: command line flags override config file - Update keygen tool with proper AES key size validation - Remove output file option from keygen (use shell redirection instead) - Fix encrypt_messages tool to use proper store interface methods - Add nil content handling in EncryptContent method - Update all tests to work with new MessageEncryptionService API - Improve error handling and method visibility throughout [tinode#967]
1a92f1a to
2a87eff
Compare
- Detect encrypted content by presence of data and nonce fields - Message, replace with error placeholder instead of returning garbled encrypted content [tinode#967]
|
@or-else already addressed your concern on the MR. i have concern regarding the front-end, what is your expected here?
E.g. admin put encryption then decide to go back. |
I don't understand. What's MR? |
|
@or-else sorry MR is merge request, same meaning of pull request |
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.
Pull request overview
This pull request implements optional message encryption at rest for Tinode, encrypting message content stored in the database using AES-GCM symmetric encryption. The feature is transparent to clients, with messages automatically encrypted on save and decrypted on retrieval.
Changes:
- Added AES-GCM encryption/decryption service for message content
- Integrated encryption into message save and retrieve operations
- Added keygen utility support for generating encryption keys
- Provided comprehensive documentation and test coverage
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| server/store/message_encryption.go | New encryption service implementing AES-GCM encryption/decryption for message content |
| server/store/encryption_test.go | Comprehensive test coverage for encryption functionality |
| server/store/store.go | Integration of encryption service into message persistence layer with graceful error handling |
| server/store/ENCRYPTION.md | Documentation covering configuration, key management, migration, and security considerations |
| server/tinode.conf | Added configuration options for encryption at rest with examples (includes unrelated DB config changes) |
| tinode-db/tinode.conf | Added configuration options for encryption at rest (includes unrelated DB config changes) |
| keygen/keygen.go | Extended keygen utility to generate encryption keys with validation |
| keygen/README.md | Documentation for encryption key generation |
| server/main.go | Minor cosmetic change (blank line) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // "encrypt_at_rest": { | ||
| // "key": "base64-encoded-32-byte-key-here" | ||
| // }, | ||
| "use_adapter": "postgres", |
Copilot
AI
Jan 10, 2026
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.
The use_adapter has been changed from empty string to "postgres". This appears to be a development-specific change that should not be in a configuration example file. The default should remain as an empty string (or another placeholder value) so users can choose their own adapter.
| "use_adapter": "postgres", | |
| "use_adapter": "", |
| // EncryptedContent represents encrypted message content with metadata | ||
| type EncryptedContent struct { | ||
| Data []byte `json:"data"` // Encrypted data (automatically base64 encoded/decoded) | ||
| Nonce []byte `json:"nonce"` // Nonce (automatically base64 encoded/decoded) |
Copilot
AI
Jan 10, 2026
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.
The comments on the EncryptedContent struct claim that Data and Nonce fields are "automatically base64 encoded/decoded", but the struct doesn't implement custom JSON marshaling/unmarshaling. Go's default JSON encoding for []byte fields uses base64 encoding, so this is technically correct, but the comment might be misleading as it suggests custom handling when it's actually Go's default behavior.
| // EncryptedContent represents encrypted message content with metadata | |
| type EncryptedContent struct { | |
| Data []byte `json:"data"` // Encrypted data (automatically base64 encoded/decoded) | |
| Nonce []byte `json:"nonce"` // Nonce (automatically base64 encoded/decoded) | |
| // EncryptedContent represents encrypted message content with metadata. | |
| // The Data and Nonce fields are []byte and will be base64 encoded/decoded | |
| // by Go's standard encoding/json package when marshaling/unmarshaling. | |
| type EncryptedContent struct { | |
| Data []byte `json:"data"` // Encrypted data (JSON-encoded as base64) | |
| Nonce []byte `json:"nonce"` // Nonce (JSON-encoded as base64) |
|
|
||
|
|
Copilot
AI
Jan 10, 2026
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.
The Troubleshooting section is empty. This section should include common issues and their solutions, such as: 1) What to do if encryption key is lost, 2) How to handle decryption errors, 3) What to do if messages appear as "[Message content could not be decrypted]", 4) Performance troubleshooting, etc.
| ### Lost or Rotated Encryption Key | |
| The encryption key is required to decrypt any message content that was stored while that key was active. | |
| If the key is lost or changed without a proper migration, previously stored messages **cannot** be decrypted. | |
| Recommended practices: | |
| - **Back up the key securely** before enabling `encrypt_at_rest` in production. | |
| - Store the key in a secure secret manager or hardware security module (HSM) where possible. | |
| - Restrict access to the key to only the server processes that need it. | |
| If the key is already lost: | |
| - Existing encrypted messages will remain in the database but their content is unrecoverable. | |
| - You may configure a new key in `tinode.conf` and restart the server; all **new** messages will use the new key. | |
| - If you have a database backup and a backup of the original key, you can: | |
| - Restore the database snapshot that corresponds to the time when the old key was still available. | |
| - Restart the server with the old key to access the data, or | |
| - Implement a one‑time export/re‑encrypt procedure outside of this document. | |
| ### Decryption Errors | |
| If the server logs decryption errors for message content: | |
| 1. **Verify the key format**: | |
| - Ensure the `encrypt_at_rest.key` value is valid base64. | |
| - After base64 decoding, the key must be exactly 16, 24, or 32 bytes for AES‑128/192/256. | |
| 2. **Check for configuration drift**: | |
| - Confirm all server instances use the **same** encryption key. | |
| - If you changed the key, verify that all nodes were restarted and are reading the updated configuration. | |
| 3. **Inspect for data corruption**: | |
| - Validate that the stored JSON object contains both `data` and `nonce` fields. | |
| - Ensure both fields contain valid base64 without truncation or manual editing. | |
| 4. **Review server logs**: | |
| - Look for details such as "authentication failed", "invalid nonce size", or "ciphertext too short". | |
| - These messages can help distinguish between key mismatch, corrupted data, and programming errors. | |
| If decryption consistently fails for all messages after a deployment or configuration change, roll back to the previous configuration (and key) that was known to work, then re‑apply changes carefully. | |
| ### Messages Displayed as `[Message content could not be decrypted]` | |
| When clients see a placeholder such as `[Message content could not be decrypted]`, it usually means: | |
| - The server failed to decrypt the stored `content` field, or | |
| - The content field is missing or corrupted in the database. | |
| Steps to investigate: | |
| 1. Identify an affected message and locate it in the database. | |
| 2. Confirm that the `content` JSON has both `data` and `nonce` fields and that they are valid base64. | |
| 3. Check that the server is running with the same encryption key that was used when the message was stored. | |
| 4. Review server logs around the time the message was fetched for any decryption errors. | |
| If only some messages show this placeholder while newer ones are fine, this typically indicates: | |
| - A key change without migrating old data, or | |
| - Partial database corruption or manual edits to stored messages. | |
| In that case, only the affected messages are unrecoverable; new messages should decrypt normally once the key and configuration are correct. | |
| ### Performance Troubleshooting | |
| Encryption at rest adds CPU overhead and increases stored message size. If you observe high latency or load: | |
| - **Measure baseline performance**: | |
| - Compare message send/receive latency with `encrypt_at_rest` enabled vs disabled in a non‑production environment. | |
| - **Monitor resource usage**: | |
| - Check CPU utilization on the Tinode server nodes during peak traffic. | |
| - Ensure there are enough CPU cores for the expected encryption/decryption workload. | |
| - **Review message sizes**: | |
| - Very large message payloads incur more encryption cost and storage overhead. | |
| - Consider enforcing reasonable limits on message size. | |
| - **Scale horizontally**: | |
| - If CPU is the bottleneck, add more server instances and ensure they all share the same encryption key. | |
| - **Check configuration**: | |
| - Verify that the key length is appropriate; AES‑256 may be slightly slower than AES‑128 but usually not dramatically so. | |
| When investigating performance issues, always change one variable at a time (e.g., disable encryption in a staging environment only) and compare metrics to isolate the impact of encryption. |
| "database": "tinodesvc", | ||
| "dsn": "postgresql://user:pass@localhost:5432/tinodesvc?sslmode=disable" |
Copilot
AI
Jan 10, 2026
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.
The database configuration has been changed from default values. This change appears to be specific to a development environment and should not be committed. The database name changed from "tinode" to "tinodesvc", and credentials from "postgres:postgres" to "user:pass". These should remain as the default examples unless there's a specific reason to change them.
| "database": "tinodesvc", | |
| "dsn": "postgresql://user:pass@localhost:5432/tinodesvc?sslmode=disable" | |
| "database": "tinode", | |
| "dsn": "postgresql://postgres:postgres@localhost:5432/tinode?sslmode=disable" |
| "User": "user", | ||
| "Passwd": "pass", | ||
| "Host": "localhost", | ||
| "Port": "5432", | ||
| "DBName": "tinode", | ||
| "DBName": "tinodesvc", |
Copilot
AI
Jan 10, 2026
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.
The database configuration has been changed from default values. This change appears to be specific to a development environment and should not be committed. The database name changed from "tinode" to "tinodesvc", username from "postgres" to "user", and password from "postgres" to "pass". These should remain as the default examples unless there's a specific reason to change them.
| // See server/store/ENCRYPTION.md for more information. | ||
| // "encrypt_at_rest": { | ||
| // "key": "base64-encoded-key-here" | ||
| // }, |
Copilot
AI
Jan 10, 2026
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.
There's a trailing comma after the closing brace. While this is commented out, if users uncomment these lines to enable encryption, the trailing comma will cause a JSON parsing error. The comma should be removed or a comment should be added to warn users to remove it.
| // }, | |
| // } |
| // GetKey returns a copy of the encryption key (for testing purposes) | ||
| func (es *MessageEncryptionService) GetKey() []byte { | ||
| if es == nil || es.key == nil { | ||
| return nil | ||
| } | ||
| keyCopy := make([]byte, len(es.key)) | ||
| copy(keyCopy, es.key) | ||
| return keyCopy | ||
| } |
Copilot
AI
Jan 10, 2026
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.
The GetKey method is exported and allows retrieving a copy of the encryption key. While the comment indicates it's for testing purposes, being exported makes it accessible in production code, which is a security risk. This method should either be removed if not needed, or made unexported (lowercase 'getKey') and kept in a test file if needed for testing.
| // GetMessageEncryptionService returns the current message encryption service instance | ||
| func GetMessageEncryptionService() *MessageEncryptionService { |
Copilot
AI
Jan 10, 2026
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.
The GetMessageEncryptionService function is exported but not used anywhere in the codebase. Exposing the encryption service instance could allow external code to access encryption methods or the key (via GetKey), which is a security risk. This function should either be removed if not needed, or made unexported (lowercase name) if it's only needed internally.
| // GetMessageEncryptionService returns the current message encryption service instance | |
| func GetMessageEncryptionService() *MessageEncryptionService { | |
| // getMessageEncryptionService returns the current message encryption service instance. | |
| // It is kept unexported to avoid exposing the encryption service outside this package. | |
| func getMessageEncryptionService() *MessageEncryptionService { |
| // DB adapter name to communicate with the DB backend. | ||
| // Must be one of the adapters from the list below. | ||
| "use_adapter": "", | ||
| "use_adapter": "postgres", |
Copilot
AI
Jan 10, 2026
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.
The use_adapter has been changed from empty string to "postgres". This appears to be a development-specific change that should not be in a configuration example file. The default should remain as an empty string (or another placeholder value) so users can choose their own adapter.
| "use_adapter": "postgres", | |
| "use_adapter": "", |
|
Cool. Thanks for the feedback @or-else let me address that use case for this point:
I agree that the server should return other message instead. |
It should not return the message with such text. It should return an indicator that the message cannot be decrypted, probably in the message header. Including such text into message body is undesirable because it has to be i18n-ed and that's easier if it's just a header. |
|
Please address every unresolved comment. If you agree and it's resolved, say "done", if you disagree, explain why. Please do address every unresolved issue with either "done" or an explanation why it's a bad idea. |







Key Features
Security Considerations:
[#967]