mon: add new profiles & audit cap checks #2560
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jecluis
referenced
this pull request
Sep 23, 2014
Adds three new profiles: read-only: able to issue all read-only (MON_CAP_R) commands. Any command that may take additional caps (MON_CAP_W or MON_CAP_X) won't be allowed. read-write: able to issue all read-write (MON_CAP_R | MON_CAP_W) commands. Commands that require MON_CAP_X will not be allowed. role-definer: solely able to issue commands on the 'auth' subsystem, to which all caps are given (MON_CAP_R | MON_CAP_W | MON_CAP_X). Signed-off-by: Joao Eduardo Luis <joao.luis@inktank.com>
jecluis
changed the title
The monitor doesn't really know how to validate caps not meant for it. The MDS or the OSD may very well allow blank caps for instance, while the monitor categorically does not. We can't simply state a capability is invalid because we wouldn't take it as such. On the other hand, we must check monitor caps and make sure they are correct, otherwise malformed caps can go unnoticed for a while, sometimes even being hard to understand what may have gone wrong. Backport: firefly Signed-off-by: Joao Eduardo Luis <joao.luis@inktank.com>
We were checking the command's permissions against what we perceived as the 'module' from parsing the command's "prefix" (specified by the client). This caused troubles with cap checks for commands without a submodule clearly defined, such as 'status' or 'health' (vs 'mon dump' or 'osd pool set', which are of submodule 'mon' and 'osd' respectively). As such, we now grab the command's submodule (right now solely for caps checks) from the monitor's internal representation of the commands (defined in mon/MonCommands.h and built at compile time and stashed in 'mon_commands'). Given that commands such as 'health', 'fsid' or 'status' have properly defined modules in MonCommands.h, we simply rely on that representation for all commands. Which is what we should have been doing from the start anyway, because we shouldn't be relying on the client to point us to what we want to authenticate against. Backport: firefly Signed-off-by: Joao Eduardo Luis <joao@redhat.com>
Adds three new profiles: read-only: able to issue all read-only (MON_CAP_R) commands. Any command that may take additional caps (MON_CAP_W or MON_CAP_X) won't be allowed. read-write: able to issue all read-write (MON_CAP_R | MON_CAP_W) commands. Commands that require MON_CAP_X will not be allowed. role-definer: solely able to issue commands on the 'auth' subsystem, to which all caps are given (MON_CAP_R | MON_CAP_W | MON_CAP_X). Fixes: #8899 Signed-off-by: Joao Eduardo Luis <joao.luis@inktank.com>
Signed-off-by: Joao Eduardo Luis <joao.luis@inktank.com>
Signed-off-by: Joao Eduardo Luis <joao.luis@inktank.com>
Instead of a single switch(), have multiple switch() and order them by increasing necessity of privileges. This patch thus divides the big switch into: - messages not requiring auth/caps checks at all - messages which caps shall be checked somewhere else - messages the Monitor class needs to deal with but only require a client to have enough caps for the monitor to consider handling them - messages that only a monitor is allowed to send. Backport: firefly Signed-off-by: Joao Eduardo Luis <joao.luis@inktank.com>
Only dispatch messages that a client may send if said client has at least MON_CAP_R, and only dispatch internal monitor messages if peer is a monitor. Backport: firefly Signed-off-by: Joao Eduardo Luis <joao.luis@inktank.com>
Backport: firefly Signed-off-by: Joao Eduardo Luis <joao.luis@inktank.com>
Signed-off-by: Joao Eduardo Luis <joao@redhat.com>
Signed-off-by: Joao Eduardo Luis <joao@redhat.com>
We have variables with the same name that are being shared! We don't hit any issues with it currently because the code just kind of works even though that happens. Add a bit of new logic that relies on an immutable return code (for instance) and we're in the woods. Signed-off-by: Joao Eduardo Luis <joao@redhat.com>
Eye-candy. We changed indentation of a few other entries and this one was just too darn obvious, itching all over, night terrors, the whole nine yards. Signed-off-by: Joao Eduardo Luis <joao@redhat.com>
test creating and entity with blank caps with and without '--force' being specified. without '--force' they must fail with EINVAL as the monitor will not be able to parse them. Signed-off-by: Joao Eduardo Luis <joao@redhat.com>
If a given client doesn't have the required caps when running a command, it must receive an EACCES or EPERM reply. This is already handled by Monitor::handle_command(), which does an exceptionally good job at it. Therefore, and unlike other messages that do not expect return values, we can't simply drop the message if the client doesn't have the appropriate capabilities, or things can get very weird very fast from the user's perspective. Dropping the message for a command without a reply has roughly the same effect as loss of quorum (timeout, pipes failing) and confusion may ensue from it. Signed-off-by: Joao Eduardo Luis <joao@redhat.com>
ghost
added
|
this branch had two clean runs of monthrash suite. |
liewegas
added a commit
that referenced
this pull request
Oct 7, 2014
mon: add new profiles & audit cap checks Reviewed-by: Sage Weil <sage@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We don't properly validate caps for some monitor-specific messages, meaning
that we would allow handle some messages that only a monitor should send
regardless of the sender. A client that would be able to authenticate with the
cluster would then be able to send, say, a MMonJoin message, and the monitors
would take it as is. Crafting specific messages, a malicious user could take
advantage of this oversight to, for instance, break quorum.
We also add a test that will attempt to send malicious messages; we don't cover
all the monitor messages but a few that should act as an indicator in case of
regressions.
This branch also contains 3 patches that can be found on a different pull
request (#2559) [EDIT: closed said pull request,
added a few patches, this pull request will do fine]
The branch still needs to undergo a few monthrash suites.