Implement alter-user, del-user and del-source #451

Merged
merged 5 commits into from Dec 24, 2013

Projects

None yet

4 participants

@Vagabond

Previously to this, you could add users and sources, but they were effectively immutable because I had not implemented API for changing them. alter-source does not exist because add-source will effectively do what you want. We could add a synonym at the riak-admin level if we want to.

Also, fixes a bug in print-users I found before RICON that didn't make it into the tech preview.

@slfritchie slfritchie commented on an outdated diff Dec 12, 2013
src/riak_core_security.erl
@@ -500,10 +594,15 @@ accumulate_grants([], Seen, Acc) ->
{Acc, Seen};
accumulate_grants([Role|Roles], Seen, Acc) ->
Options = riak_core_metadata:get({<<"security">>, <<"roles">>}, Role),
- NestedRoles = [R || R <- lookup("roles", Options), not lists:member(R, Seen)],
+ NestedRoles = [R || R <- lookup("roles", Options),
+ not lists:member(R,Seen),
+ user_exists(R)],
+ io:format("Nested roles ~p~n", [NestedRoles]),
@slfritchie
slfritchie Dec 12, 2013

Debugging leftover?

@slfritchie

I've given this a quick read. It appears sane. Didn't run it. +1 for what that's worth, sorry, I've other PRs to drive-by now on this iteration.........

@evanmcc evanmcc commented on an outdated diff Dec 21, 2013
src/riak_core_console.erl
@@ -831,6 +832,23 @@ add_user([Username|Options]) ->
Error
end.
+alter_user([Username|Options]) ->
+ case riak_core_security:alter_user(list_to_binary(Username),
+ parse_options(Options)) of
@evanmcc evanmcc commented on an outdated diff Dec 21, 2013
src/riak_core_security.erl
+ Username),
+ %% delete any associated grants, so if a user with the same name
+ %% is added again, they don't pick up these grants
+ riak_core_metadata:fold(fun({Key, _Value}, Acc) ->
+ %% apparently destructive
+ %% iteration is allowed
+ riak_core_metadata:delete({<<"security">>,
+ <<"grants">>},
+ Key),
+ Acc
+ end, undefined,
+ {<<"security">> ,<<"grants">>},
+ [{match, {Username, '_'}}]),
+ %% delete the user out of any other user's 'roles' option
+ %% this is kind of a pain, as we have to iterate ALL roles
+ riak_core_metadata:fold(fun({_, [?TOMBSTONE]}, Acc) ->
@evanmcc
evanmcc Dec 21, 2013

might be nice to factor this deeply-nested case out into a few functions.

@evanmcc

the whole system would more discoverable, I guess, if there were also a print-roles command.

@evanmcc

this PR fails to updated the help text for riak-admin secuity

@evanmcc

nevermind, found the accompanying riak PR

@evanmcc

exceptions, like invalid_option, thrown when processing console commands, aren't caught, so you get no notification (other than a 1 shell return code) that you're having a problem or doing something wrong. e.g.:

12:07:27 <<0.1113.1>> {riak_core_console,parse_options,
                          [["password","asdkljasdk"],[]]}

12:07:27 <<0.1113.1>> {riak_core_console,parse_options,2} -> {throw,
                                                              {error,
                                                               {invalid_option,
                                                                "password"}}}
@evanmcc

It looks like user-specific sources should be found and deleted when the user is deleted

evan@chain:~/src/vagabond/dev/dev1$ ./bin/riak-admin security del-user user 
evan@chain:~/src/vagabond/dev/dev1$ riak-admin security add-source all 0.0.0.0/0 trust
evan@chain:~/src/vagabond/dev/dev1$ ./bin/riak-admin security print-sources
+--------------------+------------+----------+----------+
|       users        |    cidr    |  source  | options  |
+--------------------+------------+----------+----------+
|        user        |127.0.0.1/32| password |    []    |
|        all         | 0.0.0.0/0  |  trust   |    []    |
+--------------------+------------+----------+----------+

Additionally, setting an all source again seems to break printing: Looks like all is becoming an atom when it's updated from the command line, and only the list case is handled.

Kind of a complicated setup, not sure how reproducible it'll be:

  1. build a cluster with these changes
  2. run the http_security riak-test
  3. after it fails: ./bin/riak-admin security del-user user
  4. ./bin/riak-admin security add-source all 0.0.0.0/0 trust
  5. ./bin/riak-admin security add-source user 0.0.0.0/0 trust <-- suspect this isn't actually needed
  6. ./bin/riak-admin security print-sources

I see this:

evan@chain:~/src/vagabond/dev/dev1$ ./bin/riak-admin security print-sources
RPC to 'dev1@127.0.0.1' failed: {'EXIT',
                                 {badarg,
                                  [{erlang,binary_to_list,[all],[]},
                                   {riak_core_security,
                                    '-prettyprint_users/2-lc$^0/1-0-',1,
                                    [{file,"src/riak_core_security.erl"},
                                     {line,73}]},
                                   {riak_core_security,prettyprint_users,2,
                                    [{file,"src/riak_core_security.erl"},
                                     {line,73}]},
                                   {riak_core_security,
                                    '-print_sources/1-lc$^0/1-0-',1,
                                    [{file,"src/riak_core_security.erl"},
                                     {line,88}]},
                                   {riak_core_security,
                                    '-print_sources/1-lc$^0/1-0-',1,
                                    [{file,"src/riak_core_security.erl"},
                                     {line,90}]},
                                   {riak_core_security,print_sources,1,
                                    [{file,"src/riak_core_security.erl"},
                                     {line,88}]},
                                   {rpc,'-handle_call_call/6-fun-0-',5,
                                    [{file,"rpc.erl"},{line,205}]}]}}

Lastly, I am likely doing something wrong, but changing the all role to trust doesn't seem to allow unauthenticated users to access the cluster after it has been changed.

@evanmcc

With those things said, deletion and altering of users seem to work as advertised, sources are now deletable and re-addable, so I think that this is almost ready to go.

@Vagabond

What would print-roles do that print-user(s) does not?

@evanmcc
@Vagabond

I've addressed all the concerns raised, except there is a bug with the interaction of a source for 'all' and a source for particular users. If they overlap you'll get the error you mentioned above.

As discussed with Evan, as this bug was not added by this PR, we will fike a seperate issue and resolve it independantly.

@evanmcc

+1, changes look good.

@Vagabond Vagabond merged commit 76d2260 into develop Dec 24, 2013
@jrwest

@Vagabond sorry for dropping this comment on a commit, faster than finding the original PR:

Just a style note for the future. Instead of exposing the Cluster Metadata tombstone atom in other subsystems, fold can take a default option which can be used to "replace" the tombstone w/ a subsystem friendly value. I ran into this because of #494 which is improperly handling the tombstone. Also, considering this might occur in multiple places would it be useful to have a skip_tombstones option? Given the changes I think would be necessary this would probably have to wait until the next release cycle, however.

@jrwest jrwest modified the milestone: 2.0-beta, 2.0 Mar 24, 2014
@seancribbs seancribbs deleted the adt-security-admin branch Apr 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment