Run configuration implementation security issue

[marxide]

The run configuration file is just a Python file that is loaded by importlib. When it's loaded, it's executed outside of any user context. This means that any arbitrary code in a run config file will be executed without any permission checks. Since we allow users to write and upload their own config files, this presents a serious security concern. A user could, among other things, delete an entire run that wasn't theirs, make themselves an administrator, basically anything you can do in a Django shell session.

@ajstewart and I discussed this briefly and think we should change the run configuration to use configparser. It's in the Python stdlib, is made for configuration files, and maintains a reasonable level of human readability.

The downside is that we lose the flexibility to do things like glob and sort paths for input files. This was pretty handy (I use it all the time) but thinking about it more it's probably better to provide the input files explicitly so we can be sure exactly what files were used in a run and the order in which they were provided.

[marxide]

We may want to consider using a strict YAML subset (provided by strictyaml) instead of configparser. Below are some limited examples of both (I haven't replicated the full config).

I find both options viable. Personally, I find both to be human readable and suitable for users to edit by hand through the UI. From a development standpoint, I like the YAML option because of the neat schema validation features. I welcome your thoughts!

configparser

Pros:

• in the stdlib

Cons:

• values are scalar only - no lists

Examples

Since we can't use lists, the choices for defining the input files are either: a large, comma-separated string of paths; or a series of config options with no value in their own sections. I've opted for the latter in the example below since I think the strings would become difficult to work with when large.

[Run settings]
run path = /foo/bar/pipeline-runs/run1
default survey = none
suppress astropy warnings = true

[Image files]
/foo/bar/image1.epoch1.fits
/foo/bar/image2.epoch1.fits
/foo/bar/image1.epoch2.fits
/foo/bar/image2.epoch2.fits

[Selavy files]
/foo/bar/selavy-image1.epoch1.txt
/foo/bar/selavy-image2.epoch1.txt
/foo/bar/selavy-image1.epoch2.txt
/foo/bar/selavy-image2.epoch2.txt

[Noise files]
/foo/bar/noisemap.image1.epoch1.fits
/foo/bar/noisemap.image2.epoch1.fits
/foo/bar/noisemap.image1.epoch2.fits
/foo/bar/noisemap.image2.epoch2.fits

[Background files]
/foo/bar/meanmap.image1.epoch1.fits
/foo/bar/meanmap.image2.epoch1.fits
/foo/bar/meanmap.image1.epoch2.fits
/foo/bar/meanmap.image2.epoch2.fits

[Source monitoring]
monitor = true
min sigma = 3.0
edge buffer scale = 1.2
cluster threshold = 3.0
allow nan = false

For epoch-based association, we can set the value for the input file "options" with the epoch.

[Image files]
/foo/bar/image1.epoch1.fits = 1
/foo/bar/image2.epoch1.fits = 1
/foo/bar/image1.epoch2.fits = 2
/foo/bar/image2.epoch2.fits = 2

Strict YAML

Pros:

• schema validation with feedback

Cons:

• stricter syntax compared with configparser, e.g. susceptible to bad indentation (but then so is our existing method)

Example

Run settings:
  run path: /foo/bar/pipeline-runs/run1
  default survey: none
  suppress astropy warnings: true
Input files:
  Images:
  - /foo/bar/image1.epoch1.fits
  - /foo/bar/image2.epoch1.fits
  - /foo/bar/image1.epoch2.fits
  - /foo/bar/image2.epoch2.fits
  Selavy:
  - /foo/bar/selavy-image1.epoch1.txt
  - /foo/bar/selavy-image2.epoch1.txt
  - /foo/bar/selavy-image1.epoch2.txt
  - /foo/bar/selavy-image2.epoch2.txt
  Noise:
  - /foo/bar/noiseMap.image1.epoch1.fits
  - /foo/bar/noiseMap.image2.epoch1.fits
  - /foo/bar/noiseMap.image1.epoch2.fits
  - /foo/bar/noiseMap.image2.epoch2.fits
  Background:
  - /foo/bar/meanMap.image1.epoch1.fits
  - /foo/bar/meanMap.image2.epoch1.fits
  - /foo/bar/meanMap.image1.epoch2.fits
  - /foo/bar/meanMap.image2.epoch2.fits
Source monitoring:
  monitor: true
  min sigma: 3.0
  edge buffer scale: 1.2
  cluster threshold: 3.0
  allow nan: false

Since YAML supports lists and mappings, we can activate the epoch-based association similarly to how we do it now.

Input files:
  Images:
    1:
    - /foo/bar/image1.epoch1.fits
    - /foo/bar/image2.epoch1.fits
    2:
    - /foo/bar/image1.epoch2.fits
    - /foo/bar/image2.epoch2.fits

The best part of the YAML option from my point of view is the validation. The strictyaml package allows schemas to be defined and the config files to be validated against it. It even allows multiple schema options so we can validate against regular and epoch-based association at the same time.

import strictyaml

# define the input file schemas
_standard_mode_mapping = strictyaml.UniqueSeq(strictyaml.Str())
_epoch_mode_mapping = strictyaml.MapPattern(strictyaml.Str(), strictyaml.UniqueSeq(strictyaml.Str()))
# allow either schema for the input files
_input_file_mapping = _standard_mode_mapping | _epoch_mode_mapping

schema = strictyaml.Map({
    "Run settings": strictyaml.Map({
        "run path": strictyaml.Str(),
        "default survey": strictyaml.Str(),
        "suppress astropy warnings": strictyaml.Bool(),
    }),
    "Input files": strictyaml.Map({
        "Images": _input_file_mapping,
        "Selavy": _input_file_mapping,
        "Noise": _input_file_mapping,
        "Background": _input_file_mapping,
    }),
    "Source monitoring": strictyaml.Map({
        "monitor": strictyaml.Bool(),
        "min sigma": strictyaml.Float(),
        "edge buffer scale": strictyaml.Float(),
        "cluster threshold": strictyaml.Float(),
        "allow nan": strictyaml.Bool(),
    }),
})

# config_no_run_path is the first YAML example above but with the run path removed
try:
    yaml = strictyaml.load(config_no_run_path, schema, label="run config")
except strictyaml.YAMLValidationError as e:
    print(e)

while parsing a mapping
  in "run config", line 1, column 1:
    Run settings:
     ^ (line: 1)
required key(s) 'run path' not found
  in "run config", line 3, column 1:
      suppress astropy warnings: 'true'
    ^ (line: 3)

[marxide]

I've looked over the current run config validation. The following validation steps can be removed in favor of YAML schema validation:

• No unknown config options
• Required config options present
• No duplicates in the input file lists
• Number of input files match for each type (i.e. images, selavy, noise)
• Selected source finder is a valid choice (i.e. selavy)
• Selected association method is a valid choice (i.e. basic, advanced, deruiter)
• Background files are provided if source monitoring is turned on, and the number of background files matches the other inputs

As far as I can tell, the following validation steps are not possible with schema validation and would still be required:

• Number of input files does not exceed the configured maximum
• Input files exist and are readable
• At least 2 input images are provided

[ajstewart]

I would be in support of switching to YAML from what you've said here!