fwknop
fwknop copied to clipboard
mrash
Add a new directive "include_keys"
After adding the include directive in 2.6.8, a new security cuncent was risied, that if one includes files which are edit-able by users, a user can abuse run_command and run any command as root.
A new proposal is to add a new directive (e.g include_keys /home/*/.fwknoprc)which will fetch only the keys from a user included file, and it will ignore any other directive in the included files.
other directive which can be included are
- CMD_SUDO_EXEC_USER - run a command with sudo under the user permissions.
- CMD_EXEC_USER - run command as user
I think that %include_keys would be a good solution, but forcing a CMD_EXEC_USER for a user owned access.conf file would also be a good idea. I see no reason not to implement both concepts, as they would both be useful.
%include_keys support was just added to my for-2.6.9 pull request. (Also, @mrash, is the single for-version.next pull request the best way for me to add code for review?) It's been tested with a simple access.conf file and seems to be working as expected. The code ignores all entries except: KEY, KEY_BASE64, HMAC_KEY, HMAC_KEY_BASE64, GPG_DECRYPT_ID, GPG_DECRYPT_PW, GPG_ALLOW_NO_PW, GPG_REQUIRE_SIG, GPG_DISABLE_SIG, GPG_IGNORE_SIG_VERIFY_ERROR, GPG_REMOTE_ID, and GPG_FINGERPRINT_ID.
Review is appreciated, as always. --Jonathan
white-listing what to include is safer then black-listing what not to include.
It looks like Jonathan's implementation is doing whitelisting: https://github.com/oneru/fwknop/commit/828fbdd1bb8fe2ba4070b422db7683d16448ad90
@oneru For pull requests, it is usually better to just have a single pull request associated with a particular feature just to maintain separation. But, it doesn't really matter too much since I can just merge on a per-commit basis.
Something we should think about here is what should happen if there are no keys in an %include_keys file. If the purpose is to allow underpriviledged users to set their own keys, then fwknopd shouldn't fail to start just because a single user didn't set keys. Preferably, just the stanza that contains the %include_keys directive should fail, and the rest of the access.conf should be processed as normal. I haven't worked on or even tested what happens when the included file doesn't contain keys. Will get to this at some point.
I think the logical step would be to ignore it and continue loading the reset of the access.conf file. Also in a case the file not exists, just continue.
This based on my though that all the include directive should eventually support wild card matching, and then we can have something like this %include_keys /home/*/.fwknoprc
This also allow for the package maintainers to include a basic and generic template,
which will look in the folders access.conf.d/*.conf and conf.d/*.conf to load any customization the sysadmin like to load and overwrite the default setting - w/o touching the original files.
This way the package will survive updates & system upgrades without notifying the user to confirm the update because the original files where changed.
I think the wildcard matching would actually be the way to go, as @rabin-io suggested. I'll try to get some wildcard matching working, but not sure I'll have time to get it ready for 2.6.10.
