Use ProtocolError on invalid requests

[ben-grande]

The exceptions ProtocolError and PermissionDenied are both raised by qubesd to indicate various problems with requests. However, both cause clients to print the extremely unhelpful "Got empty response from qubesd" message.

ProtocolError must be used only for client programming errors (bugs), not for problems that the client could not have detected before making the request.

PermissionDenied must be used only for service authorization denials.

Therefore, a API handler can be arranged as:

• Validation: ProtocolError otherwise
• fire_event_for_permission(): PermissionDenied if unauthorized
• Action

Ideally, validation that may leak existence of a system property should be done after asking for administrative permission, but then it would not be possible to pass only safe values to "admin-api:" events.

If we are already leaking existence of a property, it makes sense to provide a useful exception class for it.

Fixes: QubesOS/qubes-issues#10345

---

It is a draft, just posting here for CI. It is more like an idea than a finished version. Logging messages should be okay now, it is only from trusted input.

From some open TODOs:

% rg 'TODO: ben'
qubes/api/admin.py
460:        # TODO: ben: info-leak: revision existence
522:        # TODO: ben: info-leak: pool existence
525:        # TODO: ben: info-leak: volume existence
799:        # TODO: ben: info-leak: pool existence
841:        # TODO: ben: info-leak: pool existence
849:        # TODO: ben: dangerous logging any ASCII value?
949:        # TODO: ben: info-leak: label existence
963:        # TODO: ben: info-leak: label existence
975:        # TODO: ben: info-leak: label existence
1008:        # TODO: ben: info-leak: label existence
1309:                # TODO: ben: info-leak: template existence
1341:                # TODO: ben: info-leak: label existence
1392:        # TODO: ben: info-disclosure: learns if qube exists

It seems that most checks that are done before admin-permission events may leak some data but not sure of its sensitiveness.

• revision is a date
• volumes are always the same
• pool and labels have a default that most people use
• template and qube existence, not so nice

But without these checks, there is no good error message...

[codecov]

Codecov Report

❌ Patch coverage is 86.50794% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.68%. Comparing base (741d111) to head (479ac3d).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
qubes/api/admin.py	87.73%	20 Missing ⚠️
qubes/api/__init__.py	77.77%	8 Missing ⚠️
qubes/utils.py	76.47%	4 Missing ⚠️
qubes/exc.py	90.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
+ Coverage   70.37%   70.68%   +0.30%     
==========================================
  Files          61       61              
  Lines       13757    13882     +125     
==========================================
+ Hits         9682     9812     +130     
+ Misses       4075     4070       -5     

Flag	Coverage Δ
unittests	70.68% <86.50%> (+0.30%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.