Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1882547
Summary: | Review Request: osslsigncode - OpenSSL based Authenticode signing for PE/MSI/Java CAB files | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Artem <ego.cordatus> |
Component: | Package Review | Assignee: | Petr Pisar <ppisar> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | unspecified | Docs Contact: | |
Priority: | unspecified | ||
Version: | rawhide | CC: | package-review, ppisar |
Target Milestone: | --- | Flags: | ppisar:
fedora-review+
|
Target Release: | --- | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | If docs needed, set a value | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2020-10-17 14:09:12 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Artem
2020-09-24 22:13:12 UTC
This package built on koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=52185501 URL and Source addresses are usable. Ok. TODO: Source0 URL differs from the one listed on the releases page <https://github.com/mtrojnar/osslsigncode/archive/2.0.tar.gz>. I'd prefer to have them the same. Source0 archive (SHA-256: 5a60e0a4b3e0b4d655317b2f12a810211c50242138322b16e7e01c6fbb89d92f) is original. Ok. Summary is Ok. Description verified from README.md. Ok. License verified from: osslsigncode.c: GPLv3+ with OpenSSL exception autogen.sh: BSD LICENSE.txt: GPLv3+ with OpenSSL exception COPYING.txt: GPLv3 text License is Ok. TODO: I recommend listing ./configure --with-curl --with-gsf options explicitly instead of relying on an autodetection. FIX: Build-require autoconf (osslsigncode.spec:29). FIX: Build-require make (osslsigncode.spec:31). FIX: Build-require coreutils (configure.ac:45). FIX: Build-require sed (configure.ac:48). TODO: Constrain 'pkgconfig(libcrypto)' dependency with '>= 1.1.0' (configure.ac:96). TODO: Remove 'pkgconfig(openssl)' dependency (its not used if 'pkgconfig(libcrypto) >= 1.1.0' is available. TODO: Constrain 'pkgconfig(libcurl)' dependency with '>= 7.12.0' (configure.ac:114). TODO: Perform upstream tests. You can install mingw32-gcc and /usr/bin/keytool, then comile a trivial C program with i686-w64-mingw32-gcc to produce a PE executable, then rename it to tests/putty.exe, slightly patch tests/testsign.sh not to delete putty.exe, and finaly execute tests/testsign.sh. Distribution compiler and linker flags are respected. Ok. $ rpmlint osslsigncode.spec ../SRPMS/osslsigncode-2.0-2.fc34.src.rpm ../RPMS/x86_64/osslsigncode-* sh: /usr/bin/python2: No such file or directory osslsigncode.src: W: spelling-error %description -l en_US signtool -> sign tool, sign-tool, signatory osslsigncode.src: W: spelling-error %description -l en_US exe -> ex, exes, exec osslsigncode.src: W: spelling-error %description -l en_US timestamping -> time stamping, time-stamping, times tamping osslsigncode.src: W: spelling-error %description -l en_US cURL -> curl, URL, c URL osslsigncode.x86_64: W: spelling-error %description -l en_US signtool -> sign tool, sign-tool, signatory osslsigncode.x86_64: W: spelling-error %description -l en_US exe -> ex, exes, exec osslsigncode.x86_64: W: spelling-error %description -l en_US timestamping -> time stamping, time-stamping, times tamping osslsigncode.x86_64: W: spelling-error %description -l en_US cURL -> curl, URL, c URL osslsigncode.x86_64: W: incoherent-version-in-changelog 2.0-1 ['2.0-2.fc34', '2.0-2'] osslsigncode.x86_64: W: no-manual-page-for-binary osslsigncode 4 packages and 1 specfiles checked; 0 errors, 10 warnings. FIX: The latest changelog entry does not version-release strig of the package. $ rpm -q -lv -p ../RPMS/x86_64/osslsigncode-2.0-2.fc34.x86_64.rpm -rwxr-xr-x 1 root root 77880 Oct 9 16:48 /usr/bin/osslsigncode drwxr-xr-x 2 root root 0 Oct 9 16:48 /usr/lib/.build-id drwxr-xr-x 2 root root 0 Oct 9 16:48 /usr/lib/.build-id/3a lrwxrwxrwx 1 root root 32 Oct 9 16:48 /usr/lib/.build-id/3a/7f2f1b34696d85dee09c3f73c5b3545f14a2cf -> ../../../../usr/bin/osslsigncode drwxr-xr-x 2 root root 0 Oct 9 16:48 /usr/share/doc/osslsigncode -rw-r--r-- 1 root root 3158 Dec 4 2018 /usr/share/doc/osslsigncode/CHANGELOG.md -rw-r--r-- 1 root root 4945 Dec 4 2018 /usr/share/doc/osslsigncode/README.md -rw-r--r-- 1 root root 2852 Dec 4 2018 /usr/share/doc/osslsigncode/README.unauthblob.md -rw-r--r-- 1 root root 251 Dec 4 2018 /usr/share/doc/osslsigncode/TODO.md drwxr-xr-x 2 root root 0 Oct 9 16:48 /usr/share/licenses/osslsigncode -rw-r--r-- 1 root root 35147 Dec 4 2018 /usr/share/licenses/osslsigncode/COPYING.txt -rw-r--r-- 1 root root 1506 Dec 4 2018 /usr/share/licenses/osslsigncode/LICENSE.txt The permissions and file layout are Ok. $ rpm -q --requires -p ../RPMS/x86_64/osslsigncode-2.0-2.fc34.x86_64.rpm |sort -f |uniq -c 1 libc.so.6()(64bit) 1 libc.so.6(GLIBC_2.14)(64bit) 1 libc.so.6(GLIBC_2.2.5)(64bit) 1 libc.so.6(GLIBC_2.3)(64bit) 1 libc.so.6(GLIBC_2.3.4)(64bit) 1 libc.so.6(GLIBC_2.4)(64bit) 1 libcrypto.so.1.1()(64bit) 1 libcrypto.so.1.1(OPENSSL_1_1_0)(64bit) 1 libcurl.so.4()(64bit) 1 libglib-2.0.so.0()(64bit) 1 libgobject-2.0.so.0()(64bit) 1 libgsf-1.so.114()(64bit) 1 rpmlib(CompressedFileNames) <= 3.0.4-1 1 rpmlib(FileDigests) <= 4.6.0-1 1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 1 rpmlib(PayloadIsZstd) <= 5.4.18-1 1 rtld(GNU_HASH) Binary requires are Ok. $ rpm -q --provides -p ../RPMS/x86_64/osslsigncode-2.0-2.fc34.x86_64.rpm |sort -f |uniq -c 1 osslsigncode = 2.0-2.fc34 1 osslsigncode(x86-64) = 2.0-2.fc34 Binary provides are Ok. $ resolvedeps rawhide ../RPMS/x86_64/osslsigncode-2.0-2.fc34.x86_64.rpm Binary dependencies are resolvable. Ok. The package build in F34 (https://koji.fedoraproject.org/koji/taskinfo?taskID=53083946). Ok. Otherwise the package is in line with Fedora packaging guidelines. Please correct the 'FIX' items, consider fixing 'TODO' items, and provide a new spec file. Resolution: Package NOT approved. (In reply to Petr Pisar from comment #2) > TODO: Source0 URL differs from the one listed on the releases page > <https://github.com/mtrojnar/osslsigncode/archive/2.0.tar.gz>. I'd prefer to > have them the same. Disagree here, this is normal to use GitHub API for downloading release tarballs like that in Fedora https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags If all sources will named like that ("2.0.tar.gz") there will be a real mess and file naming conflicts in rpmbuild/SOURCES or just hard to find necessary source if there are many. > TODO: Perform upstream tests. You can install mingw32-gcc and > /usr/bin/keytool, then comile a trivial C program with > i686-w64-mingw32-gcc to produce a PE executable, then rename it to > tests/putty.exe, slightly patch tests/testsign.sh > not to delete putty.exe, and finaly execute tests/testsign.sh. I've added tests now and they are passed. Network not required during tests. But you need to review this and i'm not sure how you feel about it. :) Anyway we can improve this in future. The rest is fixed look like. Thanks for great review. --- https://download.copr.fedorainfracloud.org/results/atim/playground/fedora-33-x86_64/01703705-osslsigncode/osslsigncode.spec https://download.copr.fedorainfracloud.org/results/atim/playground/fedora-33-x86_64/01703705-osslsigncode/osslsigncode-2.0-3.fc33.src.rpm > Disagree here, this is normal to use GitHub API for downloading release tarballs like that in Fedora https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags > If all sources will named like that ("2.0.tar.gz") there will be a real mess and file naming conflicts in rpmbuild/SOURCES or just hard to find necessary source if there are many. The guidelines are about a snapshot from a git tree identified by a tag. I can now see that the upstream actually did not make a regular release. In case of Githab file names of a regular release are under the upstream's control and reside on releases/download path in case. > TODO: I recommend listing ./configure --with-curl --with-gsf options explicitly instead of relying on an autodetection. Ok. > FIX: Build-require autoconf (osslsigncode.spec:29). > FIX: Build-require make (osslsigncode.spec:31). > FIX: Build-require coreutils (configure.ac:45). > FIX: Build-require sed (configure.ac:48). Ok. > TODO: Constrain 'pkgconfig(libcrypto)' dependency with '>= 1.1.0' (configure.ac:96). > TODO: Constrain 'pkgconfig(libcurl)' dependency with '>= 7.12.0' (configure.ac:114). Ok. > I've added tests now and they are passed. Network not required during tests. But you need to review this and i'm not sure how you feel about it. :) Anyway we can improve this in future. %if %{with tests} +# To prevent network access during tests +Patch0: %{name}-preventnetwork-access-during-tests.patch +%endif FIX: Patch tags must be unconditional. This is to ensure that a source package will contain all the patches regardless of macros defined when creating the source package. +# Required for tests +# * https://github.com/mtrojnar/osslsigncode/blob/master/tests/testsign.sh#L5 +Source1: http://the.earth.li/~sgtatham/putty/0.64/x86/putty.exe The executable is identential to <https://the.earth.li/~sgtatham/putty/0.64/x86/putty.exe> (SHA-256: dc8d3ab6669b0a634de3e48477e7eb1282a770641194de2171ee9f3ec970c088). License (MIT) verifified from <https://git.tartarus.org/?p=simon/putty.git;a=blob_plain;f=LICENCE;hb=refs/tags/0.64>. FIX: Putty license requires that its copyright statement and license are distributed together with the executable. To resolve it, you need to copy that text to a file and add it at as another Source tag. TODO: I don't feel comfortable with including a binary. Although it's only used as a data and not included into a binary package. We never know what proprietary code could be inside. It's also a magnitude larger than the osslsigncode sources. Could you please instead build a PE executable from a known source using a crosscompiler? The test really does not need care whether it's Putty or not. Here is a spec that does that: --- osslsigncode.spec.orig 2020-10-11 05:35:50.000000000 +0200 +++ osslsigncode.spec 2020-10-12 10:38:14.233000000 +0200 @@ -9,14 +9,8 @@ URL: https://github.com/mtrojnar/osslsigncode Source0: %{url}/archive/%{version}/%{name}-%{version}.tar.gz -# Required for tests -# * https://github.com/mtrojnar/osslsigncode/blob/master/tests/testsign.sh#L5 -Source1: http://the.earth.li/~sgtatham/putty/0.64/x86/putty.exe - -%if %{with tests} # To prevent network access during tests Patch0: %{name}-preventnetwork-access-during-tests.patch -%endif BuildRequires: autoconf BuildRequires: automake @@ -31,6 +25,7 @@ %if %{with tests} BuildRequires: java-1.8.0-openjdk-headless +BuildRequires: mingw32-gcc BuildRequires: openssl >= 1.1.0 %endif @@ -61,7 +56,7 @@ # https://bugzilla.redhat.com/show_bug.cgi?id=1882547#c2 %if %{with tests} pushd tests -install -Dp -m0644 %{SOURCE1} putty.exe +echo 'int main(void) {return 0;}' | i686-w64-mingw32-gcc -x c -o putty.exe - ./testsign.sh popd %endif All test pass. Ok. $ rpmlint osslsigncode.spec ../SRPMS/osslsigncode-2.0-3.fc34.src.rpm ../RPMS/x86_64/osslsigncode-*2.0-3.fc34.x86_64.rpm sh: /usr/bin/python2: No such file or directory osslsigncode.src: W: spelling-error %description -l en_US signtool -> sign tool, sign-tool, signatory osslsigncode.src: W: spelling-error %description -l en_US exe -> ex, exes, exec osslsigncode.src: W: spelling-error %description -l en_US timestamping -> time stamping, time-stamping, times tamping osslsigncode.src: W: spelling-error %description -l en_US cURL -> curl, URL, c URL osslsigncode.x86_64: W: spelling-error %description -l en_US signtool -> sign tool, sign-tool, signatory osslsigncode.x86_64: W: spelling-error %description -l en_US exe -> ex, exes, exec osslsigncode.x86_64: W: spelling-error %description -l en_US timestamping -> time stamping, time-stamping, times tamping osslsigncode.x86_64: W: spelling-error %description -l en_US cURL -> curl, URL, c URL osslsigncode.x86_64: W: no-manual-page-for-binary osslsigncode 4 packages and 1 specfiles checked; 0 errors, 9 warnings. rpmlint is Ok. Package builds in F34 (https://koji.fedoraproject.org/koji/taskinfo?taskID=53276555). OK. Please correct the 'FIX' items, consider fixing the 'TODO' item, and provide a new spec file. (In reply to Petr Pisar from comment #4) > FIX: Patch tags must be unconditional. This is to ensure that a source > package will contain all the patches regardless of macros defined when > creating the source package. Indeed, thanks for pointing this. > TODO: I don't feel comfortable with including a binary. Fixed tests and updated .spec. Thanks a lot for help. --- https://download.copr.fedorainfracloud.org/results/atim/playground/fedora-33-x86_64/01705110-osslsigncode/osslsigncode.spec https://download.copr.fedorainfracloud.org/results/atim/playground/fedora-33-x86_64/01705110-osslsigncode/osslsigncode-2.0-4.fc33.src.rpm All tests pass. Ok. $ rpmlint osslsigncode.spec ../SRPMS/osslsigncode-2.0-4.fc34.src.rpm ../RPMS/x86_64/osslsigncode-*2.0-4.fc34.x86_64.rpm sh: /usr/bin/python2: No such file or directory osslsigncode.src: W: spelling-error %description -l en_US signtool -> sign tool, sign-tool, signatory osslsigncode.src: W: spelling-error %description -l en_US exe -> ex, exes, exec osslsigncode.src: W: spelling-error %description -l en_US timestamping -> time stamping, time-stamping, times tamping osslsigncode.src: W: spelling-error %description -l en_US cURL -> curl, URL, c URL osslsigncode.x86_64: W: spelling-error %description -l en_US signtool -> sign tool, sign-tool, signatory osslsigncode.x86_64: W: spelling-error %description -l en_US exe -> ex, exes, exec osslsigncode.x86_64: W: spelling-error %description -l en_US timestamping -> time stamping, time-stamping, times tamping osslsigncode.x86_64: W: spelling-error %description -l en_US cURL -> curl, URL, c URL osslsigncode.x86_64: W: no-manual-page-for-binary osslsigncode 4 packages and 1 specfiles checked; 0 errors, 9 warnings. rpmlint is Ok. The package builds in F34 (https://koji.fedoraproject.org/koji/taskinfo?taskID=53369918). Ok. Resolution: Package APPROVED. I noticed that ./testsign.sh swallows the exit code. It would be great if you corrected it and send it to the upstream: if [ $? -ne 0 ]; then echo "Failure is not an option." + exit 1 else echo "Yes, it works." fi FEDORA-2020-954b9a9a68 has been submitted as an update to Fedora 33. https://bodhi.fedoraproject.org/updates/FEDORA-2020-954b9a9a68 FEDORA-2020-ebcfc63acb has been submitted as an update to Fedora 32. https://bodhi.fedoraproject.org/updates/FEDORA-2020-ebcfc63acb FEDORA-2020-954b9a9a68 has been pushed to the Fedora 33 testing repository. In short time you'll be able to install the update with the following command: `sudo dnf upgrade --enablerepo=updates-testing --advisory=FEDORA-2020-954b9a9a68` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2020-954b9a9a68 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates. FEDORA-2020-ebcfc63acb has been pushed to the Fedora 32 testing repository. In short time you'll be able to install the update with the following command: `sudo dnf upgrade --enablerepo=updates-testing --advisory=FEDORA-2020-ebcfc63acb` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2020-ebcfc63acb See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates. FEDORA-2020-ebcfc63acb has been pushed to the Fedora 32 stable repository. If problem still persists, please make note of it in this bug report. FEDORA-2020-954b9a9a68 has been pushed to the Fedora 33 stable repository. If problem still persists, please make note of it in this bug report. |