Skip to content

fix(files/udhcpc/script): multi prepend#458

Closed
milkpirate wants to merge 4 commits into
jbatonnet:masterfrom
milkpirate:fix/missing-x-permission
Closed

fix(files/udhcpc/script): multi prepend#458
milkpirate wants to merge 4 commits into
jbatonnet:masterfrom
milkpirate:fix/missing-x-permission

Conversation

@milkpirate
Copy link
Copy Markdown
Contributor

@milkpirate milkpirate commented May 6, 2026

@martinbogo addresses the critic mentioned here: #392 (comment)

Signed-off-by: Paul Schroeder <milkpirate@users.noreply.github.com>
@milkpirate milkpirate marked this pull request as draft May 6, 2026 19:28
@milkpirate milkpirate force-pushed the fix/missing-x-permission branch from 45232fd to fe65e53 Compare May 7, 2026 20:46
Signed-off-by: Paul Schroeder <milkpirate@users.noreply.github.com>
@milkpirate milkpirate force-pushed the fix/missing-x-permission branch from fe65e53 to dbe3268 Compare May 7, 2026 20:47
@milkpirate milkpirate marked this pull request as ready for review May 7, 2026 20:49
@martinbogo
Copy link
Copy Markdown
Collaborator

Hey there @milkpirate

Thanks for tracking down the duplication bug and fixing the quoting issues, well done.

Two things need to be addressed before this can be merged:

1. Make backups of the original files before modifying them.

The script overwrites /etc/resolv.conf in-place without preserving the original. If something goes wrong during a DHCP event, the user's DNS configuration is silently lost with no way to recover it. The script should save a backup (e.g., /etc/resolv.conf.bak or a timestamped copy) before writing the new content.

2. The hardcoded fallback nameservers are a privacy issue ( Cloudflare & Google )

Unconditionally appending nameserver 1.1.1.1 and nameserver 8.8.8.8 to every user's resolv.conf on every DHCP renewal is a significant policy decision that should not be made silently. Users on private or enterprise networks may have deliberate reasons not to use Cloudflare or Google DNS and injecting these without their knowledge leaks all unresolved DNS queries to third-party resolvers they never chose.

If the intent is to provide a fallback when DHCP provides no nameservers, that logic should be conditional: only insert fallback entries when the DHCP lease contains no dns option. Users should also be able to configure or disable the fallback. As written, this change removes user control over their own DNS resolution.

Please revise to address both of these before we consider merging.

Sincerely,
Martin

Signed-off-by: Paul Schroeder <milkpirate@users.noreply.github.com>
@milkpirate
Copy link
Copy Markdown
Contributor Author

milkpirate commented May 20, 2026

Hey @martinbogo,

Thanks for the feedback. I have updated the PR to include the backup mechanism.

Regarding the DNS configuration concerns, I wanted to clarify how the implementation interacts with the system defaults:

System Defaults: The inclusion of Cloudflare and Google’s DNS servers is not an arbitrary choice; they are the existing system defaults. I have maintained these to ensure no regression for current setups.

Resolution Logic: resolv.conf follows a standard hierarchy: it queries the first nameserver and falls back to the next only if the first fails to provide a valid entry.

Improved Flexibility: In the original, hardcoded state, the user has no way to override these defaults without manual intervention that may be wiped upon reboot. My change allows the user's DHCP-provided DNS entries to take precedence, only falling back to public DNS servers if the DHCP source fails to provide one.

Buildroot/BusyBox Standards: I have leveraged the /usr/share/udhcpc/default.script.d/ directory. On Buildroot/BusyBox systems, this is the preferred, modular way to handle configuration updates during DHCP events. It avoids modifying the main default.script, ensuring the implementation is clean, maintainable, and resilient against system updates.

Ultimately, this implementation grants the user more control over their DNS settings than the current stock configuration. Please let me know if you have any further questions.

@martinbogo
Copy link
Copy Markdown
Collaborator

Thanks for this contribution. The Rinkhals project has moved to a new home at https://github.com/rinkhals-community/Rinkhals and this repository is no longer maintained.

We can no longer merge PRs here. If your change is still relevant, please re-open it against the new repository, and make sure it targets the develop branch (not master/main), which is where active development happens.

Apologies for the inconvenience, and thank you for helping improve Rinkhals. Closing this PR here.

@martinbogo martinbogo closed this May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants