Respect user's choice of netvm and default_dispvm #25
Conversation
|
Review from @adrelanos or @ArrayBolt3 on behavior? |
| if isinstance(vm, qubes.vm.templatevm.TemplateVM): | ||
| default_dispvm = getattr(vm, "default_dispvm", None) | ||
| if not default_dispvm: | ||
| return | ||
| if not default_dispvm.features.check_with_template( | ||
| "whonix-ws", None | ||
| ): | ||
| vm.default_dispvm = None | ||
| return | ||
| template = getattr(vm, 'template', None) | ||
| # look for appropriate default dispvm | ||
| if ( | ||
| template is not None | ||
| and 'whonix-default-dispvm' in template.features | ||
|
|
||
| template_for_dispvms = getattr(vm, "template_for_dispvms", False) |
There was a problem hiding this comment.
Could we use some of the same logic that is used for choosing a DispVM for AppVMs here for templates as well? We could try to default to vm.template.name + "-dvm". and if that doesn't exist or isn't a Whonix-Workstation VM, we can try to default to "whonix-workstation-18-dvm" (or "whonix-workstation-17-dvm" for R4.2). Only if both of those fails should we fall back to "None", IMO.
| if template_for_dispvms and getattr(vm, "template", None): | ||
| # If VM is a DVM and it's template has a DVM that is not a | ||
| # Workstation, use itself. | ||
| template_default_dispvm = getattr( | ||
| vm.template, "default_dispvm", None | ||
| ) | ||
| if ( | ||
| template_default_dispvm | ||
| and not template_default_dispvm.features.check_with_template( | ||
| "whonix-ws" | ||
| ) | ||
| ): | ||
| vm.template.default_dispvm = vm | ||
|
|
||
| curr_default_dispvm = getattr(vm, "default_dispvm", None) |
There was a problem hiding this comment.
I think this block should be removed. Modifying a VM's template's default_dispvm seems error prone, especially since we set the template's default_dispvm using other means above. We could end up with non-deterministic or at least confusing behavior I fear.
There was a problem hiding this comment.
I agree it is not elegant. It was done to ensure that templates have the correct configuration. They have the wrong configuration by default because the disposable template doesn't exist when the whonix-ws is set on the template.
There are other places where the isinstance(vm TemplateVM) code could be moved, such as the domain-load of the template, but it will have the same result, just being checked after the qubes.xml file is loaded every time. But it doesn't seem better to be there than here though, unless we are talking about enforcing it more.
There was a problem hiding this comment.
I see what you're saying I think, and your logic seems sound to me. But in this instance I don't quite see how the code will actually work - when the Whonix-Workstation template is first created, before the DispVM template is made, the Whonix-Workstation template will have its default DispVM set to None by the template-specific code above, won't it? If that happens, doesn't this block become a no-op, since template_default_dispvm will be None?
There was a problem hiding this comment.
when the Whonix-Workstation template is first created, before the DispVM template is made, the Whonix-Workstation template will have its default DispVM set to
Noneby the template-specific code above, won't it?
Yes.
If that happens, doesn't this block become a no-op, since
template_default_dispvmwill beNone?
Yes, I thought it is better to be None and stay None than use a non-Tor route.
There was a problem hiding this comment.
I see what you mean. I guess this code block is probably useful, though I do wish it could be changed so that it wasn't usually a no-op.
Idea: we want to usually respect a default_dispvm value of None, but in this instance it would be very useful to be able to override the None value once, to set up the default DispVM properly during initial setup, but not override the user's configuration going forward. While Qubes OS does provide a property_is_default call, this isn't useful because it only lets us distinguish between an explicitly set default-dvm option, or a default-dvm option that's been defaulted to.
Maybe we can use a qvm-feature that mark when the default DispVM has been properly set, which we can use here? Not sure if something like that would be useful for setting the NetVM too, maybe it would be?
There was a problem hiding this comment.
@ben-grande Does the idea above (using a qvm-feature to indicate when the DispVM is initialized) sound good to you? I think this is the only remaining issue possibly blocking this PR. I can experiment with it on my end if you need to use your time elsewhere.
There was a problem hiding this comment.
Idea: we want to usually respect a default_dispvm value of
None, but in this instance it would be very useful to be able to override theNonevalue once, to set up the default DispVM properly during initial setup, but not override the user's configuration going forward.
...
Maybe we can use a qvm-feature that mark when the default DispVM has been properly set, which we can use here?
For context, the whole template section:
def get_template_dispvm(
template: qubes.vm.templatevm.TemplateVM,
) -> str:
feature = vm.features.check_with_template(
"whonix-default-dispvm", None
)
if (
feature
and feature in app.domains
and feature.features.get("whonix-ws", None)
):
# If any VM in template chain has the special feature, use it.
default_dispvm = feature
elif template is not None:
# If we have a template, use it for assuming a name.
default_dispvm = template.name + "-dvm"
else:
# If all fails, use hardcoded name.
# HARDCODED.
default_dispvm = "whonix-workstation-17-dvm"
return default_dispvm
if isinstance(vm, qubes.vm.templatevm.TemplateVM):
default_dispvm = getattr(vm, "default_dispvm", None)
if not default_dispvm:
return
if not default_dispvm.features.check_with_template(
"whonix-ws", None
):
set_default_dispvm(vm, get_template_dispvm(vm))
return
template_for_dispvms = getattr(vm, "template_for_dispvms", False)
if template_for_dispvms and getattr(vm, "template", None):
# If VM is a DVM and it's template has a DVM that is not a
# Workstation, use itself.
template_default_dispvm = getattr(
vm.template, "default_dispvm", None
)
if (
template_default_dispvm
and not template_default_dispvm.features.check_with_template(
"whonix-ws"
)
):
vm.template.default_dispvm = vmThe code runs when the qube is created or the feature whonix-ws was enabled.
The second block is for the template. If there are no suitable Whonix default disposable templates, it will be set to None. When creating descendants, the qube's default disposable template uses the system's default disposable template (qubes.vm.qubesvm.QubesVM.default_dispvm), thus it won't impact descendants.
The third block runs when an AppVM is created as a disposable template and its template's default disposable template is not None and is not a Workstation, thus setting the template's default disposable template to the AppVM (disposable template) itself. This happens when the template was created before this extension is upgraded. It won't set the template's default disposable template to None.
Not sure if something like that would be useful for setting the NetVM too, maybe it would be?
For the netvm, we are not changing the netvm of the template, only of the qube itself (as it shouldn't have a template anyway), so I don't understand how this relates.
There was a problem hiding this comment.
This happens when the template was created before this extension is upgraded. It won't set the template's default disposable template to None.
Right... but where I'm lost is that, in a situation where this updated qubes-core-admin-addon-whonix is the only version of the package that's ever been on the system, and templates are being created for the first time during system install, things don't seem to quite work as expected. First the template's default DispVM will be set to None (because that gets set up before the first DispVM template is created), then the first DispVM template based on the real template is created. At that point the real template's default DispVM is None, and so it will remain None until the user changes it, no? Therefore the code that is intended to set the real template's default DispVM to the newly created DispVM template ends up doing effectively nothing, and the real template is stuck with no default DispVM.
I guess I can see an argument for that being intended behavior (does a template need a default DispVM? Probably not usually, and users who do need that can set it up manually without much trouble). But it seems like it would be better if the real template did have a default DispVM as long as the user never explicitly set the default DispVM to None. That's why I suggested using a feature as a flag so that we could set the real template's default DispVM to that of the newly created DispVM template.
For the netvm, we are not changing the netvm of the template, only of the qube itself (as it shouldn't have a template anyway), so I don't understand how this relates.
It was a passing thought, I didn't really know if it related when I mentioned it, I just thought maybe it was worth bringing up. Neither of us can see how this would apply to NetVMs, so let's just ignore that.
| feature = vm.features.check_with_template("whonix-default-gw", None) | ||
| if feature: | ||
| # If any VM in template chain has the special feature, use it. | ||
| netvm = feature |
There was a problem hiding this comment.
It might be unsafe to assume that a VM with the feature whonix-default-gw is actually a Whonix-Gateway template. Worth double-checking the VM in feature for the whonix-gw feature?
qubeswhonix/__init__.py
Outdated
| feature = vm.features.check_with_template( | ||
| "whonix-default-dispvm", None | ||
| ) | ||
| if feature: | ||
| # If any VM in template chain has the special feature, use it. | ||
| default_dispvm = feature |
There was a problem hiding this comment.
Similarly to one of the comments above (dunno what order Github is going to put them in), we should probably ensure that the feature VM has the whonix-ws feature set.
| @staticmethod | ||
| def set_gw_dispvm(app, vm): | ||
| """Set the default DispVM for a Whonix-Gateway qube to None if the | ||
| current one is not a Whonix-Workstation.""" | ||
| # pylint: disable=unused-argument | ||
| default_dispvm = getattr(vm, "default_dispvm", None) | ||
| if not default_dispvm: | ||
| return | ||
| if not default_dispvm.features.check_with_template("whonix-ws", None): | ||
| vm.default_dispvm = None |
There was a problem hiding this comment.
I don't think this is a good idea. It doesn't gain us anything since the Whonix-Gateway can already leak the user's identity on its own without root privileges, so this adds no privacy. I can see situations where someone might want to launch Whonix-Workstation DispVMs from sys-whonix, but the user can decide that without being denied the ability to use anything else.
There was a problem hiding this comment.
but the user can decide that without being denied the ability to use anything else.
We don't know if the default_dispvm currently set on Whonix Gateway was the user's choice or the system's default from installation, which is default-dvm.
I think that it is best to have this order:
- whonix disposable template
- None
If the user wants to use a different disposable template than the default one, they can do it:
There was a problem hiding this comment.
If anything triggers this function again in the future though, it will override the user's choice and set the default DispVM to None though. I'm pretty sure this function will get called every time dom0 is booted, so in practice any attempt to set a different DispVM will be ephemeral with this.
Are there any concrete risks with allowing Whonix-Gateway to have a default DispVM of default-dvm? I know there are with Whonix-Workstation, but I don't think there are with Whonix-Gateway (open-link-confirmation should prevent opening of links from Whonix-Gateway unless there's a mechanism in Qubes that bypasses this).
There was a problem hiding this comment.
If anything triggers this function again in the future though, it will override the user's choice and set the default DispVM to
Nonethough. I'm pretty sure this function will get called every time dom0 is booted, so in practice any attempt to set a different DispVM will be ephemeral with this.
It is fired only once.
@qubes.events.handler("domain-add")
def on_domain_add_test(self, event):
self.log.warning("TEST: domain-add")sudo journalctl -fu qubesdsudo systemctl restart qubesdIt won't be fired for all existing domains. Now try to add a domain and you will see a message.
Are there any concrete risks with allowing Whonix-Gateway to have a default DispVM of
default-dvm? I know there are with Whonix-Workstation, but I don't think there are with Whonix-Gateway (open-link-confirmation should prevent opening of links from Whonix-Gateway unless there's a mechanism in Qubes that bypasses this).
The current Whonix Gateway model is:
- Whonix Gateway tries to make everything from itself, not only it's clients, go through Tor, except the
clearnetuser
The default_dispvm can be used by any user, if user user wants to open a file on Whonix Gateway, for whatever reason, in a disposable and it does not go through Tor, it would be an exception to the model above.
There was a problem hiding this comment.
It is fired only once.
Silly me got domain-add and domain-load mixed up. In that instance this is a good improvement. Sorry about the confusion.
Edit: I also didn't realize there were defenses against an identity leak if a user other than clearnet was compromised. That's awesome, and this is very much in line with that.
| if value == '1': | ||
| self.set_ws_netvm(vm.app, vm) | ||
| self.set_ws_dispvm(vm.app, vm) | ||
| if not value: | ||
| return |
There was a problem hiding this comment.
We shouldn't accept values other than 1 here. We may have users who are relying on non-1 values to say "no, this should not be treated as a Whonix-Workstation qube". Changing the semantics here breaks the API.
There was a problem hiding this comment.
String matching is wrong, should never been queried this way on the code. On the readme, it says:
When a new VM is created based on a templte with
whonix-wsfeature set, it gets: ...`
So it never mentioned that setting other values should be used, instead, it should be set. Empty, not set or 0 will be interpreted as false. I believe this is the intended behavior.
There was a problem hiding this comment.
Valid point. I guess the question is then whether this should be considered a documentation bug, or a code bug, and whether some danger could surface from a VM being considered a Whonix-Workstation when it wasn't previously without user interaction. If a Whonix-Workstation qube stopped being considered a Whonix-Workstation, that could open the door for identity leaks, but if a non-Whonix-Workstation qube starts behaving as a Whonix-Workstation, that might be confusing and result in some configuration getting mangled, but it probably won't endanger the user.
@adrelanos Thoughts? If you're fine with the API technically breaking here, then I think this is good.
I noticed during preload tests that my Whonix qubes properties were being overridden. Setting a feature on the template is not the best option, seen that it doesn't consider multi-VM setups where there can be multiple disposable templates. The order is as follows: - None if current property is None; - Current property if it asks Netvm it is a Gateway, itself it it asks for a disposable template and it is a Workstation or current property if it asks for a disposable template and it is a Workstation; - Check for features in template chain; and - Fallback to hardcoded values Whonix Templates are a bit different, they are created with the global default_dispvm, which is not Whonix by default, they are also created before a disposable template (at least a descendant) can exist. Workstation as the default disposable template, it will be kept, For: QubesOS/qubes-issues#1512
| @staticmethod | ||
| def set_gw_dispvm(app, vm): | ||
| """Set the default DispVM for a Whonix-Gateway qube to None if the | ||
| current one is not a Whonix-Workstation.""" | ||
| # pylint: disable=unused-argument | ||
| default_dispvm = getattr(vm, "default_dispvm", None) | ||
| if not default_dispvm: | ||
| return | ||
| if not default_dispvm.features.check_with_template("whonix-ws", None): | ||
| vm.default_dispvm = None |
There was a problem hiding this comment.
but the user can decide that without being denied the ability to use anything else.
We don't know if the default_dispvm currently set on Whonix Gateway was the user's choice or the system's default from installation, which is default-dvm.
I think that it is best to have this order:
- whonix disposable template
- None
If the user wants to use a different disposable template than the default one, they can do it:
qubeswhonix/__init__.py
Outdated
| feature = vm.features.check_with_template( | ||
| "whonix-default-dispvm", None | ||
| ) | ||
| if feature: | ||
| # If any VM in template chain has the special feature, use it. | ||
| default_dispvm = feature |
| feature = vm.features.check_with_template("whonix-default-gw", None) | ||
| if feature: | ||
| # If any VM in template chain has the special feature, use it. | ||
| netvm = feature |
| if template_for_dispvms and getattr(vm, "template", None): | ||
| # If VM is a DVM and it's template has a DVM that is not a | ||
| # Workstation, use itself. | ||
| template_default_dispvm = getattr( | ||
| vm.template, "default_dispvm", None | ||
| ) | ||
| if ( | ||
| template_default_dispvm | ||
| and not template_default_dispvm.features.check_with_template( | ||
| "whonix-ws" | ||
| ) | ||
| ): | ||
| vm.template.default_dispvm = vm | ||
|
|
||
| curr_default_dispvm = getattr(vm, "default_dispvm", None) |
There was a problem hiding this comment.
I agree it is not elegant. It was done to ensure that templates have the correct configuration. They have the wrong configuration by default because the disposable template doesn't exist when the whonix-ws is set on the template.
There are other places where the isinstance(vm TemplateVM) code could be moved, such as the domain-load of the template, but it will have the same result, just being checked after the qubes.xml file is loaded every time. But it doesn't seem better to be there than here though, unless we are talking about enforcing it more.
| if isinstance(vm, qubes.vm.templatevm.TemplateVM): | ||
| default_dispvm = getattr(vm, "default_dispvm", None) | ||
| if not default_dispvm: | ||
| return | ||
| if not default_dispvm.features.check_with_template( | ||
| "whonix-ws", None | ||
| ): | ||
| vm.default_dispvm = None | ||
| return | ||
| template = getattr(vm, 'template', None) | ||
| # look for appropriate default dispvm | ||
| if ( | ||
| template is not None | ||
| and 'whonix-default-dispvm' in template.features | ||
|
|
||
| template_for_dispvms = getattr(vm, "template_for_dispvms", False) |
2 participants
I noticed during preload tests that my Whonix qubes properties were being overridden. Setting a feature on the template is not the best option, seen that it doesn't consider multi-VM setups where there can be multiple disposable templates. The order is as follows:
for a DVM and it is a Workstation or current property if it asks for a
DVM and it is a Workstation;