Conversation
|
Please rebase the PR against the |
| ) -> bool: | ||
| self.enforce(isinstance(untrusted_bytes, bytes)) | ||
|
|
||
| if len(untrusted_bytes) > 10: |
There was a problem hiding this comment.
Because its larger than 5, (len('false')), and still small. I want to show the user a shortened version of the string if its sufficiently large, but still enough of the original bytes to be easily identifiable. With the three .'s, that leaves 7 characters.
| self.enforce(isinstance(untrusted_bytes, bytes)) | ||
|
|
||
| if len(untrusted_bytes) > 10: | ||
| raise qubes.exc.QubesValueError( |
There was a problem hiding this comment.
It is a protocol error, see the class method above.
There was a problem hiding this comment.
Actually, I do not think is its a protocol error, and I'm in the process of creating a PR to fix the method above.
And invalid value here is not a protocol error, its an error for the layer above. This is an RPC protocol and its not an error in the RPC protocol to call an RPC method with invalid arguments. The protocol is simple: meth+arg src dest_type dest\0untrusted_data.
The Qubes Daemon (qubesd) takes reasonable action on true protocol errors, logging and error and closing the connection without responding. That is not appropriate for the kinds of errors that the function in question or the one above generate. Furthermore, its a terrible client experience.
You might want to test the client experience (on 4.3) for revisions_to_keep when a ProtocolError is generated versus a QubesValueError and notice how qubesd responds and what the client outputs to the user.
@marmarek, do you agree?
There was a problem hiding this comment.
Invalid parameter name, type, or number of parameters is a protocol error. Invalid parameter value, especially when come from user input, usually is not.
Here, it's about verifying if the value a proper bool - if it isn't, I'd say it's a protocol error.
To get a better UX, I'd say QubesOS/qubes-core-admin-client#501 should ensure proper type, and report an error to the user before even making the call to qubesd (see qbool helper function).
There was a problem hiding this comment.
Well in this case the protocol has no notion of types. It just takes a byte string as data. The API layer, not the RPC layer, then interprets it. Its less work and complexity to have the input veification done by qubesd and to display the response from qubesd. Of course, that's not currently possible because qubesd abruptly aborts the connection. Anyway, though I disagree, I'll accept your point of view as the way it is.
|
|
||
| if len(untrusted_bytes) > 10: | ||
| raise qubes.exc.QubesValueError( | ||
| "Invalid literal for boolean value: {!r}".format(untrusted_bytes[:7].decode() + '...') |
There was a problem hiding this comment.
This message doesn't explain why it is invalid.
Please don't include untrusted/unsanitized variables in the message.
There was a problem hiding this comment.
Nope it doesn't. I was chose to have a smaller error message. What do you suggest precisely?
While generally usage of unsanitized data is a concern. I'm not sure its valid here. This is not getting written as a log message and is being sent back to the client that sent the data in the first place. So is this actually a problem? I'm not opposed to unnecessarily sanitizing though to conform to a consistent policy.
| raise qubes.exc.QubesValueError( | ||
| "Invalid literal for boolean value: {!r}".format(untrusted_bytes[:7].decode() + '...') | ||
| ) | ||
| return qubes.property.bool(None, None, untrusted_bytes.decode()) |
There was a problem hiding this comment.
property.boolis a static method that receives aselfargument but never uses it... strange, should be fixed there.property.boolalso receives apropargument and does nothing with that, it should print the property name- please decode to
ASCII, after rebasing to main, you will see other examples that should have a try except block for decoding errors property.boolshould be protocol error in this case, you can catch theQubesValueErrorand raiseProtocolError
There was a problem hiding this comment.
property.boolis a static method that receives aselfargument but never uses it... strange, should be fixed there.
Yes, I saw that and was going to fix it separately, because to fix it requires fixing every place that its used. And I don't think that's appropriate for this PR.
2. `property.bool` also receives a `prop` argument and does nothing with that, it should print the property name 3. please decode to `ASCII`, after rebasing to main, you will see other examples that should have a try except block for decoding errors
Sure.
4. `property.bool` should be protocol error in this case, you can catch the `QubesValueError` and raise `ProtocolError`
I don't think so, unless qubesd handling of ProtocolErrors is changed. See above.
| ) | ||
| async def pool_set_snap_on_start_forensics(self, untrusted_payload): | ||
| self.enforce(self.dest.name == "dom0") | ||
| self.enforce(self.arg in self.app.pools.keys()) |
There was a problem hiding this comment.
After rebasing to main, search for dest_adminvm and wants_arg, both should be True. The wants_payload needs to be false to allow empty value such as b""`.
…roperty Additionally, add AbstractQubesAPI.validate_bool for validating protocol boolean values. Fixes: QubesOS/qubes-issues#11084 Signed-off-by: Glenn Washburn <development@efficientek.com>
6455644 to
e17cf51
Compare
Ok, well this is confusing and frustrating. Ii would be good to have this in the developer docs. I originally did write this against |
|
Would you like to submit a PR to the documentation about making a PR to main branch?
The fast way to test PRs is to have a separate computer that is on the latest possible configuration using |
I was going to write an issue to document the lack of documentation, along with a zillion others. Like everyone else, I'm not sure I have the time to write that PR.
A separate computer is not an option for me right now. Unit tests isn't a bad idea, but it doesn't allow full end-to-end testing. It sounds like there's a policy of only accepting PRs against the main branch. I think there must be exceptions to this, in fact, I don't think its a great policy. Its more reasonable for projects that don't backport changes, which is not this project. For instance, I imagine that PRs against a versioned release must be accepted for security issues that do not affect other releases. Likewise, some changes are only relevant for a particular release. For instance, Why its ridiculous in this case is because you want me to take my 4.3 patches and forward port them to main so you can later back port them to 4.3 again. On top of that, you want me to do the forward porting against a system that I can't properly test against. I think the Qubes team is better positioned to do that work. Furthermore, I assume that not all changes in main get back ported (through intent, negligence, or resource constraints). Might this be one such case? I would like to be reasonably sure that the next release of @marmarek what do you think? |
|
The policy to get things to main first, is to ensure it will not regress on future update. There are exceptions, but those are cases where a change is needed only in a release branch - when it's needed in both, should go to main first. In most cases, a change to the main branch is pretty similar to the most recent stable one, but indeed this PR will have some differences. Unit tests for the case like here should be enough, and they can be ran in any VM (or even not on qubes at all). As for when it gets backported - this one looks to have pretty small impact (in terms of regression risk), so can be backported soon after merging. |
|
@marmarek I'm not sure if you saw some of the review comments that I'd like your opinion on above. Specifically on the usage of |
It seems a label indicating that the PR needs to be ported to main seems like it would do the trick.
This is more onerous for me than I have the bandwidth for, especially considering certain differing points of view. So I'm converting to a draft PR and someone else can pick this up. I don't think anyone is clamoring for this anyway. Thanks for the consideration. |
3 participants
These are the qubesd half of the changes needed to fix QubesOS/qubes-issues#11084.