doc: add message security policy functions to man page #1285
Conversation
garlick
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1285 +/- ##
==========================================
- Coverage 78.02% 77.94% -0.09%
==========================================
Files 154 154
Lines 29104 29103 -1
==========================================
- Hits 22709 22683 -26
- Misses 6395 6420 +25
|
|
I would like to suggest a different approach. I am not a fan of using "0" as a default value when another more clear option is possible. Zeros makes the code harder to read. Every time a zero is encountered as an option in a function call, anyone that hasn't already memorized the function prototype won't know what that zero means. The reader needs to jump out of the code they are reading and look up the prototype, and maybe even further documentation, and sometimes even just read the implementation of the function. When the parameter is a mask, there is an easy way to avoid seeing zeros in the calls to the function. Usually the bits of the mask are either defined in an enum or through macros. There can then always be a name that can be specified rather than using zero as the default. zero should only be zero when the mask is litterally meant to be "match nothing". And even then, it would be better to use something like "FLUX_ROLE_NONE" rather than using zero. When a reader sees a function call and one of the options is "FLUX_ROLE_NONE", the parameter's purpose is immediately obvious, whereas a zero would require a prototype lookup to understand its purpose. I would recommend that we change the interfaces to always take FLUX_ROLE_USER, since that is always, in fact, required. If the caller doesn't include FLUX_ROLE_USER in their mask, that should simply be an error. This solves the problem of ambiguous zeros in the code, and also converts the FLUX_ROLE_USER behavior from being implicit to being explicit. Explicit always makes for more understandable code. |
|
I'm not really buying that 0 is wrong for a bitmask value. Example: symbolic values available for chmod(2). I suppose it's nice if function calls have self-describing parameters, but the opposite is probably the norm.
Assuming you mean FLUX_ROLE_OWNER? It seems like a "bring me a rock" API design - requiring the user to come up with something on every call that we already know. |
|
(yes I mean FLUX_ROLE_OWNER) You're still making them bring a rock: a zero. Which is literally rock shaped. :) I'm not clear on why a zero rock is better for readability and maintainability of the code than a descriptive rock. |
|
Heh, good one. Well if it's a rock, at least I"m proposing it look like one. I don't necessarily have a problem with what you're proposing, although I don't have a problem with what I've already worked out either. Since what you're proposing involves touching every advec call, changing the allow/deny functions to return an error instead of void, and patching sched, in addition to rewriting the man page verbage, it feels (to me) like a fair bit of work for little gain, and not high on my list of priorities. How about we merge this as is since it's an incremental improvement over what we have now (which is no documentation). Then if you want to propose your improvements in another set of PR's, you can do that. |
| which add or remove roles from the message handler rolemask. The | ||
| FLUX_ROLE_OWNER bit position is ignored in the message handler rolemask. | ||
|
|
||
| The following roles are defined in RFC 12: |
There was a problem hiding this comment.
The public interface in the code also defines FLUX_ROLE_NONE and FLUX_ROLE_ALL. I think those need to be documented as well.
| @@ -30,6 +32,12 @@ SYNOPSIS | |||
|
|
|||
| void flux_msg_handler_stop (flux_msg_handler_t *mh); | |||
|
|
|||
| void flux_msg_handler_allow_rolemask (flux_msg_handler_t *w, | |||
There was a problem hiding this comment.
Both of these functions really need a clear explanation of how they work. It is a little unual, I think, to have a function that or's the supplied mask rather than just setting the mask, and another that and's the inverse of the supplied mask. I think simple "set" and "get" mask functions would have been better. But since this is what we have, we should document what they do.
garlick
force-pushed
the
rolemask_doc
branch
from
42677b4 to
b02bc70
Compare
|
Just rebased on current master and added a commit to address your suggestions for the man page.
Hmm, that was already there, so not sure what you were looking at? I haven't changed that commit other than rebasing it just now. |
Problem: message handler filtering based on rolemasks is not discussed in flux_msg_handler_create(3). Add man page stubs for flux_msg_handler_allow_rolemask() and flux_msg_handler_deny_rolemask(), a SECURITY section, and a reference to RFC 12. Fixes flux-framework#1260
Add note about the 'rolemask' member of struct flux_msg_handler_spec, referring back to the SECTION section in flux_msg_handler_create(3).
Add FLUX_ROLEMASK_NONE and FLUX_ROLEMASK_ALL and make sure to be clear that allow/deny modify, not overwrite, the existing rolemask.
garlick
force-pushed
the
rolemask_doc
branch
from
b02bc70 to
d00050a
Compare
|
This seems to be going nowhere so I'll withdraw the PR. |
This was peeled off of #1277
Add missing man page coverage for
flux_msg_handler_allow_rolemask()anddeny_rolemask()with brief overview of message handler security and ref to RFC 12.Restructure the rolemask filtering code to make it clear to casual reader that presence of the FLUX_ROLEMASK_OWNER bit in a message rolemask is a special case, rather than a role that can be controlled like any other. The code change affects readability but not function.