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 ReviewAssignee: Petr Pisar <ppisar>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: rawhideCC: 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 
Spec URL: https://atim.fedorapeople.org//osslsigncode.spec
SRPM URL: https://atim.fedorapeople.org//osslsigncode-2.0-2.fc33.src.rpm

Description:
osslsigncode is a small tool that implements part of the functionality of the
Microsoft tool signtool.exe - more exactly the Authenticode signing and
timestamping. But osslsigncode is based on OpenSSL and cURL, and thus should
be able to compile on most platforms where these exist.
Comment 1 Artem 2020-09-24 22:13:15 UTC 
This package built on koji:  https://koji.fedoraproject.org/koji/taskinfo?taskID=52185501
Comment 2 Petr Pisar 2020-10-09 15:18:14 UTC 
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.
Comment 3 Artem 2020-10-11 03:49:11 UTC 
(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
Comment 4 Petr Pisar 2020-10-12 08:50:51 UTC 
> 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.
Comment 5 Artem 2020-10-12 18:05:16 UTC 
(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
Comment 6 Petr Pisar 2020-10-13 11:48:40 UTC 
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
Comment 7 Fedora Update System 2020-10-15 15:32:19 UTC 
FEDORA-2020-954b9a9a68 has been submitted as an update to Fedora 33. https://bodhi.fedoraproject.org/updates/FEDORA-2020-954b9a9a68
Comment 8 Fedora Update System 2020-10-15 15:38:56 UTC 
FEDORA-2020-ebcfc63acb has been submitted as an update to Fedora 32. https://bodhi.fedoraproject.org/updates/FEDORA-2020-ebcfc63acb
Comment 9 Fedora Update System 2020-10-15 19:09:43 UTC 
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.
Comment 10 Fedora Update System 2020-10-15 19:56:59 UTC 
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.
Comment 11 Fedora Update System 2020-10-17 14:09:12 UTC 
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.
Comment 12 Fedora Update System 2020-10-23 22:15:14 UTC 
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.