Conversation
8ecf46e to
ab8b4f1
Compare
arcz
left a comment
There was a problem hiding this comment.
Great work, thanks! I also wanted to add sgx support to nixpkgs to have full reproducibility for enclaves so I'll help to get this through.
If the sgxsdk derivation already builds ippcrypto I think we could have one derivation with two outputs to avoid double work.
A couple of cosmetic comments for now. I'll try it build the derivations and see how it goes.
There was a problem hiding this comment.
Can we use the intel repository and add the changes as patch? It's more transparent this way. If you opened a PR it can be downloaded as a patch.
There was a problem hiding this comment.
maybe override installPhase instead?
There was a problem hiding this comment.
Please split this into nativeBuildInputs.
There was a problem hiding this comment.
Please devendor the libs.
There was a problem hiding this comment.
@SuperSandro2000 I am not exactly sure on how to go about to do this. Just to make sure I understand, do you mean that we should not rely on using the dependencies specified in .gitmodules, but instead fetch these dependencies from nixpkgs?
Concretely speaking, there are 4 submodules, and 2 of them are by Intel (dcap & ipp-crypto), meanwhile the other 2 are openmp (not sure about the version) and oneDNN. In both cases, (openmp and onednn), Intel builds and patches them in their own "custom" way, with an external Makefile maintained under the linux-sgx repo, which itself depends on a buildenv.mk file maintained under the same repo.
Not sure if this information helps, and is relevant, but if you could provide a bit of details about what "devendoring" would mean in this context, that would be helpful. Thanks!
There was a problem hiding this comment.
| ''; | |
| ''; |
There was a problem hiding this comment.
| export BINUTILS_DIR=$binutils/bin | |
| export BINUTILS_DIR=${binutils}/bin |
|
Thank you very much @arcz and @SuperSandro2000 for the feedback! I started to address the comments. Changes are in commit 9907cf0ad2348473b44d6f967dd2c4b1b6f9022a. (I'll rebase once things are finalized) Also note that I updated the version to 2.14, as it is the latest release. I have yet to address the following:
|
Yes, I agree, this makes sense given the current way that IPP Crypto is built for SGX. Another approach would be to build the IPP Crypto independently, and to use it as a dependency when building the SDK. The advantage of doing so is that building the SDK would be much faster because building the IPP Crypto library takes the most time: ~1h45m e.g.: https://github.com/sbellem/nix-linux-sgx/actions/runs/1051408124. /cc @arcz |
There was a problem hiding this comment.
Why are we not using makeFlags?
There was a problem hiding this comment.
Good question. I guess there's a much better and cleaner way to this. The above builds the IPP Crypto library multiple times, and therefore make clean must be invoke in-between each build. Each build addresses a different mitigation or no mitigation at all.
I will look into a better approach to this. If you have suggestions please let me know.
As suggested by @arcz, it may perhaps be best to modify the derivation such that it has multiple outputs. See also related #126990 (comment).
There was a problem hiding this comment.
I am leaning towards building the ipp-crypto library in a separate derivation, such as in 0de316415d627c908c744279d6a5d7046840f4db.
Doing so somewhat reflects how Intel uses prebuilt binaries as dependencies. In other words, it would be similar to building the SGX SDK with Intel's prebuilt IPP Crypto as in https://github.com/sbellem/nix-sgx-sdk/blob/6360427aaa396d77843eb58e1db6d89884eba04a/sgxsdk-with-prebuilt-ipp.nix#L5-L8
ipp_crypto = fetchurl {
url = "https://download.01.org/intel-sgx/sgx-linux/2.13.3/optimized_libs_2.13.3.tar.gz";
sha256 = "f46aceac799e546e5c01e484d7f7c01b34c1e1d79469600f86da2bd5b3ce7ad4";
};except that it would be built with the ipp-crypto from the nixpkgs as in https://github.com/sbellem/nix-linux-sgx/blob/sdk-with-nixpkg-ipp-dep/sdk.nix:
{ lib
# ...
, ippcrypto
}:
# ...
preBuild = ''
export BINUTILS_DIR=${binutils}/bin
cp -r ${ippcrypto}/* ./external/ippcp_internal/
'';
buildFlags = [ "sdk_install_pkg" ];
# .../cc @arcz
There was a problem hiding this comment.
@sbellem I've looked into that yesterday and did some experiments and indeed it would be better to build it separately. I've made a simple derivation for ipp-crypto parameterized by extra cmake flags for versions with mitigations. The good thing about building from ipp-crypto repo is that cmake build is automatically parallelised and runs a lot faster. This can be also observed by patching https://github.com/intel/linux-sgx/blob/master/external/ippcp_internal/Makefile#L81 adding -j$NIX_BUILD_CORES to make ippcp_s. I resigned from using the ippcp_internal/Makefile though and replicate it's function inside the sgx-sdk derivation. I didn't finish the whole build yet but I can push my solution so it's clear what I'm talking about.
There was a problem hiding this comment.
Hey @arcz! That sounds good! Are you building from intel/ipp-crypto or from the submodule under intel/linux-sgx? At first I thought that it could perhaps be clearer to build from intel/ipp-crypto, but the problem is that building the mitigations (https://github.com/intel/linux-sgx/blob/master/external/ippcp_internal/Makefile#L53-L59), i.e.:
ifeq ($(MITIGATION-CVE-2020-0551), LOAD)
SUB_DIR = cve_2020_0551_load
PRE_CONFIG= ASM_NASM="python $(DIR)/../../build-scripts/sgx-asm-pp.py --assembler=nasm --MITIGATION-CVE-2020-0551=LOAD"
else ifeq ($(MITIGATION-CVE-2020-0551), CF)
SUB_DIR = cve_2020_0551_cf
PRE_CONFIG= ASM_NASM="python $(DIR)/../../build-scripts/sgx-asm-pp.py --assembler=nasm --MITIGATION-CVE-2020-0551=CF"
endifdepends on code that is under the intel/linux-sgx repo, namely build-scripts/sgx-asm-pp.py.
I initially even extracted what I think is just necessary to build IPP-Crypto for SGX into a standalone repo at https://github.com/initc3/sgx-ipp-crypto, but it does not make sense from a maintenance point of view. So I figured that it's probably best to build IPP-Crypto for SGX from the intel/linux-sgx repo. I plan to talk with Intel about this to see if the build process could somehow be improved.
In any case, I am looking forward to see the approach you've taken.
There was a problem hiding this comment.
I resigned from using the
ippcp_internal/Makefilethough and replicate it's function inside the sgx-sdk derivation.
So, are you fetching sgx-asm-pp.py, and buildenv.mk independently, and then replicating the steps of the ippcp_internal/Makefile in the derivation?
There was a problem hiding this comment.
@sbellem Here is how it looks like arcz@553f8e2. Please keep in mind this is a WIP code. The sgx-sdk derivation doesn't build (not really sure why yet) but three versions of ipp-crypto build successfully. I don't know if this is the way to go, it's more of an experiment, maybe @SuperSandro2000 can advise here.
There was a problem hiding this comment.
Hey @arcz, looks interesting, and much simpler! What is the error that you are getting when building the sdk? I have been getting a permission denied error: https://github.com/sbellem/nix-linux-sgx/runs/3131321998?check_suite_focus=true#step:5:12314
There was a problem hiding this comment.
I'll be looking into this more next week
There was a problem hiding this comment.
@sbellem Got it building but I had to patch a few more things, I'll clean it up and push. I encountered the same error on the way.
There was a problem hiding this comment.
Split this into nativeBuildInputs.
There was a problem hiding this comment.
Fetch commits instead of PRs.
There was a problem hiding this comment.
| prePatch = '' | |
| patchShebangs ./linux/installer/bin/build-installpkg.sh | |
| patchShebangs ./linux/installer/common/sdk/createTarball.sh | |
| patchShebangs ./linux/installer/common/sdk/install.sh | |
| postPatch = '' | |
| patchShebangs linux/installer/bin/build-installpkg.sh \ | |
| linux/installer/common/sdk/{createTarball.sh,install.sh} |
There was a problem hiding this comment.
| { lib, | |
| stdenvNoCC, | |
| fetchpatch, | |
| fetchurl, | |
| fetchFromGitHub, | |
| autoconf, | |
| { lib | |
| , stdenvNoCC | |
| , fetchpatch | |
| , fetchurl | |
| , fetchFromGitHub | |
| , autoconf |
and so on
There was a problem hiding this comment.
| { lib, | |
| stdenvNoCC, | |
| fetchpatch, | |
| fetchurl, | |
| fetchFromGitHub, | |
| autoconf, | |
| automake, | |
| { lib | |
| , stdenvNoCC | |
| , fetchpatch | |
| , fetchurl | |
| , fetchFromGitHub | |
| , autoconf | |
| , automake |
and so on
pkgs/top-level/all-packages.nix
Outdated
There was a problem hiding this comment.
Please sort those entries and don't append them to the end of the file.
There was a problem hiding this comment.
| buildFlags = if mitigation != null then [ "MITIGATION-CVE-2020-0551=$(mitigation)" ] else []; | |
| buildFlags = lib.optionals (mitigation != null) [ "MITIGATION-CVE-2020-0551=$(mitigation)" ]; |
|
@sbellem I have the build working, here is how it looks like https://github.com/arcz/nixpkgs/blob/sgx-sdk-wip/pkgs/os-specific/linux/sgx-sdk/default.nix. Can you confirm it's working for you? |
Hey @arcz! Sorry for the delay, I'll look at this asap! |
|
|
||
| cmakeFlags = [ "-DARCH=intel64" ] ++ extraCmakeFlags; | ||
|
|
||
| nativeBuildInputs = [ cmake python3 nasm ]; |
There was a problem hiding this comment.
this derivation is internal to sgx-sdk, should we still add meta here?
|
|
||
| dontConfigure = true; | ||
|
|
||
| # SDK built with stackprotector produces broken encalves which crash at runtime |
6b53d17 to
b4c3230
Compare
|
@SuperSandro2000 I think I addressed all of your comments. Regarding devendoring, I'm not sure if it makes sense here as dependencies in git submodules are patched so they have to be built anyway. We could look into that in the next releases. |
|
|
||
| dontConfigure = true; | ||
|
|
||
| # SDK built with stackprotector produces broken encalves which crash at runtime. |
There was a problem hiding this comment.
| # SDK built with stackprotector produces broken encalves which crash at runtime. | |
| # SDK built with stackprotector produces broken enclaves which crash at runtime. |
I'm also not sure what would be served by separating these dependencies. From my perspective, it would only make updates to this derivation harder. |
NixOS is a distro and using source code for the same project from n sources makes patching impossible. There should be only one source which gets patched once to fix a bug/vurnability in every package. |
|
The only submodule for SDK 2.14 appears to be |
If the headers come with the patch I don't understand the problem. Applying the patch to nixpkgs openmp would be the best solution to collect potential other patches. |
I did a poor job at explaining that. The dependency on headers ( If this is non-negotiable I can look how to hack the build on top of the current package but it will be non-trivial. It's also worth mentioning that we don't do any worse than the Intel build since openmp builds to a static lib and is shipped with the SDK. |
|
@SuperSandro2000 any thoughts? |
|
If it is not that trivial we can keep it vendored I guess. |
For the sgxsdk and ippcrypto packages
Co-authored-by: Sylvain Bellemare <sbellem@gmail.com> Co-authored-by: Artur Cygan <arczicygan@gmail.com>
|
@SuperSandro2000 I rebased our work, please let us know if there is anything else to fix |
@arkivm How about we have both? In the context I work in, having v2.14 available would be convenient as some prototypes have not been upgraded to v2.15. I think we definitely want to have v2.15 as soon as possible as well. Just my thoughts. |
|
@sbellem I'm ok with either one of it. Just wanted avoid going through another PR cycle for a version upgrade as v2.15 is already out. |
|
I think we can get this one merged and we will work on 2.15. The update won't take as long as this PR and there will be already something to work with while we work on 2.15. |
|
Awesome to have this merged! Thanks a lot @sbellem and @arcz. I wonder a bit about the directory structure. The resulting derivation is organized as follows: # nix build 'github:nixos/nixpkgs/c5ed8beb478a8ca035f033f659b60c89500a3034#sgx-sdk'
# tree -L 2 result/
result/
└── sgxsdk
├── bin
├── buildenv.mk
├── environment
├── include
├── lib64
├── licenses
├── pkgconfig
├── SampleCode
├── sdk_libs
└── uninstall.shAssuming that I want to create a Nix derivation which leverages the I'd propose to alter the directory structure of the output derivation to what Nix expects. To keep compatibility to SGX documentation and examples, we could still add some symlinks. For
|
veehaitch
left a comment
There was a problem hiding this comment.
Sorry, I'm late to the party. Maybe something to consider in the PR for 2.15
| (fetchpatch { | ||
| name = "sgx_ippcp.h.patch"; | ||
| url = "https://github.com/intel/linux-sgx/commit/e5929083f8161a8e7404afc0577936003fbb9d0b.patch"; | ||
| sha256 = "12bgs9rxlq82hn5prl9qz2r4mwypink8hzdz4cki4k4cmkw961f5"; | ||
| }) |
There was a problem hiding this comment.
I think you could also symlink linux-sgx/external/epid-sdk/ext/ipp/include/sgx_ippcp.h, couldn't you?
Anyways, I would recommend to commit the patches. Currently, they appear more official than they are; in fact, those patches are part of your pending PR.
| (fetchpatch { | ||
| name = "replace-bin-cp-with-cp.patch"; | ||
| url = "https://github.com/intel/linux-sgx/commit/e0db5291d46d1c124980719d63829d65f89cf2c7.patch"; | ||
| sha256 = "0xwlpm1r4rl4anfhjkr6fgz0gcyhr0ng46fv8iw9hfsh891yqb7z"; | ||
| }) |
There was a problem hiding this comment.
substituteInPlace buildenv.mk --replace '/bin/cp' 'cp'| # build faster by splitting different versions of ipp-crypto builds and to | ||
| # avoid patching the Makefile for reproducibility issues. | ||
| buildPhase = let | ||
| ipp-crypto-no_mitigation = callPackage (import ./ipp-crypto.nix) {}; |
There was a problem hiding this comment.
| ipp-crypto-no_mitigation = callPackage (import ./ipp-crypto.nix) {}; | |
| ipp-crypto-no_mitigation = callPackage ./ipp-crypto.nix { }; |
| sgx-asm-pp = "python ${src}/build-scripts/sgx-asm-pp.py --assembler=nasm"; | ||
|
|
||
| nasm-load = writeShellScript "nasm-load" "${sgx-asm-pp} --MITIGATION-CVE-2020-0551=LOAD $@"; | ||
| ipp-crypto-cve_2020_0551_load = callPackage (import ./ipp-crypto.nix) { |
There was a problem hiding this comment.
| ipp-crypto-cve_2020_0551_load = callPackage (import ./ipp-crypto.nix) { | |
| ipp-crypto-cve_2020_0551_load = callPackage ./ipp-crypto.nix { |
| }; | ||
|
|
||
| nasm-cf = writeShellScript "nasm-cf" "${sgx-asm-pp} --MITIGATION-CVE-2020-0551=CF $@"; | ||
| ipp-crypto-cve_2020_0551_cf = callPackage (import ./ipp-crypto.nix) { |
There was a problem hiding this comment.
| ipp-crypto-cve_2020_0551_cf = callPackage (import ./ipp-crypto.nix) { | |
| ipp-crypto-cve_2020_0551_cf = callPackage ./ipp-crypto.nix { |
|
@veehaitch those are really good advices, thanks! I agree, the structure definitely needs some work and the derivation generally could be better. We can work on that in the next PRs. |
Motivation for this change
Add a derivation for Intel's SGX SDK, which is a fundamental building block for enclave applications, aimed at Intel SGX -enabled computers. Trusted hardware such as Intel SGX allows user to have confidence, (under certain assumptions), that a remote computer is running the software they expect. In order to gain trust that the remote computer is indeed running the expected software, reproducible builds are required. A verifying party uses the trusted software code to reproduce an enclave build to check it against the build that is running on the remote computer, through a process known as remote attestation. This is where
nixcome into the picture asnixallows us to build software in a reproducible way, hence the motivation for this pull request.Background
The SGX SDK depends on a cryptographic library, named
ipp-cryptowhich takes around 1h30 to build. The current build toolchain provided by Intel uses prebuilt binaries, although it's possible to build it from source. The SGX SDK could be built in roughly 3 ways:Makefileand patch to build the IPP Crypto for SGX.The current draft PR contains a nix derivation to build the IPP Crypto library for SGX as a standalone package, as a demonstration and for test purposes. There's a nix derivation for the SGX SDK that follows point 2 above, meaning that both the IPP Crypto dependency and the SGX SDK are built with the same nix derivation. It should be noted that when it is built for the purpose of SGX, the IPP Crypto library appears to have a slightly different build configuration. This needs to be clarified with Intel's Linux SGX team, but is nevertheless quite clear from the
Makefile, patch and python script used that appear to be specific to SGX. (See https://github.com/intel/linux-sgx/tree/master/external/ippcp_internal for the details of how IPP Crypto is build from the linux-sgx repository, and https://github.com/initc3/sgx-ipp-crypto for an isolation of what appears to be strictly necessary to build IPP Crypto). Currently, it seems that most users do not build the IPP Crypto library from source but instead use the pre-built binaries, although some users have expressed disappointment with this as they would like to have a way to build the SGX SDK from components that are all open source. Whether the SGX SDK is built following the approaches in point 2 or 3, the current toolchain of the linux-sgx repository will need some minor changes to remove the current hard requirement of downloading pre-built binaries. This PR, for demonstration and test purposes is using a fork of the current linux-sgx repository, in which these minor changes have been implemented. This will need to go through Intel's team for approval before Intel's repository can be used.To sum up, things to clarify:
If the IPP Crypto is built separately:
buildInputs) listed in the current derivation (ippcrypto/default.nix) need to be reviewed as many are perhaps superfluous.Preliminary Build Tests
Some preliminary tests to build both derivations included in this draft PR have been done on GitHub CI using a very recent nixpkgs/nix docker image (
nixpkgs/nix@sha256:c7ab99ed60cc587ac784742e2814303331283cca121507a1d4c0dd21ed1bdf83) as a base. The results of these builds can be viewed at https://github.com/sbellem/nixpkgs/actions/workflows/sgxsdk.yml. The derivations were built for themasterandrelease-21.05branches, and for tag21.11-pre.A binary cache on cachix has been added to store the successfully built packages (only for master currently). The cache is at https://app.cachix.org/api/v1/cache/gluonixpkgs/contents.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)