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

nixos/qgroundcontrol: fix qgroundcontrol module #336183

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Pandapip1
Copy link
Contributor

Description of changes

As it turns out, there were a couple issues with the module I made that for some reason I wasn't able to find. This PR fixes those issues.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/4485

@Pandapip1
Copy link
Contributor Author

Rebased to fix merge conflict.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I lack the knowledge to verify if the c code in securitywrap has any major/glaring issues, so I am not going to approve this PR but from the nix side it looks fine to me.

@Pandapip1
Copy link
Contributor Author

Moving to draft temporarily to test new changes to securitywrap.

@Pandapip1 Pandapip1 marked this pull request as draft September 18, 2024 22:10
@Pandapip1
Copy link
Contributor Author

Apparently glibc was recently updated. See you in however long it takes to upgrade my system!

@Pandapip1
Copy link
Contributor Author

Re-tested; works

@Pandapip1 Pandapip1 marked this pull request as ready for review September 18, 2024 23:50
@philiptaron philiptaron removed their request for review September 24, 2024 16:04
@winterqt
Copy link
Member

I'm not entirely a fan of this homegrown wrapper stuff. @NixOS/security wdyt?

@Pandapip1
Copy link
Contributor Author

Would you prefer if I moved it in-tree?

@LeSuisse
Copy link
Contributor

I'm not entirely a fan of this homegrown wrapper stuff. @NixOS/security wdyt?

Also not a big fan of having something called mkSecurityWrapper maintained out of tree (it makes things annoying to audit when needed) and it does not look like something that is or should be often needed.

In this case do we need it? From a quick glance qgroundcontrol seems to do quite a lot so the corresponding attack surface is not small. I'm not sure if we are gaining a lot from a security perspective by adding complexity to run this app in the dialout group. It feels like

Maybe it would be preferable to document the possible solutions and let the end users do the choice that best fit their needs. Most likely it is either going to be:

  • write your own udev rule to let your standard user access your device
  • add your user to the dialout group because you do not care about restricting it further

@Pandapip1
Copy link
Contributor Author

it does not look like something that is or should be often needed

Was planning on also using it to address some of the bubblewrap related FHSenv issues.

I'd like the option to be there if a user chooses to enable it (for example, if it's being run on a workstation with lots of users that are expected to run qgc but don't need full dialout) although I equally understand why a user might not want to have qgroundcontrol run as dialout.

I'll change that option to be default false, if it isn't already, and split the PR into a fix and the securitywrap-enhanced fix + some related fhsenv fixes if I can get them working.

@Pandapip1
Copy link
Contributor Author

Removed the securitywrap stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants