-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat(raftwal): Add support for encryption in raftwal #6714
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.
LGTM. 32 rules errored during the review.
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.
4 issues found. 11 rules errored during the review.
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.
Got some comments.
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.
Once you write the key ID to the file, use that to determine if you should encrypt any data written to that file or not. Don't continuously check for the WorkerConfig. Do the same when reading data, see if it has a key ID, if so, look into the key registry. If we have a valid key ID, but no corresponding key registry, then we should error out.
Dismissed @codelingo[bot] and @jarifibrahim from 2 discussions.
Reviewable status: 0 of 6 files reviewed, 13 unresolved discussions (waiting on @ahsanbarkati, @codelingo[bot], @manishrjain, and @vvbalaji-dgraph)
raftwal/log.go, line 321 at r2 (raw file):
Previously, codelingo[bot] wrote…
Prefer fixed size array allocations on stack over heap
Could just directly point buf to the corresponding lf.Data.
raftwal/log.go, line 51 at r4 (raw file):
// logFileOffset is offset in the log file where data is stored. logFileOffset = 1 << 20 // 1MB // encryptionKeyOffset is offset in the log file where encryption key is stored.
The first baseIVsize bytes are for base IV and the rest of the bytes are for key ID.
raftwal/log.go, line 113 at r4 (raw file):
InMemory: false, } if lf.registry, err = badger.OpenKeyRegistry(krOpt); err != nil {
This won't open Badger. It would only use its key registry.
raftwal/log.go, line 120 at r4 (raw file):
fpath := logFname(dir, fid) // Open the file in read-write mode and create it if it doesn't exist yet. lf.MmapFile, err = z.OpenMmapFile(fpath, os.O_RDWR|os.O_CREATE, logFileSize)
If I have an existing file, which isn't encrypted, how do I know? Can I read that file?
If the key ID is zero, then we know.
raftwal/log.go, line 132 at r4 (raw file):
} else { if len(encKey) != 0 { // Openend an existing file.
typo?
raftwal/log.go, line 134 at r4 (raw file):
// Openend an existing file. buf := make([]byte, 16) copy(buf, lf.Data[encryptionKeyOffset:])
buf := lf.Data[encryptionKeyoffset:+16]
raftwal/log.go, line 178 at r4 (raw file):
// Decrypt the data if encryption is enabled. if x.WorkerConfig.EncryptionKey != nil && len(re.Data) > 0 { // No need to worry about mmap. because, XORBlock allocates a byte array to do the
s/because/Because
raftwal/log.go, line 179 at r4 (raw file):
if x.WorkerConfig.EncryptionKey != nil && len(re.Data) > 0 { // No need to worry about mmap. because, XORBlock allocates a byte array to do the // xor. So, the given slice is not being mutated.
XOR
raftwal/log.go, line 333 at r4 (raw file):
// Initialize base IV. lf.baseIV = buf[8:] copy(lf.Data[encryptionKeyOffset:], buf)
No need to copy after.
raftwal/wal.go, line 177 at r4 (raw file):
// If encryption is enabled then encrypt the data. if x.WorkerConfig.EncryptionKey != nil {
If there's a valid curr.dataKey, then we should encrypt. Because, you're going to read that file later based on whether it has the key or not.
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.
LGTM. 21 rules errored during the review.
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.
LGTM. 25 rules errored during the review.
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.
LGTM. 25 rules errored during the review.
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.
Reviewed 2 of 6 files at r1, 1 of 4 files at r2, 3 of 3 files at r5.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @ahsanbarkati, @codelingo[bot], and @vvbalaji-dgraph)
raftwal/log.go, line 132 at r4 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
typo?
[encOffset: encOffset + 16]
raftwal/log.go, line 55 at r5 (raw file):
baseIVsize = 8 // keyIDsize is the size of keyID. keyIDsize = 8
Just remove keyIDsize.
raftwal/log.go, line 138 at r5 (raw file):
} else { buf := lf.Data[encryptionKeyOffset : encryptionKeyOffset+baseIVsize+keyIDsize] keyID := binary.BigEndian.Uint64(buf[:keyIDsize])
buf[:8]
raftwal/log.go, line 150 at r5 (raw file):
return nil, err } lf.baseIV = buf[keyIDsize:]
buf[8:]
raftwal/log.go, line 189 at r5 (raw file):
decoded, err := y.XORBlockAllocate( re.Data, lf.dataKey.Data, lf.generateIV(entry.DataOffset())) x.Check(err)
Just a note saying that we could potentially put these on an allocator.
This PR adds support for encryption of the data in raftwal. It also adds some of the related tests.
This change is
Docs Preview: