[BREAKING] Change default behavior to block operations in ACL #4390

animesh2049 · 2019-12-10T12:37:54Z

Earlier we had open fail approach i.e. if no rule is specified for a predicate, all the operations were authorized. Now default behavior is to block any operation.

This change is

Earlier we followed fail open approach, meaning allow operation if no rules are specified. Now we block operation by default.

mangalaman93

A few comments, update the docs and change log for 1.1.1. This is a big breaking change and needs to be specified at the top of the change log.

mangalaman93 · 2019-12-10T13:09:47Z

edgraph/acl_cache.go

-	// by default we follow the fail open approach and allow the operation
-	return nil
+	// by default we block operation
+	return errors.Errorf("unauthorized to do %s on predicate %s",


unauthorized on predicate
Use square or angular brackets for terms (op name, predicate name)

mangalaman93 · 2019-12-10T13:13:22Z

dgraph/cmd/alpha/http_test.go

@@ -49,6 +50,43 @@ type params struct {
 	Variables map[string]string `json:"variables"`
 }

+func runGzipWithRetry(contentType, url string, buf io.Reader, gzReq, gzResp bool) (*http.Response, error) {


write the doc string

mangalaman93 · 2019-12-10T13:14:53Z

dgraph/cmd/alpha/http_test.go

+		if err != nil {
+			return nil, err
+		}
+		goto createBody


This is an infinite loop

mangalaman93 · 2019-12-10T13:19:44Z

edgraph/access_ee.go

@@ -70,6 +70,7 @@ func (s *Server) Login(ctx context.Context,
 		glog.Errorf(errMsg)
 		return nil, errors.Errorf(errMsg)
 	}
+	glog.Infof("%s logged in successfully", user.UserID)


Use a V(*) function instead.

manishrjain

Put a breaking prefix. Also, we should look into other DBs to see if default-open is a policy. If so, having a flag to let the user decide would be great.

Reviewed 10 of 10 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @animesh2049 and @mangalaman93)

dgraph/cmd/alpha/http_test.go, line 53 at r1 (raw file):

}

func runGzipWithRetry(contentType, url string, buf io.Reader, gzReq, gzResp bool) (*http.Response, error) {

100 chars.

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

Previously, mangalaman93 (Aman Mangal) wrote…

Use a V(*) function instead.

Maybe for now just keep it as it is. If users complain about too many logs, we could make it V(1).

edgraph/acl_cache.go, line 94 at r1 (raw file):

		}
	}

Put a note here that we could potentially have a default open policy as well via a flag, if requested by the users.

martinmr

Could a default-open policy be setup by creating a rule with all permissions and an empty prefix?

I am not sure if this is better than the flag but it would be preserved across restarts. With the flag, a user could restart Dgraph and forget the flag. Also, the behavior would be more explicit since the rule would show up in the list of ACL rules.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @animesh2049 and @mangalaman93)

dgraph/cmd/alpha/http_test.go, line 55 at r1 (raw file):

func runGzipWithRetry(contentType, url string, buf io.Reader, gzReq, gzResp bool) (*http.Response, error) {

createBody:

A few suggestions regarding this loop:

Like Aman said, it's infinite now. Since this is a test it doesn't matter as much but it should at least sleep for a bit before retrying to avoid hogging the CPU.
The client could probably be declared outside the loop.
I think using a for loop would make this more readable and it would make it clearer this is a loop.

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

"%s logged in successfully"

minor: Maybe "User %s logged in successfully".

edgraph/acl_cache.go, line 96 at r1 (raw file):

	// no rule has been defined that can match the predicate
	// by default we block operation

minor: This comment can fit into a single line

// No rule has been defined that can match the predicate. By default we block operation.

manishrjain

We no longer have regexps or prefixes. Only exact rule matching. Flags are typically not changed, they are part of their config files. They tend to be more permanent than anything else in the system.

Part of the task is to investigate what sort of perms other DBs have. Do they all default open policies at all and how.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @animesh2049 and @mangalaman93)

animesh2049

I looked in to some other dbs, I couldn't find anything related to default open policy. Some of them (neo4j, couchbase) have idea of reader group, which by default has read access to everything.
I looked into postgresql as well. It doesn't have anything like default open. I can't find this explicit thing in documentation but a new user in postgresql doesn't have any access on any database/table by default.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @animesh2049 and @mangalaman93)

animesh2049

Reviewable status: 9 of 10 files reviewed, 8 unresolved discussions (waiting on @animesh2049, @mangalaman93, @manishrjain, and @martinmr)

dgraph/cmd/alpha/http_test.go, line 53 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

100 chars.

Done.

dgraph/cmd/alpha/http_test.go, line 53 at r1 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

write the doc string

Done.

dgraph/cmd/alpha/http_test.go, line 55 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

A few suggestions regarding this loop:

Like Aman said, it's infinite now. Since this is a test it doesn't matter as much but it should at least sleep for a bit before retrying to avoid hogging the CPU.

The client could probably be declared outside the loop.

I think using a for loop would make this more readable and it would make it clearer this is a loop.

Loop it is.

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

Previously, martinmr (Martin Martinez Rivera) wrote…

"%s logged in successfully"
minor: Maybe "User %s logged in successfully".

Is it okay if I leave it as it is? This seems okay to me.

edgraph/acl_cache.go, line 97 at r1 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

unauthorized on predicate
Use square or angular brackets for terms (op name, predicate name)

Some tests are failing because of that. Leaving it for now.

mangalaman93

Reviewable status: 9 of 10 files reviewed, 4 unresolved discussions (waiting on @animesh2049, @manishrjain, and @martinmr)

mangalaman93

Please be careful in merging the PR. We are doing a minor release and this is breaking change. Personally, I would prefer that we add an alternate way of allowing all predicates for a user at least.

Reviewable status: 9 of 10 files reviewed, 4 unresolved discussions (waiting on @animesh2049, @manishrjain, and @martinmr)

golangcibot · 2019-12-16T11:11:29Z

contrib/integration/mutates/main.go

@@ -43,6 +43,7 @@ func main() {
 	}
 	pc := api.NewDgraphClient(conn)
 	c := dgo.NewDgraphClient(pc)
+	err = c.Login(ctx, "groot", "password")


ineffectual assignment to err (from ineffassign)

mangalaman93

Reviewable status: 4 of 14 files reviewed, 8 unresolved discussions (waiting on @animesh2049, @manishrjain, and @martinmr)

contrib/scripts/functions.sh, line 44 at r4 (raw file):

   | python -c \
   "import json; resp = raw_input(); data = json.loads(resp); print data['data']['accessJWT']"
}

newline

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

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Is it okay if I leave it as it is? This seems okay to me.

At least, put angular or square bracket around %s

edgraph/access_ee.go, line 639 at r4 (raw file):

					return errors.Errorf("the permission of ACL predicates can not be changed")
				case isAclPredMutation(gmu.Del):
					// even groot can't delete ACL predicates

Can keep this comment

edgraph/acl_cache.go, line 97 at r4 (raw file):

	// no rule has been defined that can match the predicate
	// by default we block operation
	return errors.Errorf("unauthorized to do %s on predicate %s",

angular or square bracket around predicate name and operation name

(cherry picked from commit a1964f6)

animesh2049 added 2 commits December 9, 2019 18:31

Block access of predicates by default

0340dac

Earlier we followed fail open approach, meaning allow operation if no rules are specified. Now we block operation by default.

Fix failing tests due to change in default access behavior

d4e95e0

animesh2049 requested review from manishrjain and a team as code owners December 10, 2019 12:37

mangalaman93 requested changes Dec 10, 2019

View reviewed changes

manishrjain approved these changes Dec 10, 2019

View reviewed changes

martinmr suggested changes Dec 10, 2019

View reviewed changes

manishrjain reviewed Dec 10, 2019

View reviewed changes

animesh2049 changed the title ~~Change default behavior to block operations in ACL~~ [BREAKING] Change default behavior to block operations in ACL Dec 11, 2019

animesh2049 commented Dec 11, 2019

View reviewed changes

Address PR comments

f204287

animesh2049 commented Dec 11, 2019

View reviewed changes

mangalaman93 requested review from manishrjain and martinmr December 11, 2019 13:50

mangalaman93 approved these changes Dec 11, 2019

View reviewed changes

mangalaman93 reviewed Dec 11, 2019

View reviewed changes

animesh2049 added 2 commits December 16, 2019 13:46

Add login in load test

ca95739

Add login to more tests

1741b70

golangcibot reviewed Dec 16, 2019

View reviewed changes

animesh2049 added 3 commits December 16, 2019 16:43

check err

82c53e2

Merge branch 'master' into animesh2049/change-default-access-level

0184cbb

Authorize mutate for guardians

bd5ad19

mangalaman93 approved these changes Dec 26, 2019

View reviewed changes

Adress PR comments

6433dfe

animesh2049 merged commit a1964f6 into master Dec 26, 2019

danielmai pushed a commit that referenced this pull request Jan 12, 2020

[BREAKING] Change default behavior to block operations in ACL (#4390)

a5df53b

(cherry picked from commit a1964f6)

animesh2049 deleted the animesh2049/change-default-access-level branch January 14, 2020 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BREAKING] Change default behavior to block operations in ACL #4390

[BREAKING] Change default behavior to block operations in ACL #4390

animesh2049 commented Dec 10, 2019 •

edited by manishrjain

Loading

mangalaman93 left a comment

mangalaman93 Dec 10, 2019

mangalaman93 Dec 10, 2019

mangalaman93 Dec 10, 2019

mangalaman93 Dec 10, 2019

manishrjain left a comment

martinmr left a comment

manishrjain left a comment

animesh2049 left a comment

animesh2049 left a comment

mangalaman93 left a comment

mangalaman93 left a comment

golangcibot Dec 16, 2019

mangalaman93 left a comment

[BREAKING] Change default behavior to block operations in ACL #4390

[BREAKING] Change default behavior to block operations in ACL #4390

Conversation

animesh2049 commented Dec 10, 2019 • edited by manishrjain Loading

mangalaman93 left a comment

Choose a reason for hiding this comment

mangalaman93 Dec 10, 2019

Choose a reason for hiding this comment

mangalaman93 Dec 10, 2019

Choose a reason for hiding this comment

mangalaman93 Dec 10, 2019

Choose a reason for hiding this comment

mangalaman93 Dec 10, 2019

Choose a reason for hiding this comment

manishrjain 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

animesh2049 left a comment

Choose a reason for hiding this comment

animesh2049 left a comment

Choose a reason for hiding this comment

mangalaman93 left a comment

Choose a reason for hiding this comment

mangalaman93 left a comment

Choose a reason for hiding this comment

golangcibot Dec 16, 2019

Choose a reason for hiding this comment

mangalaman93 left a comment

Choose a reason for hiding this comment

animesh2049 commented Dec 10, 2019 •

edited by manishrjain

Loading