Sasha/rbac #652
Sasha/rbac #652
Conversation
|
|
||
| clt, err := NewClient(s.srv.URL, nil) | ||
| clt, err := NewClient(s.srv.URL, nil, roundtrip.BasicAuth(teleport.RoleAdmin.User(), "<something>")) |
There was a problem hiding this comment.
need a comment explaining what is this for. I kind of understand the purpose, but not really. IIRC the security on cluster API (aka site API) is done on the SSH level, HTTP does not matter, so why basic auth?
There was a problem hiding this comment.
this is just for tests to fake auth in some cases, I will add comment
| role := services.RoleForUser(teleportUser) | ||
| err := s.a.UpsertRole(role) | ||
| c.Assert(err, IsNil) | ||
| teleportUser.Roles = []string{role.GetMetadata().Name} |
There was a problem hiding this comment.
this is so frequent that adding role.GetName() would make sense
| return trace.AccessDenied("user %v is locked until %v", utils.HumanTimeFormat(status.LockExpires)) | ||
| } | ||
| fnErr := fn() | ||
| if !trace.IsAccessDenied(fnErr) && !trace.IsBadParameter(fnErr) { |
There was a problem hiding this comment.
this is dangerous. if fn() does not return a properly wrapped trace error object (which will happen), "access denied" behavior will break.
access denied situation needs to be bulletproof, maybe like error, accessGranted := fun(). where accessGranted is a boolean.
There was a problem hiding this comment.
✅ good catch. I've made it more restrictive.
| return s.Authority.GenerateUserCert(privateKey, key, username, allowedLogins, ttl) | ||
| } | ||
|
|
||
| func (s *AuthServer) withUserLock(username string, fn func() error) error { |
There was a problem hiding this comment.
This function seems to be pretty critical to security, I would provide detailed comments on its purpose and expectations for fn().
| @@ -614,6 +677,13 @@ func (s *AuthServer) GetWebSessionInfo(userName string, id string) (*Session, er | |||
| }, nil | |||
| } | |||
|
|
|||
| func (s *AuthServer) DeleteNamespace(namespace string) error { | |||
| if namespace == defaults.Namespace { | |||
| return trace.AccessDenied("can't delete default namespace") | |||
There was a problem hiding this comment.
the default namespace: %s would be better
There was a problem hiding this comment.
or... is the default an empty string?
There was a problem hiding this comment.
no, default is literally default
| @@ -140,7 +140,7 @@ func (s *AuthServer) Create#U2FRegisterRequest(token string) (u2fRegisterRe | |||
| return nil, trace.Wrap(err) | |||
| } | |||
|
|
|||
| lock := "#token"+token | |||
| lock := "#token" + token | |||
There was a problem hiding this comment.
haha, we're running different versions of gofmt - everyone should upgrade.
| @@ -216,6 +216,13 @@ func (s *AuthServer) CreateUserWithToken(token, password, hotpToken string) (*Se | |||
| } | |||
|
|
|||
| // apply user allowed logins | |||
| role := services.RoleForUser(&tokenData.User) | |||
| if err := s.UpsertRole(role); err != nil { | |||
There was a problem hiding this comment.
this will always auto-create "user:user" role. I am not sure it's a desired behavior especially when I'm calling tctl user add --role=restricted ev
There was a problem hiding this comment.
✅ fixed will only create user once
| } | ||
| } else { | ||
| if err := a.Access.DeleteRole(role.GetMetadata().Name); err != nil { | ||
| return trace.Wrap(err) |
There was a problem hiding this comment.
"not found" error should be ignored in this case, because (see my comments above) in a role-based environment, I (as a teleport admin) would want to delete these auto-generated roles, almost always.
| @@ -525,7 +532,7 @@ func (tc *TeleportClient) Join(sessionID session.ID, input io.Reader) (err error | |||
| // connect to server: | |||
| fullNodeAddr := node.Addr | |||
| if tc.SiteName != "" { | |||
| fullNodeAddr = fmt.Sprintf("%s@%s", node.Addr, tc.SiteName) | |||
| fullNodeAddr = fmt.Sprintf("%s@%s@%s", node.Addr, tc.Namespace, tc.SiteName) | |||
There was a problem hiding this comment.
wow. this breaks compatibility between clients/servers and why is it even necessary? the server already knows the namespace of a node and will reject access based on RBAC rules.
There was a problem hiding this comment.
old format is still supported. I will double check with old clients of course
| @@ -94,6 +94,9 @@ type Config struct { | |||
| // Labels represent host Labels | |||
| Labels map[string]string | |||
|
|
|||
| // Namespace is nodes namespace | |||
| Namespace string | |||
There was a problem hiding this comment.
I do not think namespaces should be exposed to clients at all.
There was a problem hiding this comment.
talked about this in person - decided to keep it as is as experimental feature for 1.4. release
RBAC: Part 1
This PR adds core RBAC set for #620 , does not address external OIDC users yet.
I haven't vendored things either yet to simplify code reivew.