etcdctl/ctlv3: auth: wildcard terminated paths specify ranges

[glycerine]

Still needs tests, but creating pull request to facilitate the discussion in #6359

[xiang90]

What if other people ask for /*/abc? Or /*/abc/*/cef? Should we support these as well? I feel people would ask for globbing eventually, which we do not really want to support in etcd core.

[glycerine]

What if other people ask for //abc? Or //abc/*/cef? Should we support these as well? I feel people would ask for globbing eventually, which we do not really want to support in etcd core.

I don't need them or find them compelling.

But maybe I don't understand what the situations they would be used to model. Could you elaborate on the first one in particular -- what is having two slashes as the first two runes trying to do? Under the current PR, it would receive no special consideration, as it neither ends in * nor in /.

I think this simple PR is perfectly defensible. nats.io and many other pub/sub systems have used similar (filesystem) semantics.

[xiang90]

@glycerine Sorry. Github messed up the format. I updated my previous reply.

[glycerine]

My answer stays the same. : )
I don't find them compelling, nor do I need them. It seems we agree.

[glycerine]

My implementation is flawed in some way, as tests would have shown.

Where is not obvious... gets seem to work, but put does not. When I track it down, it is because the tocheck slice in the checkKeyPerm() function has length zero during the write attempt; here: https://github.com/coreos/etcd/blob/master/auth/range_perm_cache.go#L148

Hints appreciated.

[glycerine]

Yes, I'm mightily confused. Still trying to debug why I have no writes going through.

I have a role, jason, that has write permissions.

$ etcdctl --user root:123 role get jason
Role jason
KV Read:
        /home/jason/*
KV Write:
        /home/jason/*

And yet: inside isRangeOpPermitted(), even if I force a call to getMergedPerms() every time, I still get back no write permissions (at https://github.com/coreos/etcd/blob/master/auth/range_perm_cache.go#L168; with the if !ok changed to an if true)

perms back: '&auth.unifiedRangePermissions{readPerms:[]*auth.rangePerm{(*auth.rangePerm)(0xc42041a0c0)}, writePerms:[]*auth.rangePerm(nil)}'

UPDATE: never mind, I didn't assign the role to the user.
Reflection: it would still be nice to have user $USER automatically have role $USER as well.

[glycerine]

mostly working now. more testing needed.

[glycerine]

arg, had to track down of number of insidious bugs in the existing range and range-merging code. those are fixed now, but I still need tests for the new functionality.

[glycerine]

If somebody could help out with getting a test that does the following (in library/code rather than shell outs to etcdctl of couse) I would very much appreciate it. I'm out of time for a while to figure out how to write these tests, it would be nice get these fixes and additions in place, and I'm sure somebody is more familiar with the testing facilities already available than I am.

test plan:

- create a root user, pw 456, a root role, assign the root role to the root user
- create a regular user called joe, with pw 123, a role called 'regular'
- assign the regular role to the user joe
- give the regular role readonly permissions on /home/*
- create a role called joe, assign it to joe, give it readwrite on /home/joe/*  (joe ends up with 2 roles)
etcdctl auth enable
etcdctl --user root:456 put /home/haddy/hello
etcdctl --user joe:123 put /home/joe/bday 040181
etcdctl --user joe:123 put /home/joe/height "6-0"
- verify that joe can read /home/haddy/hello but not delete it or write a key alongside it. (using the regular role).
- verify that joe can read bday and height from home/joe/ using 'get /home/joe/*'
- verify that joe can delete 2 keys with the library equivalent of : 'etcdctl --user joe:123 del '/home/joe/*'

[heyitsanthony]

where is this called?

[glycerine]

It is simply the inverse of expandPrefixToRange(). Not in use at the moment, but likely to be needed in the near future, possibly in the tests that are TBD. By the way, any pointers re the test plan would be appreciated; e.g. what api calls would accomplish the steps outlined?

[heyitsanthony]

It looks like this breaks for any key with a * suffix. There's no way this can go in the way it's designed since it restricts the values keys may take even though etcd3 should be a clean binary keyspace.

[glycerine]

@heyitsanthony what would be acceptable for handing the presence of special characters like *? Do you have an approach in mind? I'm open to ideas.

We're used to bash/filesystem interfaces that treat certain characters specially, spaces and * at the shell prompt for example. To embed them literally in a filename from bash, one would have to escape them with a backslash.

Does escaping a star * with a backslash, as in \*, seem reasonable?

[glycerine]

with the latest:

• Added backslash escaping to allow \* in the presence of wildcard *.
• Addressed the other comments by centralizing wildcard handling into a separate package.
• Addressed the code duplication by defining an interface, BegEndRange, to unify treatment of ranges.

[glycerine]

Added a test file.

@xiang90, @heyitsanthony -- I think this is in good shape. Please take a look.

[heyitsanthony]

@glycerine anything that imposes special structure on the keys can't be part of the base KV API. Expecting people to escape * needlessly complicates the key semantics and absolutely breaks the contract that etcd3 will accept arbitrary binary keys without modification.

This really should be done with a proxy.

[glycerine]

conclusion was that this won't be the way to go. Closing this, and opening a separate pr with the bug fixes to range handling.