Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add userid, rolemask per RFC 3, 12 #980

Merged
merged 29 commits into from Mar 9, 2017

Conversation

4 participants
@garlick
Copy link
Member

garlick commented Feb 16, 2017

This PR implements the userid, rolemask message fields from RFC 3 and 12. These fields are set by connectors on message ingress, and checked by message handlers.

Basic getter/setters for the message fields:

/* Get/set userid
 */
enum {
    FLUX_USERID_UNKNOWN = 0xFFFFFFFF
};
int flux_msg_set_userid (flux_msg_t *msg, uint32_t userid);
int flux_msg_get_userid (const flux_msg_t *msg, uint32_t *userid);

/* Get/set rolemask
 */
enum {
    FLUX_ROLE_ANY = 0,
    FLUX_ROLE_OWNER = 1,
};
int flux_msg_set_rolemask (flux_msg_t *msg, uint32_t rolemask);
int flux_msg_get_rolemask (const flux_msg_t *msg, uint32_t *rolemask);

Logic is added to flux_msg_handler_t to automatically drop (or in the case of request, reply with EPERM) messages that don't match a rolemask embedded in the watcher.

/* By default, only messages from FLUX_ROLE_OWNER are delivered to handler.
 * Use allow() to allow more roles (use FLUX_ROLE_ANY to disable the role check).
 * Use deny() to deny roles (FLUX_ROLE_OWNER cannot be denied).
 */
void flux_msg_handler_allow_rolemask (flux_msg_handler_t *w, uint32_t rolemask);
void flux_msg_handler_deny_rolemask (flux_msg_handler_t *w, uint32_t rolemask);

Tests are coming soon and thus this isn't ready to merge, but I wanted to get a checkpoint out here for review if anyone is so inclined.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 16, 2017

Codecov Report

Merging #980 into master will increase coverage by 0.01%.
The diff coverage is 76.31%.

@@            Coverage Diff             @@
##           master     #980      +/-   ##
==========================================
+ Coverage   76.15%   76.17%   +0.01%     
==========================================
  Files         151      153       +2     
  Lines       25993    26517     +524     
==========================================
+ Hits        19795    20199     +404     
- Misses       6198     6318     +120
Impacted Files Coverage Δ
src/common/libflux/response.c 84.16% <100%> (+0.26%)
src/broker/attr.c 84.54% <100%> (+0.22%)
src/common/libflux/dispatch.c 83.06% <100%> (+0.34%)
src/broker/module.c 81.62% <100%> (+2.8%)
src/cmd/flux-ping.c 87.21% <100%> (+0.39%)
src/common/libflux/handle.c 86.94% <100%> (-0.06%)
src/modules/connector-local/local.c 67.01% <66.66%> (-0.28%)
src/modules/userdb/userdb.c 68.07% <68.07%> (ø)
src/broker/ping.c 66.03% <72.72%> (+11.49%)
src/cmd/builtin/user.c 74.69% <74.69%> (ø)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adce530...315cbf7. Read the comment docs.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 16, 2017

Coverage Status

Coverage increased (+0.004%) to 76.186% when pulling c10dde7 on garlick:rolemask into edc914e on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Feb 16, 2017

Self-review comment:

Special casing FLUX_ROLE_ANY (0) is somewhat confusing. The idea was to use that as an argument to "allow" and then a message with no roles could be handled.

Maybe we could assume that on entry to the system (via connector) at least one role is assigned. Then FLUX_ROLE_ALL (0xFFFFFFFF) could be used instead, and FLUX_ROLE_NONE (0) could be defined as the default message rolemask (before connector auth).

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Feb 16, 2017

Maybe we could assume that on entry to the system (via connector) at least one role is assigned. Then FLUX_ROLE_ALL (0xFFFFFFFF) could be used instead, and FLUX_ROLE_NONE (0) could be defined as the default message rolemask (before connector auth).

That seems much more usable to me.

This does seem to prevent any kind of anonymous "nobody" user type access to Flux instance, unless either services could accept messages with FLUX_ROLE_NONE, or there was a fallback role that could be set in connectors like FLUX_ROLE_NOBODY or something. I'm not sure at this point if the anonymous access would be necessary...

@garlick garlick force-pushed the garlick:rolemask branch from c10dde7 to c2c11be Feb 16, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 16, 2017

Coverage Status

Coverage decreased (-0.0006%) to 76.204% when pulling c2c11be on garlick:rolemask into 0889e59 on flux-framework:master.

@garlick garlick force-pushed the garlick:rolemask branch from c2c11be to e9b5011 Feb 17, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 17, 2017

Coverage Status

Coverage decreased (-0.1%) to 76.103% when pulling e9b5011 on garlick:rolemask into b877c17 on flux-framework:master.

@garlick garlick force-pushed the garlick:rolemask branch from e9b5011 to b30e7a3 Feb 17, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 17, 2017

Coverage Status

Coverage decreased (-0.1%) to 76.085% when pulling b30e7a3 on garlick:rolemask into 9074ac6 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 18, 2017

Coverage Status

Coverage decreased (-0.7%) to 75.517% when pulling 4e760fc on garlick:rolemask into 9074ac6 on flux-framework:master.

@garlick garlick force-pushed the garlick:rolemask branch from 4e760fc to e47dd91 Feb 22, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 22, 2017

Coverage Status

Coverage decreased (-0.7%) to 75.478% when pulling e47dd91 on garlick:rolemask into d42f2ca on flux-framework:master.

@garlick garlick force-pushed the garlick:rolemask branch from e47dd91 to 4c06749 Feb 22, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 22, 2017

Coverage Status

Coverage decreased (-0.8%) to 75.467% when pulling 4c06749 on garlick:rolemask into d42f2ca on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 23, 2017

Coverage Status

Coverage decreased (-0.8%) to 75.459% when pulling 1b2fdf1 on garlick:rolemask into d42f2ca on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 27, 2017

Coverage Status

Coverage decreased (-0.8%) to 75.377% when pulling 6ab1ea4 on garlick:rolemask into 1a1ab10 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 27, 2017

Coverage Status

Coverage decreased (-0.8%) to 75.389% when pulling 894f50f on garlick:rolemask into 1a1ab10 on flux-framework:master.

@garlick garlick force-pushed the garlick:rolemask branch from 894f50f to 5f9b9c0 Feb 27, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 27, 2017

Coverage Status

Coverage decreased (-0.8%) to 75.378% when pulling 5f9b9c0 on garlick:rolemask into 1a1ab10 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 5, 2017

Coverage Status

Coverage decreased (-0.7%) to 75.664% when pulling 68fef41 on garlick:rolemask into 2094ede on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 6, 2017

Coverage Status

Coverage decreased (-0.1%) to 76.258% when pulling bf91137 on garlick:rolemask into 2094ede on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 6, 2017

Coverage Status

Coverage decreased (-0.07%) to 76.333% when pulling e13c9a0 on garlick:rolemask into 2094ede on flux-framework:master.

@garlick garlick force-pushed the garlick:rolemask branch from e13c9a0 to 0184b69 Mar 6, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 6, 2017

Coverage Status

Coverage decreased (-0.04%) to 76.366% when pulling 0184b69 on garlick:rolemask into 2094ede on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Mar 8, 2017

Yeah, I think the command line for flux user could definitely be improved (and documented), including mapping usernames as input. I'll have a quick go at that.

Right now I just wanted to get the simplest possible userdb in there as a placeholder. A future step would be to document the protocol making it easy to craft a drop in replacement.

The "getnext" request is a little iffy I think. Maybe it would be better to implement a "dump" request that retrieves all entries in one go. Even at a large site with say 100K users at about 32 bytes per entry, that would only be around a 3MB message. Later maybe we could add a general RPC mechanism for transferring large payloads with multiple messages.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Mar 8, 2017

Later maybe we could add a general mechanism for transferring large payloads with multiple messages.

Yeah, sorry I just meant that as a general comment not any kind of action item for this (or any near term) PR

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 9, 2017

Coverage Status

Coverage increased (+0.07%) to 76.5% when pulling 425f7e8 on garlick:rolemask into adce530 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Mar 9, 2017

Looks good to me now.

Maybe one minor nit. The prefix on 857156b was maybe supposed to be cmd/flux-user: not modules/userdb:?

Otherwise, ready for merge?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Mar 9, 2017

I'll fix that and also squash down the incremental change if you're good with the result.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Mar 9, 2017

Yeah, looks good!

garlick added some commits Feb 17, 2017

modules/userdb: add simple userdb module
The purpose of the userdb is to map userid's to a rolemask
for connectors, and to give the instance owner flexibility
in assigning roles to users.

