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

etcdctl/ctlv3: auth: how to create /home/ directory ? #6359

Closed
glycerine opened this issue Sep 6, 2016 · 29 comments · Fixed by #6404
Closed

etcdctl/ctlv3: auth: how to create /home/ directory ? #6359

glycerine opened this issue Sep 6, 2016 · 29 comments · Fixed by #6404

Comments

@glycerine
Copy link
Contributor

glycerine commented Sep 6, 2016

Related to the wilcard '*' discussion on #6355 #5896:

I have a set of clients. I would like to give each a /home/ sub-directory, so that they may write and read in their home directory. They should be able to read other people's home directories, but not write. For example:

/home/jason
/home/xiang
/home/hitoshi

So the permissions I would like to grant to each of my users would be:

role user can read on /home/*

while individual users can write on /home/$USER/*, where $USER is the current user.
I would be fine, if really necessary, with creating a new role for each user,
in order to assign that role the write permission, for example: role jason would have write permission on /home/jason/*. This does seem a little tedious though.

I am simply organizing my client-server communication using a model
that most users will be familiar with. Namely, the unix file system /home/
directory conventions.

Without * wildcard support, I don't see a way to do this. Could you
suggest a workaround, or consider implementing * wildcard support?

@glycerine
Copy link
Contributor Author

@mitake may be working on something for this, per his comment on #6355

It seems that the prefix permission is required by some people so I'll work on it.

@glycerine
Copy link
Contributor Author

@soyking, does this capture your use case as well?

@soyking
Copy link

soyking commented Sep 6, 2016

yeah, I want this feature too:)

* seems like prefix match, but actually the menu tree in k-v system seems useful both in authentication and visualization.etcdv3 is a flat k-v system, but it seems that it leaves more work to the user in my experience.Just some gossip:)

@xiang90
Copy link
Contributor

xiang90 commented Sep 6, 2016

@glycerine @soyking

The permission supports range. You can do /home/xiang-> /home/xian+byte(g+1). We probably can make etcdctl better by adding a --prefix option to help users to do this calculation automatically.

It would be great if you can contribute. The prefix generation code is here https://github.com/coreos/etcd/blob/master/clientv3/op.go#L214

@glycerine
Copy link
Contributor Author

Range is painful to use. For example, I don't know how to specify the
range that would be the same as /home/jason/* and include all the utf8
characters. Why not just use the simple *. Wasn't it already in v2?

I'd be happy to help if you're okay with implementing *

@soyking
Copy link

soyking commented Sep 7, 2016

@xiang90

And, in my test, if you use the range such as /home/xiang to /home/xianh, then you could access to the key /home/xianh too. That's weird.

@mitake
Copy link
Contributor

mitake commented Sep 7, 2016

@glycerine v3 API provides flat key space, it is essentially different from the Unix file system model. A key of etcd doesn't have a metadata like inode and there is no concept of directly. So we need to think with the range based permission. It is also required for effective range query.

@soyking It is weird. I'll look at it and fix.

@glycerine
Copy link
Contributor Author

glycerine commented Sep 7, 2016

Hi @mitake, the point was that the system should be able to model such common situations.

At the risk of repeating an un-refuted observation, ranges are painful to use. To prove this, I issued a challenge, which has gone unmet.

There's nothing in a flat keyspace that mandates the use of ranges.

If it will be accepted when complete, I again offer to implement the much more usable wildcards.

@mitake
Copy link
Contributor

mitake commented Sep 7, 2016

@glycerine Of course I don't disagree with the prefix based permission :) I wanted to say that etcd cannot have a thing like inode because it isn't a file system and we need to implement the prefix based checking on the range based checking like @xiang90 pointed.

In addition, it allows a situation like this: some users can be granted to access to /hom* and it is a superset of /home/* so the users can access to anyone's keys under /home/*. The behavior can never happen the unix file system (permission to non existing files is impossible) and it will be also different from v2 API. So the prefix permission built on the range based permission will be a little bit complicated.

@glycerine
Copy link
Contributor Author

Hmm... indeed, * might be easy to shoot oneself in the foot with. I'd rather not loose the modeling power, though.

How about, in addition to simple prefix + * patterns, we add the following convention to address the usability issue that you point out:

If a path ends in /, then we treat it with directory semantics: having permission to read the directory gives permission to read everything beyond it. Having permission to write the directory gives permission to write everything 'inside' (beyond) it.

Thus the trailing / in the following paths

/home/jason/
/home/xiang/
/home/hitoshi/

would be sufficient to set permissions for all keys that contained them as prefixes. Hence the common model of directories used in zookeeper and filesystems would be readily used.

@glycerine
Copy link
Contributor Author

For example, #6371 would seem to accomplish this, would it not?

@mitake
Copy link
Contributor

mitake commented Sep 7, 2016

I think * patterns and the home directory concept are overkill. Simply giving up them and having the prefix permission that is built on the range permission would be enough.

I agree with your opinion that the prefix based permission is useful. However, mocking the entire semantics of the Unix file systems wouldn't be a good idea. The file system-like APIs would be comfortable because everyone knows it but it is not so strong (just directories + uninterpreted blob files). etcd v3 API is far convenient than the fs API: it already provides range query (AFAIK it is required by k8s for its scalability) and multiple key txn. In addition, secondary index seems to be supported in the future (https://speakerdeck.com/philips/etcd-next-steps-with-the-cornerstone-of-distributed-systems). These features are possible because etcd is essentially different from file systems.

ZooKeeper and Chubby provides file system like APIs but they (especially Chubby) were designed years before. I'm not sure the design isn't a negative legacy in these days because if a storage system is not scale-out type, giving up rich data types and operations isn't necessary (e.g. Dynamo vs RDBMSes).

glycerine added a commit to glycerine/etcd that referenced this issue Sep 7, 2016
 Implement wildcards: when '*' or '/' are
 then last symbol in a key, they designate
 a prefix range implicitly.

 Also: many fencepost bugs fixed in the range
 merge code, including one incorrect
 test in auth/range_perm_cache_test.go

 Fixes etcd-io#6359
glycerine added a commit to glycerine/etcd that referenced this issue Sep 7, 2016
 Implement wildcards for get, pul and del.
 When '*' or '/' is the last symbol in a key,
 the key is converted to a prefix range.

 Also: fix many fencepost bugs fixed in the range
 merge code, including one incorrect
 test in auth/range_perm_cache_test.go

 Fixes etcd-io#6359
glycerine added a commit to glycerine/etcd that referenced this issue Sep 7, 2016
 Implement wildcards for get, pul and del.
 When '*' or '/' is the last symbol in a key,
 the key is converted to a prefix range.

 Also: fix many fencepost bugs fixed in the range
 merge code, including one incorrect
 test in auth/range_perm_cache_test.go

 Fixes etcd-io#6359
glycerine added a commit to glycerine/etcd that referenced this issue Sep 7, 2016
 Implement wildcard '*' for get and del.
 When '*' is the last symbol in a key,
 the key is converted to a prefix range.
 The wildcard may only appear as the last
 symbol in a key.

 Also: fix several fencepost bugs fixed in
 the range sorting/merge code, including one
 incorrect test in auth/range_perm_cache_test.go

 Fixes etcd-io#6359
glycerine added a commit to glycerine/etcd that referenced this issue Sep 7, 2016
 Implement wildcard '*' for get and del.
 When '*' is the last symbol in a key,
 the key is converted to a prefix range.
 The wildcard may only appear as the last
 symbol in a key.

 Also: fix several fencepost bugs fixed in
 the range sorting/merge code, including one
 incorrect test in auth/range_perm_cache_test.go

 Fixes etcd-io#6359
@heyitsanthony
Copy link
Contributor

@mitake @glycerine etcd could cleanly support directory hierarchies and wildcards with a protocol extension proxy on the KV and Watch services with the regular etcd3 flat binary key-space underneath. That way the etcd core stays simple but server-imposed structure on the keyspace is still available for anyone that wants it. What do you think?

@mitake
Copy link
Contributor

mitake commented Sep 7, 2016

@heyitsanthony yes supporting the file system like API as an extension seems to be a good idea. How will you implement it? Use some keys as inodes and others as blocks?

@heyitsanthony
Copy link
Contributor

@mitake -- It'll look like the v3 API except * and / are special characters. If you do Get("/a/b/c/*") it'll return the directory listing for /a/b/c/ like @glycerine's approach in #6371 except a proxy would translate the requests to the etcd3 keyspace. It'll write a little extra metadata to the key (invisible to the user) for tracking directory depth so listing is efficient.

@mitake
Copy link
Contributor

mitake commented Sep 8, 2016

@heyitsanthony I see, thanks for the description. I agree with supporting the directory model in the proxy layer.

@glycerine
Copy link
Contributor Author

glycerine commented Sep 8, 2016

I don't know how a protocol extension proxy works, but it sounds quite complex.

@soyking
Copy link

soyking commented Sep 8, 2016

@heyitsanthony hope to see that.

Because now for the directory hierarchies, I have to get a key such as some_key and then get keys with prefix some_key/ and then combine then as a tree by myself. It's really not convenient.

glycerine added a commit to glycerine/etcd that referenced this issue Sep 8, 2016
 Implement wildcard '*' for get and del.
 When '*' is the last symbol in a key,
 the key is converted to a prefix range.
 The wildcard may only appear as the last
 symbol in a key.

 Also: fix several fencepost bugs fixed in
 the range sorting/merge code, including one
 incorrect test in auth/range_perm_cache_test.go

 Fixes etcd-io#6359
glycerine added a commit to glycerine/etcd that referenced this issue Sep 8, 2016
 Implement wildcard '*' for get and del.
 When '*' is the last symbol in a key,
 the key is converted to a prefix range.
 The wildcard may only appear as the last
 symbol in a key.

 Also: fix several fencepost bugs fixed in
 the range sorting/merge code, including one
 incorrect test in auth/range_perm_cache_test.go

 Fixes etcd-io#6359
glycerine added a commit to glycerine/etcd that referenced this issue Sep 8, 2016
 Implement wildcard '*' for get and del.
 When '*' is the last symbol in a key,
 the key is converted to a prefix range.
 The wildcard may only appear as the last
 symbol in a key.

 Also: fix several fencepost bugs fixed in
 the range sorting/merge code, including one
 incorrect test in auth/range_perm_cache_test.go

 Fixes etcd-io#6359
glycerine added a commit to glycerine/etcd that referenced this issue Sep 8, 2016
 Implement wildcard '*' for get and del.
 When '*' is the last symbol in a key,
 the key is converted to a prefix range.
 The wildcard may only appear as the last
 symbol in a key.

 Also: fix several fencepost bugs fixed in
 the range sorting/merge code, including one
 incorrect test in auth/range_perm_cache_test.go

 Fixes etcd-io#6359
@glycerine
Copy link
Contributor Author

glycerine commented Sep 8, 2016

What I discovered in doing #6371 was that special handling for / doesn't seem required after all, that handling * appeared to suffice.

The implementation in #6371 is pretty complete now, modulo the unknown proxy ideas floating around. Give it a try and let me know if you find it usable from the shell; I find it pretty reasonable. The backslash escape (\*) is not needed to write a *, and isn't needed on querying unless you want to grab a key that ends in a star. My repo has it on branch wild6371; https://github.com/glycerine/etcd/tree/wild6371

glycerine added a commit to glycerine/etcd that referenced this issue Sep 8, 2016
 Implement wildcard '*' for get and del.
 When '*' is the last symbol in a key,
 the key is converted to a prefix range.
 The wildcard may only appear as the last
 symbol in a key.

 Also: fix several fencepost bugs fixed in
 the range sorting/merge code, including one
 incorrect test in auth/range_perm_cache_test.go

 Fixes etcd-io#6359
glycerine added a commit to glycerine/etcd that referenced this issue Sep 8, 2016
 Implement wildcard '*' for get and del.
 When '*' is the last symbol in a key,
 the key is converted to a prefix range.
 The wildcard may only appear as the last
 symbol in a key.

 Also: fix several fencepost bugs fixed in
 the range sorting/merge code, including one
 incorrect test in auth/range_perm_cache_test.go

 Fixes etcd-io#6359
glycerine added a commit to glycerine/etcd that referenced this issue Sep 8, 2016
 Implement wildcard '*' for get and del.
 When '*' is the last symbol in a key,
 the key is converted to a prefix range.
 The wildcard may only appear as the last
 symbol in a key.

 Also: fix several fencepost bugs fixed in
 the range sorting/merge code, including one
 incorrect test in auth/range_perm_cache_test.go

 Fixes etcd-io#6359
@glycerine
Copy link
Contributor Author

glycerine commented Sep 8, 2016

Hmm... I'm starting to understand the tradeoff here between usability from the command line, and usability from the API.

The wildcard star * makes etcdctl much, much easier to use from the command line.

However, as @heyitsanthony pointed out, that means that it is hard to write, from the command line, a key that ends with a star. helloworld* can easily be created as a key. But querying it becomes tricky. I tried adding an escape, such as the \, but then that creates the same difficulty: the key that you write is different from the key that you read. This would be bad for a program that doesn't want to care what the keys it is handling are. The program just wants to pass through the key unchanged, and be able to query on that same key to get the stored value. With escapes \ and wildcards *, the query must change the key before making the query. So not so good.

One approach to this would be to move the transformation of terminating wildcard * into the etcdctl client, so that on the wire there are never wildcards. However plenty of APIs will want to avail themselves of the convenience of wildcards. All they have to give up in exchange is agreeing to forego keys that end in *. For me this is a cheap and readily accepted tradeoff. It seems to me that the conclusion is inescapable. An additional side-band bit is needed in the API to indicate whether etcd should expand terminating * into a prefix range or not.

@heyitsanthony
Copy link
Contributor

@glycerine etcdctl shouldn't rewrite the requests to handle wildcards because then there's no way to write binary keys using the command line without preprocessing. Plus, etcdctl's behavior would then diverge from the client behavior.

Really, this should be done with a proxy; there's already some grpc proxy code in REPO_ROOT/proxy/grpcproxy which should help with getting started writing a new proxy. If there's a proxy you can point etcdctl to the proxy and submit queries with *'s through etcdctl. The proxy will rewrite the requests and forward to the etcd server and etcdctl will respond the way you want without breaking binary keys.

@heyitsanthony
Copy link
Contributor

@glyercine do you only need to set roles for a prefix using etcdctl auth? If so, would adding a --prefix flag to etcdctl to handle your use case? There's already a similar feature in etcdctl get so it seems like the way to do it.

@glycerine
Copy link
Contributor Author

@heyitsanthony, @mitake: etcdctl --prefix grant-permissions might do the job.

If you're not going to merge it, you should at least cherry pick (manually) the range and range-merge logic fixes I did in #6371. There were a bunch of insidious and painful to track down logic bugs, as well as an incorrect test.

Specifically:

https://github.com/coreos/etcd/pull/6371/files#diff-e807c9d20ed5566b76a11ae69b345f44R40

https://github.com/coreos/etcd/pull/6371/files#diff-e807c9d20ed5566b76a11ae69b345f44R43

https://github.com/coreos/etcd/pull/6371/files#diff-e807c9d20ed5566b76a11ae69b345f44R100 (lines 100-108)

https://github.com/coreos/etcd/pull/6371/files#diff-55d74d95bb1853641dbd100adbf43f80R109

@heyitsanthony
Copy link
Contributor

@glycerine OK, the isSubset fix looks like the right thing to do. Not sure about the other ones (@mitake knows those paths better). We can do a bugfix PR if you want to be involved in that (if not, that's fine, I can pick it up for you). Thanks!

@xiang90
Copy link
Contributor

xiang90 commented Sep 8, 2016

@heyitsanthony @glycerine

We probably should still try to add --prefix option to auth permission as I mentioned in a previous reply. I agree with @glycerine that asking users to calculate what is the right range end for granting /xiang/* is not ideal.

@glycerine
Copy link
Contributor Author

glycerine commented Sep 8, 2016

@heyitsanthony @mitake

yes, please go ahead and run with an independent bugfix pr, I'm swamped.

Questions though, let me know. The test (test 15, counting from zero, in TestGetMergedPerms in etcd/auth/range_perm_cache_test.go) was trying to say that [a, b) merged with [b, "") should be [a, b), which it is not; the two cannot be merged, because [a,b) is different from [a,b]; in the range convention used, [a,b] must be represented as two ranges. Second, the basic merge logic incorrectly handled the case of merging [a,b) with [b, ""), because it tried to (incorrectly) return [a, ""). Here "" means the empty string.

@xiang90
Copy link
Contributor

xiang90 commented Sep 8, 2016

@glycerine Thanks for your analysis and work so far. We will try to take it over, and fix the issues you reported soon.

@mitake
Copy link
Contributor

mitake commented Sep 9, 2016

@glycerine Yes the existing range mechanism has some bugs. I'll cherry pick your commit and create a new PR. Thanks a lot for analysis and fix!

glycerine added a commit to glycerine/etcd that referenced this issue Sep 12, 2016
Test 15, counting from zero, in TestGetMergedPerms
in etcd/auth/range_perm_cache_test.go, was trying
incorrectly assert that [a, b) merged with [b, "")
should be [a, b). Added a test specifically for
this. This patch fixes the incorrect larger test
and the bugs in the code that it was hiding.

Fixes etcd-io#6359
glycerine added a commit to glycerine/etcd that referenced this issue Sep 12, 2016
Test 15, counting from zero, in TestGetMergedPerms
in etcd/auth/range_perm_cache_test.go, was trying
incorrectly assert that [a, b) merged with [b, "")
should be [a, b). Added a test specifically for
this. This patch fixes the incorrect larger test
and the bugs in the code that it was hiding.

Fixes etcd-io#6359
glycerine added a commit to glycerine/etcd that referenced this issue Sep 12, 2016
Test 15, counting from zero, in TestGetMergedPerms
in etcd/auth/range_perm_cache_test.go, was trying
incorrectly assert that [a, b) merged with [b, "")
should be [a, b). Added a test specifically for
this. This patch fixes the incorrect larger test
and the bugs in the code that it was hiding.

Fixes etcd-io#6359
gyuho pushed a commit that referenced this issue Sep 15, 2016
Test 15, counting from zero, in TestGetMergedPerms
in etcd/auth/range_perm_cache_test.go, was trying
incorrectly assert that [a, b) merged with [b, "")
should be [a, b). Added a test specifically for
this. This patch fixes the incorrect larger test
and the bugs in the code that it was hiding.

Fixes #6359
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

5 participants