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

allow flux-logger(1) and flux_log(3) to get an EPERM error, optionally #1000

Merged
merged 9 commits into from Mar 10, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Mar 10, 2017

This PR changes the return value of flux_log() and flux_vlog() from void to int, and adds a flag FLUX_LOG_CHECK (ored with "severity") which enables these functions to return an error.

If that flag is set, switch flux_log() from "fire and forget" mode to a synchronous RPC with the possibility of an error response. Issue the optional response in the broker "log.append" RPC handler. Then use the flag in flux-logger where attempting to log to an instance without appropriate permission really should be reported as an error.

Also: fix a mem leak in the flux_open() error path, and change the EPERM response logic to NOT send a response to fire and forget RPC's (based on the matchtag value).

Add a couple tests.

garlick added some commits Mar 9, 2017

libflux/handle: fix mem leak in flux_open error path
Problem: flux_open() does not call flux_handle_destroy()
on its new handle if errors occur after its creation.

Ensure that the handle is destroyed in those paths.
libflux/dispatch: don't respond if FLUX_MATCHTAG_NONE
Problem: if a request is rejected by the reactor message
dispatcher due to security policy, it receives a response
even if it is not expecting one.

Don't send a response if matchtag is FLUX_MATCHTAG_NONE.
broker/log: add optional reply to log.append RPC
Problem: with new multi-user security features, a log request
can be denied by the message dispatcher if it comes from a user
other than the instance owner.  In some case flux_log() users
may want to treat this as a fatal error, but they can't if
there is no RPC response.

If the log.append request was sent with FLUX_MATCTHAG_NONE, there is
no response as before.  Otherwise, a success/fail response is returned
to the sender.
libflux/flog: add FLUX_LOG_CHECK flag
Problem: flux_log() cannot return an error.

If the FLUX_LOG_CHECK bit is set in the severity parameter
passed to flux_log() or flux_vlog(), make the log.append RPC
with flags=0 and block until a response is received.  Otherwise,
make it with flags=FLUX_RPC_NORESPONSE and return immediately,
as before.

Change the return value of these functions from void to int.
If the FLUX_LOG_CHECK bit is set, a return of -1 with errno set
is possible.  Otherwise, the return value will always be 0.
cmd/flux-logger: call flux_log() with FLUX_LOG_CHECK
Problem: flux_logger returns success even if the user is
not allowed to log to the instance.

Add the FLUX_LOG_CHECK flag to the flux_log() call, and check
its return value.

Fixes #999
test/security: check dmesg, logger for non-owner
Ensure that "flux logger" and "flux dmesg" receive EPERM
errors when the owner role is not held.
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Mar 10, 2017

Just noticed there is no man page for flux-log(3) so I will add that.

garlick added some commits Mar 10, 2017

build: fix typos in man3 Makefile.am
Add a missing line separator and a missing secondary man page.
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Mar 10, 2017

Oh, nicely done without affecting most callers. Once travis is done is this ready for merge?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Mar 10, 2017

I think so, thanks.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 10, 2017

Coverage Status

Coverage increased (+0.02%) to 76.463% when pulling cf0a648 on garlick:log_response into fdc7099 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 10, 2017

Codecov Report

Merging #1000 into master will increase coverage by 0.02%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #1000      +/-   ##
==========================================
+ Coverage   76.16%   76.18%   +0.02%     
==========================================
  Files         153      153              
  Lines       26517    26545      +28     
==========================================
+ Hits        20197    20224      +27     
- Misses       6320     6321       +1
Impacted Files Coverage Δ
src/common/libflux/handle.c 86.3% <0%> (-0.12%)
src/common/libflux/flog.c 95.28% <100%> (+2.65%)
src/common/libflux/dispatch.c 83.15% <100%> (+0.09%)
src/cmd/flux-logger.c 60% <100%> (+1.37%)
src/broker/log.c 61.89% <50%> (-0.73%)
src/modules/wreck/job.c 71.37% <0%> (-0.06%)
src/common/libflux/message.c 83.77% <0%> (+0.24%)
src/broker/modservice.c 83.05% <0%> (+0.84%)
... and 1 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 fdc7099...cf0a648. Read the comment docs.

@grondo grondo merged commit aa5d395 into flux-framework:master Mar 10, 2017

4 checks passed

codecov/patch 80% of diff hit (target 76.16%)
Details
codecov/project 76.18% (+0.02%) compared to fdc7099
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 76.463%
Details

@garlick garlick deleted the garlick:log_response branch Mar 10, 2017

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

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.