Spec URL: https://jruzicka.fedorapeople.org/pkgs/netopeer2.spec SRPM URL: https://jruzicka.fedorapeople.org/pkgs/netopeer2-2.1.23-1.fc37.src.rpm Description: Netopeer2 NETCONF server and client packages Fedora Account System Username: jruzicka
This makes use of recently reviewed libnetconf2/sysrepo packages, and it's the last of related projects to be packaged. I'll try to invoke the power of @pemensik once more, because he was very helpful in the reviews of prerequisite packages 👌🏽
Ah, I haven't noticed this on that date. Doing now :)
Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - systemd_post is invoked in %post, systemd_preun in %preun, and systemd_postun in %postun for Systemd service files. Note: Systemd service file(s) in netopeer2-server See: https://docs.fedoraproject.org/en-US/packaging- guidelines/Scriptlets/#_scriptlets ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "BSD 3-Clause License", "BSD 2-Clause License". 105 files have unknown license. Detailed output of licensecheck in /home/pihhan/fedora/review/2088450-netopeer2/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [!]: Package requires other packages for directories it uses. Note: No known owner of /usr/share/yang/modules [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/lib/systemd, /usr/lib/sysusers.d, /usr/share/yang, /usr/lib/systemd/system, /usr/share/yang/modules [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. Note: There is not a single %doc file shipped in any package. At least README.md should be included [!]: Package consistently uses macros (instead of hard-coded directory names). Note: I think manual pages should use %{_mandir} instead of %{_datadir}/man. It is also recommended on Fedora to not include trailing .gz, but replace it with .* Should help if compression would change. [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [!]: Requires correct, justified where necessary. Unowned /usr/share/yang/modules [x]: Spec file is legible and written in American English. [x]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [!]: Package complies to the Packaging Guidelines I don't think modification of root user automatically during install is acceptable. Either it needs to run always under root or it should work also for sysrepo group members. But modification of any existing user it not okay. Also install and removal %post scripts seems dangerous. At least initialization should be done in single run systemd unit instead before server startup. [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: Package does not own files or directories owned by other packages. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 0 bytes in 0 files. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [!]: Final provides and requires are sane (see attachments). Note: existing unowned directories [x]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in netopeer2-server , netopeer2-cli [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [!]: Scriptlets must be sane, if used. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [x]: Package should compile and build into binary rpms on all supported architectures. [!]: %check is present and all tests pass. Note: Some tests exist. Are they possible to run during build? If not possible, consider %bcond_with check and %if %{with check} test run... %endif for local builds. [x]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). Note: There are rpmlint messages (see attachment). [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Cannot parse rpmlint output: Rpmlint (debuginfo) ------------------- Cannot parse rpmlint output: Rpmlint (installed packages) ---------------------------- Cannot parse rpmlint output: Source checksums ---------------- https://github.com/CESNET/netopeer2/archive/v2.1.23/netopeer2-2.1.23.tar.gz : CHECKSUM(SHA256) this package : 89f7572d188e7b04be4b10656d7161d65fb557bac222d8c4596a97eaf833b691 CHECKSUM(SHA256) upstream package : 89f7572d188e7b04be4b10656d7161d65fb557bac222d8c4596a97eaf833b691 Requires -------- netopeer2 (rpmlib, GLIBC filtered): netopeer2-cli netopeer2-server netopeer2-server (rpmlib, GLIBC filtered): /bin/sh /usr/bin/bash libc.so.6()(64bit) libcurl.so.4()(64bit) libnetconf2.so.3()(64bit) libssh.so.4()(64bit) libssh.so.4(LIBSSH_4_5_0)(64bit) libsysrepo.so.7()(64bit) libsystemd.so.0()(64bit) libsystemd.so.0(LIBSYSTEMD_209)(64bit) libyang.so.2()(64bit) openssl rtld(GNU_HASH) sysrepo-tools netopeer2-cli (rpmlib, GLIBC filtered): libc.so.6()(64bit) libcrypto.so.3()(64bit) libcrypto.so.3(OPENSSL_3.0.0)(64bit) libnetconf2.so.3()(64bit) libyang.so.2()(64bit) openssl-perl rtld(GNU_HASH) netopeer2-debuginfo (rpmlib, GLIBC filtered): netopeer2-debugsource (rpmlib, GLIBC filtered): Provides -------- netopeer2: netopeer2 netopeer2(x86-64) netopeer2-server: group(sysrepo) netopeer2-server netopeer2-server(x86-64) user(root) netopeer2-cli: netopeer2-cli netopeer2-cli(x86-64) netopeer2-debuginfo: netopeer2-debuginfo netopeer2-debuginfo(x86-64) netopeer2-debugsource: netopeer2-debugsource netopeer2-debugsource(x86-64) Generated by fedora-review 0.8.0 (e988316) last change: 2022-04-07 Command line :/usr/bin/fedora-review -b 2088450 Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, C/C++, Shell-api Disabled plugins: Perl, Ocaml, Haskell, Python, PHP, fonts, Java, R, SugarActivity Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
I don't like the way it prepares for running. Can just single shell script be run to prepare everything? %post script seems unnecessary complicated. I think single time systemd unit should run when first starting the server. Check bind package for example of generating rndc.key. It would not have to switch user manually from those scripts, because systemd has good way to switch to non-privileged User/Group. Scriptlets setup.sh and remove.sh do quite complicated things. Are those necessary? Why is not netopeer2-server in sbin directory, when it is a system service? Is it useful also as non-privileged user service? On non-fedora distribution like RHEL, this would try adding sysrepo to root user. But I don't think it creates sysrepo in the first place. usermod -a -G sysrepo root Either remove that and rely on presence of sysusers_create_compat macro. It should work also on 0%{?rhel} >= 8. Maybe 9, I am not sure. Or make it working.
> I don't like the way it prepares for running. Can just single shell script be run to prepare everything? %post script seems unnecessary complicated. Agreed, I voiced a preference for a single script before, but upstream insists this is the best way to setup things. I'm told the scripts are robust and also work with various states of sysrepo repo. There could be a setup-all.sh or similar wrapper script calling the other 3 scripts, but that seems like an unnecessary extra layer. bind too has multiple files to setup different things. > I think single time systemd unit should run when first starting the server. Check bind package for example of generating rndc.key. > It would not have to switch user manually from those scripts, because systemd has good way to switch to non-privileged User/Group. I see named-setup-rndc.service in bind and other services use it in Wants/After: [Service] Type=oneshot ExecStart=/usr/libexec/generate-rndc-key.sh But I don't understand how this is only run once on first server start as opposed to every server start. 🤔 Isn't that what %post is for? What if I want to use the package in a container without systemd? > Scriptlets setup.sh and remove.sh do quite complicated things. Are those necessary? Again, I agree, but upstream deems them necessary. At least this code is actively used and maintained, which is better than having a poorly maintained duplicate on a package level, no? > Why is not netopeer2-server in sbin directory, when it is a system service? Is it useful also as non-privileged user service? This changed in the latest version, I'll query upstream. > On non-fedora distribution like RHEL, this would try adding sysrepo to root user. But I don't think it creates sysrepo in the first place. > usermod -a -G sysrepo root > > Either remove that and rely on presence of sysusers_create_compat macro. It should work also on 0%{?rhel} >= 8. Maybe 9, I am not sure. Or make it working. This is from upstream packaging for SUSE without sysuser_create_compat, and it seems to work there in the CI... I'll have a look at this, fixing the conditional to %if 0%{?fedora} || 0%{?rhel} >= 8 or removing this altogether in Fedora, but having same spec files in upstream and downstream is always a plus.
Okay, better example is in unbound-keygen.service. named-setup-rndc.service is started always, just the shell skips the action. %post scriptlets are not executed at all in container based releases like Fedora Silverblue. They have no way to execute them, because package contents are installed different way. I would guess the better would be auto-initialization on the first use. It might require significant changes I understand. Or even design change in netopeer2. It could watch timestamps in yang directory and trigger initialization action when they would change from entry in current user's ~/.local directory? But that is what part which I would classify only as SHOULD. The unowned directories are blocking the successful review. It seems to me %{_datadir}/yang/modules should belong to (some sub)package of yang library and it should depend only on that. Fill a bug on yang package and make this review depens on that bug. Add just: # For provided systemd units Requires: systemd # for yang/modules directory, bug #xy Requires: libyang
Spec URL: https://jruzicka.fedorapeople.org/pkgs/netopeer2.spec SRPM URL: https://jruzicka.fedorapeople.org/pkgs/netopeer2-2.1.23-1.fc37.src.rpm This prompted a chain of new upstream libyang/libnetconf2/sysrepo/netopeer2 releases which I've packaged for rawhide. * %{_datadir}/yang/modules is now owned by the libyang package (rhbz#2094371) * netopeer2-server now lives in %{_sbindir} * Added requires you mentioned to netopeer2-server: systemd, libyang * Made the netopeer2 package Require specific versions of -server/-cli (= %{version}-%{release}) * Removed questionable SUSE if regarding users, no need for it in the Fedora .spec. I hope this is now finally ready for inclusion.
@pemensik Could you please have a look?
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.
Sorry for long delay, taking the review again. Fedora started adoption of SPDX license identifiers in License: tags. I think License should contain BSD-3-Clause now, more in [1]. The SRPM link became invalid, I had to locate more recent itself. [x]: Package requires other packages for directories it uses. 1. https://docs.fedoraproject.org/en-US/legal/allowed-licenses/ I still do not like fact the package automatically modifies root user. I find it strange, root should be able to modify all packaged files anyway. Why is that needed?
Spec URL: https://jruzicka.fedorapeople.org/pkgs/netopeer2.spec SRPM URL: https://jruzicka.fedorapeople.org/pkgs/netopeer2-2.1.36-1.fc38.src.rpm Welcome back, let's finish this ancient saga. It's great that Fedora is using SPDX now, I've updated License and also rebuilt the package for f38/rawhide. > I still do not like fact the package automatically modifies root user. I find it strange, root should be able to modify all packaged files anyway. Why is that needed? The root user is added to the group to get access to the SHM files created by any other users in /dev/shm. It cannot access them normally because of the kernel parameter fs.protected_regular being set to 2 by default.
What kind of access does root needs to other users' files? What does root need to do with those files? Would it be possible if the running service did such access on root's behalf, where it would already have the group in question? Is that requirement described somewhere in documentation? What exactly it creates in /dev/shm for root user and for unprivileged users? And in which cases it needs access from root user to files created by other users? Sounds like strange use-case.
Upstream response: > What kind of access does root needs to other users' files? What does root need to do with those files? The root needs both read and write access because the SHM files are used just like standard files for better efficiency. In general, these files are used for all IPC and the root user is supposed to be the privileged one that has unlimited access and can communicate with all the other sysrepo clients (users). This has worked fine before until the aforementioned kernel parameter was introduced a year or two ago. > Would it be possible if the running service did such access on root's behalf, where it would already have the group in question? I do not understand, what exactly is "the running service"? This service must run with the root user, we have already agreed on that. > Is that requirement described somewhere in documentation? What exactly it creates in /dev/shm for root user and for unprivileged users? And in which cases it needs access from root user to files created by other users? Sounds like strange use-case. No, this requirement is not described anywhere in the documentation because it is a standard UNIX expectation of the root user having access to everything on the system. Generally speaking, all the SHM files can be accessed by all the sysrepo users because the permissions are checked manually by sysrepo. The special group is used mainly to enhance security by not allowing direct file tampering by other system users.
What you refer to is covered by capabilities CAP_DAC_OVERRIDE and CAP_DAC_READ_SEARCH, which are NOT granted to services on Fedora. Systemd just do not grant those capabilities even to services running under root. Do I undestand it well this is limitation of sysrepo package and not directly in netopeer2? I have found a reference [1], but they are described in man 7 capabilities. I think this might be a blocker, I will have to ask someone smarter what are guidelines for similar services. Perhaps I have to read a bit more about sysrepo, what it does and how. 1. in https://fedoraproject.org/wiki/SELinux/Unsound_or_dangerous_SELinux_policy_practices
As I wrote in direct message to Michal, please consider using SupplementaryGroups=sysrepo or Group=sysrepo parameter in netopeer service instead of adding root user permanently to that group. I haven't tried it yet, but I expect it should work fine. Is there a reason why it is not used already?
I've applied changes from upstream `devel` branch: - Drop .sysusers file in favor of - SupplementaryGroups=sysrepo in .service - Fix path in .service (s#bindir#sbindir#) I've updated review files (.src.rpm/.spec/.service) in their original locations. Upstream is planning to release new versions of libs soon-ish so if there are no more issues, I could include netopeer2 in the update.
Okay, looks good. I think the package is now ready to be build, thanks for your patience! Giving review+
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/netopeer2
FEDORA-2022-7f3fe665d6 has been submitted as an update to Fedora 38. https://bodhi.fedoraproject.org/updates/FEDORA-2022-7f3fe665d6
FEDORA-2022-7f3fe665d6 has been pushed to the Fedora 38 stable repository. If problem still persists, please make note of it in this bug report.