Bug 2393738

Koji Build: https://koji.fedoraproject.org/koji/taskinfo?taskID=136871421

> %preun
> %systemd_preun caddy.service
> 
> %postun
> %systemd_postun_with_restart caddy.service

`caddy.service` seems a leftover.

The spec is correct once the `caddy.service` problem is fixed, but I've a few questions:

> AmbientCapabilities=CAP_NET_ADMIN CAP_NET_BIND_SERVICE

Why `CAP_NET_ADMIN` is required?

> %attr(0644,root,root) %config(noreplace) %{_sysconfdir}/smtprelay.ini

wouldn't it better to be 0600 as the file stores passwords?

Also, why run this as root rather than creating a `smtprelay` user/group?

Spec URL: https://neelc.org/fedora/smtprelay2/smtprelay.spec
SRPM URL: https://neelc.org/fedora/smtprelay2/smtprelay-1.12.0-1.fc44.src.rpm
Source1: https://neelc.org/fedora/smtprelay2/smtprelay-1.12.0-vendor.tar.bz2
Source2: https://neelc.org/fedora/smtprelay2/go-vendor-tools.toml
Source3: https://neelc.org/fedora/smtprelay2/smtprelay.service
Source4: https://neelc.org/fedora/smtprelay2/smtprelay.sysusers

Thank you so much for your comments. I addressed them.

Please approve/comment.

Copr build:
https://copr.fedorainfracloud.org/coprs/build/9533171
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2393738-smtprelay/fedora-rawhide-x86_64/09533171-smtprelay/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Created attachment 2105950 [details]
The .spec file difference from Copr build 9533171 to 9533176

Copr build:
https://copr.fedorainfracloud.org/coprs/build/9533176
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2393738-smtprelay/fedora-rawhide-x86_64/09533176-smtprelay/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

- Add the following after "%global gomodulesmode GO111MODULE=on"

export GO_LDFLAGS="-X main.appVersion=%{version} \
                   -X main.buildTime=$(date -d "@${SOURCE_DATE_EPOCH}" +%Y-%m-%dT%H:%M:%SZ)"

Otherwise `smtprelay --version` doesn't report a correct value.

- (optional) Adjust permissions for smtprelay.ini

From: 
> %attr(0644,root,root) %config(noreplace) %{_sysconfdir}/smtprelay.ini

To:
%attr(0640,root,smtprelay) %config(noreplace) %{_sysconfdir}/smtprelay.ini

- (optional) You can enhance the service with the following params:

CapabilityBoundingSet=CAP_NET_BIND_SERVICE
NoNewPrivileges=yes
ProtectSystem=strict
ProtectHome=yes
PrivateTmp=yes
PrivateDevices=yes
RestrictSUIDSGID=yes
Restart=on-failure
RestartSec=5s

Spec URL: https://neelc.org/fedora/smtprelay3/smtprelay.spec
SRPM URL: https://neelc.org/fedora/smtprelay3/smtprelay-1.12.0-1.fc44.src.rpm

I made the changes (again). Please approve/comment.

Created attachment 2106001 [details]
The .spec file difference from Copr build 9533176 to 9534346

Copr build:
https://copr.fedorainfracloud.org/coprs/build/9534346
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2393738-smtprelay/fedora-rawhide-x86_64/09534346-smtprelay/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Based on the documentation, the user and group creation requires some extra bits:

https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/#_creation_of_users_and_groups_with_scriptlets

- The %install part is not there:

install -p -D -m 0644 %{SOURCE4} %{buildroot}%{_sysusersdir}/smtprelay.conf

- The %files part is missing:

%{_sysusersdir}/smtprelay.conf

Beware that file needs to end with ".conf".

I think this is the last change required.

Spec URL: https://neelc.org/fedora/smtprelay4/smtprelay.spec
SRPM URL: https://neelc.org/fedora/smtprelay4/smtprelay-1.12.0-1.fc44.src.rpm

Is this good? I added the lines.

(Drive by suggestion about the config file. Thanks Mikel for the detailed review!)

The smtprelay.ini is 0644, but I assume it has sensitive data (i.e., mail server credentials) and shouldn't be world readable. I suppose the proper solution would be to make it `%attr(0640,root,smtprelay)`. Also, I see an `allowed_users` option is allowed which could point to another config file. Would it make sense to store smtprelay.ini in /etc/smtprelay instead of the parent /etc directory so, if needed, users could drop that additional file in the same directory?

