Add support for encryption at rest. #4351

parasssh · 2019-12-04T18:26:40Z

Enterprise feature.

Add support for encryption-at-rest for Dgraph Alpha.

TODO:

Support encryption with bulk loader. Need a bug fix for Badger first.
Support encryption with zero.

This change is

manishrjain

Reviewed 12 of 12 files at r1.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @danielmai and @parasssh)

dgraph/docker-compose.yml, line 181 at r1 (raw file):

      - type: bind
        source: ../ee/enc/enc_key_file
        target: /dgraph-enc/enc_key_file

enc-key

dgraph/cmd/alpha/run.go, line 108 at r1 (raw file):

			" mmap consumes more RAM, but provides better performance.")
	// expose flag for enterprise only.
	enc.EncryptionKeyFile(flag)

Let's not "hide" the flag.

dgraph/cmd/alpha/run.go, line 433 at r1 (raw file):

		BadgerTables:  Alpha.Conf.GetString("badger.tables"),
		BadgerVlog:    Alpha.Conf.GetString("badger.vlog"),
		BadgerKeyFile: enc.GetEncryptionKeyFile(Alpha.Conf),

Alpha.Conf.GetString(...)

dgraph/cmd/bulk/run.go, line 112 at r1 (raw file):

		ReplaceOutDir:    Bulk.Conf.GetBool("replace_out"),
		TmpDir:           Bulk.Conf.GetString("tmp"),
		BadgerKeyFile:    enc.GetEncryptionKeyFile(Bulk.Conf),

Bulk.Conf.GetString(...)

dgraph/cmd/zero/run.go, line 176 at r1 (raw file):

		w:                 Zero.Conf.GetString("wal"),
		rebalanceInterval: Zero.Conf.GetDuration("rebalance_interval"),
		badgerKeyFile:     enc.GetEncryptionKeyFile(Zero.Conf),

Zero.Conf.GetString(...)

dgraph/cmd/zero/run.go, line 220 at r1 (raw file):

	// zero out from memory
	opts.badgerKeyFile = ""

remove.

ee/enc/util.go, line 34 at r1 (raw file):

// GetEncryptionKeyString return empty string for OSS build
func GetEncryptionKeyFile(c *viper.Viper) string {

Probably not needed.

ee/enc/util.go, line 39 at r1 (raw file):

// ReadEncryptionKeyFile returns nil key for OSS build
func ReadEncryptionKeyFile(f string) []byte {

ReadEncryptionKey(...)

ee/enc/util.go, line 40 at r1 (raw file):

// ReadEncryptionKeyFile returns nil key for OSS build
func ReadEncryptionKeyFile(f string) []byte {
	return nil

Please do a LOG WARNING that encryption isn't enabled because of OSS. Or, you could go one step further and log fatal. Actually, let's do log fatal, because there's no way to read encrypted data with OSS anyway.

If OSS and nil flag, then OK.

ee/enc/util_ee.go, line 22 at r1 (raw file):

)

const encFile string = "encryption_key_file"

The flag name should be not here. Should be in a common place.

worker/groups.go, line 996 at r1 (raw file):

}

func enterpriseEnabled2() bool {

askZeroForEE()

worker/groups.go, line 1000 at r1 (raw file):

	var connState *pb.ConnectionState

	grp := &groupi{

g := &groupi{}

worker/groups.go, line 1004 at r1 (raw file):

		tablets:      make(map[string]*pb.Tablet),
	}
	grp.ctx, grp.cancel = context.WithCancel(context.Background())

don't think you need this. Could do this in the loop:

ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()

Could make a small nested func within this fun which would do the logic of the loop and do returns instead of continues.

worker/groups.go, line 1007 at r1 (raw file):

	for { // Keep on retrying. See: https://github.com/dgraph-io/dgraph/issues/2289
		pl := grp.connToZeroLeader()

g.connToZeroLeader()

worker/groups.go, line 1013 at r1 (raw file):

		zc := pb.NewZeroClient(pl.Get())
		connState, err = zc.Connect(grp.ctx, &pb.Member{ClusterInfoOnly: true})
		if connState == nil || connState.GetState() == nil || connState.GetState().GetLicense() == nil {

100 chars.

worker/groups.go, line 1014 at r1 (raw file):

		connState, err = zc.Connect(grp.ctx, &pb.Member{ClusterInfoOnly: true})
		if connState == nil || connState.GetState() == nil || connState.GetState().GetLicense() == nil {
			continue

Log Info or something.

worker/server_state.go, line 144 at r1 (raw file):

	if !EnterpriseEnabled() {
		glog.Infof("Enterprise License missing or OSS build. Disable Encryption and proceed.")

This log should be moved.

worker/server_state.go, line 145 at r1 (raw file):

	if !EnterpriseEnabled() {
		glog.Infof("Enterprise License missing or OSS build. Disable Encryption and proceed.")
		Config.BadgerKeyFile = ""

This should be a crash.

worker/server_state.go, line 147 at r1 (raw file):

		Config.BadgerKeyFile = ""
	} else {
		glog.Infof("Encryption feature enabled. Encryption Key file %v", Config.BadgerKeyFile)

Using encryption key file: ...

worker/server_state.go, line 168 at r1 (raw file):

		key := opt.EncryptionKey
		opt.EncryptionKey = nil
		glog.Infof("Opening write-ahead log BadgerDB with options: %+v\n", opt)

Perhaps build a stringify interface in Badger options, which can be used to print them nicely here.

ee/enc/util_ee.go

martinmr

@parasssh Can you make sure you mark comments you have fixed as done in Reviewable? I tried to review this but it looks like only some of Manish's comments have been addressed.

Reviewed 3 of 9 files at r2, 3 of 3 files at r3.
Reviewable status: 9 of 12 files reviewed, 22 unresolved discussions (waiting on @danielmai, @manishrjain, and @parasssh)

dgraph/cmd/zero/run.go, line 97 at r3 (raw file):

Quoted 5 lines of code…

	// not supported for zero for now.
	// flag.String("encryption_key_file", "",
	// 	"The file that stores the encryption key. The key size must be 16, 24, or 32 bytes long. "+
	// 		"The key size determines the corresponding block size for AES encryption "+
	// 		"(AES-128, AES-192, and AES-256 respectively). Enterprise feature.")

Personally, I'd remove the commented out code and just leave a comment saying something like "Encryption is not supported in zero yet".

parasssh

Dismissed @golangcibot and @manishrjain from 21 discussions.
Reviewable status: 9 of 12 files reviewed, 1 unresolved discussion (waiting on @danielmai, @manishrjain, and @parasssh)

parasssh

Reviewable status: 9 of 12 files reviewed, 1 unresolved discussion (waiting on @danielmai, @manishrjain, and @parasssh)

worker/server_state.go, line 168 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Perhaps build a stringify interface in Badger options, which can be used to print them nicely here.

Will add a task to do this later.

parasssh

Dismissed @martinmr from a discussion.
Reviewable status: 8 of 12 files reviewed, all discussions resolved (waiting on @danielmai, @manishrjain, and @martinmr)

dgraph/cmd/zero/run.go, line 97 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

	// not supported for zero for now.
	// flag.String("encryption_key_file", "",
	// 	"The file that stores the encryption key. The key size must be 16, 24, or 32 bytes long. "+
	// 		"The key size determines the corresponding block size for AES encryption "+
	// 		"(AES-128, AES-192, and AES-256 respectively). Enterprise feature.")

Personally, I'd remove the commented out code and just leave a comment saying something like "Encryption is not supported in zero yet".

Done.

parasssh · 2019-12-06T02:54:46Z

@parasssh Can you make sure you mark comments you have fixed as done in Reviewable? I tried to review this but it looks like only some of Manish's comments have been addressed.

Reviewed 3 of 9 files at r2, 3 of 3 files at r3.
Reviewable status: 9 of 12 files reviewed, 22 unresolved discussions (waiting on @danielmai, @manishrjain, and @parasssh)

dgraph/cmd/zero/run.go, line 97 at r3 (raw file):
Quoted 5 lines of code…

Personally, I'd remove the commented out code and just leave a comment saying something like "Encryption is not supported in zero yet".

Done.

Implemented the comment.

danielmai · 2019-12-06T21:31:21Z

dgraph/docker-compose.yml, line 101 at r5 (raw file):

        source: ../ee/enc/enc-key
        target: /dgraph-enc/enc-key
        read_only: true

nit: remove trailing spaces.

danielmai · 2019-12-06T21:44:28Z

dgraph/cmd/alpha/run.go, line 447 at r5 (raw file):

	// OSS, non-nil key file --> crash
	if !enc.EeBuild && opts.BadgerKeyFile != "" {
		glog.Fatal("Encryption is an Enterpise only feature.")

Enterprise*

Actually, we should replace this with x.ErrNotSupported for a standardized error message.

glog.Fatalf("Cannot enable encryption: %s", x.ErrNotSupported)

danielmai

Reviewed 2 of 9 files at r2, 1 of 2 files at r4, 3 of 3 files at r5.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @parasssh)

dgraph/cmd/bulk/reduce.go, line 94 at r5 (raw file):

func (r *reducer) createBadger(i int) *badger.DB {
	if r.opt.BadgerKeyFile != "" {
		// need to set zero addr in workerconfig before doing license check.

WorkerConfig

dgraph/cmd/bulk/reduce.go, line 99 at r5 (raw file):

		if !worker.EnterpriseEnabled() {
			// not licensed --> crash.
			log.Fatal("Enterprise License needed for the Encryption feature.")

Use glog, not log.

dgraph/cmd/bulk/reduce.go, line 102 at r5 (raw file):

		} else {
			// licensed --> OK.
			log.Printf("Encryption feature enabled. Using encryption Key file: %v", r.opt.BadgerKeyFile)

Use glog, not log.

lowercase "key"

dgraph/cmd/bulk/run.go, line 138 at r5 (raw file):

	// OSS, non-nil key file --> crash
	if !enc.EeBuild && opt.BadgerKeyFile != "" {
		fmt.Printf("Encryption is an Enterpise only feature.")

fmt.Printf("Cannot enable encryption: %s", x.ErrNotSupported)

dgraph/cmd/zero/run.go, line 175 at r5 (raw file):

		w:                 Zero.Conf.GetString("wal"),
		rebalanceInterval: Zero.Conf.GetDuration("rebalance_interval"),
		badgerKeyFile:     Zero.Conf.GetString("encryption_key_file"),

Do we need this code if it's not enabled in Zero yet? We should remove this and the rest of the encryption code in Zero.

ee/enc/util.go, line 28 at r5 (raw file):

// ReadEncryptionKeyFile returns nil key for OSS build
func ReadEncryptionKeyFile(filepath string) []byte {
	x.AssertTruef(filepath == "", "encryption_key_file is an Enterprise only feature.")

Seems like this is never run since there are other OSS-build checks that happen before this gets called. This is still good to have in place.

ee/enc/util_ee.go, line 29 at r5 (raw file):

	}
	k, err := ioutil.ReadFile(filepath)
	x.Checkf(err, "Error reading Encryption key file (%v)", filepath)

lowercase "encryption"

worker/groups.go, line 62 at r5 (raw file):

}

var gr *groupi = nil

Is this change needed?

worker/server_state.go, line 147 at r5 (raw file):

Enterprise License needed for the Encryption feature.

"Valid enterprise license needed to enable encryption feature."

parasssh

Dismissed @danielmai from 11 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion

dgraph/cmd/alpha/run.go, line 447 at r5 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Enterprise*

Actually, we should replace this with x.ErrNotSupported for a standardized error message.
glog.Fatalf("Cannot enable encryption: %s", x.ErrNotSupported)

Done.

dgraph/cmd/bulk/reduce.go, line 99 at r5 (raw file):

log.Fatal

glog is not used anywhere in the cmd/bulk folder. I followed the current pattern.

dgraph/cmd/bulk/reduce.go, line 102 at r5 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Use glog, not log.

lowercase "key"

glog is not used anywhere in the bulk cmd.

danielmai · 2019-12-06T22:40:33Z

dgraph/cmd/zero/run.go, line 93 at r5 (raw file):

	flag.Bool("telemetry", true, "Send anonymous telemetry data to Dgraph devs.")

	// Encryption not supported for zero for now.

Keep this comment.

parasssh

Dismissed @danielmai from a discussion.
Reviewable status: 4 of 12 files reviewed, 2 unresolved discussions (waiting on @danielmai and @martinmr)

dgraph/cmd/zero/run.go, line 93 at r5 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Keep this comment.

Dismiss this comment. Let me know if you feel strongly the otherwise.

Comment not needed as there is no enc code in zero now.

martinmr · 2019-12-07T01:03:42Z

FYI, you don't need to keep every commit message in the message of the final squashed commit.

parasssh · 2019-12-07T02:13:15Z

FYI, you don't need to keep every commit message in the message of the final squashed commit.

Got it.

Paras Shah added 6 commits December 4, 2019 10:23

enc support after refactor. TODO: bulk loader bug fix; license check

b0a8853

dont zero reducer key

11e3b1f

typo

719c687

license check to be done later in alpha and bulk cmds

758efa2

license check on alpha; tests pass

1bdcab1

cleanup; tests pass

c71268d

parasssh changed the title ~~enc support after refactor. TODO: bulk loader bug fix; license check~~ enc support after refactor. TODO: bulk loader bug fix; Dec 5, 2019

parasssh requested review from manishrjain and danielmai December 5, 2019 18:15

use filepath; EEBuild var; tests pass; local oss and ee test pass

a68d414

manishrjain reviewed Dec 5, 2019

View reviewed changes

implement review comments; tests pass; disable enc for bulk and zero

c69445e

golangcibot reviewed Dec 5, 2019

View reviewed changes

ee/enc/util_ee.go Outdated Show resolved Hide resolved

cleanup

223a291

danielmai added the area/enterprise Related to proprietary features label Dec 5, 2019

danielmai changed the title ~~enc support after refactor. TODO: bulk loader bug fix;~~ Add support for encryption at rest. Dec 5, 2019

defer cancel on contexts

d3b8fc0

parasssh marked this pull request as ready for review December 6, 2019 00:01

parasssh requested a review from a team as a code owner December 6, 2019 00:01

martinmr previously requested changes Dec 6, 2019

View reviewed changes

parasssh commented Dec 6, 2019

View reviewed changes

address review comments from Martin

3dd0fc2

parasssh commented Dec 6, 2019

View reviewed changes

parasssh requested a review from martinmr December 6, 2019 02:34

parasssh closed this Dec 6, 2019

parasssh reopened this Dec 6, 2019

Merge remote-tracking branch 'origin/master' into ps_enc_key_intg_2

b2babbe

enable bulk enc; tests pass

9cbd2ba

danielmai previously requested changes Dec 6, 2019

View reviewed changes

address review comments

7f7768c

parasssh commented Dec 6, 2019

View reviewed changes

parasssh merged commit b69c834 into master Dec 6, 2019

parasssh deleted the ps_enc_key_intg_2 branch December 6, 2019 23:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for encryption at rest. #4351

Add support for encryption at rest. #4351

parasssh commented Dec 4, 2019 •

edited

Loading

manishrjain left a comment

martinmr left a comment

parasssh left a comment

parasssh left a comment

parasssh left a comment

parasssh commented Dec 6, 2019

danielmai commented Dec 6, 2019

danielmai commented Dec 6, 2019

danielmai left a comment

parasssh left a comment

danielmai commented Dec 6, 2019

parasssh left a comment

martinmr commented Dec 7, 2019

parasssh commented Dec 7, 2019

Add support for encryption at rest. #4351

Add support for encryption at rest. #4351

Conversation

parasssh commented Dec 4, 2019 • edited Loading

manishrjain left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh commented Dec 6, 2019

danielmai commented Dec 6, 2019

danielmai commented Dec 6, 2019

danielmai left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

danielmai commented Dec 6, 2019

parasssh left a comment

Choose a reason for hiding this comment

martinmr commented Dec 7, 2019

parasssh commented Dec 7, 2019

parasssh commented Dec 4, 2019 •

edited

Loading