-
Notifications
You must be signed in to change notification settings - Fork 25
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
Implement UUID support in qrexec #135
base: main
Are you sure you want to change the base?
Conversation
7c63aab
to
75ed833
Compare
PipelineRetryFailed |
1 similar comment
PipelineRetryFailed |
Definitely not instead of the name, as most(?) services needs actually the source name (things like subdir in QubesIncoming, or split-gpg prompt). As for extra data, its usefulness would also be limited (generally, the source qube is running during the connection, and the name cannot be changed while qube is running). And adding extra info to the protocol would be an API change anyway so needs to wait for R4.3. |
This version is quite a bit simpler than I thought, but it has no fallback to the name if the UUID symlink is not present, and doesn’t set up the UUID symlink itself. Therefore, it does not work. Would it be possible to use an environment variable @marmarek? That could help avoid circular dependencies during upgrade. Of course, an alternative is to use snapshots to make the entire upgrade atomic, which is probably a better solution as so many things can fail. |
Forcing updating core-admin and core-qrexec at the same time via dependency is okay. There is no need to handle new qubesd starting old qrexec-daemon. Since it isn't necessary, I'd prefer to simply use a command line parameter for UUID, which is much cleaner than giving name via cmdline and UUID via environment. |
d7e0f94
to
50fefe3
Compare
7036de0
to
9840d38
Compare
@@ -120,6 +134,11 @@ def test_010_Source(self): | |||
parser.Source("*") | |||
with self.assertRaises(exc.PolicySyntaxError): | |||
parser.Source("@default") | |||
parser.Source("uuid:d8a249f1-b02b-4944-a9e5-437def2fbe2c") |
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.
Please add a test with uppercase letters that don't raise a syntax error. RFC 4122 allows case insensitive characters on input:
Each field is treated as an integer and has its value printed as a
zero-filled hexadecimal digit string with the most significant
digit first. The hexadecimal values "a" through "f" are output as
lower case characters and are case insensitive on input.
Meaning it is syntactically correct for the user or program to write the uuid value using uppercase letters, but the Qrexec parser should evaluate them with case insensitivity and always output it in lower case.
bb9a582
to
a73a3bc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #135 +/- ##
==========================================
- Coverage 77.75% 77.51% -0.25%
==========================================
Files 54 54
Lines 9498 9601 +103
==========================================
+ Hits 7385 7442 +57
- Misses 2113 2159 +46 ☔ View full report in Codecov by Sentry. |
This allows using UUIDs in qrexec policy, using the syntax uuid:VM_UUID. This works anywhere a VM name is expected. Since ':' is not allowed in VM names, there is no ambiguity. This requires the corresponding change to qubes-core-admin so that qubesd supports UUIDs in the admin and internal APIs. Fixes: QubesOS/qubes-issues#8510
Open questions:
SERVICE-NAME REMOTE-DOMAIN-NAME uuid UUID-OF-TARGET
orSERVICE-NAME REMOTE-DOMAIN-NAME name @uuid:UUID-OF-TARGET
?Fixes: Consider UUID syntax in qrexec policy qubes-issues#8510