[BREAKING] Change default behavior to block operations in ACL #4390
Conversation
Earlier we followed fail open approach, meaning allow operation if no rules are specified. Now we block operation by default.
| // 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", |
There was a problem hiding this comment.
unauthorized on predicate
Use square or angular brackets for terms (op name, predicate name)
dgraph/cmd/alpha/http_test.go
Outdated
| @@ -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) { | |||
dgraph/cmd/alpha/http_test.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| goto createBody |
There was a problem hiding this comment.
This is an infinite loop
| @@ -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) | |||
There was a problem hiding this comment.
Use a V(*) function instead.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
changed the title
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Reviewable status: 9 of 10 files reviewed, 4 unresolved discussions (waiting on @animesh2049, @manishrjain, and @martinmr)
There was a problem hiding this comment.
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)
| @@ -43,6 +43,7 @@ func main() { | |||
| } | |||
| pc := api.NewDgraphClient(conn) | |||
| c := dgo.NewDgraphClient(pc) | |||
| err = c.Login(ctx, "groot", "password") | |||
There was a problem hiding this comment.
ineffectual assignment to err (from ineffassign)
There was a problem hiding this comment.
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)
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