Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughThis PR removes the separate unprivileged control socket, switches control access checks to peer credentials, and updates the privsep control and root IPC paths to use a single control socket and FD_PRIV-based privilege classification. ChangesUnified control socket with credential-based privilege
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Control as src/control.c
participant PrivsepRoot as src/privsep-root.c
participant PrivsepCtl as src/privsep-control.c
Client->>Control: connect and send command
Control->>Control: recvmsg + getpeereid(uid, gid)
alt privsep mode
Control->>PrivsepRoot: ps_root_user_ispriv(uid, gid)
PrivsepRoot-->>Control: privilege result
Control->>PrivsepCtl: ps_ctl_sendmsg / ps_ctl_handleargs
else standalone mode
Control->>Control: control_user_ispriv(uid, gid)
end
Control->>Control: control_recvmsg parses payload
Control-->>Client: dispatch command or EPERM
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dhcpcd.c (1)
2570-2576: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDuplicate "spawned manager process" log.
The same
logdebugx(...)now appears both before thestart_manager:label (line 2572) and after it (line 2576). On the daemonise fall-through path both fire, logging the manager PID twice; thegoto start_managerpath only hits 2576. Drop the pre-label copy so both paths log exactly once.🩹 Remove the duplicate
/* We have now forked, setsid, forked once more. * From this point on, we are the controlling daemon. */ - logdebugx("spawned manager process on PID %d", (int)getpid()); start_manager: logdebugx("spawned manager process on PID %d", (int)getpid());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dhcpcd.c` around lines 2570 - 2576, The manager startup debug message is duplicated around the start_manager label in dhcpcd.c, causing the daemonise fall-through path to log the same PID twice. Remove the pre-label logdebugx call and keep the single logdebugx("spawned manager process on PID %d", ...) after start_manager so both the fall-through and goto start_manager paths emit the message exactly once.
🧹 Nitpick comments (4)
src/privsep-root.c (1)
328-338: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant reassignment of
p.
pis already initialized todataat Line 328, so thep = data;at Line 335 is redundant. Minor cleanup.♻️ Cleanup
- uint8_t *p = data; + uint8_t *p; if (len != sizeof(uid) + sizeof(gid)) { errno = EINVAL; return -1; } p = data;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/privsep-root.c` around lines 328 - 338, Remove the redundant reassignment of the local pointer in the privilege-separation parsing logic: in the function that reads `uid` and `gid` from `data`, `p` is already initialized before the length check, so the later `p = data;` is unnecessary. Keep the existing `memcpy` flow intact and simply delete the duplicate assignment while preserving the `uid`/`gid` extraction behavior.src/privsep-control.c (1)
40-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
PS_CTL_FLAGS_PRIVmacro
PS_CTL_FLAGS_PRIVis never referenced; privilege is already carried byPS_CTL_PRIV/PS_CTLinps_ctl_dispatch, so this macro can be dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/privsep-control.c` around lines 40 - 42, Remove the unused PS_CTL_FLAGS_PRIV macro from privsep-control.c and keep the PS_CTL_FD helper intact. Verify that ps_ctl_dispatch and the existing PS_CTL_PRIV/PS_CTL macros still provide the privilege handling needed, and delete any related dead definition so the header/source only contains symbols that are actually referenced.src/control.c (2)
453-453: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
listen()backlog usessizeof(ctx->control_fds).
ctx->control_fdsis a TAILQ head, sosizeofyields the struct size (~16), not a meaningful connection backlog. It works but is misleading; consider a named constant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/control.c` at line 453, The backlog passed to listen() in the control socket setup uses sizeof(ctx->control_fds), which is just the TAILQ head size and not a meaningful queue limit. Update the listen() call in the control initialization path to use a clear named constant or defined backlog value instead of sizeof(ctx->control_fds), keeping the existing fd/control_fds logic unchanged.
434-454: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse the
SOCK_MODEmacro and confirm the world-writable socket is intended.
SOCK_MODEis defined at line 434 (same value,0666) butcontrol_start1()chmods with the literal0666, leaving the macro unused. Static analysis also flags the world-writable mode (CWE-732). Under the new credential model any user may connect and privilege is enforced viagetpeereid/FD_PRIV, so world-writable is plausibly intentional — but then thectx->control_groupchown no longer restricts who can connect. Please confirm0666is desired rather than0660+control_group, and either way replace the literal with the macro.♻️ Use the defined macro
if (bind(fd, (struct sockaddr *)&sa, len) == -1 || - chmod(sa.sun_path, 0666) == -1 || + chmod(sa.sun_path, SOCK_MODE) == -1 || (ctx->control_group && chown(sa.sun_path, geteuid(), ctx->control_group) == -1) || listen(fd, sizeof(ctx->control_fds)) == -1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/control.c` around lines 434 - 454, In control_start1(), replace the hardcoded chmod mode with the existing SOCK_MODE macro so the defined socket permissions are used consistently. Also verify the intended access model for the control socket: if world-writable access is deliberate under the new credential/FD_PRIV checks, keep SOCK_MODE and leave a note or related adjustment; otherwise tighten the mode to a group-only setting and ensure ctx->control_group still meaningfully restricts access.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/control.c`:
- Around line 642-676: In control_user_ispriv(), treat an unknown uid from
getpwuid() as non-privileged instead of returning -1, so control_handle_data()
does not drop read-only clients whose user is missing from the passwd database.
Update the early return path in control_user_ispriv() to return 0 for this case,
and make sure any error logging in the caller does not rely on stale errno from
getpwuid() when no real error occurred.
In `@src/privsep-control.c`:
- Around line 111-123: The PS_CTL_PRIV/PS_CTL handling in privsep control is
incorrectly closing the shared proxy work fd when control_recvmsg() fails.
Update the control_new()/control_free() flow so the wrapper is dropped without
closing ctx->ps_ctl->psp_work_fd on the error path, and keep the underlying
shared fd registered for later dispatches; use the PS_CTL_PRIV/PS_CTL case block
and the control_recvmsg() cleanup branch as the places to adjust.
---
Outside diff comments:
In `@src/dhcpcd.c`:
- Around line 2570-2576: The manager startup debug message is duplicated around
the start_manager label in dhcpcd.c, causing the daemonise fall-through path to
log the same PID twice. Remove the pre-label logdebugx call and keep the single
logdebugx("spawned manager process on PID %d", ...) after start_manager so both
the fall-through and goto start_manager paths emit the message exactly once.
---
Nitpick comments:
In `@src/control.c`:
- Line 453: The backlog passed to listen() in the control socket setup uses
sizeof(ctx->control_fds), which is just the TAILQ head size and not a meaningful
queue limit. Update the listen() call in the control initialization path to use
a clear named constant or defined backlog value instead of
sizeof(ctx->control_fds), keeping the existing fd/control_fds logic unchanged.
- Around line 434-454: In control_start1(), replace the hardcoded chmod mode
with the existing SOCK_MODE macro so the defined socket permissions are used
consistently. Also verify the intended access model for the control socket: if
world-writable access is deliberate under the new credential/FD_PRIV checks,
keep SOCK_MODE and leave a note or related adjustment; otherwise tighten the
mode to a group-only setting and ensure ctx->control_group still meaningfully
restricts access.
In `@src/privsep-control.c`:
- Around line 40-42: Remove the unused PS_CTL_FLAGS_PRIV macro from
privsep-control.c and keep the PS_CTL_FD helper intact. Verify that
ps_ctl_dispatch and the existing PS_CTL_PRIV/PS_CTL macros still provide the
privilege handling needed, and delete any related dead definition so the
header/source only contains symbols that are actually referenced.
In `@src/privsep-root.c`:
- Around line 328-338: Remove the redundant reassignment of the local pointer in
the privilege-separation parsing logic: in the function that reads `uid` and
`gid` from `data`, `p` is already initialized before the length check, so the
later `p = data;` is unnecessary. Keep the existing `memcpy` flow intact and
simply delete the duplicate assignment while preserving the `uid`/`gid`
extraction behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f1cec4a-8cc9-4b2c-99f5-c3d964783ef1
📒 Files selected for processing (11)
src/control.csrc/control.hsrc/dhcpcd.8.insrc/dhcpcd.csrc/dhcpcd.hsrc/privsep-control.csrc/privsep-control.hsrc/privsep-root.csrc/privsep-root.hsrc/privsep.csrc/privsep.h
Instead workout credentials from the connected socket by getpeereid(2). Remove the limit of only one connected control client. This needs some testing and is only expected to compile on the BSDs right now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Instead workout credentials from the connected socket by getpeereid(2).
Remove the limit of only one connected control client.
This needs some testing and is only expected to compile on the BSDs right now.