```
diff --git a/smtprelay.spec b/smtprelay.spec
index 9bb231e..d02022e 100644
--- a/smtprelay.spec
+++ b/smtprelay.spec
@@ -54,11 +54,16 @@ export GO_LDFLAGS="-X main.appVersion=%{version} \
 
 %install
 %go_vendor_license_install -c %{S:2}
+# Binary
 install -m 0755 -vd                             %{buildroot}%{_bindir}
 install -m 0755 -vp %{gobuilddir}/bin/*         %{buildroot}%{_bindir}/
+# Config file
 install -m 0755 -vd                             %{buildroot}%{_sysconfdir}
-install -m 0755 -vp smtprelay.ini %{buildroot}%{_sysconfdir}/
-install -D -p -m 0640 %{S:3} %{buildroot}%{_unitdir}/smtprelay.service
+install -m 0750 -vd                             %{buildroot}%{_sysconfdir}/smtprelay
+install -m 0640 -vp smtprelay.ini %{buildroot}/%{_sysconfdir}/smtprelay
+# Systemd unit and sysusers
+install -D -p -m 0644 %{S:3} %{buildroot}%{_unitdir}/smtprelay.service
+install -D -p -m 0644 %{S:4} %{buildroot}%{_sysusersdir}/smtprelay.conf
 
 %check
 %go_vendor_license_check -c %{S:2}
@@ -82,7 +87,9 @@ install -D -p -m 0640 %{S:3} %{buildroot}%{_unitdir}/smtprelay.service
 %license vendor/modules.txt
 %doc README.md SECURITY.md
 %{_bindir}/smtprelay
-%attr(0644,root,root) %config(noreplace) %{_sysconfdir}/smtprelay.ini
+%dir %attr(0750,root,smtprelay) %config(noreplace) %{_sysconfdir}/smtprelay
+%attr(0640,root,smtprelay) %config(noreplace) %{_sysconfdir}/smtprelay/smtprelay.ini
+%{_sysusersdir}/smtprelay.conf
 %{_unitdir}/smtprelay.service
 
 %changelog
```

Spec URL: https://neelc.org/fedora/smtprelay/smtprelay.spec
SRPM URL: https://neelc.org/fedora/smtprelay/smtprelay-1.12.0-1.fc44.src.rpm

Good suggestions, Maxwell!

I have updated my spec file with your suggestions too.

Created attachment 2106019 [details]
The .spec file difference from Copr build 9534688 to 9534711

Copr build:
https://copr.fedorainfracloud.org/coprs/build/9534688
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2393738-smtprelay/fedora-rawhide-x86_64/09534688-smtprelay/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Created attachment 2106021 [details]
The .spec file difference from Copr build 9534688 to 9534711

Copr build:
https://copr.fedorainfracloud.org/coprs/build/9534711
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2393738-smtprelay/fedora-rawhide-x86_64/09534711-smtprelay/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Thanks for the suggestion Maxwell and thanks Neel for you patience :)

Golang Package Review
==============

This package was generated using go2rpm and Go Vendor Tools, which simplifies the review.

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated


- [x] The latest version is packaged or packaging an earlier version is justified.
- [x] The License tag reflects the package contents and uses the correct identifiers.
- [x] The package builds successfully in mock.
- [x] Package is installable (checked by fedora-review).
- [x] There are no relevant rpmlint errors.
- [x] The package runs tests in %check.
- [x] `%goipath` is set correctly.
- [x] The package's binaries don't conflict with binaries already in the distribution. (Some Go projects include utility binaries with very generic names)
- [x] There are no `%{_bindir}/*` wildcards in %files. (go2rpm includes these by default)
- [x] The package does not use `%gometa -f` if it has dependents that still build for %ix86.
- [x] The package complies with the Golang and general Packaging Guidelines.

Package approved! On import, don't forget to do the following:

- [ ] Add the package to release-monitoring.org
- [ ] Give go-sig privileges (at least commit) on the package
- [ ] Close the review bug by referencing its ID in the rpm changelog and the Bodhi ticket.
- [ ] Consider configuring Packit service to help with maintenance, you can use https://src.fedoraproject.org/fork/mikelo2/rpms/google-guest-agent/blob/c6d7e14023aa4115e86aaea3f10596c58e1bc81a/f/.packit.yaml as template

Thanks!

The Pagure repository was created at https://src.fedoraproject.org/rpms/smtprelay

FEDORA-2025-0ea8c734f1 (smtprelay-1.12.0-1.fc44) has been submitted as an update to Fedora 44.
https://bodhi.fedoraproject.org/updates/FEDORA-2025-0ea8c734f1

FEDORA-2025-0ea8c734f1 (smtprelay-1.12.0-1.fc44) has been pushed to the Fedora 44 stable repository.
If problem still persists, please make note of it in this bug report.