auth: add authentication and authorization interface #496

jfontan · 2018-10-23T17:45:57Z

The interface has two methods:

Mysql: returns a vitess server auth
Allow: checks if a user has permissions needed

Currently two auth methods are implemented:

None: allows all logins and permissions
Native: uses mysql_native_password method to authenticate.

Native method can read users from a json file. This contains the user
credentials and permissions for the user. The credentials can be in
plain or in mysql native format. It can also contain a list of all
permissions granted to the user. If not specified it uses default
permissions that for now is "read".

[
  {
    "name": "root",
    "password": "*9E128DA0C64A6FCCCDCFBDD0FC0A2C967C6DB36F",
    "permissions": ["read", "write"]
  },
  {
    "name": "javi",
    "password": "Passw0rd!"
  }
]

To enforce permissions a new validation rule can be added with the
builder:

ab := analyzer.NewBuilder(catalog).WithAuth(userAuth)

Related to #469, needs audit logs

On handler.NewConnection the user is still not authenticated so the connection lack some information needed to create the session. Now the session is created the first time a context is needed. Signed-off-by: Javi Fontan <jfontan@gmail.com>

The interface has two methods: * Mysql: returns a vitess server auth * Allow: checks if a user has permissions needed Currently two auth methods are implemented: * None: allows all logins and permissions * Native: uses mysql_native_password method to authenticate. Native method can read users from a json file. This contains the user credentials and permissions for the user. The credentials can be in plain or in mysql native format. It can also contain a list of all permissions granted to the user. If not specified it uses default permissions that for now is "read". [ { "name": "root", "password": "*9E128DA0C64A6FCCCDCFBDD0FC0A2C967C6DB36F", "permissions": ["read", "write"] }, { "name": "javi", "password": "Passw0rd!" } ] To enforce permissions a new validation rule can be added with the builder: ab := analyzer.NewBuilder(catalog).WithAuth(userAuth) Signed-off-by: Javi Fontan <jfontan@gmail.com>

smola · 2018-10-24T07:18:29Z

auth/auth.go

+// for users.
+type Auth interface {
+	Mysql() mysql.AuthServer
+	Allowed(user string, permission Permission) error


Maybe return (bool, error)?

This would allow to differentiate from not having permissions and failing to check permissions. The former would probably produce a warning audit log (when we have audit logs), the second would probably also produce a regular error log.

(bool, error) sounds to me like a fuzzy logic:
(true, nil)
(false, nil)
(false, err)

and hopefully it's not possible to have:
(true, err)

It's like returning *bool it can give you (nil, *true, *false)

@kuba-- It's a common pattern in Go, also in our own codebase. (true, err) is usually not relevant since the value would not be even checked if err != nil.

But alternatively, you can keep just err as return value and use a special error kind ErrNotAuthorized to differentiate from other errors.

@smola - totally understand, just as I mentioned, personally I don't like this pattern, because have a feeling that it's a boolean logic with extra dimension.

I'm moving to use go-errors and returning ErrNotAuthorized so is easier to tell apart.

erizocosmico · 2018-10-24T07:48:36Z

server/context.go

 func (s *SessionManager) NewSession(conn *mysql.Conn) {
 	s.mu.Lock()
-	s.sessions[conn.ConnectionID] = s.builder(conn, s.addr)
+	s.sessions[conn.ConnectionID] = s.newSession(conn)


i don't understand this change. Why do we need s.builder call in another function? It's just one line

erizocosmico · 2018-10-24T07:49:33Z

server/context.go

 	s.mu.Unlock()
 }

 // NewContext creates a new context for the session at the given conn.
 func (s *SessionManager) NewContext(conn *mysql.Conn) *sql.Context {
 	s.mu.Lock()
-	sess := s.sessions[conn.ConnectionID]
+	sess, ok := s.sessions[conn.ConnectionID]
+	if !ok {


This case can't happen. If it does not exist in s.sessions, the connection is not opened.

I have not mentioned that I also had to change when sessions are created. Before the session was created in NewConnection. This is called before the user is authenticated so the sessions did not contain user information (mysql.Conn had User = ""). To overcome this the sessions are created the first time NewContext is called, when he user is already authenticated and we can add the user name to the session.

Any lack of permission error should have ErrNotAuthorized kind. Signed-off-by: Javi Fontan <jfontan@gmail.com>

Signed-off-by: Javi Fontan <jfontan@gmail.com>

smola · 2018-10-25T07:07:53Z

auth/auth.go

+import (
+	"strings"
+
+	errors "gopkg.in/src-d/go-errors.v0"


smola · 2018-10-25T07:11:15Z

auth/auth.go

+// for users.
+type Auth interface {
+	Mysql() mysql.AuthServer
+	Allowed(user string, permission Permission) error


Please, add godoc to the interface. Specially a mention to errors returned fo Allowed. My guess is that the one that is not expected to be implementation-depdendent is ErrNoAuthorized.

ajnavarro · 2018-10-25T07:56:45Z

auth/native.go

+func (s *Native) Allowed(name string, permission Permission) error {
+	u, ok := s.users[name]
+	if !ok {
+		return ErrNotAuthorized.Wrap(ErrNoPermission.New(permission))


this error will print permissions correctly?

Permission has a String method so it is correctly printed:

not authorized: user does not have permission: read, write

ajnavarro · 2018-10-25T08:00:31Z

sql/analyzer/check_auth.go

+const CheckAuthorizationRule = "check_authorization"
+
+// CheckAuthorization creates an authorization check with the given Auth.
+func CheckAuthorization(au auth.Auth) RuleFunc {


Could you delete the old rule that we used to make possible read-only queries?

I'll add the possibility to specify the permissions when using NewNativeSingle so it can be used to disable writing instead of read only rule.

Signed-off-by: Javi Fontan <jfontan@gmail.com>

jfontan added 2 commits October 23, 2018 19:41

server: create session when needed

3b3b8b0

On handler.NewConnection the user is still not authenticated so the connection lack some information needed to create the session. Now the session is created the first time a context is needed. Signed-off-by: Javi Fontan <jfontan@gmail.com>

jfontan requested review from smola and a team October 23, 2018 17:45

kuba-- added the wip work in progress label Oct 23, 2018

smola reviewed Oct 24, 2018

View reviewed changes

erizocosmico suggested changes Oct 24, 2018

View reviewed changes

erizocosmico approved these changes Oct 24, 2018

View reviewed changes

jfontan added 3 commits October 24, 2018 12:54

auth: use go-errors to better identify causes

e678357

Any lack of permission error should have ErrNotAuthorized kind. Signed-off-by: Javi Fontan <jfontan@gmail.com>

server: do not use extra function to call builder

de8c11a

Signed-off-by: Javi Fontan <jfontan@gmail.com>

integration: fix test server

20d732a

Signed-off-by: Javi Fontan <jfontan@gmail.com>

jfontan changed the title ~~[WIP] auth: add authentication and authorization interface~~ auth: add authentication and authorization interface Oct 24, 2018

jfontan requested a review from a team October 24, 2018 16:44

jfontan added 2 commits October 24, 2018 19:02

auth: fix both clean and native password

f4aabc3

Signed-off-by: Javi Fontan <jfontan@gmail.com>

auth: test auth modules

277877e

Signed-off-by: Javi Fontan <jfontan@gmail.com>

jfontan force-pushed the feature/auth-system branch from 4bf1c04 to 277877e Compare October 24, 2018 17:02

jfontan removed the wip work in progress label Oct 24, 2018

smola reviewed Oct 25, 2018

View reviewed changes

ajnavarro reviewed Oct 25, 2018

View reviewed changes

jfontan added 5 commits October 25, 2018 11:12

auth: use go-errors.v1

e04a75b

Signed-off-by: Javi Fontan <jfontan@gmail.com>

auth: add documentation to auth.Auth interface

9271159

Signed-off-by: Javi Fontan <jfontan@gmail.com>

auth: specify permissions in NewNativeSingle

01ffab1

Signed-off-by: Javi Fontan <jfontan@gmail.com>

analyzer: delete readonly rule

c874286

Signed-off-by: Javi Fontan <jfontan@gmail.com>

integration: fix integration server

8cb84a7

Signed-off-by: Javi Fontan <jfontan@gmail.com>

ajnavarro approved these changes Oct 25, 2018

View reviewed changes

smola approved these changes Oct 25, 2018

View reviewed changes

ajnavarro merged commit 0c31e30 into src-d:master Oct 25, 2018

jfontan mentioned this pull request Nov 6, 2018

auth: add Audit to log user interactions #536

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auth: add authentication and authorization interface #496

auth: add authentication and authorization interface #496

jfontan commented Oct 23, 2018 •

edited

Loading

smola Oct 24, 2018

kuba-- Oct 24, 2018

smola Oct 24, 2018

kuba-- Oct 24, 2018

jfontan Oct 24, 2018

erizocosmico Oct 24, 2018

erizocosmico Oct 24, 2018

jfontan Oct 24, 2018

smola Oct 25, 2018

jfontan Oct 25, 2018

smola Oct 25, 2018 •

edited

Loading

jfontan Oct 25, 2018

ajnavarro Oct 25, 2018

jfontan Oct 25, 2018

ajnavarro Oct 25, 2018

jfontan Oct 25, 2018

jfontan Oct 25, 2018

auth: add authentication and authorization interface #496

auth: add authentication and authorization interface #496

Conversation

jfontan commented Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smola Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfontan commented Oct 23, 2018 •

edited

Loading

smola Oct 25, 2018 •

edited

Loading