Support encryption at rest

[poonai]

This PR contains encryption for sst and vlog.
I'll raise seperate pr for rotation tool

---

This change is 

[pullrequest]

✅ A review job has been created and sent to the PullRequest network.

---

Check the status or cancel PullRequest code review here.

[pullrequest]

	1 Message
⚠️	Due to its size, this pull request will likely have a little longer turnaround time and will probably require multiple passes from our reviewers.

[pullrequest]

No additional comments at this time.

---

Reviewed with ❤️ by PullRequest

[jarifibrahim]

I've reviewed only half the code. I'll review this PR again.

Reviewable status: 0 of 23 files reviewed, 18 unresolved discussions (waiting on @ashish-goswami, @balajijinnah, @jarifibrahim, and @manishrjain)

---

key_registry.go, line 115 at r3 (raw file):

// newKeyRegistryIterator returns iterator which will allow you to iterate
// over the data key of the the key registry.

I see an extra the

---

key_registry.go, line 124 at r3 (raw file):

}

// ValidRegistry checks the given encryption key is valid or not.

ValidRegistry checks that the given ...

---

key_registry.go, line 129 at r3 (raw file):

	_, err := fp.Read(iv)
	if err != nil {
		return y.Wrapf(err, "Error while reading IV for key registry.")

Can you please add the file name to the returned error. That way we know the file from which read failed.

---

key_registry.go, line 162 at r3 (raw file):

	// Check checksum.
	if crc32.Checksum(data, y.CastagnoliCrcTable) != binary.BigEndian.Uint32(kri.lenCrcBuf[4:]) {
		return nil, y.Wrapf(err, "Error while checking checksum for data key.")

Wrapping a nil error returns a nil error. err might be nil when we reach this line. Consider returning a new error. We already have a checksum mismatch error. You can wrap it up and add the file name for which it failed.

---

key_registry.go, line 169 at r3 (raw file):

	}
	if len(kri.encryptionKey) > 0 {
		// Decrypt the key if the storage key exits.

// Decrypt the key if the storage key exists

---

key_registry.go, line 195 at r3 (raw file):

			kr.lastCreated = dk.CreatedAt
		}
		// No need to lock, since we are building the initial state.

nit: The comma is not necessary.

---

key_registry.go, line 196 at r3 (raw file):

		}
		// No need to lock, since we are building the initial state.
		kr.dataKeys[kr.nextKeyID] = dk

We're inserting dk with nextKeyID index. Could it be possible that nextKeyID != dk.KeyID at any given time?
If this happens, a lot of things could fail since the IDs of data keys would be invalid.

Levels.go calls dataKey(...) function with keyID param which it reads from table manifest.
So if the data key ID changes, the function would definitely return an error.

---

key_registry.go, line 222 at r3 (raw file):

		return err
	}
	// Encrypt sanity text if the encryption key presents.

if the encryption key is present.

---

key_registry.go, line 231 at r3 (raw file):

		}
	}
	_, err = buf.Write(iv)

y.check2(...)

---

key_registry.go, line 233 at r3 (raw file):

	_, err = buf.Write(iv)
	y.Check(err)
	_, err = buf.Write(eSanity)

y.check2(...)

---

key_registry.go, line 283 at r3 (raw file):

}

// latest datakey will give you the lastest generated datakey based on the rotation

Typo: Lastest => latest

---

key_registry.go, line 284 at r3 (raw file):

// latest datakey will give you the lastest generated datakey based on the rotation
// period. If the last generated datakey lifetime exceeds more than the rotation period.

if the last generated datakey's lifetime exceeds more than the rotation period.

---

key_registry.go, line 354 at r3 (raw file):

}

// storeDataKey stores datakey in a encrypted format in the given buffer. If storage key preset.

storeDataKey stores datakey in an encrypted format in the given buffer if the storage key is present.

---

key_registry.go, line 366 at r3 (raw file):

		return err
	}
	// In memory datakey will in plain text so encrypting before storing to the disk.

