nixos/security/sgx: init #129432

RealityAnomaly · 2021-07-06T12:30:13Z

Motivation for this change

Adds packages and a module for the Intel SGX (Software Guard Extensions) service, including the SDK and PSW (Platform Software) binaries.

Things done

nixos-discourse · 2021-07-06T12:38:03Z

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/556

pkgs/os-specific/linux/intel-sgx/default.nix

pkgs/os-specific/linux/intel-sgx/sgx1/default.nix

pkgs/os-specific/linux/intel-sgx/dcap/default.nix

pkgs/os-specific/linux/intel-sgx/sgx1/default.nix

pkgs/os-specific/linux/kernel/patches.nix

Mic92

I am not in favor in packaging out-dated kernel drivers in nixpkgs. Kernel modules need a lot of maintenance so the latest version is often necessary. Also this old driver requires applying kernel patches applied to all kernels. SGX also has strict security requirements, which is why I would very much prefer the latest version. I am not aware of version incompatibilities with the later SGX kernel driver. The other components aesmd et all, are fine.

RealityAnomaly · 2021-07-07T19:01:24Z

I am not in favor in packaging out-dated kernel drivers in nixpkgs. Kernel modules need a lot of maintenance so the latest version is often necessary. Also this old driver requires applying kernel patches applied to all kernels. SGX also has strict security requirements, which is why I would very much prefer the latest version. I am not aware of version incompatibilities with the later SGX kernel driver. The other components aesmd et all, are fine.

Since the current isgx driver appears to work fine on my system, I'm going to drop my SGX1 and DCAP commits along with the kernel patch.

nixos/modules/security/sgx.nix

RealityAnomaly · 2021-07-08T22:04:40Z

Did some more testing, and apparently the current version of the isgx module we have is not supported by all platforms, namely, the Intel CPU in my tablet that does not support FLC. The version I had was using the latest unstable master version. I will add a commit to this PR that updates it to the unstable version.

sbellem · 2021-07-10T02:23:10Z

