Revert to red label while restoring on missing labels

[alimirjamali]

If we allow users to create custom labels and they backup such qubes, restore operation will fail on new systems without those custom labels. Fix this by reverting to red label and allow users to restore their backup.

fixes: QubesOS/qubes-issues#9781

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.67%. Comparing base (c3759ad) to head (c4940be).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #335      +/-   ##
==========================================
- Coverage   75.68%   75.67%   -0.02%     
==========================================
  Files          52       52              
  Lines        8975     8977       +2     
==========================================
  Hits         6793     6793              
- Misses       2182     2184       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[alimirjamali]

I am not certain why codecov has marked those two lines as uncovered. The unittests had to be specifically changed since those two lines were indeed working

[marmarek]

I am not certain why codecov has marked those two lines as uncovered. The unittests had to be specifically changed since those two lines were indeed working

Looks like some backup tests are skipped because scrypt tool is not installed. It isn't available in Fedora repo, only in Qubes repo and this one is not included in the docker used for the test...

[alimirjamali]

I am not certain why codecov has marked those two lines as uncovered. The unittests had to be specifically changed since those two lines were indeed working

Looks like some backup tests are skipped because scrypt tool is not installed. It isn't available in Fedora repo, only in Qubes repo and this one is not included in the docker used for the test...

So what is the appropriate solution here? Is there a way to include qubes-vm-r4.3-current-testing in docker to install scrypt binary and re-enable the skipped tests? Or it is necessary to switch to python3-scrypt package and do the decryption directly in Python instead of depending on the external binary?

[marmarek]

Rebase on top of #337 (soon main)

[alimirjamali]

Rebase on top of #337 (soon main)

OK. Unit tests are green again and looking at Github report, it appears that backupcompatibility tests are not skipped. But the Codecov report is still not covered.

Another issue is that the backupcompatibility tests are kinda slow. I believe after this is done, it might be better to disable ENABLE_SLOW_TESTS in .gitlab-ci.yml via another PR to make the life easier for other PRs which are not backup related.

[marmarek]

But the Codecov report is still not covered.

I think you changed label in test in a wrong place. Change it in the qubes.xml, not the parsed structure.

[marmarek]

openQA says it breaks restoring backup using a DispVM:

Feb 23 15:01:58.885143 dom0 qrexec-policy-daemon[1693]: qrexec: admin.label.List: disp-backup-restore -> dom0: denied: no matching rule found

Theoretically the policy could be extended to allow listing labels, but maybe it isn't needed? Maybe instead of check before, you can try setting the label and fallback to "red" in case of QubesLabelNotFoundError exception?

[marmarek]

The policy for DispVM restore is in core-admin. But as said above, maybe this extra permission (and also extra call during usual restore) isn't actually needed?

[alimirjamali]

The policy for DispVM restore is in core-admin. But as said above, maybe this extra permission (and also extra call during usual restore) isn't actually needed?

Unfortunatley it is needed. Unless we want to hard-code the choices of default labels, I do not know any other way to test if the label of the VM to restore is one of them.

[marmarek]

Ah, because label is mandatory to create a VM, setting it cannot be deferred to later. I was thinking about something like:

try:
    vm.label = label
except QubesLabelNotFoundError:
    vm.label = "red"

but that indeed won't work. And worse - it looks like admin.vm.Create raises generic QubesValueError on unknown label (which BTW might be a bug - QubesLabelNotFoundError would be more logical), so you can't even do that try/except with the create call.

[alimirjamali]

There is a less invasive and more privacy friendly way by using admin.label.Get call to check of the label exists

[alimirjamali]

(which BTW might be a bug - QubesLabelNotFoundError would be more logical)

Should I implement it for core?

[marmarek]

QubesOS/qubes-core-admin#663

[alimirjamali]

QubesOS/qubes-core-admin#663

Very nice. I will amend this PR and revert to try: except: approach as soon as the above is merged.

[marmarek]

Now that QubesOS/qubes-core-admin#663 is merged you can update this PR. Or do you want me to push an updated package to current-testing first (doesn't matter for openqa)?

[alimirjamali]

Now that QubesOS/qubes-core-admin#663 is merged you can update this PR. Or do you want me to push an updated package to current-testing first (doesn't matter for openqa)?

Very nice. Let's see if this works. What about the unittests? Will they fetch the main branch or use current-testing?

[alimirjamali]

Ok. It appears that the unittests fail since they depend on current-testing.

[marmarek]

No, unittests do not use real core-admin. There is a mockup that is configured to mimic its behavior. You need to update it to match the change, specifically to return an exception for the call with scarlet label (the one that is now shown in test failure message as unexpected call).