Vault Integration with Dgraph #5402

parasssh · 2020-05-08T15:22:46Z

Fixes DGRAPH-1162, DGRAPH-1350 and DGRAPH-1351

Move from files to keeping keys
Configuration pieces of Vault
Vault Connectivity using approle auth. Get the key in Dgraph.
Refactor: Common Interface to Vault and local Filesystem
Vault on Alpha
Misc: Keep one copy of key and pass pointers; Stringer key to redact key values
Tests

This change is

Docs Preview:

…e. Key are read and then the file is not needed anymore

…only for now.

manishrjain

Structure and stuff looks OK. Didn't look at the logic -- so get it reviewed by someone.

Reviewable status: 0 of 36 files reviewed, 5 unresolved discussions (waiting on @manishrjain, @martinmr, @parasssh, and @vvbalaji-dgraph)

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

	}
	if keyReaders == 2 {
		return nil, errors.Errorf("cannot have local and vault key readers. re-check the configuration")

100 chars

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

const (
	vaultAddr         = "vault_addr"
	vaultRoleIDFile   = "vault_roleID_file"

Avoid caps in flag names.

ee/enc/vault_ee.go, line 80 at r1 (raw file):

		return v, nil
	}
	return nil, errors.Errorf("%v and %v must both be specified", vaultRoleIDFile, vaultSecretIDFile)

100 chars.

ee/enc/vault_ee.go, line 84 at r1 (raw file):

// ReadKey reads the key from the vault kv store.
func (vKR *vaultKeyReader) ReadKey() (x.SensitiveByteSlice, error) {

vkr small case.

x/config.go, line 85 at r1 (raw file):

	// LudicrousMode is super fast mode with fewer guarantees.
	LudicrousMode bool
	// EncryptionKey is the key used for encryption at rest, backups, exports. Enterprise only feature.

100 chars.

parasssh

Reviewable status: 0 of 36 files reviewed, 5 unresolved discussions (waiting on @manishrjain, @martinmr, and @vvbalaji-dgraph)

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

Previously, manishrjain (Manish R Jain) wrote…

100 chars

Done.

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

Previously, manishrjain (Manish R Jain) wrote…

Avoid caps in flag names.

Done.

ee/enc/vault_ee.go, line 80 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

100 chars.

Done.

ee/enc/vault_ee.go, line 84 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

vkr small case.

Done.

x/config.go, line 85 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

100 chars.

Done.

martinmr

Reviewed 30 of 36 files at r1, 6 of 6 files at r2.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @manishrjain, @parasssh, and @vvbalaji-dgraph)

dgraph/cmd/alpha/run.go, line 573 at r2 (raw file):

kR

this can be just kr

dgraph/cmd/alpha/run.go, line 581 at r2 (raw file):

	var key x.SensitiveByteSlice
	if kR != nil {
		if key, err = kR.ReadKey(); err != nil {

instead of checking for nil here, you could do the check inside ReadKey. golang let's you call methods on nil structs as long as you handle the nil case.

dgraph/cmd/bulk/run.go, line 195 at r2 (raw file):

	key := opt.EncryptionKey
	opt.EncryptionKey = nil

why are you setting opt.EncryptionKey to nil right after copying the value to key. If this is intentional, a comment would hep understand why.

ee/enc/util_ee.go, line 56 at r2 (raw file):

(lkR

this could be just lkr since it's a small name to avoid capitalization.

ee/enc/util_ee.go, line 57 at r2 (raw file):

if lkR == nil

handle this case separately. if lkr is nil, we could make this a no-op to remove the nil checks elsewhere in the code.

ee/enc/util_ee_test.go, line 40 at r2 (raw file):

}

// func startVaultServer(t *testing.T, kvPath, kvField, kvEncKey string) (net.Listener, *api.Client) {

why is this commented out

ee/enc/util_ee_test.go, line 80 at r2 (raw file):

	require.Error(t, err)
	require.Nil(t, kR)
	t.Logf("%v", err)

I'd rather not print the errors if that's the expected output. But you could have a check on the contents of the error to verify that's the expected error.

Same for the Logf statements below.

ee/enc/util_ee_test.go, line 181 at r2 (raw file):

	require.NotNil(t, k)
	require.NoError(t, err)
	t.Logf("%v", err)

If we reached this line, we know the error is nil so there's no need to print anything.

ee/enc/test-fixtures/bad-length-enc-key, line 1 at r2 (raw file):

minor: new lines at the end of these files to avoid seeing the warning.

Same for the other files in this directory.

ee/enc/test-fixtures/dgraph.hcl, line 7 at r2 (raw file):

 capabilities = ["read", "list"]
}

minor: remove this line

parasssh

Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @manishrjain, @martinmr, and @vvbalaji-dgraph)

dgraph/cmd/alpha/run.go, line 573 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

kR
this can be just kr

Done.

dgraph/cmd/alpha/run.go, line 581 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

instead of checking for nil here, you could do the check inside ReadKey. golang let's you call methods on nil structs as long as you handle the nil case.

ReadKey is a method of an interface. So, I cannot call ReadKey on nil interface since it wouldn't know which concrete ReadKey() method to call.

dgraph/cmd/bulk/run.go, line 195 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

	key := opt.EncryptionKey
	opt.EncryptionKey = nil
why are you setting opt.EncryptionKey to nil right after copying the value to key. If this is intentional, a comment would hep understand why.

Its because we are printing all the bulk options including the key in the next couple lines. So, I save the key and set it back after the printing is done so that key is not spit out.

(In a subsequent PR for adding support for Bulk/Live with Vault, I will use the x.SensitiveByteSlice type so this hack is not needed.)

ee/enc/util_ee.go, line 56 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

(lkR
this could be just lkr since it's a small name to avoid capitalization.

Done.

ee/enc/util_ee.go, line 57 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

if lkR == nil 
handle this case separately. if lkr is nil, we could make this a no-op to remove the nil checks elsewhere in the code.

Done.

ee/enc/util_ee_test.go, line 40 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

why is this commented out

This was an experiment I was doing with TestVaultServer but its resulting in go mod issues. So, I will circle back to it later.

ee/enc/util_ee_test.go, line 80 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

I'd rather not print the errors if that's the expected output. But you could have a check on the contents of the error to verify that's the expected error.

Same for the Logf statements below.

Done.

ee/enc/util_ee_test.go, line 181 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

If we reached this line, we know the error is nil so there's no need to print anything.

Done.

ee/enc/test-fixtures/bad-length-enc-key, line 1 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

minor: new lines at the end of these files to avoid seeing the warning.

Same for the other files in this directory.

For this file, I deliberately did not want the new line. This allows me to match the key in vault for UT which for some reason doesnt allow K/V with new lines.

parasssh

Dismissed @manishrjain and @martinmr from 14 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @vvbalaji-dgraph)

martinmr

Reviewed 5 of 5 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @parasssh and @vvbalaji-dgraph)

ee/enc/util_ee.go, line 58 at r3 (raw file):

func (lkr *localKeyReader) ReadKey() (x.SensitiveByteSlice, error) {
	if lkr == nil {
		return nil, errors.Errorf("nil localKeyReader")

This shouldn't return an error. It should be a no-op.

ee/enc/util_ee_test.go, line 40 at r2 (raw file):

Previously, parasssh wrote…

This was an experiment I was doing with TestVaultServer but its resulting in go mod issues. So, I will circle back to it later.

ok. Maybe add a todo so it's clearer.

vvbalaji-dgraph · 2020-05-21T15:30:06Z

lgtm

vvbalaji-dgraph

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @parasssh)

a discussion (no related file):

parasssh

Dismissed @martinmr from a discussion.
Reviewable status: 35 of 36 files reviewed, 3 unresolved discussions (waiting on @martinmr and @parasssh)

a discussion (no related file):

Quoted 183 lines of code…


      from
      paras/vault_intg 
      to
      master       master     No branch found List is empty. Vault Integration with Dgraph

 Move from files to keeping keys
 Configuration pieces of Vault
 Vault Connectivity using approle auth. Get the key in Dgraph.
  Refactor: Common Interface to Vault and local Filesystem
 Vault on Alpha
 Misc: Keep one copy of key and pass pointers; Stringer key to redact key values
 Tests
 Vault on Bulk, Live, Debug and Restore. Also, keep option names consistent.


Docs Preview: 
New commits in r1 (88378b5) at 2020-05-16 04:26:00Z:
88336c9: Refactore to keep Keys in memory and not the input encryption key file. Key are read and then the file is not needed anymore

b9b65ab: keep keys and not keyfiles

8ab9e5e: oss

9b219cc: ut

c4a8de9: ut

ff5bff4: ut

e01e36f: config and sanity checks

838c2c5: rename

2d9ac0b: Vault pkg, config helper for registering flags, sanity checks. Alpha only for now.

f0829bc: deepsource

dd55263: Dgraph <-> Vault Communication; ReadKey

c061109: go mod/sum

74a0095: Merge branch 'master' into paras/vault_intg

92a18d3: interface KeyReader

afd4f39: files as input

b5d0eea: UT

f28c4a3: checks

457c942: remove vault pkg

d76f427: unexport some vars

87bc034: Merge branch 'master' into paras/vault_intg

7a6e69d: ut

2fdf94f: consts

c905d7b: consts more

2181036: consts

3322c10: remove hashicorp/vault/vault pkg

f6e7bd3: move test files to test-fixtures folder

6593dd3: test-fixtures folder

2f8232c: fix a panic

30d6460: Merge branch 'master' into paras/vault_intg

a65b303: use sensitive byte slice

f463245: cleanups

88378b5: use SensitiveByteSlice

Structure and stuff looks OK. Didn't look at the logic -- so get it reviewed by someone.New commits in r2 (6dd26da) at 2020-05-19 15:15:34Z:
d4db898: Merge branch 'master' into paras/vault_intg

6dd26da: review comments

New commits in r3 (de5658c) at 2020-05-19 23:37:02Z:
de5658c: review comments

lgtmee/enc/util_ee.goThis shouldn't return an error. It should be a no-op.ee/enc/util_ee_test.gowhy is this commented outThis was an experiment I was doing with TestVaultServer but its resulting in go mod issues. So, I will circle back to it later.ok. Maybe add a todo so it's clearer.!function(){"use strict";function e(){"init:reviewable";window.REVIEWABLE_PROD=true;"endinit"}var t=document.getElementById("libs-script");if(t?t.addEventListener("load",e):document.addEventListener("DOMContentLoaded",e),"/"!==location.pathname&&"/pricing"!==location.pathname){var n=document.getElementsByTagName("script");n[n.length-1].parentNode.style.display="none"}}();Serious about code reviews?Try a sample code review!You've found the right code review toolThoroughTracks where participants stand on each discussion, ensuring it won't disappear until resolved. Fully customizable logic determines when a review is complete.EfficientClearly shows net deltas since last time you looked, even if commits got rebased or amended. Batches comments and correctly threads email responses.FocusedWorks only with GitHub and GitHub Enterprise, making for a seamless integration. Minimal admin busywork, no extra fluff — just awesome code reviews.What are others saying?“GitHub, the current de facto standard for [code reviews], is letting us down.” —Justin Abrahms“It seems that the tools for code review in GitHub are not great, to put it lightly. Not great at all.” —Jaime BueltaBTW Reviewable is really awesome.After using it for a few reviews now, I hate going back to GitHub.Nicolas ArtmanFrontend Engineering Lead, UdacityI've gotta say—Reviewable is the best damn code-review tool out there.Super slick, silky-smooth integration into GitHub. Love it.Danny KoppingSenior PHP Developer, ZandoJust did my first code review on Reviewable. Very well done!Thanks for bringing this to the developers of this world!Holger RappSenior Software Engineer, GoogleReviewable is working really well for us so far. We’re migrating from a self-hosted Gerrit installation and Reviewable gives us most if not all of the benefits while integrating much better with the GitHub workflow developers are used to.Jay StotzCo-founder & CTO at Springboard RetailReviewable is freaking awesome, BTW. I'm a Xoogler myself, and I feel right at home.Austin McDonaldSoftware Engineer, SeedNot convinced yet? Here's some more featuresInstantly diff any two revisions of a file, in unified or side-by-side style. Hide minor changes: whitespace, merge, and rebase deltas.Customize margin, fonts, colors, keyboard shortcuts, etc. Jump from a comment straight to the right spot in your favorite editor.Line comments map across file revisions and stay in place until resolved, not just until changes are pushed.Modern, clean UI with a touch of whimsy. Full contextual help and fast support if you have questions.Keep track of who reviewed which revision of each file to make sure no changes are missed. Combine commits or review each one.SSO with GitHub account — no separate accounts to manage. All your code stays on GitHub, never touching our servers.Shut up and take my money!Open SourceAll public reposAll personal reposUnlimited reviewsFREE foreverStart reviewingOrganizationAll private reposUnlimited reviewsFree 30 day trial$39 / month for10 contributors 10 contributors25 contributors50 contributorsStart reviewingEnterpriseFor GitHub EnterpriseOn-prem (mostly)Free trial$17 / seat / monthFind out moreNone of these fit you? We'll work something out. Questions? Check out the user guide.                                                       .HW_badge_cont {
  visibility: hidden;
  pointer-events: none;
  display: block;
  height: 32px;
  width: 32px;
  cursor: pointer;
  position: relative;
}

.HW_badge_cont.HW_visible {
  visibility: visible;
  pointer-events: auto;
}

.HW_badge {
  display: block;
  border-radius: 20px;
  background: #CD4B5B;
  height: 16px;
  width: 16px;
  color: #fff;
  text-align: center;
  line-height: 16px;
  font-size: 11px;
  cursor: pointer;
  position: absolute;
  top: 8px;
  left: 8px;
  opacity: 0;
  transform: scale(0);
  transition: all 0.3s;
}

.HW_badge.HW_softHidden {
  background: #d4d4d4;
  opacity: 1;
  transform: scale(0.6);
}

.HW_badge.HW_hidden {
  opacity: 0;
  transform: scale(0);
}

.HW_badge.HW_visible {
  opacity: 1;
  transform: scale(1.0);
}

.HW_frame_cont {
  pointer-events: none;
  border-radius: 4px;
  box-shadow: 0 0 1px rgba(99, 114, 130, 0.32), 0 8px 16px rgba(27, 39, 51, 0.08);
  background: #fff;
  border: none;
  position: absolute;
  top: -900em;
  z-index: 2147483647;
  width: 340px;
  height: 0;
  opacity: 0;
  will-change: height, margin-top, opacity;
  margin-top: -10px;
  transition: margin-top 0.15s ease-out, opacity 0.1s ease-out;
}

#HW_frame_cont.HW_bottom {
  transition: margin-top 0.15s ease-out, opacity 0.1s ease-out, height 0.3s ease-out;
}

.HW_frame_cont.HW_visible {
  opacity: 1;
  pointer-events: auto;
  margin-top: 0px;
}

.HW_frame {
  background: #fff;
  border: none;
  width: 100%;
  height: 100%;
  overflow: hidden;
  border-radius: 4px;
  position: relative;
  z-index: 2147483647;
  transition: height 0.3s ease-out;
}

.HW_frame_cont.HW_embed {
  position: static;
  margin-top: 0;
}

ee/enc/util_ee.go, line 58 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

This shouldn't return an error. It should be a no-op.

As we spoke, err is more appropriate.

jarifibrahim

LGTM

jarifibrahim · 2020-06-04T17:47:51Z

ee/enc/vault_ee.go

+	if v.roleID != "" && v.secretID != "" {
+		return v, nil
+	}
+	return nil, errors.Errorf("%v and %v must both be specified",


I prefer writing the error conditions in the if. It makes it easier to read code. I would've written this as

if rolID == "" || secret == "" { return err .. }

Fixes DGRAPH-1162, DGRAPH-1350 and DGRAPH-1351 -Move from files to keeping keys -Configuration pieces of Vault -Vault Connectivity using approle auth. Get the key in Dgraph. -Refactor: Common Interface to Vault and local Filesystem -Vault on Alpha -Misc: Keep one copy of key and pass pointers; Stringer key to redact key values -Tests

Refactore to keep Keys in memory and not the input encryption key fil…

88336c9

…e. Key are read and then the file is not needed anymore

parasssh requested review from manishrjain, martinmr and a team as code owners May 8, 2020 15:22

github-actions bot added area/bulk-loader Issues related to bulk loading. area/enterprise Related to proprietary features area/live-loader Issues related to live loading. labels May 8, 2020

parasssh added 10 commits May 8, 2020 09:57

keep keys and not keyfiles

b9b65ab

oss

8ab9e5e

ut

9b219cc

ut

c4a8de9

ut

ff5bff4

config and sanity checks

e01e36f

rename

838c2c5

Vault pkg, config helper for registering flags, sanity checks. Alpha …

2d9ac0b

…only for now.

deepsource

f0829bc

Dgraph <-> Vault Communication; ReadKey

dd55263

parasssh added 3 commits May 9, 2020 13:49

go mod/sum

c061109

Merge branch 'master' into paras/vault_intg

74a0095

interface KeyReader

92a18d3

parasssh changed the title ~~[WIP]: Vault Integration with Dgraph~~ [WIP]: [Breaking] Vault Integration with Dgraph May 11, 2020

parasssh added 4 commits May 11, 2020 20:14

files as input

afd4f39

UT

b5d0eea

checks

f28c4a3

remove vault pkg

457c942

parasssh added 8 commits May 13, 2020 20:42

consts more

c905d7b

consts

2181036

remove hashicorp/vault/vault pkg

3322c10

move test files to test-fixtures folder

f6e7bd3

test-fixtures folder

6593dd3

fix a panic

2f8232c

Merge branch 'master' into paras/vault_intg

30d6460

use sensitive byte slice

a65b303

parasssh requested a review from vvbalaji-dgraph as a code owner May 15, 2020 20:45

parasssh changed the title ~~[WIP]: [Breaking] Vault Integration with Dgraph~~ Vault Integration with Dgraph May 15, 2020

parasssh added 2 commits May 15, 2020 15:26

cleanups

f463245

use SensitiveByteSlice

88378b5

manishrjain approved these changes May 18, 2020

View reviewed changes

parasssh added 2 commits May 19, 2020 08:08

Merge branch 'master' into paras/vault_intg

d4db898

review comments

6dd26da

parasssh commented May 19, 2020

View reviewed changes

martinmr suggested changes May 19, 2020

View reviewed changes

parasssh commented May 19, 2020

View reviewed changes

review comments

de5658c

martinmr suggested changes May 21, 2020

View reviewed changes

vvbalaji-dgraph approved these changes May 21, 2020

View reviewed changes

review comments

388ac9d

parasssh commented May 21, 2020

View reviewed changes

Merge branch 'master' into paras/vault_intg

908edbe

parasssh merged commit ee9be0b into master May 21, 2020

jarifibrahim reviewed Jun 4, 2020

View reviewed changes

joshua-goldstein deleted the paras/vault_intg branch August 11, 2022 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vault Integration with Dgraph #5402

Vault Integration with Dgraph #5402

parasssh commented May 8, 2020 •

edited by dgraph-bot

Loading

manishrjain left a comment

parasssh left a comment

martinmr left a comment

parasssh left a comment

parasssh left a comment

martinmr left a comment

vvbalaji-dgraph commented May 21, 2020

vvbalaji-dgraph left a comment

parasssh left a comment

jarifibrahim left a comment

jarifibrahim Jun 4, 2020

Vault Integration with Dgraph #5402

Vault Integration with Dgraph #5402

Conversation

parasssh commented May 8, 2020 • edited by dgraph-bot Loading

manishrjain left a comment

Choose a reason for hiding this comment

parasssh 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

martinmr left a comment

Choose a reason for hiding this comment

vvbalaji-dgraph commented May 21, 2020

vvbalaji-dgraph left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

jarifibrahim left a comment

Choose a reason for hiding this comment

jarifibrahim Jun 4, 2020

Choose a reason for hiding this comment

parasssh commented May 8, 2020 •

edited by dgraph-bot

Loading