In-memory datakey will be in plain text so encrypt it before storing it to the disk.

---

key_registry.go, line 373 at r3 (raw file):

	var data []byte
	if data, err = k.Marshal(); err != nil {
		return err

When we return the error from here, the data key is in encrypted format. We should decrypt it before returning.

---

key_registry.go, line 378 at r3 (raw file):

	binary.BigEndian.PutUint32(lenCrcBuf[0:4], uint32(len(data)))
	binary.BigEndian.PutUint32(lenCrcBuf[4:8], crc32.Checksum(data, y.CastagnoliCrcTable))
	_, err = buf.Write(lenCrcBuf[:])

y.Check2(...)

---

key_registry.go, line 380 at r3 (raw file):

	_, err = buf.Write(lenCrcBuf[:])
	y.Check(err)
	_, err = buf.Write(data)

y.Check2(...)

---

value.go, line 911 at r3 (raw file):

		path:        path,
		loadingMode: vlog.opt.ValueLogLoadingMode,
		dataKey:     dk,

We've created and assigned a data key to the lf here but lf.bootstrap re-assigns the data key. Look at lf.bootstrap() function.

Why do we reassign the dk?

[coveralls]

Coverage decreased (-0.4%) to 77.83% when pulling 2a01130 on encryption into 1621eca on master.

[jarifibrahim]

Good work @balajijinnah . Adding encryption wasn't a simple task :)

Reviewed 17 of 23 files at r1, 1 of 1 files at r2, 4 of 4 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 54 unresolved discussions (waiting on @ashish-goswami, @balajijinnah, and @manishrjain)

---

db.go, line 905 at r4 (raw file):

	dk, err := db.registry.latestDataKey()
	if err != nil {
		return y.Wrap(err)

Wrap with a message

---

errors.go, line 117 at r4 (raw file):

	// ErrEncryptionKeyMismatch is returned when the storage key is not
	// matched with the key previously given

nit: missing period.

---

errors.go, line 120 at r4 (raw file):

	ErrEncryptionKeyMismatch = errors.New("Encryption key mismatch")

	// ErrInvalidDataKeyID is returned if the datakey id is invalid

nit: missing period.

---

key_registry.go, line 52 at r4 (raw file):

	nextKeyID   uint64
	fp          *os.File
	opt         Options

We don't really need all the db options in this struct. I guess we need only the encryption key (readonly and dir can be passed as params).

Consider replacing this encryptionKey or use a pointer. We don't want to create a copy of the entire options struct. (it's a big struct 😉 )

---

key_registry.go, line 66 at r4 (raw file):

// OpenKeyRegistry opens key registry if it exists, otherwise it'll create key registry
// and returns key registry.
func OpenKeyRegistry(opt Options) (*KeyRegistry, error) {

We're not checking the length of the encryption key. AES requires key size to be 12 bytes I guess. If a user gives only 4 bytes, the current code returns an error

crypto/aes: invalid key size 3

Return a proper error so that the user knows how much should be the length of the encryption key.

---

key_registry.go, line 116 at r4 (raw file):

// newKeyRegistryIterator returns iterator which will allow you to iterate
// over the data key of the the key registry.
func newKeyRegistryIterator(fp *os.File, encryptionKey []byte) (*keyRegistryIterator, error) {

newKeyRegistryIterator can be a method on keyRegistry

func (kr *KeyRegistry) newKeyRegistryIterator() (*keyRegistryIterator, error)

---

key_registry.go, line 125 at r4 (raw file):

// ValidRegistry checks the given encryption key is valid or not.
func validRegistry(fp *os.File, encryptionKey []byte) error {

ValidRegistry can be a method on keyRegistry

func (kr *KeyRegistry) validRegistry() error 

---

key_registry.go, line 127 at r4 (raw file):

func validRegistry(fp *os.File, encryptionKey []byte) error {
	iv := make([]byte, aes.BlockSize)
	_, err := fp.Read(iv)

if _, err := fp.Read(...); err != nil { ... }

---

key_registry.go, line 138 at r4 (raw file):

		// Decrypting sanity text.
		if eSanityText, err = y.XORBlock(eSanityText, encryptionKey, iv); err != nil {
			return err

wrap the error

---

key_registry.go, line 178 at r4 (raw file):

// readKeyRegistry will read the key registry file and build the key registry struct.
func readKeyRegistry(fp *os.File, opt Options) (*KeyRegistry, error) {

Even this can be made (kr *KeyRegistry)readKeyRegistry(...)

---

key_registry.go, line 216 at r4 (raw file):

// WriteKeyRegistry will rewrite the existing key registry file with new one.
// It is okay to give closed key registry. Since, it's using only the datakey.
func WriteKeyRegistry(reg *KeyRegistry, opt Options) error {

Even this can be changed to

func(kr *KeyRegistry) writeKeyRegistry() error

---

key_registry.go, line 220 at r4 (raw file):

	iv, err := y.GenerateIV()
	if err != nil {
		return err

wrap the error.

---

key_registry.go, line 228 at r4 (raw file):

		eSanity, err = y.XORBlock(eSanity, opt.EncryptionKey, iv)
		if err != nil {
			return err

wrap the error.

---

key_registry.go, line 239 at r4 (raw file):

		// Writing the datakey to the given buffer.
		if err := storeDataKey(buf, opt.EncryptionKey, k); err != nil {
			return err

wrap the error

---

key_registry.go, line 262 at r4 (raw file):

	// Rename to the original file.
	if err = os.Rename(tmpPath, filepath.Join(opt.Dir, KeyRegistryFileName)); err != nil {
		return err

wrap the error

---

key_registry.go, line 351 at r4 (raw file):

// Close closes the key registry.
func (kr *KeyRegistry) Close() error {
	return kr.fp.Close()

This would return an error if kr is opened in readonly mode. Add a test for this.

---

key_registry.go, line 369 at r4 (raw file):

	var err error
	if err = xor(); err != nil {
		return err

wrap the error

---

key_registry_test.go, line 27 at r4 (raw file):

)

func TestBuildRegistry(t *testing.T) {

Add a test for Open Key registry with

1. Correct sanity text
2. Without sanity text
3. File already exists
4. File does not exists
5. Key rotation.

---

key_registry_test.go, line 34 at r4 (raw file):

	opt := getTestOptions(dir).WithEncryptionKey(encryptionKey)
	kr, err := OpenKeyRegistry(opt)
	defer os.Remove(dir)

Wrap the os.Remove in a require.NoError block and write it after ioutil.TempDir(...).
We will have a stale test directory left if the require between the directory creation and removal fails.

---

key_registry_test.go, line 62 at r4 (raw file):

	opt := getTestOptions(dir).WithEncryptionKey(encryptionKey)
	kr, err := OpenKeyRegistry(opt)
	defer os.Remove(dir)

Same here. Move it to the next line after you create the directory.

---

key_registry_test.go, line 87 at r4 (raw file):

	opt := getTestOptions(dir).WithEncryptionKey(encryptionKey)
	kr, err := OpenKeyRegistry(opt)
	defer os.Remove(dir)

Same here. Move it up.

---

key_registry_test.go, line 111 at r4 (raw file):

	opt := getTestOptions(dir).WithEncryptionKey(encryptionKey)
	kr, err := OpenKeyRegistry(opt)
	defer os.Remove(dir)

Same here. Move it up and wrap it.

---

key_registry_test.go, line 116 at r4 (raw file):

	require.NoError(t, err)
	require.NoError(t, kr.Close())
	// Checking the corretness of the datakey after closing and

// checking the correctness ...

---

levels.go, line 154 at r4 (raw file):

			dk, err := db.registry.dataKey(tf.KeyID)
			if err != nil {
				rerr = errors.Wrapf(err, "Error while creating datakey")

Error while reading datakey

---

levels.go, line 508 at r4 (raw file):

		dk, err := s.kv.registry.latestDataKey()
		if err != nil {
			return nil, nil, err

Wrap the error, please.

---

options.go, line 447 at r4 (raw file):

// Key Registry will use this duration to create new keys. If the previous generated
// key exceed the given duration. Then the key registry will create new key.
func (opt Options) WithEncryptionRotationDuration(d time.Duration) Options {

Typo EncryptionRotationDuration => WithEncryptionKeyRotationDuration

---

stream_writer.go, line 305 at r4 (raw file):

	dk, err := w.db.registry.latestDataKey()
	if err != nil {
		return err

Wrap the error, please.

---

value.go, line 832 at r4 (raw file):

	var err error
	if lf.fd, err = y.OpenExistingFile(path, flags); err != nil {
		return err

Wrap the error

---

value.go, line 875 at r4 (raw file):

	if _, err = lf.fd.Seek(0, io.SeekStart); err != nil {
		return err

Wrap the error

---

value.go, line 880 at r4 (raw file):

	var dk *pb.DataKey
	if dk, err = lf.registry.latestDataKey(); err != nil {
		return err

Wrap the error

---

value.go, line 1084 at r4 (raw file):

		_, err := vlog.createVlogFile(newid)
		if err != nil {
			return err

Wrap the error

---

table/builder.go, line 310 at r4 (raw file):

	iv, err := y.GenerateIV()
	if err != nil {
		return data, err

Wrap the error, please

---

table/builder.go, line 314 at r4 (raw file):

	data, err = y.XORBlock(data, b.DataKey().Data, iv)
	if err != nil {
		return data, err

wrap the error, please

---

table/table.go, line 44 at r4 (raw file):

// Options contains configurable options for Table/Builder.
type Options struct {
	// Options for Opening Table.

Options for opening/building table.

---

table/table.go, line 322 at r4 (raw file):

		var err error
		if data, err = t.decrypt(data); err != nil {
			return err

wrap the error.

---

table/table.go, line 439 at r4 (raw file):

		return t.opt.DataKey.KeyId
	}
	// By default it's 0, if it is plain text.

Add test for keyID == 0 case.

[pullrequest]

⚠️ Warning

PullRequest detected a force-push on this branch. This may have caused some information to be lost, and additional time may be required to complete review of the code. Read More

[poonai]

Reviewable status: 12 of 24 files reviewed, 51 unresolved discussions (waiting on @ashish-goswami, @balajijinnah, @jarifibrahim, and @manishrjain)

---

db.go, line 905 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Wrap with a message

Done.

---

key_registry.go, line 115 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

I see an extra the

Done.

---

key_registry.go, line 124 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

ValidRegistry checks that the given ...

Done.

---

key_registry.go, line 129 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Can you please add the file name to the returned error. That way we know the file from which read failed.

There is only one file

---

key_registry.go, line 162 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Wrapping a nil error returns a nil error. err might be nil when we reach this line. Consider returning a new error. We already have a checksum mismatch error. You can wrap it up and add the file name for which it failed.

Done.

---

key_registry.go, line 169 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

// Decrypt the key if the storage key exists

Done.

---

key_registry.go, line 196 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

We're inserting dk with nextKeyID index. Could it be possible that nextKeyID != dk.KeyID at any given time?
If this happens, a lot of things could fail since the IDs of data keys would be invalid.

Levels.go calls dataKey(...) function with keyID param which it reads from table manifest.
So if the data key ID changes, the function would definitely return an error.

Done.

---

key_registry.go, line 222 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

if the encryption key is present.

Done.

---

key_registry.go, line 231 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

y.check2(...)

Done.

---

key_registry.go, line 233 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

y.check2(...)

Done.

---

key_registry.go, line 283 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Typo: Lastest => latest

Done.

---

key_registry.go, line 284 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

if the last generated datakey's lifetime exceeds more than the rotation period.

Done.

---

key_registry.go, line 354 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

storeDataKey stores datakey in an encrypted format in the given buffer if the storage key is present.

Done.

---

key_registry.go, line 366 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

In-memory datakey will be in plain text so encrypt it before storing it to the disk.

Done.

---

key_registry.go, line 373 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

When we return the error from here, the data key is in encrypted format. We should decrypt it before returning.

Done.

---

key_registry.go, line 378 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

y.Check2(...)

Done.

---

key_registry.go, line 380 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

y.Check2(...)

Done.

---

key_registry.go, line 52 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

We don't really need all the db options in this struct. I guess we need only the encryption key (readonly and dir can be passed as params).

Consider replacing this encryptionKey or use a pointer. We don't want to create a copy of the entire options struct. (it's a big struct 😉 )

Done.

---

key_registry.go, line 116 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

newKeyRegistryIterator can be a method on keyRegistry

func (kr *KeyRegistry) newKeyRegistryIterator() (*keyRegistryIterator, error)

Idea is to give a fp. key registry always hold fp at seeklast

---

key_registry.go, line 127 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

if _, err := fp.Read(...); err != nil { ... }

Done.

---

key_registry.go, line 138 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

wrap the error

Done.

---

key_registry.go, line 220 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

wrap the error.

Done.

---

key_registry.go, line 228 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

wrap the error.

Done.

---

key_registry.go, line 239 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

wrap the error

Done.

---

key_registry.go, line 262 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

wrap the error

Done.

---

key_registry.go, line 369 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

wrap the error

Done.

---

key_registry_test.go, line 27 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Add a test for Open Key registry with

1. Correct sanity text
2. Without sanity text
3. File already exists
4. File does not exists
5. Key rotation.

Already exist

---

key_registry_test.go, line 34 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Wrap the os.Remove in a require.NoError block and write it after ioutil.TempDir(...).
We will have a stale test directory left if the require between the directory creation and removal fails.

Done.

---

key_registry_test.go, line 62 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Same here. Move it to the next line after you create the directory.

Done.

---

key_registry_test.go, line 87 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Same here. Move it up.

Done.

---

key_registry_test.go, line 111 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Same here. Move it up and wrap it.

Done.

---

key_registry_test.go, line 116 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

// checking the correctness ...

Done.

---

levels.go, line 154 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Error while reading datakey

Done.

---

levels.go, line 508 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Wrap the error, please.

Done.

---

options.go, line 447 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Typo EncryptionRotationDuration => WithEncryptionKeyRotationDuration

Done.

---

stream_writer.go, line 305 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Wrap the error, please.

Done.

---

value.go, line 911 at r3 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

We've created and assigned a data key to the lf here but lf.bootstrap re-assigns the data key. Look at lf.bootstrap() function.

Why do we reassign the dk?

Done.

---

value.go, line 832 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Wrap the error

Done.

---

value.go, line 875 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Wrap the error

Done.

---

table/builder.go, line 310 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Wrap the error, please

Done.

---

table/builder.go, line 314 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

wrap the error, please

Done.

---

table/table.go, line 44 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Options for opening/building table.

Done.

---

table/table.go, line 322 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

wrap the error.

Done.

---

table/table.go, line 439 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Add test for keyID == 0 case.

Done.

[poonai]

Reviewable status: 12 of 24 files reviewed, 51 unresolved discussions (waiting on @ashish-goswami, @balajijinnah, @jarifibrahim, and @manishrjain)

---

key_registry.go, line 125 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

ValidRegistry can be a method on keyRegistry

func (kr *KeyRegistry) validRegistry() error 

This are constructor kind of stuf.. I would prefer this way

---

key_registry.go, line 178 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Even this can be made (kr *KeyRegistry)readKeyRegistry(...)

This are constructor kind of stuf.. I would prefer this way

---

key_registry.go, line 216 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Even this can be changed to

func(kr *KeyRegistry) writeKeyRegistry() error

I'm using this rotation tool, where I give reencrypted key registry to write it to file

[poonai]

Reviewable status: 12 of 24 files reviewed, 51 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)

---

key_registry.go, line 66 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

We're not checking the length of the encryption key. AES requires key size to be 12 bytes I guess. If a user gives only 4 bytes, the current code returns an error

crypto/aes: invalid key size 3

Return a proper error so that the user knows how much should be the length of the encryption key.

Yeah, cypto library is already asserting.
I thought it is double work. So ignoring it. Anyways, It is documented

---

key_registry.go, line 351 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

This would return an error if kr is opened in readonly mode. Add a test for this.

Done.

---

value.go, line 880 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Wrap the error

Done.

---

value.go, line 1084 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Wrap the error

Done.

[jarifibrahim]

Reviewable status: 12 of 24 files reviewed, 17 unresolved discussions (waiting on @ashish-goswami, @balajijinnah, @jarifibrahim, and @manishrjain)

---

errors.go, line 124 at r5 (raw file):

	// ErrChecksumMismatch is returned when checksum mismatch.
	ErrChecksumMismatch = errors.New("Checksum mismatch")

We already have a ErrChecksumMismatch in y.go

Consider using it instead.

---

key_registry.go, line 129 at r3 (raw file):

Previously, balajijinnah (balaji) wrote…

There is only one file

Ah, right. There's only one key registry file.

---

key_registry.go, line 283 at r3 (raw file):

Previously, balajijinnah (balaji) wrote…

Done.

I still see the typo

// latestDataKey will give you the latest generated...

---

key_registry.go, line 66 at r4 (raw file):

Previously, balajijinnah (balaji) wrote…

Yeah, cypto library is already asserting.
I thought it is double work. So ignoring it. Anyways, It is documented

The user wouldn't know the correct length of the encryption key (unless they care to check the documentation for the encryption page). Add simple if to validate the key length.

---

key_registry.go, line 351 at r4 (raw file):

Previously, balajijinnah (balaji) wrote…

Done.

This is not done. The above code will return an error (when it shouldn't) if the key registry was opened in read-only mode.

---

levels.go, line 509 at r5 (raw file):

		if err != nil {
			return nil, nil,
				y.Wrapf(err, "Error while retriving datakey in levelsController.compactBuildTables")

Typo: retrieving

---

stream_writer.go, line 305 at r4 (raw file):

Previously, balajijinnah (balaji) wrote…

Done.

Typo: retrieving

---

value.go, line 875 at r4 (raw file):

Previously, balajijinnah (balaji) wrote…

Done.

Missing file name in error.

---

value.go, line 880 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Wrap the error

typo: retrieving

---

value.go, line 258 at r5 (raw file):

	}

	// acquire lock before unmap.

Nice catch 👍

---

badger/cmd/bank.go, line 103 at r5 (raw file):

		"Starting from the violation txn, how many previous versions to retrieve.")
	bankDisect.Flags().BoolVarP(&encryptionEnabled, "encryption", "e", false,
		"If it is true, badger will encrypt all the data storing on the disk.")

all the data stored on the disk.

---

table/table.go, line 322 at r4 (raw file):

Previously, balajijinnah (balaji) wrote…

Done.

Add table name/id, please.

[poonai]

Reviewable status: 12 of 24 files reviewed, 17 unresolved discussions (waiting on @ashish-goswami, @balajijinnah, @jarifibrahim, and @manishrjain)

---

key_registry.go, line 66 at r4 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

The user wouldn't know the correct length of the encryption key (unless they care to check the documentation for the encryption page). Add simple if to validate the key length.

Done.

---

levels.go, line 509 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Typo: retrieving

Done.

---

badger/cmd/bank.go, line 103 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

all the data stored on the disk.

Done.

[jarifibrahim]

Reviewable status: 12 of 24 files reviewed, 4 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)

---

key_registry.go, line 125 at r4 (raw file):

Previously, balajijinnah (balaji) wrote…

This are constructor kind of stuf.. I would prefer this way

I am not very sure about the function signature. I'll let @manishrjain make the call for this comment and all the ones related to function signature.