[PL-133773] Add initial ModSecurity implementation

[nichmoe]

@flyingcircusio/release-managers

PR release workflow (internal)

• PR has internal ticket
• internal issue ID (PL-…) part of branch name
• internal issue ID mentioned in PR description text
• ticket is on Platform agile board
• ensure all checks are green
• get a review from a colleague

Design notes

• Provide a feature toggle if the change might need to be adjusted/reverted quickly depending on context. Consider whether the default should be on or off. Example: rate limiting.
• Provide warnings in previous platform version and upgrade notes when introducing a breaking change --> no breaking changes

Security implications

• Security requirements defined? (WHERE)
  No specific requirements
• Security requirements tested? (EVIDENCE)
  Manual tests on dev server

This PR adds a minimal initial integration of ModSecurity. It is intended to be properly documented and supported with tests, after a bit of practical testing and further development.

See PL-133773

[osnyx]

Ambiguous naming, which config is this supposed to refer to? My intuition would be cfg, but you provide config which refers to the whole top-level NixOS system config?

[osnyx]

It is probably a better idea to simplify the option definitions into one set of options, instead of deferring to multiple submodules, and then puzzel this together in the single config block with mkIf conditionals.

[osnyx]

This config attribute name refers to the top-level config of this submodule scope. Let's see if this could be renamed or has to be this way.
The general idea here is that the rulesetImplModule generates a modsecurity config that is standalone in a way that it could be used in several places like nginx, apache, or other supporting software. This is then expose dthrough the configContent option.

According to @dpausp, this is built this way to avoid infinite recursions encoutered when building otherwise. (together with lazyAttrs in line 96).

[osnyx]

What is the reason of deferring this to an own submodule? At least now, the nginxIntegrationSubmodule does not even provide an option interface on its own, so it would be straightforward to just inline the code here.

[nichmoe]

Sensible point, will be set inline.

[osnyx]

AFAIK this is the wrong type signature, usually an option description goes here. Did this ever evaluate successfully?

[nichmoe]

It did evaluate, but it will most likely blow up, when something tries to get the description. This needs fixing.

[osnyx]

Yes, thhis is the wrong type signature, but currently only causes the description to fail to evaluate.

[osnyx]

Why does the submodule definition have its own file, do you plan to re-use it somewhere else?

[nichmoe]

Can be moved into the rulesetImplModule, or a renamed equivalent.

[osnyx]

Why does this function definition have its own file, do you plan to re-use it somewhere else?

[nichmoe]

This isn't reused but could be in the future, see ModSecurity for Apache.

[osnyx]

top-level with declarations are strongly discouraged against.
I know when have plenty of those in our code, but for new modules I prefer starting with a cleaner slate.

[osnyx]

This conflicts with the approach taken in

fc-nixos/nixos/services/nginx/default.nix

Lines 325 to 348 in 875c603

	# Inject our custom access/error logging to every vHosts' extraConfig
	options.services.nginx.virtualHosts = lib.mkOption {
	type = lib.types.attrsOf (
	lib.types.submodule (
	{ name, config, ... }:
	let
	servername = if config.serverName != null then config.serverName else name;
	in
	{
	options = {
	extraConfig = lib.mkOption {
	apply =
	orig:
	orig
	+ lib.optionalString (cfg.logPerVirtualHost) ''
	access_log /var/log/nginx/access-${servername}.log;
	error_log /var/log/nginx/error-${servername}.log;
	'';
	};
	};
	}
	)
	);
	};

.

[osnyx]

The mkBefore approach taken here is probably even preferrable to the apply approach in the nginx module, as it composes better.
Just make sure to unify both sides to take the same approach, I suggest adjusting the code in ngix/default.nix.

[osnyx]

I suggest moving this downwards to flyingcircus.services.modsecurity or, as this is currently nginx-specific, even flyingcircus.services.nginx.modsecurity.

[nichmoe]

Moving to flyingcircus.services.modsecurity is the best solutions, as ModSecurity can also be used with Apache.

[osnyx]

I am not really convinced by several architecture decisions taken here, these are a bit unusual at least.
Additionally, please provide an actual example config using this module so I can make sense of what this is used for in practice. Then we can simplify the module together.

[osnyx]

The idea here was to expose this function as a handle for deployments to re-use, enabling more composability.

[osnyx]

Okay, I think it still makes sense to expose this for re-use in a deployment, but here is my idea:

• turn this into a pure function: add the ruleset to use as a 3rd argument
• then expose this as a function, similar to like generators are exposed in the NixOS lib code

Because this is not really an option in the option sense of "it makes use of the NixOS evalModules for achieving merging of option values, checking eligible option values, and keeping state. It's just a convenience function.

Update: At a closer look, it is apparently not that simple to expose plain non-options under the same naming scope as the surrounding options. Then at least mark the option as lib.types.raw.

[nichmoe]

extraConfig needs a trailing newline.

[nichmoe]

  flyingcircus.services.nginx.virtualHosts."modsectest.test.fcio.net" = {
    forceSSL = true;
    enableACME = true;
    extraConfig = config.flyingcircus.modsecurity.rulesets."modsectest.test.fcio.net.foo.bar".mkNginxConfig "On" "##foo";
  };

Creates

modsecurity On;
modsecurity_rules_file /nix/store/mxs83kyjnsv38k91cl6q3ynppjlvrph6-modsectest.test.fcio.net.foo.bar;
modsecurity_transaction_id $request_id;
##fooaccess_log /var/log/nginx/access-modsectest.test.fcio.net.log;
error_log /var/log/nginx/error-modsectest.test.fcio.net.log;

[osnyx]

After reviewing this together, here are the suggestions for restructuring this:

• Split this into 2 main modules, similar to how security.acme defines the certificates that are then used and referenced by several other modules, like in the nginx modules
• flyingcircus.services.modsecurity module: merge the rulesetOptionsModule and rulesetImplModule. Specifying a ruleset and then rendering it into an actual modsecurity config format are tightly coupled
  • we might even be able to get rid of the submodule-level config in line 62, as this is now a module on its own, not a submodule
• flyingcircus.(?)services.nginx.virtualHosts receives an option that can then reference these rule sets and configure their particular usage within that nginx host
  • this is then also the place for helpers generating the nginx-specific config embedding

Avoid convoluting submodules unless they are really necessary. The nginxIntegrationSubmodule stops being a submodule and instead configures nginx directly.