Command-line switches should have priority over config file

[martinetd]

Hello,

I finally bothered testing half-internet-routes, but my config file has set-routes=0 so I figured I could just pass --set-routes=1 to restore the default value except that.. It does not work like that, and config file won.

The way we currently fill our cfg struct is:

• set default values
• parse options
• parse config file

This order cannot be changed completely, because options can change where the config file is, so I see two solutions:

• Split options parsing in two, and loop over arguments twice - the first time only looks for -c/--config while the second time properly sets all the rest.

or

• Instead of setting defaults at start, set all values to something invalid (e.g. -1), then when parsing config only set fields if they are currently invalid (means they weren't touched by command line options), then finally restore defaults.
  We could do that without changing anything for most string arguments which are set to NULL and handled in code, but we really need to separate setting default values for int arguments e.g. set-routes to differentiate user asking for 1 and the default value.

The second one is more invasive in the code (adds many if (field) checks), but feels somewhat cleaner, I don't have much preference.
Happy to submit a PR if there is agreement on either way to do this (or a 3rd one comes out)

[mrbaseman]

Hmm.. I'm not so sure either, just sharing my thoughts: The user experience would be the same for both solutions. To me the second one looks much more complicated than the first solution.

With the first solution, we would set the defaults at the beginning like we do now, then we would have to parse for -c/--config somehow (without using getopt_long which would drop all the other options), next would be the part that calls load_config, and after that we would have more or less the existing loop around getopt_long.

Parsing for -c/--config could perhaps be done with getopt_long on a copy of argv[], but probably one has to copy all the arguments as well and build an analogous array of pointers like argv[] is. Maybe this is more effort than a simpler loop that just checks for -c/--config.

Another issue that comes to my mind is that the default config file does not contain valid entries upon first installation. The user can call openfortivpn and tell it all the necessary parameters on the command line, and still he would see a warning "Bad port in config file". When parsing for -c/--config one could already build a list of options specified on the command line and skip parsing of the corresponding lines in the config file. This in turn would be an argument for using getopt_long on a copy of argv[]. Alternatively, this problem would be automatically solved with your second solution, which is more invasive in the code as you write.

There is also the security topic about #54 and #270 where an unprivileged user may use a privileged process to execute user-provided code. If the config file wins in this situation, it is more secure than if the command line wins. However, if the config file can be supplied on the command line, the latter argument doesn't hold anymore. Maybe we should check the UID/GID before allowing overwriting the config file location, or is this something that startup scripts and access rights should handle in the process of packaging/installing?

[martinetd]

I totally had forgotten about this, but if there's no other opinion I'll implement the first way within the next few weeks then :)

I think the security topic will need more thought, I don't give openfortivpn seteuid for this reason; checking if euid matches the uid might be enough but I don't really know which options to allow in the non-trusted case.
Tempted to say allow nothing at all in this case e.g. just parse config file from standard location and nothing else, but that might break some people's usage of openfortivpn, I don't know.

[DimitriPapadopoulos]

@martinetd As far as I'm concerned, I would find it easier to understand code that would:

1. populate two configurations (vpn_config structures or other object?), first from the command line, then optionally from a file,
2. define one or many distinct merging strategies and apply them to these two configurations depending on the context (UID/GID?),
3. finally feed the resulting vpn_config to run_tunnel().

So I'd rather go for the second way because I feel the logic would be better exposed.

[martinetd]

What I dislike about this is adding another long list of all the arguments, and if some new argument pops whoever adds it will need to remember to update that place as well... But I guess we already have quite a few (main for getopt, main for help, config.h for the struct, config.c for file parsing, man page...) so one more probably won't hurt too much™

I'll sit on this a bit longer

[DimitriPapadopoulos]

Oh, I hadn't thought about that. But which exact additional place have you identified? There are already distinct parts of the code for reading the configuration file in config.c and for processing command line options in main.c. Would the "merging" algorithm be this additional place? If so I think it's worth it, because merging would be done explicitly, instead of ordering configuration-related parts of the code depending on non-written rules. A "merging" function would auto-document these "merging" rules.

[martinetd]

Yeah, was just thinking of a merging function. There'd be no new place if merging directly while reading the config but that might be messy and isn't easy to maintain going forward, so keeping it separate is probably best.

The default values could be moved to config.h in a static const struct so there'd be no duplication there, it's only going through all the fields to merge them later on that I don't really like.

[DimitriPapadopoulos]

Why don't you like merging later on? I fail to understand the root reason. Code duplication? More complex, less readable code?

From my own point of view (but I may be wrong of course) the code would be more readable. I do feel there would be some level of apparent duplication (if this option then ..., if that option then ...) but then those "duplicated" if would appear in a different context, operate on additional/different variables (UID/GID for example) and in any way would be better documented even if repeated in distinct read and merge functions.