Default to global default_dispvm for *named* disposables #735
Conversation
| lambda self: ( | ||
| self.template | ||
| if self.name == "disp" + str(self.dispid) | ||
| else self.app.default_dispvm |
There was a problem hiding this comment.
but not named disposables. The latter cannot be leveraged to e.g. bypass networking restrictions.
How not?
- Global default dispvm netvm: sys-firewall
- Named disposable netvm: vpn
Also checking name would be wrong (could break in the future on naming scheme changes, which there is something planned, not executed, for management qubes), check for auto_cleanup property.
See this branch of the documentation from my PR: https://qubes-doc--1554.org.readthedocs.build/en/1554/developer/services/disposablevm-implementation.html#disposable-behavior
There was a problem hiding this comment.
How not?
- Global default dispvm netvm: sys-firewall
- Named disposable netvm: vpn
By saying it cannot be leveraged I meant that a named disposable can't just be created without user intervention like an unnamed one.
But yes, in the case where a named disposable is based on a different disposable template than the global default disposable template, the commit would change how someone might want to properly configure this named disposable. Do you think it's too harsh of a change to spring upon users?
Also checking name would be wrong, check for
auto_cleanupproperty.
Hmm, I actually started out with auto_cleanup but then wondered about exotic named disposables with auto_cleanup == True (odd but... why not), then thought maybe check for a disp-created-by- tag instead (as something that only exists for disp1234 disposables created via admin.vm.CreateDisposable), and ultimately arrived at checking for name == "disp" + dispid because this seemed like the most natural way to express the concept of an "unnamed" disposable.
(Of course checking for auto_cleanup is more natural in other contexts, like omitting those disposables from backups by default.)
There was a problem hiding this comment.
How not?
- Global default dispvm netvm: sys-firewall
- Named disposable netvm: vpn
By saying it cannot be leveraged I meant that a named disposable can't just be created without user intervention like an unnamed one.
A compromised named disposable that can open unnamed disposables that have a different netvm can be considered network leak.
But yes, in the case where a named disposable uses a different disposable template than the global default disposable template, the commit would change how someone might want to properly configure this named disposable. Do you think it's too harsh of a change to spring upon users?
Also checking name would be wrong, check for
auto_cleanupproperty.Hmm, I actually started out with
auto_cleanupbut then wondered about exotic named disposables withauto_cleanup=True(odd but... why not), then thought maybe check for adisp-created-by-tag instead (as something that only exists for disposables created viaadmin.vm.CreateDisposable), and ultimately arrived at checking forname == "disp" + dispidbecause this seemed like the most natural way to express the concept of an "unnamed" disposable.
The main differentiator of unnamed disposables is having the auto_cleanup enabled, the name is just something for us humans to differentiate them. Unnamed and named disposables behave exactly alike except on shutdown (which scripts such as qvm-run handle manually cleaning the disposable). If a named disposable has the auto_cleanup enabled, perhaps it should be treated as an unnamed disposable?
Unfortunately, this confusion appears to be a lack of definitions, unnamed and named might not be the best name, but I don't know any better right know. I am doing a big PR to the disposable documentation, if you want to propose different vocabulary, I think it would be nice if it happened on qubes-devel before the PR is merged.
There was a problem hiding this comment.
Definitely do not rely on the name for distinguishing unnamed disposables. It's technically possible to create an app qube with name disp1234, you can even create a standalone with such name. Whether it's a good idea is another question, but technically it's perfectly possible.
There was a problem hiding this comment.
By saying it cannot be leveraged I meant that a named disposable can't just be created without user intervention like an unnamed one.
A compromised named disposable that can open unnamed disposables that have a different netvm can be considered network leak.
For sure. But with a named disposable, it seems intuitive to me that I should explicitly configure its default_dispvm to avoid that leak, same as I should explicitly configure an AppVM's default_dispvm to avoid it.
There probably are some users though who haven't yet configured this explicitly, because so far it wasn't necessary. Maybe this tweak isn't worth the trouble that comes with changing configuration semantics for these existing users?
If a named disposable has the auto_cleanup enabled, perhaps it should be treated as an unnamed disposable?
As weird as a named disposable with auto_cleanup is, it doesn't pose the same risk here as a disposable created through qrexec-client-vm @dispvm, i.e. admin.vm.CreateDisposable. I guess conceptually it might be clearer to check for a disp-created-by- tag after all, rather than the name?
Unfortunately, this confusion appears to be a lack of definitions, unnamed and named might not be the best name, but I don't know any better right know. I am doing a big PR to the disposable documentation, if you want to propose different vocabulary, I think it would be nice if it happened on qubes-devel before the PR is merged.
Named and unnamed are fine IMO :)
There was a problem hiding this comment.
@marmarek It's checking for "disp" + dispid (not "disp" + any number suffix). But disp-created-by- tag might be better (see above)
There was a problem hiding this comment.
A compromised named disposable that can open unnamed disposables that have a different netvm can be considered network leak.
Strongly agree with this, having been protected by the current behaviour.
Users (including me) may not have considered the "disposable of a disposable" issue, as a way "open in disposable" could create new risk of information leakage. Current default behaviour guarantees no greater privilege is given to the content of a disposable without explicit intervention by the user to change the default.
|
Generally on this PR, I think it's a bad idea to make this exception this way. If anything, it may default to "default_dispvm" property of the disposable template, not the global one. It would mean the same thing in default configuration, but would allow to change it, without changing the global one. The current change is problematic for example for named disposable based on whonix-workstation. |
|
As for detection of name/unnamed, better use auto_cleanup property - that's what we use in several other places already. |
|
Ugh, I think we got pylint update :/ |
Tweak the logic from 8962452 ("Make DispVMs started from a DispVM to use the same DVM template by default") to apply more narrowly to only those disposables created via admin.vm.CreateDisposable (the unnamed disp1234 kind) - typically reachable, without user intervention, from 'qrexec-client-vm @dispvm' - but not named disposables. The latter cannot be leveraged to e.g. bypass networking restrictions, because they are deliberately created by the user. So treat them like any other VM in regard to their default_dispvm property. https://forum.qubes-os.org/t/default-disposable-for-a-named-disposable/36681
I think I haven't properly explained why it seems so ill fitting to me to predicate this particular check on "is
qubes-core-admin-addon-whonix takes care of this. It gives Whonix based AppVMs (including the disposable template) an explicitly configured |
rustybird
force-pushed
the
named-disposables-default_dispvm
branch
from
6658236 to
9042296
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #735 +/- ##
=======================================
Coverage 70.34% 70.34%
=======================================
Files 61 61
Lines 13682 13682
=======================================
Hits 9625 9625
Misses 4057 4057
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Yes, that's part of the reason why I don't like doing this change. It isn't really much better to have such significant behavior difference between disposables just because you created them a bit differently.
Yes, for whonix specifically only. But if you create similar dvm template for your vpn, your change will break it. As for the user requesting this feature, they can simply use different template for named and unnamed disposables, and have one of them have different value for default_dispvm (+ the change I proposed earlier - to have disposable "default_dispvm" property default to the value of "default_dispvm" of its template). |
Isn't that more or less how it works out already, without this PR? If the disposable template has an explicitly configured value for |
Fair enough. I considered it to be a simplification because I perceive named DisposableVMs like they're in the same category as any other AppVM/TemplateVM/StandaloneVM, compared to unnamed disp1234 DisposableVMs which somehow feel like a completely different animal that it makes sense to special case. But there's no consensus on this (and even if there was one, changing the configuration defaults is tricky to do right without harming some existing users), so I'm closing this PR. |
4 participants
See commit message