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

idea for safe config parser for config repos #3542

Open
rptaylor opened this issue Mar 15, 2024 · 4 comments
Open

idea for safe config parser for config repos #3542

rptaylor opened this issue Mar 15, 2024 · 4 comments

Comments

@rptaylor
Copy link
Contributor

rptaylor commented Mar 15, 2024

Following our recent discussion here are some notes on a proposed idea, to be further considered/discussed.

As more organizations and groups continue to adopt CVMFS (including ones who have apparently been doing it for years yet we have apparently never heard about them), the number of scenarios that call for use of a config repo is going to increase, exacerbating the ~ N^2 problem of coordinating and managing the contents of N config repos with each other, and posing a barrier to interoperability of repositories for users, who would generally be limited to accessing a subset of repositories on any given client.
Addressing the significant security implications of using a config repo, aside from being a big security improvement in its own right, would open a way to provide a low-risk global config repo that would really be feasible to use for all repos and clients in a truly unified way.

Rough outline of a possible approach:

  • new config option like CVMFS_SAFE_PARSER=yes to activate a new parser mode
  • For compatibility (so as not to underestimate the habits of a generation of sysadmins) the new config format is still of the same format name="value" , except no other syntax (e.g. commands and bash statements) is permissible
  • There needs to be some annotation on config files in a config repo to indicate whether they are of the new or old type, to allow a config repo to be used by both old and new parsers
    • It seems to me that local config files in /etc/cvmfs do not have this requirement, since a given client setup can be configured to use the old or new way, but does not need to support both simultaneously. In any case local config files in /etc/cvmfs do not suffer from the use of bash-isms AFAIK
    • The only sort of annotation I can think of to differentiate old vs new config files in a config repo is via a different suffix, e.g. atlas.cern.ch.conf (old) vs atlas.cern.ch.cfg (new)
  • With the new parser/config, the CVMFS_CLIENT_PROFILE parameter is interpreted in a new way, to select profile-specific configurations that are applied via profile-specific config files. For example, atlas.cern.ch.cfg would constitute the base config that is always applied, while atlas.cern.ch.cfg.single, atlas.cern.ch.cfg.grid, atlas.cern.ch.cfg.whatever etc. would then also be applied on top for the case of the single, grid, whatever, etc. profile names.

In this way, any arbitrary parameters can be set based on creation/selection of any profile, without relying on if statements.
It can be done in a backwards compatible way to allow time to move to the new safe parser without disruptions. "Clean" configuration that does not rely on bash-isms can easily support both old and new by just symlinking the new config file name to the old one.

@DrDaveD
Copy link
Contributor

DrDaveD commented Mar 15, 2024

What is it about if statements or other bashisms that lead you to the conclusion that they're insecure? Isn't the basic problem that the configuration files are run as root? Why not just run it as a nobody user that has no capability of writing anything? I can also think of some cases where that will also cause difficulties doing what's needed (especially with authenticated repositories) but it least is a less drastic reduction in configuration power.

@rptaylor
Copy link
Contributor Author

If there was an easy way to drop privileges before executing the config files/scripts I assumed it would have been done already, given the very significant security implications.
What I understood from previous discussions IIRC is that it was not possible because autofs invokes the cvmfs2 executable as root, and one of the first things it has to do is set up the configuration, and filesystems can only be mounted by root so any drop of privileges would have to come after that.

But if there is an easy fix available it would be a major improvement to do that ASAP.

@DrDaveD
Copy link
Contributor

DrDaveD commented Mar 18, 2024

The bash command is run in a subprocess so I don't see why privileges couldn't be dropped in the forked process before execing bash. As far as I recall there were just worries that some existing configuration scripts might be depending on enhanced privileges.

I don't know that there is any such requirement on enhanced privileges in the current default, egi, or osg configuration repositories. For authenticated repositories I know that the authentication helper needs to run as root because it needs to be able to read environment variables from any user's process. That's a separate step from the configuration process, but the configuration process specifies the path to it which is a script in the configuration repo, so from a security standpoint it is just as dangerous. If we are trying to prevent configuration repositories from using any privileges, that would need to be done in a different way. Mostly what the script does now is set environment variables for an executable that is installed by rpm. Conceivably those environment variables could come from a separate unprivileged bash script and those variables then passed by privileged code to the authentication helper, but currently it expects to execute libraries (via LD_LIBRARY_PATH) from cvmfs so that's another potential attack surface and there would need to be another solution for that too, and something done to block variables that could alter the code executed by the helper.

@vvolkl
Copy link
Contributor

vvolkl commented Mar 18, 2024

Jakob pointed out that we already have a safe parser (although that has to be enabled directly in the mount options with -o simple_options_parsing).

I also don't think that reduced privileges for options parsing is impossible, I can take a look at implementing this (although not immediately).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants