-
Notifications
You must be signed in to change notification settings - Fork 132
NAS-104777 / 12.0 / Change devfs ruleset handling so that configured rulesets != 4 are #1106
Conversation
cloned/copied into dynamic ones. This makes devfs rule handling more symmetrical to what is done when the default ruleset 4 is configured (which in fact never applies devfs ruleset 4, but creates an iocage specific dynamic ruleset instead - which can be quite confusing). As a result, this addresses the problem of non-dynamic rulesets being removed on `iocage stop` raised in iocage#952. This also makes iocage fail to start a jail if the configured devfs ruleset is not available - beforehand it would start with a default ruleset in this case, which can have severe unwanted side effects. Finally, this sets the minimum dynamically assigned devfs rule id to 1000 to reserve the lower ids for static configuration by the admin in devfs.rules (this is mostly for convenience).
@sonicaj Do you think there is a chance to look into this any time soon? |
@skarekrow would you have some time to have a peek at this please ? |
I don’t have much free time unfortunately. Possibly next weekend, but no promises :(
|
To give some more background: The previous protecting behaviour was removed in 19cc401#diff-2be5e949a8b1c257fbb3dbc48a059c90 as a consequence of another change. Please note that this is not only an inconvenience, but in some scenarios could also cause security problems (in cases where a strict ruleset is removed and replaced by the more permissive default one - or a firewall device isn't made available, leaving a vnet jail not firewalled - all triggered by an innocent iocage restart jailname). |
Is there anything I can do to help? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @grembo for the contribution. This looks good to me and good catch ;)
I just have a minor nit
iocage_lib/ioc_common.py
Outdated
logit( | ||
{ | ||
"level": "EXCEPTION", | ||
"message": f"devfs_ruleset {ruleset} doesn't exist" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grembo can we please catch the exceptions which this would raise in ioc_start.py
and raise an appropriate error message saying that Failed to start {jail_name}: the current one we have right now here
. Thoughts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see comment on not-outdated commit...
- Users get a helpful error message in case a configured devfs_ruleset doesn't exist (also shows the configured ruleset and not the dynamically created one, which was not helpful). - Users learn on jail start about how the devfs_ruleset is created (show id it was cloned from or that it is based on iocage's default). - Avoid leaking devfs_rulesets on starting plugins that define devfs_paths/devfs_includes (would lose one ruleset everytime otherwise). - Show a warning in case a plugin with devfs_paths/devfs_includes is started with a manually configured devfs_ruleset (as this won't - and never did - apply those. - Move magic numbers to constants in ioc_common.py. This doesn't fix the iocage man page, which shows (and had shown) inaccurate information on this for a while.
c94703c
to
751a215
Compare
Hi @sonicaj, Thanks for looking into this. I changed the code now, so it shows a proper error message in case a devfs_ruleset isn't configured. While doing so I found a couple of more issues that I addressed, please see the commit message of the new patch. This makes the patch a bit bigger, but also a lot more useful. Let me know if you have any questions. See below on what startup messages look like now. devfs_ruleset non-existent:
devfs_ruleset existent:
devfs_ruleset default/unconfigured:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @grembo, some minor nits.
return | ||
|
||
# Manually configured devfs_ruleset doesn't support all iocage features | ||
if manual_devfs_config: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if we should increase verbosity to this level for something which the user deliberately configured. Thoughts @skarekrow ? In my opinion we shouldn't add these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They may have done so erroneously, so being aware of what the ramifications are should help to make better decisions so I’m in favor of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Half of that check was in there before [DHCP] (I just moved it to the same block to take advantage of having only one check if a devfs ruleset was manually configured). I also think that having that kind of check in there could be helpful, as debugging such things can be hard.
…s_ruleset message to be one-liner containing information on configured devfs ruleset (configured or iocage generated).
There are many more numeric Settings stored as strings (like timeouts etc), but not all. There are also more things to fix around devfs rules, but I would like to limit the scope of this change, so we can finish it and do a patch in the ports tree - the current behavior can make systems more vulnerable/less resilient and should be fixed ASAP. If you want to change devfs_rules to numeric later, I can change min_... to numeric now. |
@grembo let's please convert this to integer right now ( with devfs_ruleset too ) while we are bumping a config version. Also let's please make sure that if user already has an invalid value configured, we handle that case gracefully and default to 4 like we do for other jails in the correcting migration. Also let's please ensure that the user is not able to add an invalid value for this i.e string. Please let me know if you run into any issues. Thank you |
Update: Not sure if graceful fallback to 4 in case of invalid input is needed, as this already dies in the current version of iocage (1.2) with a ValueError and in previous versions it would fail on jail start due to using an invalid devfs_ruleset id. So all this would do is unbreak setups that never worked before, which can be surprising at best or even dangerous, while papering over new invalid setups that are created by manually editing config.json.
Do you really want conf['devfs_ruleset'] and conf['min_dyn_devfs_ruleset'] to be ints? It would make them the first ones in the entire configuration (besides truthy_props, which are handled separately and have distinct logic all over the place). All other numeric values like exec_timeout etc. are all strings. This would also mean a lot of changes to the settings infrastructure in ioc_json.py + changing all places where devfs_ruleset is used (e.g. in ioc_start.py). Lots of room for introducing errors. Wouldn't it make more sense to keep them as strings, but validate them on set/load, so most of the code can stay untouched? Converting settings to ints seems more like a project to be done separately and applied to all values that are numeric by nature (like timeouts and counts). The end-goal of this patch was to fix a security problem, as removing existing devfs_rulesets (and then applying a default one) is a bad thing. I would be grateful if we won't widen the scope of this patch further. If it helps, I could also remove the 'min_dyn_devfs_ruleset' configuration, avoiding bumping the revision and returning to a hard coded value in generate_devfs_ruleset for the time being (in the end, having this detail not tunable right now shouldn't be a showstopper). Like I said before, there are more problems with devfs_rules (e.g. how they're handled while the jail is running - you can't see what is configured, but only what is applied, which has funny side effects), the man page is wrong on them etc. - all these things can be fixed later/separately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grembo we are already doing a config bump and it made sense to have the props in question updated to be proper integers. But yes, you are right that we have quite a number of other props which could use the same treatment which we can indeed retarget for a later date.
Let's please only add validation for the devfs props and have this merged ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you @grembo. @skarekrow can you please have a peek as well ? Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too, thanks @grembo 👍
@skarekrow @sonicaj Cool! Once this has been merged, I'll open a PR with @araujobsd to patch the FreeBSD port sysutils/iocage. |
…rulesets != 4 are (iocage#1106) * Change devfs ruleset handling so that configured rulesets != 4 are cloned/copied into dynamic ones. This makes devfs rule handling more symmetrical to what is done when the default ruleset 4 is configured (which in fact never applies devfs ruleset 4, but creates an iocage specific dynamic ruleset instead - which can be quite confusing). As a result, this addresses the problem of non-dynamic rulesets being removed on `iocage stop` raised in iocage#952. This also makes iocage fail to start a jail if the configured devfs ruleset is not available - beforehand it would start with a default ruleset in this case, which can have severe unwanted side effects. Finally, this sets the minimum dynamically assigned devfs rule id to 1000 to reserve the lower ids for static configuration by the admin in devfs.rules (this is mostly for convenience). * Change devfs_ruleset config parsing on jail start, so that: - Users get a helpful error message in case a configured devfs_ruleset doesn't exist (also shows the configured ruleset and not the dynamically created one, which was not helpful). - Users learn on jail start about how the devfs_ruleset is created (show id it was cloned from or that it is based on iocage's default). - Avoid leaking devfs_rulesets on starting plugins that define devfs_paths/devfs_includes (would lose one ruleset everytime otherwise). - Show a warning in case a plugin with devfs_paths/devfs_includes is started with a manually configured devfs_ruleset (as this won't - and never did - apply those. - Move magic numbers to constants in ioc_common.py. This doesn't fix the iocage man page, which shows (and had shown) inaccurate information on this for a while. * Move "min_dyn_devfs_ruleset" to jail property, change jail start devfs_ruleset message to be one-liner containing information on configured devfs ruleset (configured or iocage generated). * Validate 'devfs_ruleset' and 'min_dyn_devfs_ruleset' while setting.
Pulls in a bugfix[0] that has been merged upstream[1]. This bug caused manually configured devfs_rulesets to get deleted on jail stop, causing jails to come up with a default devfs rules on next start, which can be a nuisance or even dangerous, depending on the specific setup. Also adds a minimum devfs_ruleset id for dynamically created devfs rulesets and fixes a devfs_ruleset resource leak when using plugins. Take maintainership, as suggested by araujo@ (thanks for taking care of that port for so long!). [0]iocage/iocage#1106 [1]iocage/iocage@753b2f3 PR: 243510 Approved by: araujo (maintainer) MFH: 2020Q1 git-svn-id: svn+ssh://svn.freebsd.org/ports/head@523920 35697150-7ecd-e111-bb59-0022644237b5
Pulls in a bugfix[0] that has been merged upstream[1]. This bug caused manually configured devfs_rulesets to get deleted on jail stop, causing jails to come up with a default devfs rules on next start, which can be a nuisance or even dangerous, depending on the specific setup. Also adds a minimum devfs_ruleset id for dynamically created devfs rulesets and fixes a devfs_ruleset resource leak when using plugins. Take maintainership, as suggested by araujo@ (thanks for taking care of that port for so long!). [0]iocage/iocage#1106 [1]iocage/iocage@753b2f3 PR: 243510 Approved by: araujo (maintainer) MFH: 2020Q1
Pulls in a bugfix[0] that has been merged upstream[1]. This bug caused manually configured devfs_rulesets to get deleted on jail stop, causing jails to come up with a default devfs rules on next start, which can be a nuisance or even dangerous, depending on the specific setup. Also adds a minimum devfs_ruleset id for dynamically created devfs rulesets and fixes a devfs_ruleset resource leak when using plugins. Take maintainership, as suggested by araujo@ (thanks for taking care of that port for so long!). [0]iocage/iocage#1106 [1]iocage/iocage@753b2f3 PR: 243510 Approved by: araujo (maintainer) MFH: 2020Q1 git-svn-id: svn+ssh://svn.freebsd.org/ports/head@523920 35697150-7ecd-e111-bb59-0022644237b5
Added patch in FreeBSD ports: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=243510 |
Fix accidental removal of devfs_rulesets on jail stop Pulls in a bugfix[0] that has been merged upstream[1]. This bug caused manually configured devfs_rulesets to get deleted on jail stop, causing jails to come up with a default devfs rules on next start, which can be a nuisance or even dangerous, depending on the specific setup. Also adds a minimum devfs_ruleset id for dynamically created devfs rulesets and fixes a devfs_ruleset resource leak when using plugins. Take maintainership, as suggested by araujo@ (thanks for taking care of that port for so long!). [0]iocage/iocage#1106 [1]iocage/iocage@753b2f3 PR: 243510 Approved by: araujo (maintainer) Approved by: ports-secteam (joneum)
Pulls in a bugfix[0] that has been merged upstream[1]. This bug caused manually configured devfs_rulesets to get deleted on jail stop, causing jails to come up with a default devfs rules on next start, which can be a nuisance or even dangerous, depending on the specific setup. Also adds a minimum devfs_ruleset id for dynamically created devfs rulesets and fixes a devfs_ruleset resource leak when using plugins. Take maintainership, as suggested by araujo@ (thanks for taking care of that port for so long!). [0]iocage/iocage#1106 [1]iocage/iocage@753b2f3 PR: 243510 Approved by: araujo (maintainer) MFH: 2020Q1
I noticed this today:
This differs from the start:
Can the stop also mentioned 'cloned from 42'? I ask so that I don't wonder why there is no 1005 ruleset in the conf file:
|
@dlangille Please see #1138 - once differentiating between the configured and the dynamically created ruleset works, it will be easy to do what you’re asking for. |
…rulesets != 4 are (iocage#1106) * Change devfs ruleset handling so that configured rulesets != 4 are cloned/copied into dynamic ones. This makes devfs rule handling more symmetrical to what is done when the default ruleset 4 is configured (which in fact never applies devfs ruleset 4, but creates an iocage specific dynamic ruleset instead - which can be quite confusing). As a result, this addresses the problem of non-dynamic rulesets being removed on `iocage stop` raised in iocage#952. This also makes iocage fail to start a jail if the configured devfs ruleset is not available - beforehand it would start with a default ruleset in this case, which can have severe unwanted side effects. Finally, this sets the minimum dynamically assigned devfs rule id to 1000 to reserve the lower ids for static configuration by the admin in devfs.rules (this is mostly for convenience). * Change devfs_ruleset config parsing on jail start, so that: - Users get a helpful error message in case a configured devfs_ruleset doesn't exist (also shows the configured ruleset and not the dynamically created one, which was not helpful). - Users learn on jail start about how the devfs_ruleset is created (show id it was cloned from or that it is based on iocage's default). - Avoid leaking devfs_rulesets on starting plugins that define devfs_paths/devfs_includes (would lose one ruleset everytime otherwise). - Show a warning in case a plugin with devfs_paths/devfs_includes is started with a manually configured devfs_ruleset (as this won't - and never did - apply those. - Move magic numbers to constants in ioc_common.py. This doesn't fix the iocage man page, which shows (and had shown) inaccurate information on this for a while. * Move "min_dyn_devfs_ruleset" to jail property, change jail start devfs_ruleset message to be one-liner containing information on configured devfs ruleset (configured or iocage generated). * Validate 'devfs_ruleset' and 'min_dyn_devfs_ruleset' while setting. (cherry picked from commit a69d58b)
…rulesets != 4 are (#1106) * Change devfs ruleset handling so that configured rulesets != 4 are cloned/copied into dynamic ones. This makes devfs rule handling more symmetrical to what is done when the default ruleset 4 is configured (which in fact never applies devfs ruleset 4, but creates an iocage specific dynamic ruleset instead - which can be quite confusing). As a result, this addresses the problem of non-dynamic rulesets being removed on `iocage stop` raised in #952. This also makes iocage fail to start a jail if the configured devfs ruleset is not available - beforehand it would start with a default ruleset in this case, which can have severe unwanted side effects. Finally, this sets the minimum dynamically assigned devfs rule id to 1000 to reserve the lower ids for static configuration by the admin in devfs.rules (this is mostly for convenience). * Change devfs_ruleset config parsing on jail start, so that: - Users get a helpful error message in case a configured devfs_ruleset doesn't exist (also shows the configured ruleset and not the dynamically created one, which was not helpful). - Users learn on jail start about how the devfs_ruleset is created (show id it was cloned from or that it is based on iocage's default). - Avoid leaking devfs_rulesets on starting plugins that define devfs_paths/devfs_includes (would lose one ruleset everytime otherwise). - Show a warning in case a plugin with devfs_paths/devfs_includes is started with a manually configured devfs_ruleset (as this won't - and never did - apply those. - Move magic numbers to constants in ioc_common.py. This doesn't fix the iocage man page, which shows (and had shown) inaccurate information on this for a while. * Move "min_dyn_devfs_ruleset" to jail property, change jail start devfs_ruleset message to be one-liner containing information on configured devfs ruleset (configured or iocage generated). * Validate 'devfs_ruleset' and 'min_dyn_devfs_ruleset' while setting.
Fix accidental removal of devfs_rulesets on jail stop Pulls in a bugfix[0] that has been merged upstream[1]. This bug caused manually configured devfs_rulesets to get deleted on jail stop, causing jails to come up with a default devfs rules on next start, which can be a nuisance or even dangerous, depending on the specific setup. Also adds a minimum devfs_ruleset id for dynamically created devfs rulesets and fixes a devfs_ruleset resource leak when using plugins. Take maintainership, as suggested by araujo@ (thanks for taking care of that port for so long!). [0]iocage/iocage#1106 [1]iocage/iocage@753b2f3 PR: 243510 Approved by: araujo (maintainer) Approved by: ports-secteam (joneum)
cloned/copied into dynamic ones. This makes devfs rule handling more
symmetrical to what is done when the default ruleset 4 is configured (which
in fact never applies devfs ruleset 4, but creates an iocage specific
dynamic ruleset instead - which can be quite confusing).
As a result, this addresses the problem of non-dynamic rulesets being
removed on
iocage stop
raised in #952.This also makes iocage fail to start a jail if the configured devfs ruleset
is not available - beforehand it would start with a default ruleset in this
case, which can have severe unwanted side effects.
Finally, this sets the minimum dynamically assigned devfs rule id to 1000 to
reserve the lower ids for static configuration by the admin in devfs.rules
(this is mostly for convenience).
Make sure to follow and check these boxes before submitting a PR! Thank you.