Skip to content
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

Support encryption at rest #1042

Merged
merged 21 commits into from Sep 24, 2019

Conversation

@balajijinnah
Copy link
Member

balajijinnah commented Sep 18, 2019

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


This change is Reviewable

balajijinnah and others added 11 commits Aug 16, 2019
For encryption at rest support. We'll be using two keys for encryption. One key is provided
by the user. Another one will be generated by the badger(data key). The key generated by the badger
will be used for encryption and decryption. The key generated by the badger will be encrypted by the user-provided key and persisted to the disk. The main reason behind this implementation is
that It'll be easy to rotate keys. We just need to decrypt the data key with the old key and encrypt back with the new key. 


As a first step in this commit. Key Registry is added. Which is responsible for maintaining all
the badger generated key and doing key rotation. We generate a new key for every 10 days, which can be changed by the option.
In this PR, support for encryption added to the table.

Implementation details.

The data format on disk will be the same as before, except we'll add 
IV to the end of the data block, which we are encrypting.

We'll decide whether to decrypt or encrypt based on the datakey. 
If datakey present, we'll encrypt or decrypt. Otherwise, we don't do anything.

We'll encrypt while storing to the disk. (table builder)
We'll decrypt while reading the data. (table)
* add encrption to the tab᷆le
Signed-off-by: பாலாஜி ஜின்னா <balaji@dgraph.io>
Signed-off-by: பாலாஜி ஜின்னா <balaji@dgraph.io>
* vlog encryption
@balajijinnah balajijinnah requested review from ashish-goswami, jarifibrahim, manishrjain and dgraph-io/team as code owners Sep 18, 2019
Copy link

pullrequest bot left a comment

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


Check the status or cancel PullRequest code review here.

Copy link

pullrequest bot left a comment

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.

Copy link

pullrequest bot left a comment

No additional comments at this time.


Reviewed with ❤️ by PullRequest

Copy link
Member

jarifibrahim left a comment

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

This comment has been minimized.

Copy link

coveralls commented Sep 19, 2019

Coverage Status

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

Signed-off-by: பாலாஜி ஜின்னா <balaji@dgraph.io>
Copy link
Member

jarifibrahim left a comment

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.

Signed-off-by: பாலாஜி ஜின்னா <balaji@dgraph.io>
@balajijinnah balajijinnah force-pushed the encryption branch from dce9cea to 24c7970 Sep 23, 2019
Copy link

pullrequest bot left a comment

⚠️ 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

Copy link
Member Author

balajijinnah left a comment

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.

Copy link
Member Author

balajijinnah left a comment

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

fix
Signed-off-by: பாலாஜி ஜின்னா <balaji@dgraph.io>
fix
Signed-off-by: பாலாஜி ஜின்னா <balaji@dgraph.io>
Copy link
Member Author

balajijinnah left a comment

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.

Copy link
Member

jarifibrahim left a comment

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.

Signed-off-by: பாலாஜி ஜின்னா <balaji@dgraph.io>
Copy link
Member Author

balajijinnah left a comment

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.

Copy link
Member

jarifibrahim left a comment

:lgtm:

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.

Signed-off-by: பாலாஜி ஜின்னா <balaji@dgraph.io>
fix test
Signed-off-by: பாலாஜி ஜின்னா <balaji@dgraph.io>
Signed-off-by: பாலாஜி ஜின்னா <balaji@dgraph.io>
@balajijinnah balajijinnah merged commit a425b0e into master Sep 24, 2019
7 of 9 checks passed
7 of 9 checks passed
coverage/coveralls Coverage decreased (-0.4%) to 77.83%
Details
code-review/reviewable 14 files, 4 discussions left (ashish-goswami, jarifibrahim, manishrjain)
Details
GolangCI No issues found!
Details
Unit Tests (badger) TeamCity build finished
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.