Make sure client supplied N <= bucket N #569

Merged
merged 1 commit into from Jun 25, 2013

Projects

None yet

3 participants

@Vagabond

Arbitrary, non-validated N values are scary, and you can probably DOS the cluster with them.

(riak@127.0.0.1)1> {ok, RC} = riak:local_client().
{ok,{riak_client,'riak@127.0.0.1',undefined}}
(riak@127.0.0.1)2> RC:get(<<"bucket">>, <<"key">>, [{n_val, 60}]).
{error,{n_val_violation,60}}
(riak@127.0.0.1)3> RC:put(riak_object:new(<<"bucket">>, <<"key">>, <<"value">>) , [{n_val, 60}]).
{error,{n_val_violation,60}}
(riak@127.0.0.1)4> RC:put(riak_object:new(<<"bucket">>, <<"key">>, <<"value">>) , [{n_val, 1}]).
ok
(riak@127.0.0.1)5> RC:get(<<"bucket">>, <<"key">>, [{n_val, 3}]).
{error,notfound}
(riak@127.0.0.1)6> RC:get(<<"bucket">>, <<"key">>, [{n_val, 3}, {notfound_ok, true}]).
{ok,{r_object,<<"bucket">>,<<"key">>,
              [{r_content,{dict,3,16,16,8,80,48,
                                {[],[],[],[],[],[],[],[],[],[],[],[],...},
                                {{[],[],[],[],[],[],[],[],[],[],...}}},
                          <<"value">>}],
              [{<<"#\tþùQ§~\r">>,{1,63537151143}}],
              {dict,1,16,16,8,80,48,
                    {[],[],[],[],[],[],[],[],[],[],[],[],[],...},
                    {{[],[],[],[],[],[],[],[],[],[],[],...}}},
              undefined}}
(riak@127.0.0.1)7>

Without this patch, a user would be able to write 60 replicas of data in a bucket that says it only has 3 replicas.

cc @slfritchie and @jtuple

@reiddraper

rage-driven development at its best

@jtuple

Not too worried about DDOS issues as users with client access to the cluster can just as easily change the bucket N to 60 as well. However, you are right -- arbitrary N values are scary, and this is something we should fix. The issue was raised during the original review of the custom N work (hence the addition of the comment about not verifying), but the issue was punted on to meet the code freeze deadline. But, I think it's better to be safe than sorry here. Changes are minimal enough to slip into 1.4 before we cut the RC.

Code changes looks good. Diff looks way worse than it is given the whitespace changes, disregarding whitespace changes and the code is nice and too the point. For the curious:
https://github.com/basho/riak_kv/compare/006f820665ecd779f7ef344454cfa91c8acc1494...8ff2bf23d0d6ce7b34ee76781d9bb3ad5a9efd95?w=1

Merged into master and tested things a bit. Change works as advertised without harming anything else.

+1 merge

Since this doesn't merge cleanly and I've already taken time to merge for testing, I'm going to go ahead and merge to master myself. Thanks for the targeted rage @Vagabond!

@jtuple jtuple merged commit 8ff2bf2 into master Jun 25, 2013

1 check failed

Details default The Travis CI build failed
@jaredmorrow jaredmorrow deleted the adt-validate-custom-n branch Jun 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment