ovis icon indicating copy to clipboard operation
ovis copied to clipboard

Default permsissions in ldmsd

Open morrone opened this issue 4 years ago • 2 comments

It would be very nice to have a way to set default permissions one time in ldmsd that are used by all metrics. @baallan suggested having environment variables to pass those values to ldmsd. That seems reasonable, and consistent with current configuration of ldmsd (the other option being command line options).

So I would propose that we create following option environment variables:

LDMSD_AUTH_UID
LDMSD_AUTH_GID
LDMSD_AUTH_PERM

I would like to see these values automatically pre-set as the default values in the ldms_set_t when ldms_set_new() is called.

There are currently two ways that plugin can set uid/gid/perm:

ldms_set_t ldms_set_new_with_auth(const char *instance_name,
                                  ldms_schema_t schema,
                                  uid_t uid, gid_t gid, mode_t perm);
int ldms_set_config_auth(ldms_set_t set, uid_t uid, gid_t gid, mode_t perm);

There is a compatibility issue between these two functions and the proposed defaults. One, two, or all three of uid, gid, and perm to be specified on a plugin's configuration line. These two functions only permit all three to be set at once. This would not allow a plugin to accept ldmsd's defaults for the values it does not want to set.

So first of all, I would recommend that we deprecate ldms_set_new_with_auth(), because the only case in which that would be useful when defaults are added is if the plugin is going to set all three of the options. I see no issue with calling "ldms_set_new()" followed by a call, or calls, to ldms_set_config_auth*() functions. Presumably the new set will not be visible to consumers until ldms_set_publish() is called, so there is no risk of the metric being temporarily exposed with incorrect permissions between ldms_set_new() and ldms_set_config_auth*().

Next, I see two main solutions for ldms_set_config_auth():

  1. Introduce an ldms_set_config_auth_get() function. This would allow the plugin to read the default values and supply those to ldms_set_config_auth() for any options it does not choose to override.
  2. Split ldms_set_config_auth() into three separate functions: ldms_set_config_auth_uid(), ldms_set_config_auth_gid(), and ldms_set_config_auth_perm().

It looks like sampler_base.c already uses ldms_set_new() and ldms_set_config_auth() separately rather than using ldms_set_new_with_auth(), and only four other samplers call lmds_set_with_new_auth(), so this is probably a pretty low impact change to implement.

morrone avatar Jul 22 '20 18:07 morrone

It seems like a reasonable extension to put this on the command line as well would be; -a perm=string where we can argue about whether string is something like 600 or the full gnu chmod perm syntax in a separate thread. I love having things NOT require env vars to be controllable.

baallan avatar Aug 18 '20 17:08 baallan

RE the C api, perhaps modify set_new_with_auth(..., uid, gid, perm) to set_new_with_auth(..., struct auth_data *d); and a helper function:

xxx_set_auth_data_set(struct auth_data *data, uid, gid, perm, int adflags)
#define AD_PERM 1
#define AD_UID 2
#define AD_GID 4

The proliferation of getter/setter is somewhat annoying.

baallan avatar Nov 06 '20 19:11 baallan