Bug 1352047
Summary: | [cli] possibly dangerous treatment of environment variables when running a subprocess | ||
---|---|---|---|
Product: | Red Hat Enterprise Linux 7 | Reporter: | Jan Pokorný [poki] <jpokorny> |
Component: | pcs | Assignee: | Tomas Jelinek <tojeline> |
Status: | CLOSED WONTFIX | QA Contact: | cluster-qe <cluster-qe> |
Severity: | unspecified | Docs Contact: | |
Priority: | medium | ||
Version: | 7.1 | CC: | cfeist, cluster-maint, idevat, kgaillot, omular, tojeline |
Target Milestone: | rc | ||
Target Release: | --- | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | If docs needed, set a value | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2020-12-15 07:42:44 UTC | Type: | Bug |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
We also need to document how pcs handles these environment variables in pcs man page. We definitely want to propagate CIB_user as we use it to relay login of user who requested an action from web UI down to pacemaker. We do not want to propagate CIB_file, because this is handled by pcs based on -f option. Can you provide a list of other environment variables which pcs should and should not pass to pacemaker? I tried running cibadmin and crm_mon with various PCMK_* debugging options with no effect. Does it mean these only apply at pacemaker daemons startup and therefore pcs does not need to care about them? Thanks Admittedly, most PCMK_* variables mentioned in /etc/sysconfig/pacemaker are only applicable to the actual daemons. I'll compile the list of which variables are relevant in the context of CLI utilities (keeping needinfo). (In reply to Jan Pokorný from comment #4) > Admittedly, most PCMK_* variables mentioned in /etc/sysconfig/pacemaker > are only applicable to the actual daemons. > > I'll compile the list of which variables are relevant in the context > of CLI utilities (keeping needinfo). Jan, do you have any updates on this? Thanks. (In reply to Jan Pokorný from comment #4) > Admittedly, most PCMK_* variables mentioned in /etc/sysconfig/pacemaker > are only applicable to the actual daemons. > > I'll compile the list of which variables are relevant in the context > of CLI utilities (keeping needinfo). Jan, do you have any updates on this? Thanks. (In reply to Jan Pokorný from comment #4) > Admittedly, most PCMK_* variables mentioned in /etc/sysconfig/pacemaker > are only applicable to the actual daemons. > > I'll compile the list of which variables are relevant in the context > of CLI utilities (keeping needinfo). Jan, do you have any updates on this? Thanks. (In reply to Tomas Jelinek from comment #3) > We definitely want to propagate CIB_user as we use it to relay login of user > who requested an action from web UI down to pacemaker. > We do not want to propagate CIB_file, because this is handled by pcs based > on -f option. > > Can you provide a list of other environment variables which pcs should and > should not pass to pacemaker? I tried running cibadmin and crm_mon with > various PCMK_* debugging options with no effect. Does it mean these only > apply at pacemaker daemons startup and therefore pcs does not need to care > about them? > > Thanks (In reply to Jan Pokorný from comment #4) > Admittedly, most PCMK_* variables mentioned in /etc/sysconfig/pacemaker > are only applicable to the actual daemons. > > I'll compile the list of which variables are relevant in the context > of CLI utilities (keeping needinfo). Jan is no longer available. Ken, can you provide a list of those variables? I'm not sure what the goal is here -- how did the variables get into the environment to begin with? If the user (or script) sourced /etc/sysconfig/pacemaker, then they are the same values the daemons will use, so there should be no harm done. If the user explicitly changed some value, do we want to deny that? Or maybe print a warning and require --force? New Pacemaker versions sometime adds new variables, so keeping this list up to date might be a challenge. If we do want to filter the user's environment, it's worth considering whether we want a deny list (strip such-and-such from the environment) or an allow list (strip all PCMK_*, CIB_*, and SBD_* except such-and-such). I wouldn't strip any OCF_* variables in case pcs is run from a resource agent. (Pacemaker accepts HA_* instead of PCMK_* for a few options for backward compatibility with heartbeat, but that will eventually go away, and none of them matter for this purpose anyway.) PCMK_schema_directory (which has the RNG files used to validate the CIB schema) is the only option from /etc/sysconfig/pacemaker that should matter. If someone sets this, I'm guessing they installed from source in a nonstandard location, and they need to set it to whatever the daemons are using. Or they're experimenting with changing the schema for some reason. The CIB_* variables CIB_user, CIB_password, CIB_server, CIB_port, and CIB_encrypted allow a user to modify the CIB from a non-cluster machine, as documented in "Pacemaker Administration": https://clusterlabs.org/pacemaker/doc/en-US/Pacemaker/2.0/html-single/Pacemaker_Administration/#s-remote-connection If you don't want to allow pcs to run on non-cluster machines, it would make sense to filter those (if not set by pcs). Similarly if you only want pcs -f to modify CIB_file, you could otherwise filter that. The crm_shadow command sets the CIB_shadow variable behind the scenes: https://clusterlabs.org/pacemaker/doc/en-US/Pacemaker/2.0/html-single/Pacemaker_Administration/#s-crm_shadow If it's set, then someone is likely in the middle of a crm_shadow session. That makes other command-line tools modify a temporary copy of the CIB. You could either allow that, or require --force (especially if a pcs command involves anything other than querying or modifying the CIB). This is the current situation in pcs: Legacy function to run external processes passes the whole environment to them. The only exceptions are: If -f is used, pcs sets CIB_file to the value of -f option. Pcs always sets LC_ALL = "C" so that we don't have to implement parsers for various locales and deal with ASCII vs UTF issues. New architecture uses different approach: External processes are run with an empty environment. Only LC_ALL, CIB_user and CIB_file is set - details in comment 3. Pcsd, being a daemon, has its own environment, so it cannot pass anything from shell environment in the way legacy pcs function does. Because of that, this bz and our long term plan to make CLI fully client-server as well we went with an empty environment approach in the new architecture. The original intention of this bz was to filter environment in the legacy function. Jan was to provide more details and an overview of the variables so that we could work on that, but he never did. After evaluating this issue, there are no plans to address it further or fix it in an upcoming release. Therefore, it is being closed. If plans change such that this issue will be fixed in an upcoming release, then the bug can be reopened. |
pcs should be careful about the variables from the environment (at the moment of running pcs command) as they may influence the spawned subprocesses (pacemaker CLI utilities call-outs in most of the cases) substantially. Idea: - by default, do not propagate CIB_* and PCMK_* variables and warn about their presence - establish an expert-mode switch that may propagate all variables without a change and even overwrite those established by pcs internally (it's an expert mode, after all) Example: - PCMK_schema_directory - tells where the pacemaker will search for the schemas to approve the validation; it would be really bad if the validation process would be subverted artificially only to hit the running cluster badly later on from pcs.utils:run: > 817 def run( > 818 args, ignore_stderr=False, string_for_stdin=None, env_extend=None, > 819 binary_output=False > 820 ): > 821 if not env_extend: > 822 env_extend = dict() > 823 env_var = env_extend > 824 env_var.update(dict(os.environ)) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^