https://bugzilla.redhat.com/show_bug.cgi?id=1756582 --- Comment #12 from Michal Schorm <mschorm@xxxxxxxxxx> --- Alright, I went through the review again. This time I also checked & compared the RPM built by upstream. ----- 1) Requires & Provides Once it started building for systemd correctly, the Requires and Provides are sane now. Checked in a directory with only resulting rpms: # for i in `ls -1`; do echo -e "\t$i" ; rpm -q --requires $i ; echo " " ; done # for i in `ls -1`; do echo -e "\t$i" ; rpm -q --provides $i ; echo " " ; done I won't paste it whole here, because I don't see the point in that. Just the highlights: Requires for sshguard-2.4.0-9.fc32.x86_64.rpm systemd Requires for sshguard-firewalld-2.4.0-9.fc32.x86_64.rpm firewalld sshguard Requires for sshguard-iptables-2.4.0-9.fc32.x86_64.rpm iptables-services sshguard Requires for sshguard-nftables-2.4.0-9.fc32.x86_64.rpm nftables sshguard Provides of sshguard-2.4.0-9.fc32.x86_64.rpm bundled(fnv) = 5.0.2 bundled(simclist) = 1.4.4 1.1) However, when checking up the RPM package built by the sshguard upstream, the sshguard package also requires: coreutils diffutils fillup grep openssh The first four probabbly as a dependencies of the shell scripts. Please check if they are needed and if yes, add those requires. 1.2) One more suggestion: since you are using systemd in scriptlets, you should also use: %{?systemd_requires} in the main package, which is macro that expands to: Requires(post): systemd Requires(preun): systemd Requires(postun): systemd That will guarantee, the systemd will be there also at the time the scriptlets run, not just when the package is installed on the system. ----- 2) Files & file ownership All files are now owned by the package as they should be. 2.1) However, the upstream package has better structured docs. usr └── share └── doc └── packages └── sshguard ├── CHANGELOG.rst ├── doc │ ├── sshguard.8 │ ├── sshguard.8.rst │ ├── sshguard-setup.7 │ └── sshguard-setup.7.rst ├── examples │ ├── net.sshguard.plist │ ├── sshguard.conf.sample │ ├── sshguard.service │ └── whitelistfile.example └── README.rst I'd follow them and add an "examples" folder 2.2) What concerns me, the upstream package also owns └── var └── lib └── sshguard └── db "/var/lib/<package>" holds "State data for packages and subsystems (optional)." Please check, if the package need to save some data like this and if it really needs theese directories. If yes, create and add them. 2.3) Also the config file from the upstream upstream package is IMHO more helpful, describing more options. I'd love to see it in Fedora too. /etc/sshguard.conf | #### OPTIONS #### | # Block attackers when their cumulative attack score exceeds THRESHOLD. | # Most attacks have a score of 10. (optional, default 30) | THRESHOLD=30 | | # Block attackers for initially BLOCK_TIME seconds after exceeding THRESHOLD. | # Subsequent blocks increase by a factor of 1.5. (optional, default 120) | BLOCK_TIME=120 | | # Remember potential attackers for up to DETECTION_TIME seconds before | # resetting their score. (optional, default 1800) | DETECTION_TIME=1800 | | # Size of IPv6 'subnet to block. Defaults to a single address, CIDR notation. (optional, default to 128) | IPV6_SUBNET=128 | | # Size of IPv4 subnet to block. Defaults to a single address, CIDR notation. (optional, default to 32) | IPV4_SUBNET=32 | | #### EXTRAS #### | # !! Warning: These features may not work correctly with sandboxing. !! | | # Full path to PID file (optional, no default) | PID_FILE="/run/sshguard.pid" | | # Colon-separated blacklist threshold and full path to blacklist file. | # (optional, no default) | BLACKLIST_FILE="90:/var/lib/sshguard/db/blacklist.db" | | # IP addresses listed in the WHITELIST_FILE are considered to be | # friendlies and will never be blocked. | WHITELIST_FILE="/etc/sshguard/whitelist" 2.4) And the config also point at the whitelist, which holds nice description and example of what and how to configure there. Also a very nice possible feature for this Fedora package. /etc/sshguard/whitelist | # comment line (a '#' as very first character) | # a single ip address | #1.2.3.4 | # address blocks in CIDR notation | #127.0.0.0/8 | #10.11.128.0/17 | #192.168.0.0/24 | # hostnames | #rome-fw.enterprise.com | #hosts.friends.com | # 2.5) Also, you pack a file "/usr/share/doc/sshguard/net.sshguard.plist", which is IMHO for MacOS and thus is completely irrelevant to Fedora (thus shouldn't be there): https://fileinfo.com/extension/plist Please verify my finding and if I'm correct, don't pack the file. 2.6) Please, prefix the directory you pack (line 193) with "%dir", like: %dir %{_libexecdir}/%{name} ----- 3) > simclist: The 1.6 version of simclist most likely comes from the author's page on sourceforge [1], it was released in 2011, so similarly ancient. > I'll contact the dev to verify. That would be awesome. ----- >From my POV the packages becomes slowly more or less ready to be included in Fedora. However beacuse the review isn't just about package GO/NOGO, but also it should show your understanding of the packager job and packaging guidelines (and ability to comply with them) - especcialy if you are looking for a sponsorship - I won't grant you the "fedora-review +" ACK yet. Let's take a look at what I found this time. 4) Also can you share some script (, setup or instructions), how do you test the package works as expected - blocking the attack over ssh? I'd be interested :) It could be then added to the Fedora Continuous Integration Testing (https://docs.fedoraproject.org/en-US/ci/), once the package would get into Fedora -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
