libflux/message: add message cred helpers #2670
Conversation
|
Great idea! 👍 |
|
Restarted builder that failed with |
|
Trying this: @Mergifyio rebase |
|
Command
Hey, I reacted but my real name is @Mergifyio |
|
Had mergify rebase on current master so we can see if codecov.io reports this time. |
|
Thanks for that! |
|
This looks great! If I had any comments it would be that the credential However, these comments shouldn't prevent this PR from being merged as-is. |
Despite some contrary examples in the flux-core internals in the broker, connectors and "message routers", I felt like the primary external usage would be validating requests in user-written services, so the new functions felt like they ought to be there next to the individual rolemask/userid message accessors. I think you could successfully argue the credential belongs in its own security interface also. I just didn't go there.
Yeah, me too. I could have been more aggressive in refactoring code that used the individual accessors, but I didn't want to get too invasive here. |
Problem: accessors for message creds do not protect against NULL pointer arguments. Add checks that return EINVAL when arguments are NULL, before they are dereferneced.
Problem: boiler plate for checking request message authorization involves defining rolemask and userid individually, fetching them from the request with two accessors, and then implementing logic that is identical for most services. It is simple, but there is a high consequence of error. To reduce size of boiler plate and reduce chance of mistakes: - add 'struct flux_msg_cred' to contain rolemask, userid - add flux_msg_get_cred(), flux_msg_set_cred() accessors that operate on the cred - add flux_msg_authorize() and flux_msg_cred_authorize() to authorize an action if the (message) cred userid matches a supplied one, or the message cred rolemask includes OWNER. Add unit tests.
Problem: librouter defines a struct identical to flux_msg_cred in its private API. Switch to the public struct definition in librouter and its internal users.
Also, the unit test alluded to in kill.h was missing, and was simple, so added that here.
Codecov Report
@@ Coverage Diff @@
## master #2670 +/- ##
==========================================
- Coverage 81.27% 81.19% -0.09%
==========================================
Files 247 246 -1
Lines 38263 38113 -150
==========================================
- Hits 31100 30946 -154
- Misses 7163 7167 +4
|
In the course of adding more job manager services, I found myself repeating the authorization check for 1) is the requestor the instance owner? 2) If no, is the requestor the owner of the job being manipulated? I also noted that one such instance in the job manager didn't have tests, and that not all implementations are consistent with regard to whether FLUX_ROLE_USER is required in addition to a userid match, and whether a match on FLUX_USERID_UNKNOWN is allowed.
It's a little bit verbose to make this check, and although the authorization code is simple, repeating it all over the place seems like a bad idea, so I added the following helpers in message.h:
Adding this (and its tests) was simple. Unfortunately updating all the places where it could be employed touched a lot of code. We'll see from the coverage how well tested it all is.