Spec URL: https://yunyings.fedorapeople.org/sgx-aesm-service.spec SRPM URL: https://yunyings.fedorapeople.org/sgx-aesm-service-2.15.100.0-1.fc35.src.rpm Description: Intel Software Guard Extentions(SGX) architectural enclave service manager(AESM) handles all system services for Intel SGX enclaves. This package also includes many other libraries and tools provided by SGX platform software(PSW) and Data Center Attestation Primitives(DCAP). SGX PSW&SDK upstream: https://github.com/intel/linux-sgx SGX DCAP upstream: https://github.com/intel/SGXDataCenterAttestationPrimitives/ Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=79740210 Fedora Account System Username: yunyings
Thank you for the package review request! We'll review the package soon after the New Year. Thanks!
Previous koji build link expires. Here is an updated build with same spec and srpm: https://koji.fedoraproject.org/koji/taskinfo?taskID=81082317 .
Stanislav, Will you update the status on this package review? Thanks
We're currently reviewing the package, we should be able to post the results within a few days. Thank you!
Source package includes the text of the license(s) in its own file not included in %license. Package contains bundled 3rd party libraries, header files (e.g., OpenSSL). Packaged is BSD licensed, BSD non-compatible sources distributed as part of the RPM. The package does not build for rawhide, see https://koji.fedoraproject.org/koji/taskinfo?taskID=82183471 rpmlint: 2.2.0 checks: 32, packages: 1 sgx-aesm-service.spec: E: superfluous-%clean-section sgx-aesm-service.spec:37: W: mixed-use-of-spaces-and-tabs (spaces: line 8, tab: line 37) AutoTools: Obsoleted m4s found AC_PROG_LIBTOOL found in: sgx-aesm-service-2.15.100.0/external/protobuf/protobuf_code/configure.ac:109, sgx-aesm-service-2.15.100.0/sdk/gperftools/gperftools-2.7/configure.ac:163
The bundled 3rd party libraries cannot be replaced with the system libraries, because enclaves cannot use the system libraries (all code must be linked statically and signed). See for example ENCLAVE_CFLAGS and ENCLAVE_CXXFLAGS in buildenv.mk, which include -ffreestanding -nostdinc -nostdinc++, and ENCLAVE_LDFLAGS which includes the linker flag -eenclave_entry. However, these should be open source and limited to the minimum necessary. Based on my analysis of the build process: * the static binary-only libraries in external/ should be removed from the sources tarball and should not be used during the build process. * OpenSSL should be built from source, and the OpenSSL sources should be passed through the "hobble-openssl" script from the Fedora OpenSSL package. The script removes certain patented algorithms from the tarball. * No network access should be done during the build (see for example external/sgxssl/prepare_sgxssl.sh); all sources should be included in the tarball.
Thanks a lot for the review Cestmir and Paolo. We will align internally with Intel SGX team to address the issues, and get back with updated spec and srpm.
About spec： (In reply to Cestmir Kalina from comment #5) > Source package includes the text of the license(s) in its own file not > included in %license. %license License.txt > rpmlint: 2.2.0 checks: 32, packages: 1 > sgx-aesm-service.spec: E: superfluous-%clean-section > sgx-aesm-service.spec:37: W: mixed-use-of-spaces-and-tabs (spaces: line 8,tab: line 37) 37 ExclusiveArch: x86_64(two spaces replace the tap) 362 %clean delete this line 363 make clean delete this line About upstream srpm: I find even workaround the "error: type ‘struct _thread_data_t’ violates the C++ One Definition Rule [-Werror=odr]" There are still many other C++ build error: inlined from ‘bool Json::OurReader::readValue()’ at /home/jmiao/rpmbuild/BUILD/sgx-aesm-service-2.15.100.0/external/CppMicroServices/third_party/jsoncpp.cpp:1228:31: /usr/include/c++/12/bits/move.h:205:7: error: ‘v.Json::Value::value_’ may be used uninitialized [-Werror=maybe-uninitialized] 205 | __a = _GLIBCXX_MOVE(__b); | ^~~ inlined from ‘bool Json::OurReader::readValue()’ at /home/jmiao/rpmbuild/BUILD/sgx-aesm-service-2.15.100.0/external/CppMicroServices/third_party/jsoncpp.cpp:1257:33: /usr/include/c++/12/bits/move.h:205:7: error: ‘v.Json::Value::value_’ may be used uninitialized [-Werror=maybe-uninitialized] 205 | __a = _GLIBCXX_MOVE(__b); | ^~~ home/jmiao/rpmbuild/BUILD/sgx-aesm-service-2.15.100.0/external/CppMicroServices/build/include/cppmicroservices/GlobalConfig.h:144:26: error: ‘template<class _Arg, class _Result> struct std::unary_function’ is deprecated [-Werror=deprecated-declarations] 144 | struct hash<type> : std::unary_function<type, std::size_t> { \ | ^~~~~~~~~~~~~~
SPEC file and src.rpm shared on fedora people has been updated to fix the issues in comment 5: 1. Package does not build for rawhide. 2. Licence file not included. 3. rpmlint errors and warnings. 4. Obsoleted AC_PROG_LIBTOOL being used. 5. BSD non-compatible licensed sources included. New koji build link: https://koji.fedoraproject.org/koji/taskinfo?taskID=82674855 For Paolo's proposals in comment 6, as they might lead to tremendous code changes, upstream are still investigating.
Cestmir - Could you kindly review comment #9. Thx
Hi Paolo, My comments are as below. * the static binary-only libraries in external/ should be removed from the sources tarball and should not be used during the build process. [Xiangquan] 1. Binutils Toolset can be removed since it is not needed for fedora36. For the prebuilt opt libs, only IPP is a must for the default build. Reasons we use the prebuilt IPP instead of building from OpenSource: a. IPP build has some tool dependency and tool version requirement. Not all the OSes we supported meet this requirement. We don’t want to bring the dependency to SGX build. b. It takes long time to build the IPP from source code. * OpenSSL should be built from source, and the OpenSSL sources should be passed through the "hobble-openssl" script from the Fedora OpenSSL package. The script removes certain patented algorithms from the tarball. [Xiangquan] Not quite clear about this issue. Could you please provide more information? Thanks! * No network access should be done during the build (see for example external/sgxssl/prepare_sgxssl.sh); all sources should be included in the tarball. [Xiangquan] Please don't care about this script which won't be called in this build process. All source codes are included in the tarball.
Hi Xiangquan, here are my replies. > For the prebuilt opt libs, only IPP is a must for the default build. Reasons we use > the prebuilt IPP instead of building from OpenSource: Unfortunately for Fedora you need to build all from source. We understand that it can take a long time. > a. IPP build has some tool dependency and tool version requirement. Not all the OSes > we supported meet this requirement. We don’t want to bring the dependency to SGX build. What is the requirement? > Not quite clear about this issue. Could you please provide more information? Thanks! Just set up to build OpenSSL from source, using a separate source tarball. We can help satisfying the legal requirements later. > [Xiangquan] Please don't care about this script which won't be called in this build > process. All source codes are included in the tarball. Please remove the script together with all .a files, that makes the review simpler.
Hi Paolo, Still have one question. Thanks! > Not quite clear about this issue. Could you please provide more information? Thanks! Just set up to build OpenSSL from source, using a separate source tarball. We can help satisfying the legal requirements later. [Xiangquan] Sorry for asking again. Could you please provide the directory of OpenSSL. This may help us fix this issue quickly.
> Could you please provide the directory of OpenSSL OpenSSL is used by SGX SSL. The full source of SGX SSL, including the unmodified sources of OpenSSL and the SGX SDK, must be included in the RPM and built from source. The README.md for SGX SSL says > To build Intel® SGX SSL package in Linux OS: > 1. Download OpenSSL 1.1.1m package into openssl_source/ directory. (tar.gz package, e.g. openssl-1.1.1m.tar.gz) > 2. Download and install latest SGX SDK from [01.org](https://01.org/intel-software-guard-extensions/downloads). You can find installation guide in the same website.
I see. Thanks! Actually the full of SGX SSL is included in the tar ball. The README.md is for all distros and it can be removed from this tar ball if you agree this solution.
> Actually the full of SGX SSL is included in the tar ball. Ok, I see now that only a few files are needed from OpenSSL, thanks.
Not sure I answered you question clearly. The full of SGX SSL is pre-downloaded under external/dcap_source/QuoteVerification/sgxssl.
This is an automatic action taken by review-stats script. The ticket reviewer failed to clear the NEEDINFO flag in a month. As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews we reset the status and the assignee of this ticket.
Thank you for changing the license information. Looking at https://github.com/intel/linux-sgx/blob/master/License.txt, it says: > libsgx_le.signed.so, libsgx_pce.signed.so, libsgx_pve.signed.so, libsgx_qe.signed.so, libsgx_pse_pr.signed.so, libsgx_pse_pr_2.signed.so and libsgx_pse_op.signed.so are licensed as Intel redistributable binary firmware and other blobs. in these cases, Fedora would seem to require (https://fedoraproject.org/wiki/Licensing:Main): > The License tag for any firmware that disallows modification must be set to: "Redistributable, no modification permitted" perhaps this should be worked into the spec file as well? Since it's been deemed unnecessary above, please remove: ./external/toolset/ubuntu18.04/ld ./external/toolset/ubuntu18.04/ar ./external/toolset/ubuntu18.04/objcopy ./external/toolset/ubuntu18.04/ranlib ./external/toolset/ubuntu18.04/objdump ./external/toolset/ubuntu18.04/as ./external/toolset/fedora32/ld ./external/toolset/fedora32/ar ./external/toolset/fedora32/objcopy ./external/toolset/fedora32/ranlib ./external/toolset/fedora32/objdump ./external/toolset/fedora32/as ./external/toolset/nix/ld ./external/toolset/nix/ar ./external/toolset/nix/objcopy ./external/toolset/nix/ranlib ./external/toolset/nix/objdump ./external/toolset/nix/as ./external/toolset/ubuntu20.04/ld ./external/toolset/ubuntu20.04/ar ./external/toolset/ubuntu20.04/objcopy ./external/toolset/ubuntu20.04/ranlib ./external/toolset/ubuntu20.04/objdump ./external/toolset/ubuntu20.04/as ./external/toolset/rhel8.2/ld ./external/toolset/rhel8.2/ar ./external/toolset/rhel8.2/objcopy ./external/toolset/rhel8.2/ranlib ./external/toolset/rhel8.2/objdump ./external/toolset/rhel8.2/as ./external/toolset/centos8.2/ld ./external/toolset/centos8.2/ar ./external/toolset/centos8.2/objcopy ./external/toolset/centos8.2/ranlib ./external/toolset/centos8.2/objdump ./external/toolset/centos8.2/as ./external/toolset/fedora31/ld ./external/toolset/fedora31/ar ./external/toolset/fedora31/objcopy ./external/toolset/fedora31/ranlib ./external/toolset/fedora31/objdump ./external/toolset/fedora31/as
Yes, we will remove the toolset binaries.
> %define _aesm_service_path /opt/intel/sgx-aesm-service > %define _ra_service_path /opt/intel/sgx-ra-service > %define _dcap_pccs_path /opt/intel/sgx-dcap-pccs > %define _psw_version 2.15.100.0 > %define _dcap_version 1.12.100.0 > %define _license_file COPYING Those defines are very suspicious. They should at least use %global: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_global_preferred_over_define %_psw_version is just the package version? Please drop the define, put the version in Version:, drop redundant Version lines on the subpackages. What are those paths? Files that are part of the package or some external stuff? If those files are part of the package, they should be in %_libdir/%name/ or %_libexedir/%name/. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_filesystem_layout What is the point of %_license_file? Just put "COPYING" directly in the relevant places. It's shorter and obvious. > Release: 1%{?dist} I'd recommend %autorelease+%autochangelog: https://docs.pagure.org/fedora-infra.rpmautospec/autochangelog.html > %description > Intel(R) Software Guard Extensions AESM Service The description should *describe*: what is this package good for, what it does, etc. Usually this is at least a paragraph of text. For complicated packages, maybe a few paragraphs. %description should also be wrapped to 80 columns. > Intel(R) Software Guard Extensions QE and PvE > Intel(R) Software Guard Extensions LE > ... Those summaries generally just repeat the package name, but don't say much about the package. In those cases, "LE" and "QE" and "PvE" are the interesting parts, but after reading the Summary, I have no idea what that is. > Requires: %{name} >= %{version}-%{release} libsgx-qe3-logic >= %{_dcap_version}-%{release} libsgx-aesm-pce-plugin >= %{version}-%{release} One per line please > Requires: gcc gcc-c++ make One per line. Hmm, why does package require a compiler at runtime? > make %{?_smp_mflags} build → %make_build > make DESTDIR=%{?buildroot} install → %make_install > if [ -c /dev/sgx_provision -o -c /dev/sgx/provision ] Generally, you need to assume that the rpm may be installed on a different machine (e.g. when we're creating a "golden image" for installation on other machines), or the rpm may installed into a chroot (e.g. when doing dnf install --sysroot=…). So conditionalization on /dev/sgx_provision being there is not possible. The group should be added unconditionally. A sysusers file should be used to declare the group and create a scriptlet automatically. See https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/#_dynamic_allocation > %post > if [ -x %{_aesm_service_path}/startup.sh ]; then %{_aesm_service_path}/startup.sh; fi > %preun > if [ -x %{_aesm_service_path}/cleanup.sh ]; then %{_aesm_service_path}/cleanup.sh; fi > %post -n sgx-dcap-pccs > if [ -x %{_dcap_pccs_path}/startup.sh ]; then %{_dcap_pccs_path}/startup.sh; fi > %preun -n sgx-dcap-pccs > if [ -x %{_dcap_pccs_path}/cleanup.sh ]; then %{_dcap_pccs_path}/cleanup.sh; fi > %post -n sgx-ra-service > if [ -x %{_ra_service_path}/startup.sh ]; then %{_ra_service_path}/startup.sh; fi > %preun -n sgx-ra-service > if [ -x %{_ra_service_path}/cleanup.sh ]; then %{_ra_service_path}/cleanup.sh; fi What are those files? Who provides them? What do they do? (Note that those scriptlets will fail the transaction if those programs fail. You should always use '|| :'. But I'm not sure if they should be called at all.) > trigger_udev() { systemd-udev already does 'udevadm control --reload' from a transfiletrigger. But it doesn't do 'udevadm trigger'. Maybe it should. I'll discuss this with other maintainers of systemd and return to this later. Why is %install so complicated? %{buildroot} should not be conditionalized with ?. It's always defined. > find license -type f -print0 | \ > xargs -0 -n1 cat >> %{?buildroot}/${pkg}/%{_docdir}/${pkg}/%{_license_file} This merges multiple files into one file? Please don't do this. Just list the license files with %license, they will be copied automatically. (https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text) > find %{?buildroot}/${pkg} -type d -links 2 | \ > sed -e "s#^%{?buildroot}/${pkg}##" | So, you want to have a file list without the prefix? Just do "(cd %{buildroot}/$pkg; find * ...)". Also, %_specdir shouldn't be used to write anything. %buildsubdir is better. In general this automatic generation of file lists makes it very hard to see what files are where. Is it really necessary? I tried to build the package, but that fails, so I can't even figure out what files the package has :( -- inlined from 'void Json::Value::swapPayload(Json::Value&)' at /builddir/build/BUILD/sgx-aesm-service-2.15.100.0/external/CppMicroServices/third_party/jsoncpp.cpp:2678:12, inlined from 'bool Json::OurReader::readValue()' at /builddir/build/BUILD/sgx-aesm-service-2.15.100.0/external/CppMicroServices/third_party/jsoncpp.cpp:1228:31: /usr/include/c++/12/bits/move.h:205:7: error: 'v.Json::Value::value_' may be used uninitialized [-Werror=maybe-uninitialized] 205 | __a = _GLIBCXX_MOVE(__b); | ^~~ Using -Werror for compiler heuristics is generally a bad idea. It means that the package may build fine one day, but fail to build the next, after the compiler has been upgraded. -- I didn't look at the contents yet. I think the most important part would be to improve the Summaries and %descriptions, and obviously to get the package to build again. I expect that there'll be a lot of work on unbundling and file locations…
Regarding the signed binaries, I understand that they need to be signed by Intel but we need to at least be able to build them (as reproducibly as possible, to verify what was signed). So in parallel to aesm the SGX SDK needs to be packaged in Fedora as well. Fedora cannot use the RPMs at https://download.01.org/intel-sgx/.
Nevermind comment 22, I messed up.
Thanks a lot for the reviews and comments. We have managed to fix the issues in Cestmir's comment 19 with updated spec & 2.16.100.0-1.fc35.src.rpm, and currently are working on the issues mentioned in Zbigniew's comment 21. Will update here when all issues are fixed or clarified. Meanwhile, we found a compiling issue which seems more likely related to gcc/toolchain than to SGX source codes: /builddir/build/BUILD/sgx-aesm-service-2.16.100.0/external/CppMicroServices/framework/include/cppmicroservices/ServiceReference.h:93:24: internal compiler error: Segmentation fault 93 | this->operator=(0); | ~~~~~~~~~~~~~~~^~~ Ticket created: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=2073643. Anyone has clue about this?
Re the signed binaries: can you please describe how this is supposed to work in general? Is the stuff being signed built as part of the rpm build?
(In reply to Zbigniew Jędrzejewski-Szmek from comment #25) > Re the signed binaries: can you please describe how this is supposed to work > in general? > Is the stuff being signed built as part of the rpm build? These are pre-signed binaries which means it is already signed before rpm build. It is not in the process of rpm build. Not quite sure it answers your question. Any further concerns, please let us know.
> Re the signed binaries: can you please describe how this is supposed to work in general? > Is the stuff being signed built as part of the rpm build? The signing tool is included, but no signing is done as part of the rpm build. One way to arrange this could be for the spec file to have two modes, one that uses the prebuilt files and one that builds the enclaves without signature.
Signing tool is part of SDK and PSW/DCAP building depends on SDK. Since there is no SDK RPM solution currently, we just put SDK build/installation in the package. In the future, all of the SDK pars will be removed from this package.
(In reply to xiangquan.liu from comment #28) > Signing tool is part of SDK and PSW/DCAP building depends on SDK. Since > there is no SDK RPM solution currently, we just put SDK build/installation > in the package. In the future, all of the SDK pars will be removed from this > package. Please split things into the final packages where they are supposed to be. Splitting or packages is complicated. It's much less work to have the desired layout of packages from the beginning.
(In reply to Zbigniew Jędrzejewski-Szmek from comment #29) > Please split things into the final packages where they are supposed to be. > Splitting > or packages is complicated. It's much less work to have the desired layout > of packages > from the beginning. So it means sgx sdk has to be packaged first, then we use it as a dependent package for SDK and DCAP packaging. Is that right?
Yes. Please mark the new review request as blocking this one (in the "Depends on" field).
Thanks for confirming. That will require some changes from upstream, probably like splitting the releases of SDK and PSW. We will align internally and update here as soon as we have the needed split and sdk review request ready.
(In reply to Zbigniew Jędrzejewski-Szmek from comment #31) > Yes. Please mark the new review request as blocking this one (in the > "Depends on" field). Hi Zbigniew, I've created the review request for SGX SDK, and marked it as blocking this one: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=2085444 Could you help to review it as well? Thank you.
In common with my feedback on bug 2085444, I'd like to see this spec explicitly list packaged files in the %files section, rather than using to script to simply include everything that exists. It will be way easier to review the .spec to validate that suitable content is being packaged in the right locations, and will avoid accidentally shipping undesirable files that may appear in future releases of the upstream packages.
Hi, apologies for the delay. I keep meaning to return to this, but I was busy with too many other things. If somebody wants to take over the review, please go ahead. I won't be able to look at this before the middle of next week at the earliest.
(In reply to Yunying Sun from comment #33) > (In reply to Zbigniew Jędrzejewski-Szmek from comment #31) > > Yes. Please mark the new review request as blocking this one (in the > > "Depends on" field). > > Hi Zbigniew, I've created the review request for SGX SDK, and marked it as > blocking this one: > https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=2085444 > Could you help to review it as well? Thank you. Could update this package to reflect what you have split off for the SDK package. Since they are mutually dependant, it is desirable to be able to review both as a pair.
(In reply to Daniel Berrangé from comment #36) > > Could update this package to reflect what you have split off for the SDK > package. Since they are mutually dependant, it is desirable to be able to > review both as a pair. Sure. I will work internally with Intel SGX team to update this package to match with the SDK package under review.
This is an automatic check from review-stats script. This review request ticket hasn't been updated for some time. We're sorry it is taking so long. If you're still interested in packaging this software into Fedora repositories, please respond to this comment clearing the NEEDINFO flag. You may want to update the specfile and the src.rpm to the latest version available and to propose a review swap on Fedora devel mailing list to increase chances to have your package reviewed. If this is your first package and you need a sponsor, you may want to post some informal reviews. Read more at https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group. Without any reply, this request will shortly be considered abandoned and will be closed. Thank you for your patience.
This is an automatic action taken by review-stats script. The ticket submitter failed to clear the NEEDINFO flag in a month. As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews we consider this ticket as DEADREVIEW and proceed to close it.