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

sgx-psw: init package and module #148593

Merged
merged 6 commits into from
Dec 13, 2021
Merged

sgx-psw: init package and module #148593

merged 6 commits into from
Dec 13, 2021

Conversation

veehaitch
Copy link
Member

@veehaitch veehaitch commented Dec 4, 2021

Motivation for this change
  • sgx-sdk: create sgx dir and move
  • sgx-psw: init at 2.14.100.2
  • sgx-sdk, sgx-psw: add debug argument
  • nixos/sgx: add module
  • nixosTests.aesmd: init

Supersedes #129432

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.05 Release Notes (or backporting 21.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 4, 2021
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Dec 4, 2021
@ofborg ofborg bot requested a review from RealityAnomaly December 4, 2021 14:50
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 4, 2021
@veehaitch
Copy link
Member Author

# nix develop "github:yaxitech/nix-linux-sgx/sgxs-tools" --command "sgx-detect"
SGX_SDK         = /nix/store/m3sna34srcj8s2q98kcz43aly57wxynk-sgx-sdk-2.14.100.2
SGX_PSW         = /nix/store/17zbdsmmc1w11maxqbhff4i6wc7bn5ck-sgx-psw-2.14.100.2
LD_LIBRARY_PATH = /nix/store/17zbdsmmc1w11maxqbhff4i6wc7bn5ck-sgx-psw-2.14.100.2/lib:/nix/store/m3sna34srcj8s2q98kcz43aly57wxynk-sgx-sdk-2.14.100.2/lib
Detecting SGX, this may take a minute...
✔  SGX instruction set
  ✔  CPU support
  ✔  CPU configuration
  ✔  Enclave attributes
  ✔  Enclave Page Cache
  SGX features
    ✘  SGX2  ✘  EXINFO  ✘  ENCLV  ✘  OVERSUB  ✘  KSS
    Total EPC size: 93.5MiB
✔  Flexible launch control
  ✔  CPU support
  ? CPU configuration
  ✔  Able to launch production mode enclave
✔  SGX system software
  ✔  SGX kernel device (/dev/sgx/enclave)
  ✔  libsgx_enclave_common
  ✔  AESM service
  ✔  Able to launch enclaves
    ✔  Debug mode
    ✔  Production mode
    ✔  Production mode (Intel whitelisted)

You're all set to start running SGX programs!

@veehaitch veehaitch requested a review from Mic92 December 4, 2021 22:46
pkgs/os-specific/linux/sgx/psw/default.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/sgx/psw/default.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/sgx/psw/default.nix Outdated Show resolved Hide resolved
Copy link
Contributor

@blitz blitz left a comment

Choose a reason for hiding this comment

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

This looks very nice! Thanks for bringing SGX forward on NixOS. 👍

Copy link
Member

@RealityAnomaly RealityAnomaly left a comment

Choose a reason for hiding this comment

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

LGTM


users.groups = {
"${cfg.group}" = { };
"sgx_prv" = { };
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still curious about https://github.com/systemd/systemd-stable/blob/main/rules.d/50-udev-default.rules.in#L43 and if we need the "sgx" group or how "sgx_prv" is different.

Copy link
Member Author

@veehaitch veehaitch Dec 6, 2021

Choose a reason for hiding this comment

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

The sgx group is for managing access to the application enclave device, i.e., /dev/sgx_enclave. The sgx_prv group is for the provisioning device, i.e., /dev/sgx_provision. Using the group sgx for sgx_enclave and the group sgx_prv for sgx_provision is a convention which we should follow.

Access to the provisioning device implies access to the CPU-unique provisioning certificate key. Therefore, a user which is allowed to create application enclaves usually should not be allowed to create provisioning enclaves. The AESM service is a privileged exception as it provides remote attestation.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the right module in which to create the sgx group? I keep bringing this up because the existence or nonexistence of this group caused some problems in systemd startup in stage2 when there were differing bios settings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Before #148649, we did not create the sgx group for the systemd-bundled udev rule you mentioned above. This led to a warning during boot which is now resolved. We currently take care of the sgx_prv group in this PR only: create the group (the position you annotated) and the AESM-bundled udev rules assign permissions and create symlinks.

As an alternative, we could take an approach similar to #148649 and create the sgx_prv and the respective udev rule (change group to sgx_prv, assign permissions 0660) independent of having the AESM service enabled or not. I think that would make sense and would leave us with the following permissions:

# stat -c '%U %G %a %n' /dev/sgx_*
root sgx 660 /dev/sgx_enclave
root sgx_prv 660 /dev/sgx_provision

Anyone who should be allowed to create application enclaves joins the sgx group. Anyone who should be allowed to create provisioning enclaves (for example the AESM service of this PR) joins the sgx_prv group.

Copy link
Member Author

@veehaitch veehaitch Dec 7, 2021

Choose a reason for hiding this comment

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

Maybe something like veehaitch@0666d1b?

See latest commits.

@blitz
Copy link
Contributor

blitz commented Dec 6, 2021

Note for other reviewers: This needs the changes from #147693 to work. It has already been merged, but still sits in staging.

@veehaitch
Copy link
Member Author

veehaitch commented Dec 6, 2021

Note for other reviewers: This needs the changes from #147693 to work. It has already been merged, but still sits in staging.

If you expect a fully working setup on your SGX-capable bare-metal hardware by just setting services.aesmd.enable = true in your NixOS configuration, you need #147693, that's right. I, for example, also run the AESM service within a systemd-nspawn container on a non-NixOS host which uses the DCAP driver. The host binds /dev/sgx_{enclave,provision} to the container and, therefore, does not need kernel support for it.

This PR is certainly more useful with #147693 being part of the master branch but I wouldn’t say it's blocked by it.

@blitz
Copy link
Contributor

blitz commented Dec 7, 2021

@veehaitch True, given that everything comes together anyhow when staging is merged, there is no reason to keep this PR sitting around. :)

Meanwhile, I've been able to test this on a Intel Celeron J4005 and everything seems to be working just fine! 🥳

Detecting SGX, this may take a minute...
✔  SGX instruction set
  ✔  CPU support
  ✔  CPU configuration
  ✔  Enclave attributes
  ✔  Enclave Page Cache
  SGX features
    ✔  SGX2  ✔  EXINFO  ✘  ENCLV  ✘  OVERSUB  ✘  KSS  
    Total EPC size: 94.0MiB
✔  Flexible launch control
  ✔  CPU support
  ? CPU configuration
  ✔  Able to launch production mode enclave
✔  SGX system software
  ✔  SGX kernel device (/dev/sgx/enclave)
  ✔  libsgx_enclave_common
  ✔  AESM service
  ✔  Able to launch enclaves
    ✔  Debug mode
    ✔  Production mode
    ✔  Production mode (Intel whitelisted)

You're all set to start running SGX programs!

@veehaitch
Copy link
Member Author

Following a discussion with @tomberek, I decided to take a different approach regarding the users and groups involved. I considered opening another PR following the merge of this one but I think it makes sense to discuss and ideally incorporate the changes in this PR already:

  • Add a new config options attribute hardware.cpu.intel.sgx.provision. If enabled = true, it installs a udev rule to set the user, group and mode for the SGX provisioning device (/dev/sgx_provision). The user defaults to root, the group to sgx_prv and the mode to 0660. As a result, a user has to join the configured group to access the provisioning device. I didn't deem a dedicated release note entry necessary for the module. I'm happy to create one if you want me to.
  • Cherry-picked nixos: add sgx group with gid 304 #148649 (currently not part of the PR's base) which creates and assigns the group sgx to the sgx_enclave device with permissions 0660. That means, a user has to join the sgx group to create SGX application enclaves.
  • To reflect these changes, the AESM service now runs as a dynamically allocated systemd user (DynamicUser=) with a primary group of sgx and the supplementary group sgx_prv. The runtime directory is set to 0750 to limit access to the AESM UNIX socket to members of the sgx group. Set the state directory permissions to 0700 to only allow the service user to access its contents.
  • As a result, we don't need the AESM-bundled udev rules anymore. Those rules set the sgx_enclave device mode to 0666 allowing anybody to create SGX application enclaves. This is actually a bit more liberal than I'd like and also overrides the systemd mode of 0660. The recent changes keep the systemd udev rule for sgx_enclave and adapt a similar approach for sgx_provision.

I'm going to rebase those commits as soon as I heard back from you. I didn't do that yet to make it easier to review the latest changes.

@veehaitch veehaitch requested a review from blitz December 8, 2021 13:20
@blitz
Copy link
Contributor

blitz commented Dec 8, 2021

@veehaitch Sounds reasonable to me. 👍

@veehaitch veehaitch mentioned this pull request Dec 10, 2021
13 tasks
@veehaitch
Copy link
Member Author

I'm going to rebase those commits as soon as I heard back from you. I didn't do that yet to make it easier to review the latest changes.

Rebased.

Copy link
Contributor

@blitz blitz left a comment

Choose a reason for hiding this comment

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

👍

@veehaitch
Copy link
Member Author

I think we're good to go. Any thoughts, @Mic92?

@Mic92 Mic92 merged commit afa3c99 into NixOS:master Dec 13, 2021
@Mic92
Copy link
Member

Mic92 commented Dec 13, 2021

Thanks. Looks solid!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 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.

6 participants