Implement riak security enable/disable/status #437

Merged
merged 1 commit into from Dec 24, 2013

Projects

None yet

3 participants

@Vagabond
Contributor

Implemented in terms of a cluster_metadata modulated capability.

I'm not actually sure this yields the right semantics. On the one hand it makes sure that the status of security across the cluster is consistent, but on the other it means that if you join one old (or malicious) node to the cluster, your security goes bye-bye.

@engelsanchez engelsanchez was assigned Oct 23, 2013
@engelsanchez engelsanchez commented on the diff Oct 23, 2013
src/riak_core_security.erl
@@ -366,9 +366,46 @@ add_source(User, CIDR, Source, Options) ->
add_source([User], CIDR, Source, Options).
is_enabled() ->
- %% TODO this should be some kind of capability or cluster-wide config
- app_helper:get_env(riak_core, security, false).
+ case riak_core_capability:get({riak_core, security}) of
+ true ->
+ case riak_core_metadata:get({<<"security">>, <<"status">>},
@engelsanchez
engelsanchez Oct 23, 2013 Contributor

Why not atoms instead of binaries?

@Vagabond
Vagabond Oct 23, 2013 Contributor

Because binaries are used everywhere else in security because cluster_metadata didn't use to support atoms.

No better reason.

@engelsanchez
Contributor

Ok, so we have a switch but nobody is using it to avoid the security checks, etc?

@Vagabond
Contributor

On Wed, Oct 23, 2013 at 07:07:24AM -0700, Engel A. Sanchez wrote:

Ok, so we have a switch but nobody is using it to avoid the security checks, etc?

I don't know what this means. I updated the is_available function to
read the value out of the cluster metadata...

@engelsanchez
Contributor

Oops. You mean is_enabled(). I missed that, nevermind.

@engelsanchez
Contributor

The code looks good. There is a pending discussion to follow up regarding cluster metadata and its convergence properties here #440.

We should open an issue to track what we can do to improve the situation with PB requests, which only check if security is enabled/disabled on connection creation. Connections tend to stick around for a long time, so the switch may not have a visible effect for a long time, or not on all requests.

With those caveats, I've tested the basic functionally and is as advertised.

👍

@slfritchie
Contributor

Ping? I see a +1 from @engelsanchez, is there any other reason to wait, aside from ETOOBUSY from @Vagabond?

@Vagabond
Contributor

Rebased.

@Vagabond Vagabond Implement riak security enable/disable/status
Implemented in terms of a cluster_metadata modulated capability.
23bd5a4
@Vagabond
Contributor

I'm going to merge this, including the test that nobody has been able to review because of breakage on develop.

The reasons I'm doing so are the following:

  • Both code PR have beem +1ed
  • The failing test can be addressed in the bugfixing phase

There is going to be one regression after we merge, riak_search may service requests over a secured connection if the node started up with security disabled but it was enabled at runtime (or if the cluster was in mixed mode with an older release without the security capability). The riak_test tests for this, so it will break once we fix the other issues with develop.

cc @jaredmorrow

@Vagabond Vagabond merged commit 22c8c2c into develop Dec 24, 2013
@seancribbs seancribbs deleted the adt-security-enable branch Apr 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment