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

Socket-based services should support exiting when remote shuts down write end #9176

Closed
5 tasks
DemiMarie opened this issue Apr 30, 2024 · 2 comments · Fixed by QubesOS/qubes-core-qrexec#159
Closed
5 tasks
Assignees
Labels
C: core P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. pr submitted A pull request has been submitted for this issue.

Comments

@DemiMarie
Copy link

How to file a helpful issue

The problem you're addressing (if any)

Existing VMs assume that qubes.UpdatesProxy and qubes.ConnectTCP will terminate after the server shuts down the write end of its connection. This is currently implemented by socat, with a useless 0.5s timeout. Native socket-based services would be better, but runs into #9169 with the current qrexec-client-vm implementation. #9169 can be fixed by changes to qrexec-client-vm and [email protected], but these changes work around this problem on the client side, rather than restoring the previous server behavior. This means that the new services are incompatible with existing VMs that need to be updated, which is very bad.

The solution you'd like

Add a new service configuration directive that causes the service to send MSG_DATA_EXIT_CODE and terminate when the socket server shuts down the connection for writing, the VM shuts down the connection for reading, or both. These should be controllable separately.

Proposed names: terminate-on-send-eof and terminate-on-recv-eof. Both will be incompatible with force-user= and executable services.

The value to a user, and who that user might be

Users will be able to use a native socket-based service instead of socat for qubes.ConnectTCP and qubes.UpdatesProxy, without needing a new qrexec-client-vm.

Completion criteria checklist

(This section is for developer use only. Please do not modify it.)

  • terminate-on-send-eof=true works
  • terminate-on-recv-eof=true works
  • Combination of terminate-on-send-eof=true and terminate-on-recv-eof=true works
  • Tests for all of the above
  • Tests that these two options are incompatible with force-user= and with executable services.
@DemiMarie DemiMarie added T: enhancement C: core P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. labels Apr 30, 2024
@ben-grande
Copy link

Tests that these two options are incompatible with force-user= and with executable services.

Does it make sense to create a lint tool for rpc-config to detect these problems at will instead of at runtime?

The tool qubes-policy-lint is a wrapper of qrexec.policy.parser, is such thing possible for the rpc-config files I can only see libqrexec/qrxec.h and qrexec/tests/socket/{daemon,agent}.py handling this bit, so there is no python parser just manual tests? Especially because conflicting options are being implemented (skip-service-descriptor and force-user already seem to be conflicting from what I gathered).

If it is possible, should I create another issue about this?

@DemiMarie
Copy link
Author

Please do create another issue.

The format is a strict subset of TOML. TOML allows lots of syntax that the configuration parser will reject. However, if the configuration parser accepts a file, it should interpret the file according to the TOML specification. If you can get the configuration parser to accept a file and parse it dfferently than the TOML specification requires, please report it as a bug.

@marmarek marmarek moved this to Ready in Current team tasks May 1, 2024
@DemiMarie DemiMarie self-assigned this May 1, 2024
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue May 2, 2024
This adds two new boolean service configuration options:

- exit-on-stdout-eof: exit when the socket service shuts down its
  output stream for writing.

- exit-on-stdin-eof: exit when the client sends EOF.

To avoid compatibility problems, global variables are used to pass this
information from the configuration parser to the I/O code.  In the
future, there should be a better way to pass this information, so global
variables are used right now.  Fortunately, the configuration parser
only runs once in the life of any process right now, so this commit adds
assertions to check this.  Qubes OS ships with assertions enabled, so
any violation of this rule will be detected.

The main use of these features is to emulate the old qubes.ConnectTCP
and qubes.UpdatesProxy services, which already had this behavior due to
the use of socat.

These features are only supported for socket-based services, as
executable services are more complicated and do not have a use case
right now.

Currently, if a service exits due to exit-on-stdin-eof, the empty
MSG_DATA_STDOUT that indicates EOF is not sent.  This is not a problem
because qrexec-client-vm interprets MSG_DATA_EXIT_CODE as also
indicating EOF on stdout and stderr.

Fixes: QubesOS/qubes-issues#9176
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue May 4, 2024
This adds two new boolean service configuration options:

- exit-on-stdout-eof: exit when the socket service shuts down its
  output stream for writing.

- exit-on-stdin-eof: exit when the client sends EOF.

To avoid compatibility problems, global variables are used to pass this
information from the configuration parser to the I/O code.  In the
future, there should be a better way to pass this information, so global
variables are used right now.  Fortunately, the configuration parser
only runs once in the life of any process right now, so this commit adds
assertions to check this.  Qubes OS ships with assertions enabled, so
any violation of this rule will be detected.

The main use of these features is to emulate the old qubes.ConnectTCP
and qubes.UpdatesProxy services, which already had this behavior due to
the use of socat.

These features are only supported for socket-based services, as
executable services are more complicated and do not have a use case
right now.

