fix(files/udhcpc/script): multi prepend#458
Conversation
Signed-off-by: Paul Schroeder <milkpirate@users.noreply.github.com>
45232fd to
fe65e53
Compare
Signed-off-by: Paul Schroeder <milkpirate@users.noreply.github.com>
fe65e53 to
dbe3268
Compare
|
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, |
Signed-off-by: Paul Schroeder <milkpirate@users.noreply.github.com>
|
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. |
|
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 Apologies for the inconvenience, and thank you for helping improve Rinkhals. Closing this PR here. |
@martinbogo addresses the critic mentioned here: #392 (comment)