-
-
Notifications
You must be signed in to change notification settings - Fork 116
Default to global default_dispvm for *named* disposables #735
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
How not?
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_cleanupproperty.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
Uh oh!
There was an error while loading. Please reload this page.
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.
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?
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 disp1234 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.(Of course checking for
auto_cleanupis more natural in other contexts, like omitting those disposables from backups by default.)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.
A compromised named disposable that can open unnamed disposables that have a different netvm can be considered network leak.
The main differentiator of unnamed disposables is having the
auto_cleanupenabled, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure. But with a named disposable, it seems intuitive to me that I should explicitly configure its
default_dispvmto avoid that leak, same as I should explicitly configure an AppVM'sdefault_dispvmto 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?
As weird as a named disposable with
auto_cleanupis, it doesn't pose the same risk here as a disposable created throughqrexec-client-vm @dispvm, i.e.admin.vm.CreateDisposable. I guess conceptually it might be clearer to check for adisp-created-by-tag after all, rather than the name?Named and unnamed are fine IMO :)
Uh oh!
There was an error while loading. Please reload this page.
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.