Enabling a plugin throws invalid configuration

[VakarisZ]

To Reproduce

Steps to reproduce the behavior:

1. From any other page go to configuration
2. Enable hadoop plugin
3. Submit the configuration
4. See "invalid configuration" error

Expected behavior

Should submit fine, but the plugin is not populated with defaults

[ilija-lazoroski]

Simple solution would be to remove required. In any case we have defaults for each options.

[VakarisZ]

IMO it's a bad decision to always fill in defaults. For some options it's OK, but I think sometimes we want to make sure that user configured a specific option. If I send Hadoop: {"try_all_discovered_http_ports": true} to the config API it doesn't mean I want monkey to try and exploit port 8088. Maybe I just forgot to add the ports. Consider an example where plugin can be configured to be unsafe:

ransomware/config-schema.json

{
  "properties": {
    "file_extension": {
      "title": "File extension",
      "type": "string",
      "pattern": "^(\\.[A-Za-z0-9_]+)?$",
      "description": "The file extension that the Infection Monkey will use for the encrypted file.",
      "default": ".m0nk3y"
    },
    "real_encryption": {
      "title": "Real encryption",
      "description": "This option is dangerous as it will use real encryption.",
      "type": "object",
      "properties": {
        "algorithm": {
          "title": "Algorithm",
          "type": "string",
          "description": "The algorithm that the Infection Monkey will use for the encryption.",
          "default": "AES"
        },
        "encryption_key": {
          "title": "Encryption key",
          "type": "string",
          "description": "The encryption key that the Infection Monkey will use for the encryption.",
          "default": "12345678901234567890123456789012"
        }
      }
    }
  }
}

If I send to the endpoint

{
    "file_extension": ".txt"
}

Doesn't mean I want the files to be encrypted with real encryption algorithms. In this case we DON'T want "real_encryption" to be enabled by default. But if a user enables it we want to suggest the reasonable "defaults" (I guess jsonschema would call it "recommended")

TL;DR

Having defaults is OK for majority of configuration values, but we should support "recommended + required" combo for values that are more impactful and where we want to know that user deliberately configured them.

[VakarisZ]

Since now we don't have any plugins that use required we can solve the bug by removing the required fields from the plugin configuration

[mssalvatore]

If we want to force the user to configure it, it shouldn't have a default. Default values that we choose should be sane and safe.

[VakarisZ]

Do you have any better pattern in mind than "required + recommended"? We need to agree what options we want to support. If we want to support "recommended + required" we need to add an issue for UI refactoring, because our current UI doesn't support it.

[mssalvatore]

Why would we have something that's recommended but not the default?

[VakarisZ]

We might want to suggest reasonable values even for the fields that shouldn't be enabled by default. If we think we shouldn't implicitly try to exploit 8088 just because the user enabled Hadoop, we still want to suggest 8088 as it's the default hadoop port. Right now it's largely irrelevant as we don't have plugins that can be configured to be unsafe, but we should consider whether we want to support it in the future.

[mssalvatore]

I'm still not sure I understand. If I think something is good enough to "recommend" it to you, why wouldn't I just save you the trouble of typing and make it the default?

[VakarisZ]

Because "default" means always enabled. We should be able to recommend a value for properties that are disabled by default.

[mssalvatore]

"Default" does not mean "always enabled". For example, the leave_readme option in the ransomware plugin defaults to True, which means "enabled". But we could easily switch the default to False, which would mean "disabled".

[VakarisZ]

"Default" does not mean "always enabled".

Not for an object. Take a look at the schema example above. It's impossible to remove real_encryption from the json object, because its options of algorithm and encryption_key have a default. real_encryption will always be enabled, there's no way to turn it off.