When a user tries to establish a connection to the broker via
a connector, the connector authenticates the user and looks
up the user's rolemask in the userdb.  If a user is not in the
userdb, or has a rolemask of FLUX_ROLE_NONE, connection is
denied.  Otherwise, connection is allowed and the rolemask
and userid are stamped on messages sent from that connection.
These become input to multi-user security policy in each service.

This simple implementation is designed to be run only
on rank 0, since it has no mechanism to synchronize its
in-memory state on other ranks.  A future version might
use the KVS for shared state.

One entry for the instance owner is added when the module
is loaded.  This user has the FLUX_ROLE_OWNER role.
The instance owner is determined by calling geteuid().

If the --default-rolemask option is set at module load time,
users are automatically added to the userdb when they are
looked up, and are assigned the roles listed in the option's
arguments.  For example to give users a default role
of FLUX_ROLE_USER:

  flux module load userdb --default-rolemask=user

Methods are available to support a command line tool that can
manage the roles assigned to individual users after the userdb
module has loaded (e.g. in rc1 or later, interactively).
modules/connector-local: lookup users
After authenticating a user on the UNIX domain socket using
SO_PEERCRED, look the user up in the userdb to find what
roles have been assigned, if any.  If the userdb cannot find
the user, or the user is assigned no roles, drop the connection.

Since the local connector has to be functional before modules
such as the userdb module are loaded by rc1, the userdb is
bypassed if the authenticating userid matches geteuid().
In this case, FLUX_ROLE_OWNER is assigned without a userdb lookup.
libflux/response: reset userid/rolemask on reply
When deriving a response message from the request,
clear the userid and rolemask so they will be set
by the connnector.

If this is not done, then on connections authenticated
as the instance owner, responses may be tagged with
the sender's userid/rolemask, which may be rejected
by the reactor's message dispatch engine.
broker/attr: allow any role to get and list broker attrs
Allow all broker attrs to be fetched (but not set) by any user.
Only the instance owner can set broker attrs.

This is mainly to allow size and rank to be fetched.
In the future the security policy could be extended
to allow each attribute to have mode bits if we want.
modules/connector-local: add debug to force userdb
If DEBUG_USERDB_ONESHOT debug flag is set, look up user
in userdb on next connection attempt, even if it is the
instance owner.
test/security: add userdb tests
Ensure that the "flux user" command line tool works
and the local connector uses the userdb appropriately.

Simulate access to various services with different rolemasks
and observe the effects.  These tests use the local connector
and the FLUX_HANDLE_USERID and FLUX_HANDLE_ROLEMASK hooks for
setting userid, rolemask message fields to arbitrary values,
while authenticated as the instance owner.

check userdb --default-rolemask option

test flux user by name
libflux/handle: set fake rolemask, uid
If the environment variables FLUX_HANDLE_ROLEMASK or
FLUX_HANDLE_USERID are set, set connector-specific options
to set rolemask and/or userid in all messages sent via this
handle.

This can be used in to validate service security policy in
a connector-independent manner.
broker: add userid, rolemask to ping response
Consolidate "ping handlers" in broker and modservice, then
add userid and rolemask JSON fields in the response so that
we can see what userid and rolemask arrived in the ping
request.

Update flux-ping to parse the new JSON fields, and
add --userid option to display them in the ping output.
Update flux-ping(1) manual page for new option.
broker/module: remove dead code
module_stop_all() is dead code.
Remove it to boost code coverage.

@garlick garlick force-pushed the garlick:rolemask branch from 425f7e8 to 315cbf7 Mar 9, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 9, 2017

Coverage Status

Coverage increased (+0.02%) to 76.452% when pulling 315cbf7 on garlick:rolemask into adce530 on flux-framework:master.

@grondo grondo merged commit fdc7099 into flux-framework:master Mar 9, 2017

4 checks passed

codecov/patch 76.31% of diff hit (target 76.15%)
Details
codecov/project 76.17% (+0.01%) compared to adce530
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 76.452%
Details

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

@garlick garlick deleted the garlick:rolemask branch Sep 6, 2017

@garlick garlick added this to To do in multi-user parallel jobs via automation Feb 13, 2018

@garlick garlick self-assigned this Feb 13, 2018

@garlick garlick moved this from To do to Done in multi-user parallel jobs Feb 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.