Improving ACL #2951

gitlw · 2019-01-30T02:27:29Z

Added a new subcommand to reset password for a user
Allow the useradd command to take a user's password through terminal
Added support to specify ACL rules through regex on predicates, e.g. ^user(.*)name$ -> Read

This change is

martinmr

Reviewable status: 0 of 7 files reviewed, 11 unresolved discussions (waiting on @gitlw and @manishrjain)

edgraph/access_ee.go, line 285 at r1 (raw file):

type PredRegexRule struct {
	PredRegex *regexp.Regexp
	Perm      int32

I am assuming Perm stands for permissions. Since we can have r+w permissions, would it be better to call it Perms?

edgraph/access_ee.go, line 288 at r1 (raw file):

}

// convert the acl blob to two data sets:

start the comment with

UnmarshalAcl converts the acl blob ...

to avoid a lint warning.

edgraph/access_ee.go, line 306 at r1 (raw file):

			predRegex, err := regexp.Compile(acl.PredFilter.Regex)
			if err != nil {
				glog.Errorf("Unable to compile the predicate regex %v to create an ACL rule",

Should we ignore this error? How will users realize that the ACL rules were not added?

I can see people not verifying the rules leading to unauthorized access.

edgraph/access_ee.go, line 679 at r1 (raw file):

// the operation on the given predicate
func hasAccess(groupId string, predicate string, operation *acl.Operation) error {
	// first try to evaluate the request using the predicate -> permission map

First, try to ...

Also period at the end.

edgraph/access_ee.go, line 712 at r1 (raw file):

	}

	return fmt.Errorf("unable to find a matched rule")

s/matched/matching

ee/acl/groups.go, line 173 at r1 (raw file):

	}
	if len(predicate) == 0 && len(predRegex) == 0 {
		return fmt.Errorf("either --pred or --pred_regex should be specified, but not both")

I think this error message is wrong. It should be something like

"one of --pred or --pred_regex must be specified"

ee/acl/groups.go, line 264 at r1 (raw file):

Quoted 6 lines of code…

		if (!curFilter.IsRegex &&
			!newFilter.IsRegex &&
			curFilter.Predicate == newFilter.Predicate) ||
			(curFilter.IsRegex &&
				newFilter.IsRegex &&
				curFilter.Regex == newFilter.Regex) {

For readability, I would move this into a method that compares two filters and returns true if they are equal.

ee/acl/groups_test.go, line 75 at r1 (raw file):

	require.Equal(t, "friend", updatedAcls5[0].PredFilter.Predicate,
		"the left acl should have the original "+
			"first predicate")

I think this fits well into the previous line. No need to split it.

ee/acl/run_ee.go, line 99 at r1 (raw file):

		},
	}
	chPdFlags := cmdPasswd.Cmd.Flags()

nit: chPwdFlags is a slightly better name

ee/acl/users.go, line 30 at r1 (raw file):

	userid := conf.GetString("user")
	if len(userid) == 0 {
		return fmt.Errorf("The user must not be empty")

lowercase to match the rest of the error messages.

ee/acl/utils.go, line 135 at r1 (raw file):

func askUserPassword(userid string, times int) (string, error) {
	x.AssertTrue(times == 1 || times == 2)

Why do we want to allow users to type their passwords without confirming them?

gitlw

Reviewable status: 0 of 7 files reviewed, 10 unresolved discussions (waiting on @manishrjain and @martinmr)

edgraph/access_ee.go, line 285 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

I am assuming Perm stands for permissions. Since we can have r+w permissions, would it be better to call it Perms?

Done.

edgraph/access_ee.go, line 288 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

start the comment with

UnmarshalAcl converts the acl blob ...

to avoid a lint warning.

Done.

edgraph/access_ee.go, line 306 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Should we ignore this error? How will users realize that the ACL rules were not added?

I can see people not verifying the rules leading to unauthorized access.

I added a check in the chMod command to make sure the passed in regex strings can be compiled.
After that change, the error here should not happen. But I feel it's still worthwhile to log the error for unexpected cases.

edgraph/access_ee.go, line 679 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

First, try to ...

Also period at the end.

Done.

edgraph/access_ee.go, line 712 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

s/matched/matching

Done.

ee/acl/groups.go, line 173 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

I think this error message is wrong. It should be something like

"one of --pred or --pred_regex must be specified"

Done.

ee/acl/groups.go, line 264 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

		if (!curFilter.IsRegex &&
			!newFilter.IsRegex &&
			curFilter.Predicate == newFilter.Predicate) ||
			(curFilter.IsRegex &&
				newFilter.IsRegex &&
				curFilter.Regex == newFilter.Regex) {
For readability, I would move this into a method that compares two filters and returns true if they are equal.

Done.

ee/acl/groups_test.go, line 75 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

I think this fits well into the previous line. No need to split it.

Done.

ee/acl/users.go, line 30 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

lowercase to match the rest of the error messages.

Done.

ee/acl/utils.go, line 135 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Why do we want to allow users to type their passwords without confirming them?

Asking for password once is used when we ask for the groot password before proceeding in the subcommands. In that case, it's not used for changing its password.

golangcibot · 2019-01-31T20:32:51Z

ee/acl/run_ee.go

-		groupSB.WriteString(fmt.Sprintf("acls:%v\n", strings.Join(aclStrs, " ")))
-
-		glog.Infof(groupSB.String())
+		fmt.Sprintf("ACLs : %5s\n", strings.Join(aclStrs, " "))


result of fmt.Sprintf call not used (from govet)

martinmr

Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions (waiting on @gitlw, @golangcibot, and @manishrjain)

edgraph/access_ee.go, line 306 at r1 (raw file):

Previously, gitlw (Lucas Wang) wrote…

I added a check in the chMod command to make sure the passed in regex strings can be compiled.
After that change, the error here should not happen. But I feel it's still worthwhile to log the error for unexpected cases.

Maybe it's better to do both? I can see why it's not a concern right now but in the future someone could mess up the code in chMod that does the check, causing the regex to fail to compile. Logging and then throwing an error would safeguard against that case.

Again, not very likely something like that would happen but it's better to err on the side of caution.

golangcibot · 2019-02-07T00:38:12Z

dgraph/cmd/alpha/run.go

@@ -89,6 +89,8 @@ they form a Raft group and provide synchronous replication.
 	// with the flag name so that the values are picked up by Cobra/Viper's various config inputs
 	// (e.g, config file, env vars, cli flags, etc.)
 	flag := Alpha.Cmd.Flags()
+	flag.Bool("enterprise_features", false,
+		"Enable Dgraph enterprise features. If you set this to true, you agree to the Dgraph Community License.")


line is 107 characters (from lll)

gitlw

Reviewable status: 0 of 13 files reviewed, 3 unresolved discussions (waiting on @golangcibot, @manishrjain, and @martinmr)

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

Previously, golangcibot (Bot from GolangCI) wrote…

line is 107 characters (from lll)

Done.

edgraph/access_ee.go, line 306 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Maybe it's better to do both? I can see why it's not a concern right now but in the future someone could mess up the code in chMod that does the check, causing the regex to fail to compile. Logging and then throwing an error would safeguard against that case.

Again, not very likely something like that would happen but it's better to err on the side of caution.

Done.

ee/acl/run_ee.go, line 297 at r3 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

result of fmt.Sprintf call not used (from govet)

Done.

dgraph/cmd/alpha/login_ee.go

gitlw

Reviewable status: 0 of 20 files reviewed, 4 unresolved discussions (waiting on @golangcibot, @manishrjain, and @martinmr)

dgraph/cmd/alpha/login_ee.go, line 60 at r6 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

Error return value of writeResponse is not checked (from errcheck)

Done.

manishrjain

Reviewable status: 0 of 20 files reviewed, 12 unresolved discussions (waiting on @gitlw, @golangcibot, @manishrjain, and @martinmr)

dgraph/cmd/root.go, line 85 at r7 (raw file):

	RootCmd.PersistentFlags().Bool("expose_trace", false,
		"Allow trace endpoint to be accessible from remote")
	RootCmd.PersistentFlags().Bool("enterprise_features", false,

Why remove this?

dgraph/cmd/alpha/login_ee.go, line 59 at r7 (raw file):

	var out bytes.Buffer
	out.WriteString(fmt.Sprintf("ACCESS JWT:\n%s\n", jwt.AccessJwt))

send back a JSON, so they can parse easily.

dgraph/cmd/alpha/run.go, line 93 at r7 (raw file):

	flag := Alpha.Cmd.Flags()
	flag.Bool("enterprise_features", false,
		"Enable Dgraph enterprise features. "+

This can lie in the last line.

dgraph/cmd/zero/zero.go, line 554 at r7 (raw file):

	// Multiple Groups might be assigned to same tablet, so during proposal we will check again.
	tablet.Force = false
	if _, isAclPred := x.AclPreds[tablet.Predicate]; isAclPred {

Can we just add a function in x, which can do this comparison in a good way, and use it here and elsewhere?

edgraph/access_ee.go, line 333 at r7 (raw file):

		// predPerms is the map descriebed above that maps a single
		// predicate to a submap, and the submap maps a group to a permission
		predPerms := make(map[string]map[string]int32)

This would go out of hands really quickly. Let's encapsulate all this logic of updating and accessing this cache in a separate class. And add tests for it.

edgraph/access_ee.go, line 383 at r7 (raw file):

		aclCache.Lock()
		aclCache.predPerms = predPerms

aclCache can just have a pointer to an object, which handles the logic of checking the group/pred perms.

edgraph/access_ee.go, line 541 at r7 (raw file):

	// if we get here, we know the user is not Groot.
	if op.DropAll {
		return fmt.Errorf("only Groot is allowed to drop all data")

Can still mention who the current user is. Can be anonymous.

edgraph/access_ee.go, line 740 at r7 (raw file):

func authorizePredicate(groups []string, predicate string, operation *acl.Operation) error {
	aclCache.RLock()
	predPerms, predRegexRules := aclCache.predPerms, aclCache.predRegexRules

still not safe. They are maps, which are pointers by default. This is not a copy.

You can just run the whole thing within the read lock. Not going to have much performance impact, because writes to these are rare.

gitlw

Reviewable status: 0 of 20 files reviewed, 12 unresolved discussions (waiting on @golangcibot, @manishrjain, and @martinmr)

dgraph/cmd/root.go, line 85 at r7 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Why remove this?

In our last review, we said we wanted to move this flag to be only applicable to alpha, since it does not make sense for the live loader, bulk loader, increment commands etc.

dgraph/cmd/alpha/login_ee.go, line 59 at r7 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

send back a JSON, so they can parse easily.

Done.

dgraph/cmd/alpha/run.go, line 93 at r7 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This can lie in the last line.

Done.

dgraph/cmd/zero/zero.go, line 554 at r7 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Can we just add a function in x, which can do this comparison in a good way, and use it here and elsewhere?

Done.

edgraph/access_ee.go, line 333 at r7 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This would go out of hands really quickly. Let's encapsulate all this logic of updating and accessing this cache in a separate class. And add tests for it.

Done.

edgraph/access_ee.go, line 383 at r7 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

aclCache can just have a pointer to an object, which handles the logic of checking the group/pred perms.

Done.

edgraph/access_ee.go, line 541 at r7 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Can still mention who the current user is. Can be anonymous.

Done.

edgraph/access_ee.go, line 740 at r7 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

still not safe. They are maps, which are pointers by default. This is not a copy.

You can just run the whole thing within the read lock. Not going to have much performance impact, because writes to these are rare.

My current design is that these two data structures are immutable, and each update would create new maps and lists. Hence only the top level references are protected by the mutex.

dgraph/cmd/alpha/login_ee.go

martinmr

Reviewed 1 of 7 files at r1, 3 of 10 files at r5, 5 of 13 files at r6, 5 of 10 files at r7, 8 of 10 files at r8, 1 of 1 files at r9.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @gitlw, @golangcibot, @manishrjain, and @martinmr)

dgraph/cmd/alpha/http.go, line 264 at r9 (raw file):

Quoted 5 lines of code…

	if accessJwt := r.Header.Get("X-Dgraph-AccessJWT"); accessJwt != "" {
		md := metadata.New(nil)
		md.Append("accessJwt", accessJwt)
		ctx = metadata.NewIncomingContext(ctx, md)
	}

This logic is repeated twice. Would it make sense to have it as a helper method.

dgraph/cmd/alpha/http.go, line 440 at r9 (raw file):

	if accessJwt := r.Header.Get("X-Dgraph-AccessJWT"); accessJwt != "" {
		md.Append("accessJwt", accessJwt)
	}

This logic is also similar. Would it make sense to include it in the helper method as well.

edgraph/access_ee.go, line 524 at r1 (raw file):

			}); err != nil {
			return status.Error(codes.PermissionDenied,
				fmt.Sprintf("unauthorized to alter the predicate:%v", err))

add space after the colon

edgraph/access_ee.go, line 578 at r9 (raw file):

}

type ACLAccessLog struct {

I'd prefer AclAccessLog. All those uppercase letters together are harder to read.

ee/acl/groups.go, line 44 at r9 (raw file):

	defer func() {
		if err := txn.Discard(ctx); err != nil {
			fmt.Printf("Unable to discard transaction: %v\n", err)

why are you moving this messages to stdout? Is this meant to be run from the console?

ee/acl/groups.go, line 157 at r9 (raw file):

	}
	if len(predicate) > 0 && len(predRegex) > 0 {
		return fmt.Errorf("one of --pred or --pred_regex must be specified")

change both error messages to "only one of ..."

martinmr

Reviewed 1 of 3 files at r10.
Reviewable status: 20 of 22 files reviewed, 14 unresolved discussions (waiting on @gitlw, @golangcibot, @manishrjain, and @martinmr)

dgraph/cmd/alpha/http.go, line 264 at r9 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

	if accessJwt := r.Header.Get("X-Dgraph-AccessJWT"); accessJwt != "" {
		md := metadata.New(nil)
		md.Append("accessJwt", accessJwt)
		ctx = metadata.NewIncomingContext(ctx, md)
	}
This logic is repeated twice. Would it make sense to have it as a helper method.

Ok.

dgraph/cmd/alpha/http.go, line 440 at r9 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

	if accessJwt := r.Header.Get("X-Dgraph-AccessJWT"); accessJwt != "" {
		md.Append("accessJwt", accessJwt)
	}
This logic is also similar. Would it make sense to include it in the helper method as well.

Ok.

edgraph/access_ee.go, line 524 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

add space after the colon

Ok.

edgraph/access_ee.go, line 578 at r9 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

I'd prefer AclAccessLog. All those uppercase letters together are harder to read.

Ok.

ee/acl/groups.go, line 44 at r9 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

why are you moving this messages to stdout? Is this meant to be run from the console?

Feel free to mark this as done if there's nothing to do here.

ee/acl/groups.go, line 157 at r9 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

change both error messages to "only one of ..."

Ok.

manishrjain

Couple of comments. Address those and merge.

Reviewable status: 20 of 22 files reviewed, 10 unresolved discussions (waiting on @gitlw, @golangcibot, @manishrjain, and @martinmr)

dgraph/cmd/alpha/login_ee.go, line 58 at r10 (raw file):

	response := map[string]interface{}{}
	mp := map[string]interface{}{}

aren't they strings? If so, can be a map[string]string.

edgraph/access_ee.go, line 462 at r10 (raw file):

		// check that we have the modify permission on the predicate
		err := aclCache.authorizePredicate(groupIds, op.DropAttr, acl.Modify)
		logACLAccess(&AclAccessLog{

Rename to logAccess(&AccessRecord)

edgraph/access_ee.go, line 578 at r10 (raw file):

}

type AclAccessLog struct {

rename to accessEntry.

…graph-io#2951)

gitlw requested review from manishrjain and a team and removed request for manishrjain January 30, 2019 02:27

martinmr suggested changes Jan 31, 2019

View reviewed changes

gitlw commented Jan 31, 2019

View reviewed changes

golangcibot reviewed Jan 31, 2019

View reviewed changes

martinmr suggested changes Jan 31, 2019

View reviewed changes

gitlw force-pushed the gitlw/improving_acl branch from f22348e to 8ddea45 Compare February 1, 2019 19:43

golangcibot reviewed Feb 7, 2019

View reviewed changes

gitlw force-pushed the gitlw/improving_acl branch from 047f29a to fb10d1c Compare February 7, 2019 01:35

gitlw commented Feb 7, 2019

View reviewed changes

golangcibot reviewed Feb 7, 2019

View reviewed changes

dgraph/cmd/alpha/login_ee.go Outdated Show resolved Hide resolved

Lucas Wang added 6 commits February 8, 2019 11:23

Improving ACL

2d6a6f0

make the log captital case

b8153d7

pinning acl predicates to group 1

30d9ae9

added acl support for curl commands

1b38f12

Fixing golang error

bd910d0

fixing flaky tests

c2c337d

gitlw force-pushed the gitlw/improving_acl branch from ecd68cf to c2c337d Compare February 8, 2019 19:27

Lucas Wang added 3 commits February 8, 2019 12:12

fixed issue caused by rebase

be22390

allowing more cases for retry

02a353c

removed the enterprise_features flag from the zero instance

97d77ab

gitlw commented Feb 8, 2019

View reviewed changes

self review and improved code

dbf92b1

manishrjain suggested changes Feb 11, 2019

View reviewed changes

Addressed Manish's comments

fbbd5bb

gitlw commented Feb 12, 2019

View reviewed changes

gitlw force-pushed the gitlw/improving_acl branch from fe34e26 to fbbd5bb Compare February 12, 2019 02:12

golangcibot reviewed Feb 12, 2019

View reviewed changes

dgraph/cmd/alpha/login_ee.go Outdated Show resolved Hide resolved

making gobot happy

bce34d3

martinmr suggested changes Feb 12, 2019

View reviewed changes

addressed Martin's comments

b0d89b3

martinmr approved these changes Feb 13, 2019

View reviewed changes

manishrjain approved these changes Feb 13, 2019

View reviewed changes

addressed Manish's comment

89cc18c

gitlw merged commit c8281f1 into master Feb 13, 2019

gitlw deleted the gitlw/improving_acl branch February 13, 2019 18:58

dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019

Added curl support for ACLs, pinning ACL predicates to group 1 etc (d…

fdf57b5

…graph-io#2951)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improving ACL #2951

Improving ACL #2951

gitlw commented Jan 30, 2019 •

edited

Loading

martinmr left a comment

gitlw left a comment

golangcibot Jan 31, 2019

martinmr left a comment

golangcibot Feb 7, 2019

gitlw left a comment

gitlw left a comment

manishrjain left a comment

gitlw left a comment

martinmr left a comment

martinmr left a comment

manishrjain left a comment

Improving ACL #2951

Improving ACL #2951

Conversation

gitlw commented Jan 30, 2019 • edited Loading

martinmr left a comment

Choose a reason for hiding this comment

gitlw left a comment

Choose a reason for hiding this comment

golangcibot Jan 31, 2019

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

golangcibot Feb 7, 2019

Choose a reason for hiding this comment

gitlw left a comment

Choose a reason for hiding this comment

gitlw left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

gitlw left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

gitlw commented Jan 30, 2019 •

edited

Loading