feat(nodes): encrypt all records before disk, decrypt on get #1229
Conversation
joshuef
force-pushed
the
NodesEncrypt
branch
3 times, most recently
from
643e41c to
23d4f49
Compare
reviewpad
bot
added
Medium
joshuef
force-pushed
the
NodesEncrypt
branch
4 times, most recently
from
82176d4 to
c52952c
Compare
sn_networking/src/record_store.rs
Outdated
| nonce_bytes.extend_from_slice(r.key.as_ref()); | ||
| // Ensure the final nonce is exactly 96 bits long by padding or truncating as necessary | ||
| // https://crypto.stackexchange.com/questions/26790/how-bad-it-is-using-the-same-iv-twice-with-aes-gcm | ||
| nonce_bytes.resize(12, 0); // 12 (u8) * 8 = 96 bits |
There was a problem hiding this comment.
Maybe a function nonce(starter, key) -> GenericArray could help keep consistency instead of repeating the logic from L144?
The println below on L302 looks like a leftover, should it be removed?
Otherwise looking good :)
|
I thought this was no longer needed when the minimum file size for encryption was reduced to three bytes. Is there still a need for this? |
|
@happybeing this is not about users encrypting data, but preventing node operators from storing unencrypted data to disk. |
joshuef
force-pushed
the
NodesEncrypt
branch
2 times, most recently
from
8ca0942 to
69933b8
Compare
|
Updated for that @iancoleman . Looking pretty tidy there at last I think. |
Exactly, and no longer needed because of the reduction of min chunk size for encryption to three bytes. Since all chunks are now encrypted before upload there's no need for nodes to do this. Unless you are worried about one and two byte chunks? |
A malicious client may not encrypt before upload, ie send plaintext bytes to nodes, so the node protects itself by encrypting before storing on disk. |
| @@ -56,6 +61,9 @@ pub struct NodeRecordStore { | |||
| record_count_metric: Option<Gauge>, | |||
| /// Counting how many times got paid | |||
| received_payment_count: usize, | |||
| /// Encyption cipher for the records, randomly generated at node startup | |||
| /// Plus a 4 byte nonce starter | |||
| encryption_details: (Aes256GcmSiv, [u8; 4]), | |||
There was a problem hiding this comment.
As I understand, this encryption_details shall be written to disk in case restart of node is allowed.
Otherwise, if a node restart with the same key, it won't be able to read the previously stored records.
ideally, I think the Aes cipher and the nonce shall be deduced from the private key instead, to avoid have to store extra stuff to disk.
There was a problem hiding this comment.
I was not imagining to store this to disk. Node restart here will necesitate wiping the data (I thought any restart is always with new keys at the moment, eg).
fixes maidsafe#1158 + also icnrases benchmark limit to account for extra work done here
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
fixes #1158
Description
Summary generated by Reviewpad on 25 Jan 24 15:08 UTC
This pull request introduces a new feature to the codebase. It modifies the
NodeRecordStoreto encrypt all records before writing them to disk and decrypts them when retrieving. The encryption key is randomly generated at node startup. Additionally, the patch includes some bug fixes and code improvements.