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

xen: remove stubdomains #TODO note #343111

Merged
merged 1 commit into from
Sep 21, 2024
Merged

Conversation

SigmaSquadron
Copy link
Contributor

@SigmaSquadron SigmaSquadron commented Sep 19, 2024

Description of changes

MiniOS stubdomains are legacy and on their way to being replaced by unikraft. There's no need to work towards them.

Things done


Add a 👍 reaction to pull requests you find important.

Copy link
Member

@CertainLach CertainLach left a comment

Choose a reason for hiding this comment

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

Other than that - LGTM.
Can't say anything about functionality, as I haven't used it despite implementing in my PR.

pkgs/applications/virtualization/xen/update.sh Outdated Show resolved Hide resolved
@SigmaSquadron

This comment was marked as outdated.

@SigmaSquadron SigmaSquadron added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 20, 2024
Comment on lines 256 to 262
gmp = extFile "gmp" ".tar.bz2";
lwip = extFile "lwip" ".tar.gz";
newlib = extFile "newlib" ".tar.gz";
pciutils = extFile "pciutils" ".tar.bz2";
polarssl = extFile "polarssl" "-gpl.tgz";
tpm_emulator = extFile "tpm_emulator" ".tar.gz";
zlib = extFile "zlib" ".tar.gz";
Copy link
Member

Choose a reason for hiding this comment

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

Do these absolutely need to be vendored? Obviously I don’t really know what’s going on here but I would much prefer to use the Nixpkgs versions of these if possible, especially if we’re having to patch their build systems again anyway. I guess we’re invoking their own Buildroot type thing to build their own Linux tree here? It’d be really nice if we could at least use our own .srcs. Happy to hear an explanation of what’s going on here and why this is a dumb request though :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do these absolutely need to be vendored?

They do, unfortunately. MiniOS stubdomains require ancient versions of all of these libraries, and Xen patches them up as needed during the build process. I tried using nixpkgs' .srcs1, but the build naturally failed as the patches Xen applies did not match the new versions' indexes.

I guess we’re invoking their own Buildroot type thing to build their own Linux tree here?

More accurately, we're sneaking a whole new (non-Linux) kernel build in here.

Happy to hear an explanation of what’s going on here and why this is a dumb request though

One of Xen's exclusive features is the stubdomain system. They're tiny, lightweight VMs that take away some of dom0's privilege over the system's hardware. You could have a stubdomain that runs the QEMU device model (xen.qemu-system-i386), removing the need for running it in dom0, thus improving the system's security by isolating this attack vector from the host.
MiniOS is one of the operating system/microkernel that runs inside a stubdomain, and is what Xen builds by default. One could also use a Linux-based or MirageOS-based stubdomain.

I'll try asking on XenDevel if building MiniOS with ancient dependencies is still relevant, and see what they answer.

Footnotes

  1. I tried doing this by adding a hacky function that takes the ${source}.src, calls tar to package them using the correct file extensions, and copy them to the right places in the build's working directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, looks like MiniOS is considered legacy, and is being replaced by unikraft. We can still build it, but it should at the very least be a disabled-by-default component.

@CertainLach are you sure this is necessary for Qubes, considering ITL uses Linux-based stubdomains?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the detailed explanation! Having to bundle these old library versions for their bespoke system build system sucks but it sounds like it’s unavoidable at least for now if we want to package MiniOS. However, if it’s deprecated and being replaced, then I might prefer to dodge this ugliness by skipping packaging it altogether in favour of the newer stuff, as long as nothing like Qubes actually needs it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure Qubes uses it. https://github.com/QubesOS/qubes-vmm-xen-stubdom-linux

If there are no objections from lach, I'd be in favour of closing this. On my personal Xen deployment, I use Linux-based stubdoms anyway.

@digitalrayne do you think there's a need for MiniOS-based stubdoms?

Copy link
Contributor Author

@SigmaSquadron SigmaSquadron Sep 20, 2024

Choose a reason for hiding this comment

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

Still, I believe NixOS Xen needs to be secure OOTB, and stubdom isolation is a good thing, if MiniOS-based stubdoms are only official way to do that - I think it would be better to have it?

{Linux,MirageOS,Unikraft}-based stubdomains are also a valid way to ensure device isolation. Since they're a legacy component, we can assume that Xen itself doesn't want them being used anymore.

Copy link
Member

Choose a reason for hiding this comment

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

This uses a TLS library from 2012, an IP stack from 2008, and a Zlib from 2005. All of those projects have had CVEs since those releases. I’d be more inclined to mark MiniOS with knownVulnerabilities than to enable it by default for security reasons.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that it is ancient, the only remaining question from me is

Does Xen even uses it by default when built with stubdom target?

If not and it needs further configuration anyway - then sure, kill it with fire.

Copy link
Contributor Author

@SigmaSquadron SigmaSquadron Sep 20, 2024

Choose a reason for hiding this comment

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

I don't know about MiniOS alternatives, correct me if I'm wrong. Does Xen even uses it by default when built with stubdom target?

All the stubdom target builds are the four .gz files listed in passthru. You can optionally use them to run pre-built stubdomains by calling them in an xl configuration. They're technically a default, but a default that shouldn't be used.

Copy link

@digitalrane digitalrane Sep 21, 2024

Choose a reason for hiding this comment

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

@SigmaSquadron - I agree that it makes sense to remove support for MiniOS stubdomains. I think it is added maintenance burden for a fairly niche use case, I'm not aware of these being used by any default functionality, especially in the "NixOS as Dom0" use case. I also think that @emilazy makes an excellent point around the additional security overhead of carrying this and making it available. If it were ever re-added I absolutely agree it should be marked with knownVulnerabilities.

@SigmaSquadron SigmaSquadron marked this pull request as draft September 20, 2024 13:50
@SigmaSquadron SigmaSquadron added 2.status: work-in-progress This PR isn't done and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Sep 20, 2024
@SigmaSquadron SigmaSquadron changed the title xen: build stubdomains xen: remove stubdomains #TODO note Sep 20, 2024
@SigmaSquadron SigmaSquadron marked this pull request as ready for review September 20, 2024 23:37
@SigmaSquadron SigmaSquadron removed the 2.status: work-in-progress This PR isn't done label Sep 20, 2024
It has been decided in NixOS#343111 that we should not build MiniOS
stubdomains, and instead use Linux-based ones for now.

Once MirageOS or Unikraft is packaged, we can switch to using those
microkernels as stubdomains.

Signed-off-by: Fernando Rodrigues <[email protected]>
@SigmaSquadron
Copy link
Contributor Author

@ofborg build xen

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild and removed 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Sep 21, 2024
@SigmaSquadron
Copy link
Contributor Author

Anyone interested in MiniOS may find this branch useful: https://github.com/SigmaSquadron/nixpkgs/tree/xen-stubdomains-legacy

@emilazy
Copy link
Member

emilazy commented Sep 21, 2024

Love it when a PR gets smaller :)

@emilazy emilazy merged commit 6a1ffee into NixOS:master Sep 21, 2024
25 of 27 checks passed
@SigmaSquadron SigmaSquadron deleted the xen-stubdomains branch September 22, 2024 15:54
@SigmaSquadron SigmaSquadron added the 6.topic: xen-project The Xen Project hypervisor label Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: xen-project The Xen Project hypervisor 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants