dhcp/kea
SHA256
7
0
Fork 3
forked from pool/kea
Code Pull Requests Activity

Update services, user, group and dir access #2

Manually merged
jengelh merged 1 commits from jcronenberg/kea:master into master 2025-04-15 14:16:00 +02:00
Conversation 10 Commits 1 Files Changed 8 +150 -78
jcronenberg commented 2025-04-09 14:59:11 +02:00
Owner
Copy Link
  • Split off services into separate ones to allow more fine grained
    control for e.g. capabilities.
  • Add new kea user and group under which these services will run.
  • Tighten access to state and log directories
- Split off services into separate ones to allow more fine grained control for e.g. capabilities. - Add new kea user and group under which these services will run. - Tighten access to state and log directories
jcronenberg added 1 commit 2025-04-09 14:59:11 +02:00
Update services, user, group and dir access ab5bf5d6a7 
- Split off services into separate ones to allow more fine grained
  control for e.g. capabilities.
- Add new kea user and group under which these services will run.
- Tighten access to state and log directories
jcronenberg requested review from jengelh 2025-04-09 14:59:11 +02:00
jengelh commented 2025-04-09 15:28:14 +02:00
Collaborator
Copy Link

These services are incompatible with the existing kea.service though and I don't see that reflected in the units.

(Right, that's because kea.service is gone)

~~These services are incompatible with the existing kea.service though and I don't see that reflected in the units.~~ (Right, that's because kea.service is gone)
jcronenberg force-pushed master from ab5bf5d6a7 to 0d283cf070 2025-04-09 16:41:19 +02:00 Compare
jengelh reviewed 2025-04-10 15:06:18 +02:00
kea.spec Outdated
@@ -399,0 +384,4 @@
install -D -m 0644 %{SOURCE4} %{buildroot}%{_unitdir}/kea-dhcp4.service
install -D -m 0644 %{SOURCE5} %{buildroot}%{_unitdir}/kea-dhcp6.service
install -D -m 0644 %{SOURCE6} %{buildroot}%{_unitdir}/kea-dhcp-ddns.service
install -D -m 0644 %{SOURCE7} %{buildroot}%{_unitdir}/kea-ctrl-agent.service
jengelh commented 2025-04-10 15:06:18 +02:00
Collaborator
Copy Link

8 9 4 5 6 7, this is terrible.

8 9 4 5 6 7, this is terrible.
jcronenberg marked this conversation as resolved
jengelh reviewed 2025-04-10 15:07:00 +02:00
kea.spec Outdated
@@ -415,0 +409,4 @@
%service_add_post kea-ctrl-agent.service
if [ $1 -gt 1 ]; then
chown -R kea:kea %{_sharedstatedir}/kea
chown -R kea:kea %{_localstatedir}/log/kea
jengelh commented 2025-04-10 15:07:00 +02:00
Collaborator
Copy Link

security-team will not like this

security-team will not like this
jcronenberg commented 2025-04-10 15:10:33 +02:00
Author
Owner
Copy Link

I have checked it with a few people, because I also wasn't sure of this, but it seems to be the best possible solution or what would you suggest instead?

I have checked it with a few people, because I also wasn't sure of this, but it seems to be the best possible solution or what would you suggest instead?
jengelh commented 2025-04-10 15:16:42 +02:00
Collaborator
Copy Link

just don't change the username. "kea" is even so short someone could be using it as a normal user (and openSUSE does not use _kea username notation like other platforms) hence the original decision to go for "keadhcp".

just don't change the username. "kea" is even so short someone could be using it as a normal user (and openSUSE does not use `_kea` username notation like other platforms) hence the original decision to go for "keadhcp".
jcronenberg commented 2025-04-10 15:20:29 +02:00
Author
Owner
Copy Link

