Skip to content
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

RExec: Close connection with peer on unknown msgs #61

Merged
merged 2 commits into from Dec 2, 2020

Conversation

reynir
Copy link
Member

@reynir reynir commented Nov 27, 2020

Do not raise an exception. Instead, `Eof is returned. This does not change the interface, but I'm thinking it's perhaps better returning `Error - or maybe it's not interesting to know which end closed the connection?

Fixes #45

@hannesm
Copy link
Member

hannesm commented Nov 27, 2020

This greatly improves the semantics of QRExec: instead of raising an exception, now an error is logged, and the connection is closed. Returning an error could be done at some point, the benefit would be that API clients would be able to handle such an Error different from Eof -- I'm not sure whether this is worth it, and clearly this can be done when needed.

LGTM - imho once this is merged a new release would make sense (what about #60 - is that PR ready & tested)? //what do you think @talex5?

@reynir
Copy link
Member Author

reynir commented Nov 27, 2020

Yes, I agree we should wait until there's a need.

In RExec.listen we just log (as INFO) unexpected messages and ignore them. I'd like to log it at WARN level at least. In that function we're talking with dom0 which is presumably more trusted to be well-behaved. I am wondering if we should instead close the connection or raise an error. I'm not sure whether dom0 will reopen the connection to domU if the agent closes the connection.

About #60, I haven't tested it yet. Basically, it allows for up to 16× larger data chunks in qrexec at the cost of more complexity. I think it's not a deal breaker if it doesn't make it in next release, though the performance gains would be nice.

@hannesm hannesm merged commit 6939417 into mirage:master Dec 2, 2020
hannesm added a commit to hannesm/opam-repository that referenced this pull request Dec 2, 2020
CHANGES:

- RExec: being future compatible, and negotiate protocol version 2 (mirage/mirage-qubes#56 mirage/mirage-qubes#57 @reynir)
- Remove Utils module (mirage/mirage-qubes#58 @hannesm)
- RExec: close connection with peer on unknown messages, instead of raising an exception (mirage/mirage-qubes#61 @reynir)
@hannesm
Copy link
Member

hannesm commented Dec 2, 2020

Thanks @reynir. I went ahead, merged this PR and released 0.9.1 (see ocaml/opam-repository#17748) without #60 -- this should pave the road for Qubes 4.1 users, and once #60 is ready and tested we can merge and cut a new release.

@reynir reynir deleted the close-on-unknown-message branch September 16, 2022 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RExec: misbehaving clients can trigger exceptions
2 participants