Currently, if a service exits due to exit-on-stdin-eof, the empty
MSG_DATA_STDOUT that indicates EOF is not sent.  This is not a problem
because qrexec-client-vm interprets MSG_DATA_EXIT_CODE as also
indicating EOF on stdout and stderr.

Fixes: QubesOS/qubes-issues#9176
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue May 4, 2024
This adds two new boolean service configuration options:

- exit-on-stdout-eof: exit when the socket service shuts down its
  output stream for writing.

- exit-on-stdin-eof: exit when the client sends EOF.

To avoid compatibility problems, global variables are used to pass this
information from the configuration parser to the I/O code.  In the
future, there should be a better way to pass this information, but this
is not possible without more extensive changes.  Fortunately, the
configuration parser only runs once in the life of any process right
now, so this commit adds assertions to check this.  Qubes OS ships with
assertions enabled, so any violation of this rule will be detected.

The main use of these features is to emulate the old qubes.ConnectTCP
and qubes.UpdatesProxy services, which already had this behavior due to
the use of socat.

These features are only supported for socket-based services, as
executable services are more complicated and do not have a use case
right now.

Currently, if a service exits due to exit-on-stdin-eof, the empty
MSG_DATA_STDOUT that indicates EOF is not sent.  This is not a problem
because qrexec-client-vm interprets MSG_DATA_EXIT_CODE as also
indicating EOF on stdout and stderr.

Fixes: QubesOS/qubes-issues#9176
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue May 4, 2024
This adds two new boolean service configuration options:

- exit-on-stdout-eof: exit when the socket service shuts down its
  output stream for writing.

- exit-on-stdin-eof: exit when the client sends EOF.

To avoid compatibility problems, global variables are used to pass this
information from the configuration parser to the I/O code.  In the
future, there should be a better way to pass this information, but this
is not possible without more extensive changes.  Fortunately, the
configuration parser only runs once in the life of any process right
now, so this commit adds assertions to check this.  Qubes OS ships with
assertions enabled, so any violation of this rule will be detected.

The main use of these features is to emulate the old qubes.ConnectTCP
and qubes.UpdatesProxy services, which already had this behavior due to
the use of socat.

These features are only supported for socket-based services, as
executable services are more complicated and do not have a use case
right now.

Currently, if a service exits due to exit-on-stdin-eof, the empty
MSG_DATA_STDOUT that indicates EOF is not sent.  This is not a problem
because qrexec-client-vm interprets MSG_DATA_EXIT_CODE as also
indicating EOF on stdout and stderr.

Fixes: QubesOS/qubes-issues#9176
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue May 5, 2024
This adds two new boolean service configuration options:

- exit-on-stdout-eof: exit when the socket service shuts down its
  output stream for writing.

- exit-on-stdin-eof: exit when the client sends EOF.

The information is passed through an extended qrexec_parsed_command
struct.  New functions are added that avoid the executables having to
access members that are private to libqrexec.

The main use of these features is to emulate the old qubes.ConnectTCP
and qubes.UpdatesProxy services, which already had this behavior due to
the use of socat.

These features are only supported for socket-based services, as
executable services are more complicated and do not have a use case
right now.

Currently, if a service exits due to exit-on-stdin-eof, the empty
MSG_DATA_STDOUT that indicates EOF is not sent.  This is not a problem
because qrexec-client-vm interprets MSG_DATA_EXIT_CODE as also
indicating EOF on stdout and stderr.

Fixes: QubesOS/qubes-issues#9176
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue May 5, 2024
This adds two new boolean service configuration options:

- exit-on-service-eof: exit when the socket service shuts down its
  output stream for writing.

- exit-on-client-eof: exit when the client shuts down its output stream
  for writing.

The information is passed through an extended qrexec_parsed_command
struct.  New functions are added that avoid the executables having to
access members that are private to libqrexec.

The main use of these features is to emulate the old qubes.ConnectTCP
and qubes.UpdatesProxy services, which already had this behavior due to
the use of socat.

These features are only supported for socket-based services, as
executable services are more complicated and do not have a use case
right now.

Currently, if a service exits due to exit-on-stdin-eof, the empty
MSG_DATA_STDOUT that indicates EOF is not sent.  This is not a problem
because qrexec-client-vm interprets MSG_DATA_EXIT_CODE as also
indicating EOF on stdout and stderr.

Fixes: QubesOS/qubes-issues#9176
@marmarek marmarek moved this from Ready to In review in Current team tasks May 5, 2024
@github-project-automation github-project-automation bot moved this from In review to Done in Current team tasks May 9, 2024
@andrewdavidwong andrewdavidwong added the pr submitted A pull request has been submitted for this issue. label May 10, 2024
ben-grande added a commit to ben-grande/qubes-core-qrexec that referenced this issue May 11, 2024
ben-grande added a commit to ben-grande/qubes-core-qrexec that referenced this issue May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: core P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. pr submitted A pull request has been submitted for this issue.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants