[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582 Christopher Engelhard changed: What|Removed |Added Status|POST|CLOSED Resolution|--- |RAWHIDE Last Closed||2019-12-20 21:54:50 -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582
--- Comment #18 from Christopher Engelhard ---
> typo ;)
Fixed.
> You only need systemd-rpm-macros, which is much more lightweight.
Thanks. I've changed the deps for f30+, f29 & epel still ship those macros as
part of the systemd package, unless I'm very much mistaken.
> > # for scriptlets
> > %{?systemd_requires}
>
> The guidelines changed on that a while back, and this is only necessary when
> using
> some other systemd tools like systemd-tmpfiles or systemd-sysusers, which you
> are
> not. Please remove. [https://pagure.io/packaging-committee/pull-request/890]
Done.
> You should run 'fedpkg request-repo' and 'fedpkg request-branch' now.
Done, it all seems to be working fine, though I haven't triggered a build yet.
https://src.fedoraproject.org/rpms/sshguard/blob/master/f/sshguard.spec
--
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@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
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@lists.fedoraproject.org
[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582 --- Comment #17 from Gwyn Ciesla --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/sshguard -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582 --- Comment #15 from Christopher Engelhard --- OK, that makes sense, re: dependencies. Thanks for your extensive review, I'll start looking for sponsors. I'll let the people who bundle simclist know about the version status once I get hold of the dev. Let me know if you want to be kept up-to-date as well. -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582 Michal Schorm changed: What|Removed |Added Flags|fedora-review? |fedora-review+ --- Comment #14 from Michal Schorm --- In Fedora, we have a "minimal buildroot". Set of packages that are always present for any build. Currectly, it consist of ~150 packages and you can check them e.g. in mock, when you init a rawhide environment, then you shell inside and run "rpm -qa". I'm, however, not aware of any "minimal root". You can install your package alone, by e.g. "dnf install --releasever="30" --installroot="/tmp/empty_test_area/" sshguard-2.4.0-10.fc32.x86_64.rpm". It will pull in tree of the dependencies you specified, but I wouldn't trust that all of the runtime dependecies your package need will be magically pulled in. Let's say: 'mkfs' a very usefull and standard utility. If you'd depend on the feeling, all mkfs binaries are from the same package, you'd be mistaken. Just run "rpm -qf /usr/sbin/mkfs*" to see yourself. Last real-world example, why knowing your dependencies pays off: Time to time, your dependency tree changes, as the packages you depend on develop or dies. Suddenly, your package stops building. Searching for why uncover, that your dependency used zlib. You used it also for your package. But because you haven't specified it as a buildrequires, and the package you depend on changed in a way it doesn't need zlib anymore, it won't successfuly build anymore. --- I like how the package looks now. I'm granting you the "fedora-review +". I also believe, you are worthy of the packager status, but since I'm not a Sponsor, I can't grant it to you. Yoou need to find a sponsor. If you'd have any questions, feel free to ask me. Also I recommend Fedora IRC channels and mailing lists, where you can find help, answers, guidance and explanations. -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582
--- Comment #13 from Christopher Engelhard ---
Sorry for the delay, work intruded. Also, thanks again for the time & effort
you put into this.
- Just to clarify, afaik the packages linked on the sshguard website are not
built/maintained by upstream, though the debian package might be derived from
their 'debian' branch.
- Regarding 1.1, is there something similar to Arch's base-devel group, i.e. a
set of packages that forms a standard build environment and is thus assumed to
be present at buildtime and thus not listed explicitly? Anything using a
configure script is likely to require coreutils and diffutils for example, but
I don't usually see them as builddeps.
spec: https://gitlab.com/lcts-rpm/sshguard/raw/sshguard-2.4.0-10/sshguard.spec
diff:
https://gitlab.com/lcts-rpm/sshguard/compare/sshguard-2.4.0-9...sshguard-2.4.0-10
srpm:
https://copr-be.cloud.fedoraproject.org/results/lcts/sshguard/fedora-rawhide-x86_64/01045896-sshguard/sshguard-2.4.0-10.fc32.src.rpm
1.1) Additional dependencies.
fixed.
coreutils - 'echo' is used in /usr/sbin/sshguard
diffutils - 'diff' and 'cmp' are used at buildtime (in 'configure' and
'ylwrap', respectively), see question above
fillup- not needed. opensuse-specific dependency, I don't know what they
use it for.
grep - grep is used by /usr/sbin/sshguard
openssh - not needed. People might in fact want to monitor e.g. ftp on a
host without (a need for) ssh
so I'd say it's bad to include this as a dependency. If ssh is
missing/not running, the configs if unmodified
will cause sshguard to monitor an empty log, which it is
perfectly happy to do
1.2) %{?systemd_requires}
fixed - nice, learned something new.
2.1) the upstream package has better structured docs.
fixed:
%{_pkgdocdir}/sshguard/
|- CONTRIBUTING.rst
|- README.rst
|- examples/
|- sshguard.conf.sample
|- whitelistfile.example
2.2) /var/lib/sshguard/db ownership
- used by the blacklist feature in the opensuse package, but not expected or
created by sshguard on its own
- not upstream's suggested path for the blacklist, they put it directly in
/var/lib/sshguard/. According to the spec .../db is an opensuse default
- see 2.4
2.3) better config file
fixed - I adapted examples/sshguard.conf.sample, which is extensively commented
2.4) white/blacklisting
I decided to set up the package so that it is prepared for this feature, but
leaves it disabled. That way, a user can simply uncomment the relevant lines in
the config file without further configuration
- added /etc/sshguard.whitelist containing only commented instructions for use
- added /var/lib/sshguard as package-owned dir
- preset blacklist file to /var/lib/sshguard/blacklist, which will be created
by sshguard if bl is enabled. It's not owned by the package, so RPM will delete
the folder if the user didn't enable it, but retain the blacklist file if they
did
2.5) net.sshguard.plist
correct, removed
2.6) prefix the directory you pack with "%dir"
fixed
3) simclist
I haven't heard back yet.
4) CI testing
It's a bit convoluted right now:
- 'make check' already checks that all log parsing / pattern matching is
working as expected
- GitLab CI (config:
https://gitlab.com/lcts-rpm/sshguard/blob/master/.gitlab-ci.yml) tests
installation and basic functionality
- set up minimal docker installations of the various official CentOS & Fedora
images on docker hub
- build & install the (sub-)package(s)
- test that systemctl/service start|stop|status works without issues
- lint the packages
- sshguard-testing repo on copr tests that the build also works on other
architectures
- I have a tiny cloud server that uses those test builds and checks that the
package is safe to use (for 'checks', read: it installs & uses the test build
and I test if I can lock myself out etc.)
GitLab CI is triggered by commits, test builds on copr by commits to master,
test server updates via dnf-automatic.
It should not be too difficult to integrate all that into a single pipeline - I
only use copr+gitlab ci because the latter only does x86_64. Simulating an
attack programmatically is certainly possible. If ssh is available, one could
probably just try to log into localhost with fake credentials to trigger a
block. I'll have to test that a bit.
I'll have a look at fedora's CI ... it uses ansible, so that's a plus already
...
--
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@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
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:
[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582
--- Comment #12 from Michal Schorm ---
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/" 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)
[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582 Christopher Engelhard changed: What|Removed |Added Flags|needinfo?(c...@lcts.de) | --- Comment #11 from Christopher Engelhard --- Thanks for sniffing around for fnv/simclist, also for setting FE-NEEDSPONSOR, I forgot that. I've added fnv & simclist as bundled provides. spec: https://copr-be.cloud.fedoraproject.org/results/lcts/sshguard/fedora-rawhide-x86_64/01042870-sshguard/sshguard.spec diff 2.4.0-8 -> 2.4.0-9: https://gitlab.com/lcts-rpm/sshguard/compare/sshguard-2.4.0-8...sshguard-2.4.0-9 src.rpm: https://copr-be.cloud.fedoraproject.org/results/lcts/sshguard/fedora-rawhide-x86_64/01042870-sshguard/sshguard-2.4.0-9.fc32.src.rpm fnv: sshguard doesn't bundle the entire library, only one specific 32bit hash function. Is it still OK to use bundled(fnv)? 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. [1] http://freshmeat.sourceforge.net/projects/simclist -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582 Robert-André Mauchin changed: What|Removed |Added Status|ASSIGNED|NEW Flags|needinfo?(zebo...@gmail.com | |) | --- Comment #9 from Robert-André Mauchin --- Sorry I don't know why DEADREVIEW was copied from the duplicate. -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582 --- Comment #8 from Conrad Meyer --- (In reply to Michal Schorm from comment #7) > I belive this BZ doesn't deserve "FE-DEADREVIEW" status since it is days old > and actively worked on. > Haven't you meant different BZ? > I did set the "FE-NEEDSPONSOR" as the reporter stated that he is looking for > a sponsor. > -- > I removed the "FE-DEADREVIEW" flag. > If you set it intentionaly to this BZ, please provide further explanation. > Thank you Hi Michal, I believe DEADREVIEW was accidentally copied from the duplicate bug (my original submission from 2015 that languished without reviewers for years). Clearing it on this one is correct. Thanks Michal and Christopher for taking this on. Best, Conrad -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582 Michal Schorm changed: What|Removed |Added CC||zebo...@gmail.com Blocks|201449 (FE-DEADREVIEW) | Flags||needinfo?(zebo...@gmail.com ||) --- Comment #7 from Michal Schorm --- Hello Robert-André Mauchin. I belive this BZ doesn't deserve "FE-DEADREVIEW" status since it is days old and actively worked on. Haven't you meant different BZ? I did set the "FE-NEEDSPONSOR" as the reporter stated that he is looking for a sponsor. -- I removed the "FE-DEADREVIEW" flag. If you set it intentionaly to this BZ, please provide further explanation. Thank you Referenced Bugs: https://bugzilla.redhat.com/show_bug.cgi?id=201449 [Bug 201449] FE-DEADREVIEW -- Reviews stalled due to lack of submitter response should be blocking this bug. -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582 Robert-André Mauchin changed: What|Removed |Added Blocks||201449 (FE-DEADREVIEW) CC||cse.cem+redhatb...@gmail.co ||m --- Comment #6 from Robert-André Mauchin --- *** Bug 1260845 has been marked as a duplicate of this bug. *** Referenced Bugs: https://bugzilla.redhat.com/show_bug.cgi?id=201449 [Bug 201449] FE-DEADREVIEW -- Reviews stalled due to lack of submitter response should be blocking this bug. -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582 --- Comment #5 from Michal Schorm --- 2.5) Personally, I see as the most important having an usefull error message on a expected and easy-to-find place. The program provides such message, so I'm absolutelly fine with that. I primarly wanted to find out if that's expected. 4) > Should I open a new review request for each and link to them from here? If you would pack them to Fedora, definitelly yes - it needs a standard new package review. I took a look at the libraries. I grepped SPECfiles of all of the pcakges in Rawhide; tarball of just those SPECs can be found here: http://src.fedoraproject.org/repo/rpm-specs-latest.tar.xz (Mentioning just in case you'd find it handy later in your packager life) The 'fnv' library is packed in Fedora only as a rust version, which won't help us much. https://src.fedoraproject.org/rpms/rust-fnv However ... as I'm strolling through the author's pages (http://www.isthe.com/chongo/tech/comp/fnv/#FNV-reference-source), I see the latest version: fnv-5.0.3.tar.gz [updated: 2012 May 20] so it's not upated much often :) In this case, I'd be fine with bundling it; even though it's a MUST to properly mark it as bundled in the SPEC. With a short justification. There is another package which bundles the 'simclist' library: https://src.fedoraproject.org/rpms/pcsc-lite-acsccid/blob/master/f/pcsc-lite-acsccid.spec#_22 It's not clear from the SPEC, but in its pcakage review, the reason is stated as purpose not strong enough to pack it as a standalone package. UPDATE: * I've contacted it't maintainer and ve uncovered, there's were more packages which were missing the mention of the bundling. Since the 'simclist' is not updated from 2010, I'm fine to bundle it too. http://mij.oltrelinux.com/devel/simclist/#downloadinstall http://mij.oltrelinux.com/devel/simclist/simclist-1.5-changes.txt This should be reconsidered once more packages that would require it would appear. UPDATE: * The latest upstream version is 1.5, however there are project on the internet (and in Fedora and Debian too), which has version 1.6, that appeared ... somewhere. Hopefully as a typo, but it may be worth deeper investigation and eventually need to ask upstream to release a bumped version to keep the sync. -- Mark the bundles correctly for both packages. After that I'll re-do the review. But it looks promising now :) -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582
--- Comment #4 from Christopher Engelhard ---
Hi Michal, thanks for the review & detailed feedback.
Issues:
===
1) The package does not own '/usr/libexec/sshguard' directory
Reply: Fixed.
2) systemd vs sysvinit
2.1) Fix the condition
Reply: I swear, one day I will wtite a flawless rpm macro ...
I reverted it to checking for el6, specifically. I think the package will also
work on older RHELs, but I have never checked.
2.2) Logrotate is used only with sysvinit and not with systemd - is that an
intention?
Reply: Yes. Usually, sshguard expects to run with systemd and logs via
journald. I only have it log to a file on SysVInit.
2.3) When the package is build with systemd service instead of sysvinit script,
do you think it still worth to ship the example [...] file [...]?
Reply: You're right. I don't remember why I switched to creating my own service
file. I changed it back to using upstream's example.
2.4) Please note, that the base package contains systemd service file, but the
package does not require systemd. [...]
Reply: Fixed. It should require systemd unless run on a sysvinit system.
2.5) The systemd service contains e.g. "After=firewalld.service". If the
service is not present or not started, this won't have any effect. [...] Check
if that's OK.
Reply: I'd say it's OK, but that is debatable. The After= lines are really just
there to ensure proper ordering IF the backend is present. If people use the
backend-subpackages, the appropriate service will be pulled in as a dependency.
If they're not, they will in any case have to configure sshguard themselves,
including which backend to use, so I think it's fair to assume they'll also
install that backend, or if not, understand what went wrong.
Please advise.
3) I'd suggest to have every changelog entry (each header) separated by a
newline [...].
Reply: Fixed.
4) I saw two bundled libraries that I suspect they are bundled, can you please
confirm?
Reply: Yes, that seems to be the case, I overlooked that. I don't think they're
in Fedora, I'll package them separately if necessary and get back to you.
Should I open a new review request for each and link to them from here?
spec correcting {1-3):
https://copr-be.cloud.fedoraproject.org/results/lcts/sshguard-testing/fedora-rawhide-x86_64/01042441-sshguard/sshguard.spec
diff: https://gitlab.com/lcts-rpm/sshguard/compare/sshguard-2.4.0-8...master
src package:
https://copr-be.cloud.fedoraproject.org/results/lcts/sshguard-testing/fedora-rawhide-x86_64/01042441-sshguard/sshguard-2.4.0-8.git.5.9154ef5.fc32.src.rpm
I'll make a new package release once I've taken care of (4).
--
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@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
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@lists.fedoraproject.org
[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582 --- Comment #2 from Michal Schorm --- Package Review == Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated Issues: === 1) The package does not own '/usr/libexec/sshguard' directory, but it should. 2) Systemd X SysVinit The package has an option to not build sysvinit stuff. Which it fine - we want systemd files in Fedora instead. However the condition on the first 3 lines is broken. On Fedora it will expand to "0 <= 6", which is always TRUE, so it will always pack sysvinit stuff over systemd stuff. 2.1) Fix the condition 2.2) Logrotate is used only with sysvinit and not with systemd - is that an intention? 2.3) When the package is build with systemd service instead of sysvinit script, do you think it still worth to ship the example '/usr/share/doc/sshguard/sshguard.service' file, even if it's nearly the same as the actual service file? 2.4) Please note, that the base package contains systemd service file, but the package does not require systemd. Thus it can end up in a state, when it is installed, but systemd is not. If the main functionality remains even without systemd, it's fine. If the functionality depends on the service, you need to fix the package requirements. Please check if it's OK. Also note that the e.g. teh firewalld subpackage depends on firewalld which depends on systemd ... so in that case the systemd will be pulled in. 2.5) The systemd service contains e.g. "After=firewalld.service". If the service is not present or not started, this won't have any effect. Thus you can end up with sshguard service running but firewalld service not running; and an error message in the systemd journal: "sshguard[1518]: sshg-fw-firewalld: Could not initialize firewall" Check if that's OK. 3) I'd suggest to have every changelog entry (each header) separated by a newline, to have it consistent with both itself and all other pkgs in Fedora. 4) I saw two bundled libraries that I suspect they are bundled, can you please confirm? * simclist library? * Fowler/Noll/Vo Hash (fnv) library ? They are mentioned here too: https://bitbucket.org/sshguard/sshguard/src/036efe21bd46122fde9d3d85aa71ee72b4c8d7e4/COPYING And i see them amongst the debugsource files, so they were used during the build process. If they are packed in Fedora, those packages MUST be used instead. If they are not packaged in Fedora, you MUST pack them too. Only in very special cases, when neither of those two steps above are a good solution, you may bundle it. But in such case you MUST mark that the package provide those bundles. = 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. [?]: License field in the package spec file matches the actual license. "sshguard is available under the terms of the OpenBSD license, which is based on the ISC License." The "OpenBSD" license is not specified here: https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#SoftwareLicenses, so it need aditional check. [x]: License file installed when any subpackage combination is installed. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [!]: Package requires other packages for directories it uses. Note: No known owner of /usr/libexec/sshguard [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/libexec/sshguard [x]: %build honors applicable compiler flags or justifies otherwise. [!]: 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. [x]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [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. [x]: Spec file is legible and written in American English. [!]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to
[Bug 1756582] Review Request: sshguard - Protect hosts from brute-force attacks
https://bugzilla.redhat.com/show_bug.cgi?id=1756582 Michal Schorm changed: What|Removed |Added Status|NEW |ASSIGNED CC||msch...@redhat.com Assignee|nob...@fedoraproject.org|msch...@redhat.com Flags||fedora-review? --- Comment #1 from Michal Schorm --- Hello, I'm a Fedora packager and I'll do this review. I'll test it and I'll post my findings no later than overmorrow. -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
