Suricata

This PR might be enough? It does require the yaml to use the env vars though: https://github.com/inliniac/suricata/pull/321

E.g. https://github.com/inliniac/suricata/pull/321/files#L3R805

Using env variables is bad as it can lead to situations where you forget that you have set an env var and you go around trying to figure out why something is not working. The reason why the env var feature of logging api was supposed to be for dev/debug purposes only.

Command line in this regard is better.

Perhaps something in the middle would be better? I like environment variable expansion in config's but can see Anoop's point. And I'm not too keen on overrides as its hard to do consistently throughout the config file, for instance where sequences are used.

Instead something like Java system properties could be used... Stick with the syntax I'm using in PR 321, but instead of pulling from the environment, pull from user supplied defines, eg: --define SURICATA_RULE_PATH=/etc/rules, which would expand ${SURICATA_RULE_PATH} to /etc/rules. A wrapper script could then pull define values from environment variables if it desired, but the config would no pull directly from the environment itself making it more predictable.

@ish: Not sure I would like to put these vars in our default config though.

Yes, the would somewhat uglify the default config, in which case the variable expansion is not as useful either, as it would have the same affect on the configuration.

Perhaps a --set or --define command line option the results in a ConfSet (or ConfSetFinal if that gets merged in) to set basic values.. For example:

--set unix-command.enabled=yes

or

--set default-log-dir=/tmp

This will give the desired result keeping variable names out of the configuration. I still may see a need for something like variable or define expansion, but YAML anchor/alias support may handle those requirement.

I've made a branch (dev-config-set) with a single commit implementing:

https://github.com/jasonish/suricata/commit/f46aa7402e312c600d610e7bcebf8bce316d65ff

Its pretty unobtrusive. It could also form the basis of variable expansion down the road, where some token like %{NAME} looks up "NAME" in the set of --set parameters. But YAML aliases/anchors may remove any need for that.

One of the use cases is in #622.

Would this look something like: --set af-packet.interfaces.eth0.threads=8

Unfortunately no, and the reason is because the interfaces are defined as a sequence, and sequences are harder to deal with.

This could potentially work (note that it doesn't work right now, but could look at fixing it):

--set af-packet.0.threads=8

But you can see the issue here, you have to know the index of the interface in the config file to set the threads for it. A better af-packet configuration (based on what is in suricata.yaml.in right now) might look like:

af-packet:
  defaults:
    threads: 2
    use-mmap: yes
  interfaces:
    eth0:
      threads: 1
      cluster-id: 99
      cluster-type: cluster_flow
      defrag: yes
      use-mmap: yes
    eth1:
      threads: 1
      cluster-id: 98
      cluster-type: cluster_flow
      defrag: yes

then --set af-packet.interfaces.eth0.threads=8 would work as expected, plus I think the af-packet configuration section is a little tidier this way.

The use case --set af-packet.interfaces.eth0.threads=8 is a cadidate for variable/define expansion in the configuration file. For instance,

--define AF_PACKET_THREADS=8

would replace all occurrences of ${AF_PACKET_THREADS} in the configuration file with "8". I do consider this feature more useful to integrators than anyone, and I think Doug's case might be that of an integrator (Security Onion)?

Jason Ish wrote:

The use case --set af-packet.interfaces.eth0.threads=8 is a cadidate for variable/define expansion in the configuration file. For instance,

--define AF_PACKET_THREADS=8

would replace all occurrences of ${AF_PACKET_THREADS} in the configuration file with "8". I do consider this feature more useful to integrators than anyone, and I think Doug's case might be that of an integrator (Security Onion)?

What I don't like about this approach is that it clutters the yaml (further). What about allowing the code itself to register such vars? So in afpacket parsing code the we would have logic like "if (defined_on_cmndline) use_commandline; else use_yaml_value;" It would simply register it in the code: "ConfRegisterOverride("AF_PACKET_THREADS");" or something. Make sense?

e.g. ConfGetWithOverride("threads", "AF_PACKET_THREADS");

Ok, so for forget about any form of expansion in the config file.

What you suggest is fine, but is not generic. Its very much like individual modules using environment variables set init parameters, where the environment variable overrides the configuration value.

I also don't think this is a replacement for the more generic --set parameter=value, which could be applied almost anywhere, it only fails for af-packet because of the sequence (thought --set af-packet.*.threads=8).

So back to your suggestion of overrides, does it require registration? For arguments sake, lets use --define AF_PACKET_THREAD=8, these defines/overrides (ok, maybe --override is better?) go into their own lookup table. Then wrap some api around that:
```
if (!ConfGetDefine("AF_PACKET_THREADS")) {
ConfGet(...)
}
```
or like your ConfGetWithOverride. But that might get kinda long when replacing ConfGetChildValueWithDefault where you want to resolution order to be, the override, then the configuration value, or the default. but thats an implementation detail.

I do think we should avoid code solutions, when changing up the configuration a bit would do.

Is it described anywhere how can this be used?
A user example or something?

There are examples in here https://github.com/inliniac/suricata/pull/705

Pull request to address this issue: https://github.com/inliniac/suricata/pull/839

This addresses the af-packet.0.threads issue. Basically when set on the command line, it is not a proper "sequence" node as we don't know that yet. I suppose we could guess based on the integer portion of the name, but instead the pull request will promote node to a sequence.

Regarding the tread-detect-ratio. It works for me, but not "ratio" vs "ration".

yes - about "ration" wrong copy/paste from the source:
https://github.com/inliniac/suricata/pull/705

--set threading.detect-thread-ration=1.5

Comment fixed in the PR.