Bug 2214269 - Review requested: openssl-pkcs11-sign-provider - OpenSSL 3.0 provider for pkcs#11 (private key operations only)
Summary: Review requested: openssl-pkcs11-sign-provider - OpenSSL 3.0 provider for pkc...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
unspecified
Target Milestone: ---
Assignee: Dan Horák
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2023-06-12 12:44 UTC by Holger Dengler
Modified: 2023-06-23 01:01 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2023-06-13 15:07:01 UTC
Type: Bug
Embargoed:
dan: fedora-review+


Attachments (Terms of Use)
Specfile (1.33 KB, text/plain)
2023-06-12 12:44 UTC, Holger Dengler
no flags Details
Source RPM (445.06 KB, application/x-rpm)
2023-06-12 12:47 UTC, Holger Dengler
no flags Details
Specfile (with empty %check section) (1.51 KB, text/plain)
2023-06-13 11:03 UTC, Holger Dengler
no flags Details
Source RPM (445.36 KB, application/x-rpm)
2023-06-13 11:06 UTC, Holger Dengler
no flags Details

Description Holger Dengler 2023-06-12 12:44:35 UTC
Created attachment 1970417 [details]
Specfile

Description:
The openssl-pkcs11-sign-provider is a provider module for openssl v3.0, which uses the PKCS#11 API to perform operations (sign/decrypt) with private asymmetric keys. The main purpose of the openssl-pkcs11-sign-provider is to provide TLS applictions (e.g. web-servers) access to sensitive key material, which is stored in secure PKCS#11 tokens. 

The openssl-pkcs11-sign-provider can be used on any architecture and with any implementation of the PKCS#11 standard (v3.0). At the moment, most of development and test has been done with opencryptoki.

Operations with public asymmetric keys are re-directed to a (configurable) built-in provider. Symmetric operations are not supported.

Fedora Account System Username: dengler

Version-Release number of selected component (if applicable):
1.0.0

Additional info:
upstream project:
https://github.com/opencryptoki/openssl-pkcs11-sign-provider

Comment 1 Holger Dengler 2023-06-12 12:47:36 UTC
Created attachment 1970418 [details]
Source RPM

Comment 2 Dan Horák 2023-06-13 08:16:48 UTC
I think it looks good (a formal review will follow), but I have a question about the tests. There are BuildRequires listed for tests, but the %check section is missing. If I understand it right, then the tests need the pkcsslotd service running, which can't be expected during the regular rpm build.

I think there are ~3 options
- comment out (or remove) the test related BRs
- add %check with commented out "make check" with some comment about running pkcsslotd in advance
- hide both the test BRs and %check behind a "with_test" condition, again with comment about pkcsslotd
I suppose starting pkcsslotd from the tests is not possible ...

Holger, what do you think?

Comment 3 Holger Dengler 2023-06-13 10:30:00 UTC
You're right, for executing the tests, pkcsslotd is (currently) required, as well as a working opencryptoki and p11-kit setup.

For the moment, I would like to go forward with option 2. As soon as there are other test-modules available (as the test-framework allow it), I can remove the comments and activate `make check` during package build.

Comment 4 Holger Dengler 2023-06-13 11:03:15 UTC
Created attachment 1970647 [details]
Specfile (with empty %check section)

Comment 5 Holger Dengler 2023-06-13 11:06:15 UTC
Created attachment 1970648 [details]
Source RPM

Comment 6 Dan Horák 2023-06-13 11:58:33 UTC
formal review is here, see the notes explaining OK* and BAD statuses below:

OK	source files match upstream:
	    4e28e1e21ee93bd15c8ae9fe6cad565d417bc0f2 openssl-pkcs11-sign-provider-1.0.0.tar.gz
OK	package meets naming and versioning guidelines.
OK*	specfile is properly named, is cleanly written and uses macros consistently.
OK	dist tag is present.
OK	license field matches the actual license.
OK	license is open source-compatible (Apache-2.0). License text included in package.
OK	latest version is being packaged.
OK	BuildRequires are proper.
OK	compiler flags are appropriate.
OK	package builds in mock (Rawhide/ppc64le).
OK	debuginfo package looks complete.
OK	rpmlint is silent.
OK	final provides and requires look sane.
N/A	%check is present and all tests pass.
OK	no shared libraries are added to the regular linker search paths.
OK	owns the directories it creates.
OK	doesn't own any directories it shouldn't.
OK	no duplicates in %files.
OK	file permissions are appropriate.
OK	no scriptlets present.
OK	code, not content.
OK	documentation is small, so no -docs subpackage is necessary.
OK	%docs are not necessary for the proper functioning of the package.
OK	no headers.
OK	no pkgconfig files.
OK	no libtool .la droppings.
OK	not a GUI app.

- I think you can switch to %autochangelog in addition to %autorelease before importing, see https://fedoraproject.org/wiki/Changes/rpmautospec for some details

The package is APPROVED and I will sponsor you into the packager group as well.

Comment 7 Petr Menšík 2023-06-13 12:47:54 UTC
I would recommend using %bcond_with tests. That way it can enabled during build time. Makes it easy to do test local build by:
fedpkg local --with tests

But will stay disabled by default, where it requires something extra to pass.
I did something similar on cpp-httplib package with %bcond_with online:
https://src.fedoraproject.org/rpms/cpp-httplib/blob/rawhide/f/cpp-httplib.spec#_5

That makes it possible to change behaviour slightly by defining --with parameter.
fedpkg mockbuild allows it too, in case it can work even from chroot.

Comment 8 Fedora Admin user for bugzilla script actions 2023-06-13 12:56:51 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/openssl-pkcs11-sign-provider

Comment 9 Fedora Update System 2023-06-13 15:05:47 UTC
FEDORA-2023-42193d9705 has been submitted as an update to Fedora 39. https://bodhi.fedoraproject.org/updates/FEDORA-2023-42193d9705

Comment 10 Fedora Update System 2023-06-13 15:07:01 UTC
FEDORA-2023-42193d9705 has been pushed to the Fedora 39 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 11 Fedora Update System 2023-06-14 19:32:56 UTC
FEDORA-2023-76b638f02b has been submitted as an update to Fedora 38. https://bodhi.fedoraproject.org/updates/FEDORA-2023-76b638f02b

Comment 12 Fedora Update System 2023-06-14 19:54:52 UTC
FEDORA-2023-128b5cecf0 has been submitted as an update to Fedora 37. https://bodhi.fedoraproject.org/updates/FEDORA-2023-128b5cecf0

Comment 13 Fedora Update System 2023-06-15 01:40:38 UTC
FEDORA-2023-76b638f02b has been pushed to the Fedora 38 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2023-76b638f02b \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2023-76b638f02b

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 14 Fedora Update System 2023-06-15 01:43:29 UTC
FEDORA-2023-128b5cecf0 has been pushed to the Fedora 37 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2023-128b5cecf0 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2023-128b5cecf0

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 15 Fedora Update System 2023-06-23 01:01:01 UTC
FEDORA-2023-128b5cecf0 has been pushed to the Fedora 37 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 16 Fedora Update System 2023-06-23 01:01:32 UTC
FEDORA-2023-76b638f02b has been pushed to the Fedora 38 stable repository.
If problem still persists, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.