Skip to content

Commit

Permalink
Fix policy parser to accept multiple IP address per statement
Browse files Browse the repository at this point in the history
Following style of IP address description in IPAddress Condition have
not been working as parser implicitly assumed single IP address per
'aws:SourceIp' condition.

```
"IpAddress": {
    "aws:SourceIp": ["127.0.0.1/32", "192.168.100.0/24"]
}
```
  • Loading branch information
kuenishi committed Jul 31, 2015
1 parent 8eaf49f commit 0ec4bee
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 19 deletions.
17 changes: 14 additions & 3 deletions src/riak_cs_s3_policy.erl
Original file line number Diff line number Diff line change
Expand Up @@ -531,8 +531,8 @@ eval_ip_address(Req, Conds) ->
{error, _} ->
false;
{Peer, _} ->
IPConds = [ IPCond || {'aws:SourceIp', IPCond} <- Conds ],
eval_all_ip_addr(IPConds, Peer)
IPConds = [IPCond || {'aws:SourceIp', IPCond} <- Conds ],
eval_all_ip_addr(lists:flatten(IPConds), Peer)
end.

eval_all_ip_addr([], _) -> false;
Expand Down Expand Up @@ -568,7 +568,10 @@ statement_to_pairs(#statement{sid=Sid, effect=E, principal=P, action=A,

-spec condition_block_from_condition_pair(condition_pair()) -> {binary(), list()}.
condition_block_from_condition_pair({AtomKey, Conds})->
Fun = fun({'aws:SourceIp', IP}) -> {'aws:SourceIp', print_ip(IP)};
Fun = fun({'aws:SourceIp', IPs}) when is_list(IPs) ->
{'aws:SourceIp', [print_ip(IP) || IP <- IPs]};
({'aws:SourceIp', IP}) when is_tuple(IP) ->
{'aws:SourceIp', print_ip(IP)};
(Cond) -> Cond
end,
{atom_to_binary(AtomKey, latin1), lists:map(Fun, Conds)}.
Expand Down Expand Up @@ -794,6 +797,14 @@ condition_({<<"aws:SourceIp">>, Bin}) when is_binary(Bin)->
IP ->
{'aws:SourceIp', IP}
end;
condition_({<<"aws:SourceIp">>, BinIPs}) when is_list(BinIPs)->
IPs = [case parse_ip(Bin) of
{error, _} ->
throw({error, malformed_policy_condition});
IP ->
IP
end || Bin <- BinIPs],
{'aws:SourceIp', IPs};
condition_({<<"aws:UserAgent">>, Bin}) -> % TODO: check string condition
{'aws:UserAgent', Bin};
condition_({<<"aws:Referer">>, Bin}) -> % TODO: check string condition
Expand Down
1 change: 1 addition & 0 deletions src/riak_cs_wm_utils.erl
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,7 @@ handle_policy_eval_result(_, true, OwnerId, RD, Ctx) ->
%% Policy says yes while ACL says no
shift_to_owner(RD, Ctx, OwnerId, Ctx#context.riak_client);
handle_policy_eval_result(User, _, _, RD, Ctx) ->
%% Policy says no
#context{riak_client=RcPid,
response_module=ResponseMod,
user=User,
Expand Down
38 changes: 23 additions & 15 deletions test/riak_cs_s3_policy_eqc.erl
Original file line number Diff line number Diff line change
Expand Up @@ -33,25 +33,30 @@
-include_lib("webmachine/include/wm_reqdata.hrl").

-define(TEST_ITERATIONS, 500).
-define(SET_MODULE, twop_set).
-define(QC_OUT(P),
eqc:on_output(fun(Str, Args) -> io:format(user, Str, Args) end, P)).

-define(TIMEOUT, 60).

eqc_test_()->
{spawn,
{inparallel,
[
{timeout, 20, ?_assertEqual(true,
quickcheck(numtests(?TEST_ITERATIONS,
?QC_OUT(prop_ip_filter()))))},
{timeout, 20, ?_assertEqual(true,
quickcheck(numtests(?TEST_ITERATIONS,
?QC_OUT(prop_secure_transport()))))},
{timeout, 20, ?_assertEqual(true,
quickcheck(numtests(?TEST_ITERATIONS,
?QC_OUT(prop_eval()))))},
{timeout, 20, ?_assertEqual(true,
quickcheck(numtests(?TEST_ITERATIONS,
?QC_OUT(prop_policy_v1()))))}
{timeout, ?TIMEOUT,
?_assertEqual(true,
quickcheck(numtests(?TEST_ITERATIONS,
?QC_OUT(prop_ip_filter()))))},
{timeout, ?TIMEOUT,
?_assertEqual(true,
quickcheck(numtests(?TEST_ITERATIONS,
?QC_OUT(prop_secure_transport()))))},
{timeout, ?TIMEOUT,
?_assertEqual(true,
quickcheck(numtests(?TEST_ITERATIONS,
?QC_OUT(prop_eval()))))},
{timeout, ?TIMEOUT,
?_assertEqual(true,
quickcheck(numtests(?TEST_ITERATIONS,
?QC_OUT(prop_policy_v1()))))}
]}.

%% accept case of ip filtering
Expand Down Expand Up @@ -182,11 +187,14 @@ condition_pair() ->
%% {date_condition(), [{'aws:CurrentTime', binary_char_string()}]},
%% {numeric_condition(), [{'aws:EpochTime', nat()}]},
{'Bool', [{'aws:SecureTransport', bool()}]},
{ip_addr_condition(), [{'aws:SourceIp', ip_with_mask()}]}
{ip_addr_condition(), [{'aws:SourceIp', one_or_more_ip_with_mask()}]}
%% {string_condition(), [{'aws:UserAgent', binary_char_string()}]},
%% {string_condition(), [{'aws:Referer', binary_char_string()}]}
]).

one_or_more_ip_with_mask() ->
oneof([ip_with_mask(), non_empty(list(ip_with_mask()))]).

%% TODO: FIXME: add a more various form of path
path() ->
"test/*".
Expand Down
2 changes: 1 addition & 1 deletion test/riak_cs_s3_policy_test.erl
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ sample_plain_allow_policy()->
" \"s3:PutObjectAcl\""
" ],"
" \"Condition\":{"
" \"IpAddress\": { \"aws:SourceIp\":\"192.168.0.1/8\" }"
" \"IpAddress\": { \"aws:SourceIp\":[\"192.168.0.1/8\", \"192.168.0.2/17\"] }"
" },"
" \"Effect\": \"Allow\","
" \"Resource\": \"arn:aws:s3:::test\","
Expand Down

12 comments on commit 0ec4bee

@shino
Copy link
Contributor

@shino shino commented on 0ec4bee Jul 31, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@borshop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saw approval from shino
at 0ec4bee

@borshop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merging basho/riak_cs/bugfix/multiple-ip-in-policy = 0ec4bee into borshop-integration-1178-bugfix/multiple-ip-in-policy

@borshop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basho/riak_cs/bugfix/multiple-ip-in-policy = 0ec4bee merged ok, testing candidate = 25ee8a6

@borshop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shino
Copy link
Contributor

@shino shino commented on 0ec4bee Jul 31, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@borshop retry

@kuenishi
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@borshop: retry

@borshop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saw approval from shino
at 0ec4bee

@borshop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merging basho/riak_cs/bugfix/multiple-ip-in-policy = 0ec4bee into borshop-integration-1178-bugfix/multiple-ip-in-policy

@borshop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basho/riak_cs/bugfix/multiple-ip-in-policy = 0ec4bee merged ok, testing candidate = f1e5b14

@borshop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@borshop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fast-forwarding develop to borshop-integration-1178-bugfix/multiple-ip-in-policy = f1e5b14

Please sign in to comment.