It doesn't matter if it's kea or keadhcp, I chose kea because it's what upstream and pretty much all other distros use, no need for suseism here. And even if I don't change it, these lines would still be necessary, because the files in these dirs are currently owned by root:root not keadhcp:keadhcp.

It doesn't matter if it's `kea` or `keadhcp`, I chose `kea` because it's what upstream and pretty much all other distros use, no need for suseism here. And even if I don't change it, these lines would still be necessary, because the files in these dirs are currently owned by `root:root` not `keadhcp:keadhcp`.
jengelh reviewed 2025-04-10 15:07:39 +02:00
kea.spec Outdated
@@ -412,0 +399,4 @@
%service_add_pre kea-dhcp4.service
%service_add_pre kea-dhcp6.service
%service_add_pre kea-dhcp-ddns.service
%service_add_pre kea-ctrl-agent.service
jengelh commented 2025-04-10 15:07:39 +02:00
Collaborator
Copy Link

generates so much shell code

generates so much shell code
jcronenberg commented 2025-04-10 15:10:58 +02:00
Author
Owner
Copy Link

wdym?

wdym?
jengelh commented 2025-04-10 15:24:27 +02:00
Collaborator
Copy Link

just call %service_add_pre et al once, with all args?

just call %service_add_pre et al once, with all args?
jcronenberg commented 2025-04-10 15:33:25 +02:00
Author
Owner
Copy Link

Ah, I didn't know about this, thx!

Ah, I didn't know about this, thx!
jcronenberg marked this conversation as resolved
jengelh reviewed 2025-04-10 15:07:51 +02:00
kea.spec Outdated
@@ -458,0 +460,4 @@
%{_sysusersdir}/%{name}-user.conf
%{_tmpfilesdir}/%{name}-tmpfiles.conf
%attr(0750,kea,kea) %{_localstatedir}/log/kea/
%ghost %{_rundir}/kea
jengelh commented 2025-04-10 15:07:51 +02:00
Collaborator
Copy Link

the diff is needlessy larger than it needs to be

the diff is needlessy larger than it needs to be
jcronenberg commented 2025-04-10 15:11:22 +02:00
Author
Owner
Copy Link

wdym?

wdym?
jengelh commented 2025-04-10 15:24:54 +02:00
Collaborator
Copy Link

don't edit lines adding { } that don't need to be edited

don't edit lines adding { } that don't need to be edited
jcronenberg commented 2025-04-10 15:26:53 +02:00
Author
Owner
Copy Link

I just ran a few regexes over it, I always prefer the explicit syntax for paths and I don't see what's bad about it being a bit larger diff

I just ran a few regexes over it, I always prefer the explicit syntax for paths and I don't see what's bad about it being a bit larger diff
jengelh reviewed 2025-04-10 15:08:36 +02:00
kea-sysusers.conf Outdated
@@ -0,0 +1,3 @@
#Type Name ID GECOS Home directory Shell
g kea - - - -
u kea -:kea "Kea DHCP Server" /var/lib/kea -
jengelh commented 2025-04-10 15:08:36 +02:00
Collaborator
Copy Link

bad idea, no transition path from existing installation

bad idea, no transition path from existing installation
jcronenberg commented 2025-04-10 15:12:23 +02:00
Author
Owner
Copy Link

Again, I don't understand what you mean? I tested it with an existing installation and it works as expected.

Again, I don't understand what you mean? I tested it with an existing installation and it works as expected.
jcronenberg force-pushed master from 0d283cf070 to 26feed312e 2025-04-10 15:08:44 +02:00 Compare
jengelh reviewed 2025-04-10 15:13:20 +02:00
kea-ctrl-agent.service Outdated
@@ -0,0 +1,15 @@
[Unit]
jengelh commented 2025-04-10 15:13:20 +02:00
Collaborator
Copy Link

no migration path from kea.service

no migration path from kea.service
jcronenberg commented 2025-04-10 15:14:45 +02:00
Author
Owner
Copy Link

What would you expect a "migration path" to look like?

What would you expect a "migration path" to look like?
jengelh commented 2025-04-10 15:24:06 +02:00
Collaborator
Copy Link

kea.service:Requires=kea-dhcp4.service kea-dhcp6.service kea-ctrl-agent.service ddns

kea.service:Requires=kea-dhcp4.service kea-dhcp6.service kea-ctrl-agent.service ddns
jcronenberg commented 2025-04-10 15:30:06 +02:00
Author
Owner
Copy Link

AFAICT this would be a bad idea, because it would start e.g. the dhcp6 service even if it was disabled by the config.

AFAICT this would be a bad idea, because it would start e.g. the dhcp6 service even if it was disabled by the config.
jengelh commented 2025-04-10 15:32:43 +02:00
Collaborator
Copy Link

I know; but at least they run after the rpm is upgraded in the system. The alternative would be to havve Conflict=(4 services)

I know; but at least they run after the rpm is upgraded in the system. The alternative would be to havve Conflict=(4 services)
jcronenberg commented 2025-04-10 15:39:53 +02:00
Author
Owner
Copy Link

I won't add the Requires= because I think this is against what this PR is trying to achieve, improving the security of the package. Personally I would keep it as is, it will require an admin to change some things yes, but that will likely be the case anyway. If you really insist I guess I can add the Conflicts= kea.service.

I won't add the `Requires=` because I think this is against what this PR is trying to achieve, improving the security of the package. Personally I would keep it as is, it will require an admin to change some things yes, but that will likely be the case anyway. If you really insist I guess I can add the `Conflicts=` `kea.service`.
jcronenberg force-pushed master from 26feed312e to 8c4e98db95 2025-04-10 15:33:57 +02:00 Compare
jcronenberg commented 2025-04-15 10:52:47 +02:00
Author
Owner
Copy Link

@jengelh In my opinion this is good to go and due to some time restrictions I would like to merge this as soon as possible

@jengelh In my opinion this is good to go and due to some time restrictions I would like to merge this as soon as possible
jengelh manually merged commit c32b9b08fa into master 2025-04-15 14:16:00 +02:00
jcronenberg commented 2025-04-15 18:08:09 +02:00
Author
Owner
Copy Link

@jengelh if you only merge it to then revert most of the changes that isn't what co-maintainership is about. I could have just pushed my changes as well without discussing it, but instead I wanted to discuss it with you first.
Btw chown -R is safe against symlink attacks, the chown -Rh isn't necessary.

@jengelh if you only merge it to then revert most of the changes that isn't what co-maintainership is about. I could have just pushed my changes as well without discussing it, but instead I wanted to discuss it with you first. Btw `chown -R` is safe against symlink attacks, the `chown -Rh` isn't necessary.
jengelh commented 2025-04-15 18:35:31 +02:00
Collaborator
Copy Link

It's not co-maintainership, it's fallback maintainer. SUSE traditionally wanted such entries on OBS packages for packages which are in SLE, to be able to act e.g. in the face of CVE patches, if the maintainer was unavailable for a longer time. OBS's data model has long known to be inadequate to encode the basis on which a role was given/granted. You are free to remove yourself.

It's not co-maintainership, it's fallback maintainer. SUSE traditionally wanted such entries on OBS packages for packages which are in SLE, to be able to act e.g. in the face of CVE patches, if the maintainer was unavailable for a longer time. OBS's data model has long known to be [inadequate to encode](https://github.com/openSUSE/open-build-service/issues/1905) the basis on which a role was given/granted. You are free to remove yourself.
Sign in to join this conversation.
Reviewers
No Reviewers
jengelh
Labels
No items
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
RBrownFactory adamm autogits-devel dimstar_suse favogt_factory jcronenberg jengelh lkocman-factory maxlin_factory
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: dhcp/kea#2
No description provided.
Delete Branch "jcronenberg/kea:master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?