Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow users to know their roles and permissions #8164

Merged
merged 2 commits into from
Jul 5, 2017

Conversation

mitake
Copy link
Contributor

@mitake mitake commented Jun 23, 2017

This PR adds a new option --all-readable to etcdctl 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() and RoleGet() 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.

@hzektser
Copy link

Looks good for what I had in mind!

Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can the server-side user role fetch code be separated into another PR or just drop the etcdctl changes? the server-side stuff is needed for auth soft-fail regardless and looks OK, but I don't think etcdctl is the right place for the client side logic

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HasRole?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll change the name

continue
}

opts := []clientv3.OpOption{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this ignores options passed in as flags?


opts := []clientv3.OpOption{}
opts = append(opts, clientv3.WithRange(string(perm.RangeEnd)))
getResp, err := client.KV.Get(context.TODO(), string(perm.Key))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if role == r?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warningf?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll fix it.

@@ -19,7 +19,9 @@ import (
"strings"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r.Name != aa.authInfo.Username?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix it, thanks.

mitake and others added 2 commits June 26, 2017 22:20
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
Copy link
Contributor Author

mitake commented Jun 27, 2017

@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 mitake changed the title WIP: --all-readable option of etcdctl get allow users to know their roles and permissions Jun 29, 2017
@xiang90
Copy link
Contributor

xiang90 commented Jul 3, 2017

lgtm. defer to @heyitsanthony

Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm thanks

@xiang90 xiang90 merged commit 894751e into etcd-io:master Jul 5, 2017
@mitake mitake deleted the auth-granted-keys branch July 6, 2017 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants