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

kvs: support non-instance owners of namespaces #1323

Merged
merged 5 commits into from Feb 5, 2018

Conversation

4 participants
@chu11
Copy link
Contributor

chu11 commented Feb 1, 2018

This PR allows instance owners to create a namespace that is owned by a non-instance owner, and thus can let that non-instance owner read/write to that KVS namespace. In this implementation, only a single non-instance owner can read/write to a namespace. Instance owners are always able to read/write to any namespace under their creation. Only instance owners are only able to create/remove a namespace.

After writing the unit tests, I realized there's a lot of cleanup that could be done in both the t1004 kvs tests and t1005 kvs tests (sharing common code, writing better common functions, etc.). But I am going to let that cleanup be for another day, to limit churn in this PR.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 1, 2018

Codecov Report

Merging #1323 into master will increase coverage by 0.02%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #1323      +/-   ##
==========================================
+ Coverage    78.2%   78.22%   +0.02%     
==========================================
  Files         156      156              
  Lines       28269    28290      +21     
==========================================
+ Hits        22107    22131      +24     
+ Misses       6162     6159       -3
Impacted Files Coverage Δ
src/cmd/builtin/user.c 75.14% <ø> (ø) ⬆️
src/common/libkvs/kvs.c 100% <ø> (ø) ⬆️
src/modules/kvs/kvsroot.c 68.75% <100%> (+0.32%) ⬆️
src/cmd/flux-kvs.c 81.17% <83.33%> (+0.14%) ⬆️
src/modules/kvs/kvs.c 65.61% <90%> (+0.37%) ⬆️
src/modules/connector-local/local.c 72.95% <0%> (-1.44%) ⬇️
src/broker/modservice.c 79.61% <0%> (-0.98%) ⬇️
src/common/libutil/base64.c 95.07% <0%> (-0.71%) ⬇️
src/common/libflux/message.c 81.13% <0%> (-0.36%) ⬇️
src/broker/overlay.c 74.2% <0%> (ø) ⬆️
... and 7 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 1, 2018

Coverage Status

Coverage increased (+0.07%) to 78.584% when pulling b0b2768 on chu11:kvsrolemask into 49b2916 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 1, 2018

Cool! Couple suggestions:

Currently, adding FLUX_ROLE_OWNER to the message handler rolemask is redundant. The instance owner is like "root" and doesn't need to be explicitly allowed. So I think the first commit could be dropped.

The root_authenticate() function probably should be renamed to check_user() or something less ominous sounding from a security point of view. Also, that function should check the message rolemask for FLUX_ROLE_OWNER rather than caching the owner userid and checking that. Conceivably more than one userid could be given the OWNER role.

@chu11 chu11 force-pushed the chu11:kvsrolemask branch from c86a2c1 to b9498bf Feb 1, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Feb 1, 2018

Just re-pushed, already squashed with all suggested changes (remove redundant FLUX_ROLE_OWNER, root_authenticate() is now check_user(), and it checks the rolemask).

One thing I forgot to mention earlier. As far as I can tell, jansson does not support sending an unsigned integer type when transferring around the userid. So I use a plain integer and allowing normal casting affects to take care of the rest. I noticed this used in other parts of code as well. It's not how I'd prefer to do it, but it probably suffices for now.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 1, 2018

I presume that works, e.g. a userid of say 2^31 + 2 gets transferred on the wire as a negative number, but comes out like it went in after casting? o/w we could use jansson's json_int_t...

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Feb 2, 2018

@garlick Yeah, I initially used json_int_t, but saw that this wasn't done in src/cmd/builtin/user.c so decided to not bother. Perhaps we should convert everywhere?

On that note, shouldn't it be uid_t instead of uint23_t?

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 2, 2018

It's documented in RFC 12 as a 32-bit unsigned, for better or worse.

POSIX is apparently ambiguous on uid_t, e.g.
https://stackoverflow.com/questions/21370094/is-the-uid-t-type-signed-or-unsigned

I'd stick to the Flux RFC unless we feel we need to revisit the design.

One point: FLUX_USERID_UNKNOWN is defined as 0xFFFFFFFF, and you're sending that across the wire in this PR, so if that's working, what you have is not broken.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Feb 2, 2018

I'd stick to the Flux RFC unless we feel we need to revisit the design.

Sounds good.

One point: FLUX_USERID_UNKNOWN is defined as 0xFFFFFFFF, and you're sending that across the wire in this PR, so if that's working, what you have is not broken.

Ahh you're right.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Feb 3, 2018

@morrone if I'm reading this correctly, buildbot didn't run? I'm not quite sure how to kick it to try again.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 5, 2018

LGTM - just needs a rebase on current master.
Nice job on the test coverage.

chu11 added some commits Jan 26, 2018

modules/kvs: Add 'owner' to namespace create rpc
In addition, store namespace owner in kvsroot.
common/libkvs: Add owner to namespace create
Modify flux_kvs_namespace_create() to allow caller to specify
an alternate namespace owner.
cmd/flux-kvs: Support namespace-create owner
Allow user to specify an alternate owner of a namespace when it
is created.
modules/kvs: Add namespace message authentication
For a number of kvs callback functions, allow FLUX_ROLE_USER
messages to be received by the callback functions.  In those callbacks,
ensure that the message is coming specifically from the instance
owner or the owner of the namespace.
t/kvs: Add t1005-kvs-security.t
Add security tests to ensure namespaces operate properly for
owner/user access.

@chu11 chu11 force-pushed the chu11:kvsrolemask branch from b9498bf to b0b2768 Feb 5, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Feb 5, 2018

Just rebased

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 5, 2018

Restarted one builder that failed with #1077

FAIL: t0001-basic.t 53 - instance can stop cleanly with subscribers

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 5, 2018

Restarted again due to apt-get failure.

@garlick garlick merged commit 0f7cadb into flux-framework:master Feb 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.07%) to 78.584%
Details

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

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

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

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.