Sort sources correctly so that 'all' comes last

[l2dy]

sort_sources/1 is supposed to put 'all' matches at the end of the list.

Therefore, the ordering function should return false for {all, _}, given the rule that Fun(A, B) is to return true only if A compares less than or equal to B in the ordering.

[martinsumner]

So I agree that the code does not match the comment at the moment. Also given that netmasks are ordered by most specific first, it makes sense that user should be ordered by more specific first too (i.e. we should probably believe the comment is the actual intention and not the code).

This only matters in the case that the netmask is the same (only then does the user become relevant, I think). The swapping of order on equality is also strange - why do that?

What was the overall purpose? The sorted list is checked in the match_source function, and the first match is taken. So I assume the idea is that a resource could be protected by multiple rules, so that you could say allow password only access from this specific IP, or a specific user but require certificates from everyone else.

I'm pretty sure this isn't documented or tested. These are the two main tests for security:

https://github.com/OpenRiak/riak_test/blob/openriak-3.4/tests/http_security.erl
https://github.com/OpenRiak/riak_test/blob/openriak-3.4/tests/pb_security.erl

There is also the issue of groups - https://github.com/OpenRiak/riak_core/blob/openriak-3.4/src/riak_core_security.erl#L1128-L1140 - which have the same sorting error.

I have a general criticism of riak_core_security as-is, in that it tries to be too configurable, and in being too configurable opens up cases where the security applied might not be predictable.

Not sure what the right thing to do:

• Accept as a low-level fix of an "obvious" error (perhaps with the group sorting changed as well);
• Require the fix to more broadly resolve the problem i.e. document and test multiple match behaviour so that we can be clear what an operator can expect;
• Use the emergence of this issue as a trigger to simplify the security module.

@WarpEngineer - do you have thoughts on this?

I'm not so bothered about the risk of changing current behaviour, as there is limited evidence of the security module being used in the wild, and this is a PR targeting a major release.

[WarpEngineer]

@martinsumner I took a look at this and agree with you. Since this isn't used much I don't see the harm in fixing it. We should fix the group one as well and make sure all tests either pass or are updated properly. With that said, just having someone open this PR should be a sign for us to raise the priority a little bit on reexamining the whole security architecture.

[l2dy]

I have updated the branch to fix the group one as well and a quick test of ./rebar3 eunit passed:

=======================================================
  All 2553 tests passed.

[martinsumner]

Normally we would some sort of test, but the overhead do actually trying to create this test scenario I don't think is worth the effort - it is an obvious mistake, so just fixing it is sufficient.

[martinsumner]

CI build now broken, but not related to this change - #48