allow users to know their roles and permissions #8164
Conversation
|
Looks good for what I had in mind! |
auth/store.go
Outdated
| @@ -165,6 +165,9 @@ type AuthStore interface { | |||
|
|
|||
| // WithRoot generates and installs a token that can be used as a root credential | |||
| WithRoot(ctx context.Context) context.Context | |||
|
|
|||
| // BelongTo checks that user belongs to role | |||
| BelongTo(user, role string) bool | |||
There was a problem hiding this comment.
OK, I'll change the name
etcdctl/ctlv3/command/get_command.go
Outdated
| continue | ||
| } | ||
|
|
||
| opts := []clientv3.OpOption{} |
There was a problem hiding this comment.
this ignores options passed in as flags?
etcdctl/ctlv3/command/get_command.go
Outdated
|
|
||
| opts := []clientv3.OpOption{} | ||
| opts = append(opts, clientv3.WithRange(string(perm.RangeEnd))) | ||
| getResp, err := client.KV.Get(context.TODO(), string(perm.Key)) |
There was a problem hiding this comment.
possibly inconsistent since there's no WithRev to fix the revision across multiple gets
auth/store.go
Outdated
| } | ||
|
|
||
| for _, r := range u.Roles { | ||
| if strings.Compare(role, r) == 0 { |
auth/store.go
Outdated
|
|
||
| u := getUser(tx, user) | ||
| if u == nil { | ||
| plog.Errorf("tried to check user %s belongs to role %s, but user %s doesn't exist", user, role, user) |
There was a problem hiding this comment.
Thanks, I'll fix it.
etcdctl/ctlv3/command/get_command.go
Outdated
| @@ -19,7 +19,9 @@ import ( | |||
| "strings" | |||
There was a problem hiding this comment.
I'm not especially enthusiastic about supporting this soft-failure mode as a one-off feature that only fetches all keys since it's considerable baggage and doesn't fit well with the rest of the system. For example, should delete range also have a soft-fail mode like this? Txns? Watches?
If it's worth having auth soft-fail, I feel it should be a client wrapper so that it can be used anywhere. For instance, etcdctl could take a flag --auth-soft-fail that would replace the client with the soft fail client so every command supports it.
There was a problem hiding this comment.
I see. As you commented, I'll drop the etcdctl part in this PR and revisit it later. Providing the soft-fail mode for other operations for checking write permissions would be valuable.
etcdserver/apply_auth.go
Outdated
| @@ -189,6 +190,28 @@ func (aa *authApplierV3) checkLeasePuts(leaseID lease.LeaseID) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (aa *authApplierV3) UserGet(r *pb.AuthUserGetRequest) (*pb.AuthUserGetResponse, error) { | |||
| err := aa.as.IsAdminPermitted(&aa.authInfo) | |||
| if err != nil && strings.Compare(r.Name, aa.authInfo.Username) != 0 { | |||
There was a problem hiding this comment.
r.Name != aa.authInfo.Username?
There was a problem hiding this comment.
I'll fix it, thanks.
Current UserGet() and RoleGet() RPCs require admin permission. It means that users cannot know which roles they belong to and what permissions the roles have. This commit change the semantics and now users can know their roles and permissions.
mitake
force-pushed
the
auth-granted-keys
branch
from
b0ecc0c to
db59588
Compare
|
@heyitsanthony updated based on your comments. I also added e2e test cases for covering the new semantics of the RPCs. Could you take a look? |
mitake
changed the title
|
lgtm. defer to @heyitsanthony |
This PR adds a new option
--all-readabletoetcdctl get. If the option is passed, etcdctl tries to get all read granted keys from a user specified with--user.For doing it, the first commit changes the semantics of
UserGet()andRoleGet()semantics. Now they can be called by non admin users if the information belongs to the user itself.Fix #8157
@hzektser @japhar81 could you check that this is what you want?
/cc @heyitsanthony I'll add e2e test cases for the changes until earlier next week.