Hi @citadelcore! Thanks for your work! I attempted a pull request (#126990) a few weeks ago for the SGX SDK alone. Our works partially overlap, and it probably will not make sense to keep my PR up.

The main motivation for my approach was reproducibility. Consequently, the IPP Crypto library is built from source, addressing concerns expressed in intel/linux-sgx#363. Also, since recent versions of binutils contain the mitigations that Intel includes in their prebuilt binaries, (see intel/linux-sgx#719 (comment)), I simply used the recent binutils from nixpkgs.

From the point of view of reproducible builds, if the IPP Crypto is not built from source, a verifier will need to use Intel's toolchain to verify the reproducibility of the IPP Crypto, which is very cumbersome. Same reasoning goes for binutils.

RealityAnomaly · 2021-07-10T02:39:18Z

Thanks, this is interesting! I'll look into integrating an ippcrypto derviation into this PR in order to make things reproductible.

sbellem · 2021-07-10T04:44:49Z

Although the IPP Crypto library has its own repository, for the SGX SDK, it is built from the source code of the linux-sgx repository. I wrote a derivation for it at https://github.com/NixOS/nixpkgs/pull/126990/files#diff-bcead6f21cf360352863772a4cc6011209461ecba27e7b5f39540f2c64c26f36.

As mentioned in intel/linux-sgx#719, the header file sgx_ippcp,h is missing. I added it in a linux-sgx fork, for the above derivation, as a proof-of-concept. It seems to me that this is something Intel could address.

You probably won't need this, but just in case it can be useful: https://github.com/initc3/sgx-ipp-crypto gives an idea of what is more or less strictly necessary to build IPP Crypto for SGX.

sbellem · 2021-07-10T05:13:31Z

@citadelcore Out of curiosity, for the sdk, is there a specific reason why you install things "manually" via multiple cp commands as opposed to using the installer generated when invoking make sdk_install_pkg

sdk_install_pkg: sdk
	./linux/installer/bin/build-installpkg.sh sdk cve-2020-0551

I did this in my PR https://github.com/NixOS/nixpkgs/blob/f00ed27355fab3584fa5bba97cac347b0cb81fd7/pkgs/os-specific/linux/sgxsdk/default.nix#L77-L86

I am just curious. I assume you had a good reason to do so, especially given the extra tedious work required.

Mic92 · 2021-07-10T08:04:49Z

pkgs/os-specific/linux/intel-sgx/default.nix

+
+    substituteInPlace psw/ae/aesm_service/source/CMakeLists.txt \
+        --replace '/usr/bin/getconf' '${getconf}/bin/getconf'
+


nitpick to make editorconfig-checker happy:

Suggested change

Mic92 · 2021-07-10T08:11:52Z

pkgs/os-specific/linux/intel-sgx/default.nix

@@ -0,0 +1,296 @@
+{ type, debug ? false }:
+assert builtins.elem type [ "sdk" "psw" ];


Suggested change

assert builtins.elem type [ "sdk" "psw" ];

# SDK: Software development kit for developing enclave applications

# PSW: Platform software to run the enclaves

assert builtins.elem type [ "sdk" "psw" ];

sbellem · 2021-07-10T17:24:51Z

Regarding the sgx version, 2.13.3 is the latest and contains bug fixes.

veehaitch · 2021-08-03T22:26:46Z

FWIW, we created packages for the SGX SDK & PSW and a module as well. I just published our flake: https://github.com/yaxitech/nix-linux-sgx

Maybe it is helpful to advance this PR 🙂

veehaitch

Thanks for your work (and also @sbellem)! Looking forward to having this merged.

veehaitch · 2021-08-03T22:43:35Z

nixos/modules/security/sgx.nix

+        serviceConfig = {
+          ExecStart = "${path}/aesm_service --no-daemon";
+          ExecReload = "${pkgs.coreutils}/bin/kill -HUP $MAINPID";
+          ExecStartPre = [ "${pkgs.coreutils}/bin/mkdir -p /var/opt/aesmd/data /var/opt/aesmd/fwdir/data" ];


You could also leverage systemd sandboxing using StateDirectory and BindPaths to avoid the system-wide FHS path and tmpfiles.

veehaitch · 2021-08-03T22:53:04Z

nixos/modules/security/sgx.nix

+          RuntimeDirectoryMode = "0755";
+          ReadWritePaths = [ "/var/opt/aesmd" ];
+
+          # Hardening


I really appreciate all the effort you've put into hardening. We should definitely have that but I wonder how tried and tested this configuration is as the upstream service definition does not declare any hardening. Do you run this service in production somewhere or can you share any other experiences?

From my own experience with running aesmd and systemd hardening option, the option chosen here looks fine.
It looks scary of the high number but to be honest most services should not do the majority of options that were disabled here. I also prefer remove hardening afterwards rather than breaking existing setups by adding more hardening later.

Upstream probably don't care as much about adding them because they might target some old redhat linux that does not have those hardening options yet.

veehaitch · 2021-08-03T23:05:20Z

pkgs/os-specific/linux/intel-sgx/default.nix

+  installPhase =
+    let build = "build/linux";
+    in
+    if type == "sdk" then ''


Personally, I'd prefer a common base derivation with two distinct derivations over a flag. In fact, the SDK is a dependency of the PSW package and also in general they have different dependencies.

veehaitch · 2021-08-03T23:11:14Z

pkgs/os-specific/linux/intel-sgx/default.nix

+    homepage = "https://01.org/intel-softwareguard-extensions";
+    license = licenses.bsd3;
+    maintainers = with maintainers; [ citadelcore ];
+    platforms = platforms.linux;


The packages also only work on x86_64:

Suggested change

platforms = platforms.linux;

platforms = [ "x86_64-linux" ];

veehaitch · 2021-08-03T23:16:10Z

pkgs/os-specific/linux/intel-sgx/default.nix

+      mkdir -p $out/etc/udev/rules.d
+      cp linux/installer/common/libsgx-enclave-common/91-sgx-enclave.rules $out/etc/udev/rules.d/
+      cp linux/installer/common/sgx-aesm-service/92-sgx-provision.rules $out/etc/udev/rules.d/
+    '';


I like the idea to compile and run some of the SDK samples in simulation mode in the installCheckPhase. That way we can have some confidence the SDK is working.

https://github.com/yaxitech/nix-linux-sgx/blob/7aea4d11910d011b37a3f5bc1daa9307a86b15c2/pkgs/intel-sgx/sdk.nix#L205-L241

sbellem · 2021-08-04T03:58:18Z

Hey @citadelcore, @veehaitch, @arcz, and @SuperSandro2000, I am a bit confused about what to do with my PR at #126990. It has received reviews from @arcz and @SuperSandro2000 which I have tried to address as best as I could so far. @arcz has also been helping to improve the nix pkg for the IPP Crypto library in the original PR (#126990). His proof-of-concept is at https://github.com/arcz/nixpkgs/tree/553f8e22d35ef060f5c4de19d0d0d575206c9a66/pkgs/os-specific/linux/sgx-sdk.

My PR was primarily focused on the SDK and IPP-Crypto library (a dependency), as the goal is to provide a reproducible SDK build, fully built from source, which can be used as a building block in user-defined enclaves, thus allowing these enclaves to also be built reproducibly. This is very important for audits.

After consulting with Intel, I did not bother to work on the PSW, because from the point of view of reproducible enclave builds, the PSW is not involved, as it's a runtime component.

Now, my understanding is that this PR addresses a broader set of concerns, most notably that of making the SGX SDK & PSW available on NixOS, which is great.

It does not however fully address the issue of reproducible builds, as pointed out in comments #129432 (comment) &
#129432 (comment). The IPP Crypto library can be built from source, and from the point of view of reproducible builds and audits my understanding is that it's preferable to do so, as explained in the comments.

So this brings us back to the opening statement of this comment, which is that we have 2 partially overlapping pull requests, both proposing to add a package for the SGX SDK.

Moreover, in addition to the two PRs, we have works in:

It seems to me that a consolidating effort would make sense (?) ... Any feedback welcome!

Please note that I am new to the nix community, and hence I am not aware of how this particular repository manages its very high number of pull requests.

Mic92 · 2021-08-04T04:24:30Z

Hey @citadelcore, @veehaitch, @arcz, and @SuperSandro2000, I am a bit confused about what to do with my PR at #126990. It has received reviews from @arcz and @SuperSandro2000 which I have tried to address as best as I could so far. @arcz has also been helping to improve the nix pkg for the IPP Crypto library in the original PR (#126990). His proof-of-concept is at https://github.com/arcz/nixpkgs/tree/553f8e22d35ef060f5c4de19d0d0d575206c9a66/pkgs/os-specific/linux/sgx-sdk.

I was about to merge this PR when you came up and proposed to build it from source, which @citadelcore also confirmed.
I would be still ok with merging this PR here first and than adding improvements on top. Also your PR is still marked as draft, which usually people don't merge or even look at.

veehaitch · 2021-08-04T12:45:16Z

My PR was primarily focused on the SDK and IPP-Crypto library (a dependency), as the goal is to provide a reproducible SDK build, fully built from source, which can be used as a building block in user-defined enclaves, thus allowing these enclaves to also be built reproducibly. This is very important for audits.

I wholeheartedly agree that having a fully reproducible SDK is very important and that reproducibility didn't yet get the attention it should. Ideally, we'd have bit-perfect reproducibility of the SDK and all its (transitive) dependencies (maybe similar to what we have for the minimal ISO). Therefore, it makes a lot of sense to integrate your efforts and we should continue to go down that road.

Maybe an incremental approach makes sense: First, add working packages for the SDK and PSW. Together with a module, this would already allow anybody to create and test SGX applications on NixOS. This is what the present PR does. Second, make the SDK reproducible. As far as I understand, this also requires upstream changes and, ideally, Intel should also adopt the approach we come up with to supersede the Nix derivation they—after a fashion—currently use. I don't know how eager they are to achieve proper reproducibility with Nix although it makes a lot of sense for all parties involved. Nevertheless, we should push for that. Happy to lend a hand and join forces.

RealityAnomaly · 2021-08-04T16:18:34Z

It doesn't look like it'd be much effort to add reproducibility from @sbellem's PR into mine. As it seems fairly straightforward, I'll do that as soon as I can and update this.

veehaitch · 2021-08-25T13:10:34Z

Any news here, @citadelcore 🙂

Artturin · 2021-10-02T04:50:17Z

fyi #131618 (comment)

Found warning on systemlog:

/nix/store/69fq5zlibddvbx46km0xcn0y3li1bnqf-systemd-249.4/lib/udev/rules.d/50-udev-default.rules:42  Unknown group 'sgx', ignoring

cat /nix/store/69fq5zlibddvbx46km0xcn0y3li1bnqf-systemd-249.4/lib/udev/rules.d/50-udev-default.rules

...
SUBSYSTEM=="misc", KERNEL=="sgx_enclave", GROUP="sgx", MODE="0660"
...

blitz · 2021-10-14T09:21:26Z

nixos/modules/security/sgx.nix

+
+      driver = mkOption {
+        type = types.package;
+        default = pkgs.linuxPackages.isgx;


This needs to be config.boot.kernelPackages.isgx. Otherwise, it will mix up kernel modules and kernel with different versions.

As a side note: Newer kernels come with SGX support built-in, but I'm not sure whether this is a realistic option just yet.

Using the in-tree SGX support works just fine! 🙂

Then we could flip the SGX kernel option on for newer kernels by default and get rid of the third-party module.

I'll have a modern Atom to test this on next week or so.

blitz · 2021-10-14T09:30:29Z

@citadelcore It would be really awesome to get this into nixpkgs! If there is something, you need help with, just say so. :)

tomberek · 2021-11-08T03:56:38Z

I've been seeing this:

Oct 29 09:20:35 tframe systemd-udevd[1214]: /nix/store/27855idyr8dkmh0xrzg7jln7a3fa7viy-systemd-249.4/lib/udev/rules.d/50-udev-default.rules:42 Unknown group 'sgx', ignoring

Is this related? Should there be a users.groups.sgx.gid?

Due to systemd/systemd-stable@c9c4899

SuperSandro2000 · 2021-11-09T21:46:59Z

pkgs/os-specific/linux/intel-sgx/default.nix

+assert builtins.elem type [ "sdk" "psw" ];
+
+{ lib
+, pkgs


Suggested change

, pkgs

SuperSandro2000 · 2021-11-09T21:47:21Z

pkgs/os-specific/linux/intel-sgx/default.nix

+
+  makeFlags = [ type ]
+    # include the path to the SDK for PSW builds which depend on it
+    ++ lib.optional (type == "psw") "SGX_SDK=${pkgs.intel-sgx-sdk}/usr/share/sgxsdk"


Suggested change

++ lib.optional (type == "psw") "SGX_SDK=${pkgs.intel-sgx-sdk}/usr/share/sgxsdk"

++ lib.optional (type == "psw") "SGX_SDK=${intel-sgx-sdk}/usr/share/sgxsdk"

SuperSandro2000 · 2021-11-09T21:48:01Z

pkgs/os-specific/linux/intel-sgx/default.nix

+  buildInputs = [
+    curl
+    openssl
+  ] ++ (lib.optional (type == "psw") pkgs.intel-sgx-sdk);


Suggested change

] ++ (lib.optional (type == "psw") pkgs.intel-sgx-sdk);

] ++ lib.optional (type == "psw") intel-sgx-sdk;

blitz · 2021-11-11T08:43:40Z

pkgs/top-level/all-packages.nix

@@ -20603,6 +20603,9 @@ in

  intel-ocl = callPackage ../os-specific/linux/intel-ocl { };

+  intel-sgx-sdk = callPackage (import ../os-specific/linux/intel-sgx { type = "sdk"; }) { };


With #126990 in the tree, I think this PR now needs to be rebased.

veehaitch · 2021-12-06T11:35:43Z

Following #126990, #148584, and #147693, I created a PR to add the sgx-psw package and the aesmd module: #148593. It's supposed to replace this PR and incorporates some work from it. I'd appreciate your reviews 🙂

blitz · 2021-12-06T12:03:56Z

Following #126990, #148584, and #147693, I created a PR to add the sgx-psw package and the aesmd module: #148593. It's supposed to replace this PR and incorporates some work from it. I'd appreciate your reviews slightly_smiling_face

#148593 looks very good. From my point this PR is obsolete and can be closed.

RealityAnomaly requested review from edolstra, infinisil and nbp as code owners July 6, 2021 12:30

github-actions bot added 6.topic: kernel 6.topic: nixos 8.has: module (update) labels Jul 6, 2021

RealityAnomaly force-pushed the init/sgx branch from 9acb458 to 8685abe Compare July 6, 2021 12:36

mweinelt requested a review from Mic92 July 6, 2021 12:36

ofborg bot added 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500 labels Jul 6, 2021

SuperSandro2000 reviewed Jul 6, 2021

View reviewed changes

Mic92 reviewed Jul 7, 2021

View reviewed changes

pkgs/os-specific/linux/intel-sgx/sgx1/default.nix Outdated Show resolved Hide resolved

Mic92 reviewed Jul 7, 2021

View reviewed changes

pkgs/os-specific/linux/kernel/patches.nix Outdated Show resolved Hide resolved

Mic92 requested changes Jul 7, 2021

View reviewed changes

RealityAnomaly force-pushed the init/sgx branch from 8685abe to 2268dea Compare July 7, 2021 19:05

RealityAnomaly requested a review from ryantm as a code owner July 7, 2021 19:05

github-actions bot added 8.has: changelog 8.has: documentation and removed 6.topic: kernel labels Jul 7, 2021

RealityAnomaly force-pushed the init/sgx branch from 2268dea to bb4d061 Compare July 7, 2021 19:06

ofborg bot added 11.by: package-maintainer 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500 labels Jul 7, 2021

Mic92 reviewed Jul 7, 2021

View reviewed changes

nixos/modules/security/sgx.nix Show resolved Hide resolved

Mic92 reviewed Jul 7, 2021

View reviewed changes

nixos/modules/security/sgx.nix Show resolved Hide resolved

RealityAnomaly added 2 commits July 8, 2021 17:26

intel-sgx: init at 2.13

1da7b6b

nixos/security/sgx: init

319c9ae

RealityAnomaly force-pushed the init/sgx branch from bb4d061 to 319c9ae Compare July 8, 2021 16:28

Mic92 reviewed Jul 10, 2021

View reviewed changes

veehaitch suggested changes Aug 3, 2021

View reviewed changes

sbellem mentioned this pull request Oct 13, 2021

Packaging aesm service initc3/nix-linux-sgx#5

Closed

blitz reviewed Oct 14, 2021

View reviewed changes

SuperSandro2000 reviewed Nov 9, 2021

View reviewed changes

blitz reviewed Nov 11, 2021

View reviewed changes

veehaitch mentioned this pull request Dec 4, 2021

sgx-psw: init package and module #148593

Merged

13 tasks

Artturin mentioned this pull request Dec 4, 2021

nixos: add sgx group with gid 304 #148649

Merged

13 tasks

RealityAnomaly closed this Dec 6, 2021


		substituteInPlace psw/ae/aesm_service/source/CMakeLists.txt \
		--replace '/usr/bin/getconf' '${getconf}/bin/getconf'

		@@ -0,0 +1,296 @@
		{ type, debug ? false }:
		assert builtins.elem type [ "sdk" "psw" ];

	++ lib.optional (type == "psw") "SGX_SDK=${pkgs.intel-sgx-sdk}/usr/share/sgxsdk"
	++ lib.optional (type == "psw") "SGX_SDK=${intel-sgx-sdk}/usr/share/sgxsdk"

	] ++ (lib.optional (type == "psw") pkgs.intel-sgx-sdk);
	] ++ lib.optional (type == "psw") intel-sgx-sdk;

		@@ -20603,6 +20603,9 @@ in

		intel-ocl = callPackage ../os-specific/linux/intel-ocl { };

		intel-sgx-sdk = callPackage (import ../os-specific/linux/intel-sgx { type = "sdk"; }) { };

nixos/security/sgx: init #129432

nixos/security/sgx: init #129432

Conversation

RealityAnomaly commented Jul 6, 2021 • edited Loading

Motivation for this change

Things done

nixos-discourse commented Jul 6, 2021

Mic92 left a comment • edited Loading

Choose a reason for hiding this comment

RealityAnomaly commented Jul 7, 2021

RealityAnomaly commented Jul 8, 2021

sbellem commented Jul 10, 2021

RealityAnomaly commented Jul 10, 2021

sbellem commented Jul 10, 2021

sbellem commented Jul 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbellem commented Jul 10, 2021

veehaitch commented Aug 3, 2021

veehaitch left a comment

Choose a reason for hiding this comment

veehaitch Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbellem commented Aug 4, 2021 • edited Loading

Mic92 commented Aug 4, 2021 • edited Loading

veehaitch commented Aug 4, 2021

RealityAnomaly commented Aug 4, 2021

veehaitch commented Aug 25, 2021

Artturin commented Oct 2, 2021

blitz Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blitz commented Oct 14, 2021

tomberek commented Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperSandro2000 Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veehaitch commented Dec 6, 2021

blitz commented Dec 6, 2021

RealityAnomaly commented Jul 6, 2021 •

edited

Loading

Mic92 left a comment •

edited

Loading

sbellem commented Jul 10, 2021 •

edited

Loading

veehaitch Aug 3, 2021 •

edited

Loading

Mic92 Aug 4, 2021 •

edited

Loading

sbellem commented Aug 4, 2021 •

edited

Loading

Mic92 commented Aug 4, 2021 •

edited

Loading

blitz Oct 14, 2021 •

edited

Loading

tomberek commented Nov 8, 2021 •

edited

Loading

SuperSandro2000 Nov 9, 2021 •

edited

Loading