Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/dhcpcd: Expose less information to servers #45813

Closed
wants to merge 1 commit into from
Closed

nixos/dhcpcd: Expose less information to servers #45813

wants to merge 1 commit into from

Conversation

dasJ
Copy link
Member

@dasJ dasJ commented Aug 30, 2018

Motivation for this change

dhcpcd exposes a lot of information which is IMO not necessary and a possible security risk because of exposed version numbers.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@vcunat
Copy link
Member

vcunat commented Aug 30, 2018

Sounds OK, superficially – I know almost nothing about dhcpcd or the protocol.

@dasJ
Copy link
Member Author

dasJ commented Aug 30, 2018

@vcunat To elaborate more: The data is sent during DHCPv6 solicitation. The packet looks like this (look at the last two):

Wireshark Screenshot

It surely works here with my router, but I don't know if other routers could fail. I doubt it but you never be sure…

@xeji
Copy link
Contributor

xeji commented Sep 1, 2018

I don't think we should make this change because some routers may reject this vendorclassid, and the user cannot override it in extraConfig. As per dhcpcd.conf(5):

vendclass en data
Add the DHCPv6 Vendor Indetifying Vendor Class with the IANA assigned Enterprise Number en with the data. This option can be set more than once to add more data, but the
behaviour, as per RFC(3925) is undefined if the Enterprise Number differs.

So I'm afraid it's up to the user to set this manually if they want to expose less information.

@dasJ
Copy link
Member Author

dasJ commented Sep 2, 2018

@xeji So do you think it would be okay if I do it as optionalString?

@globin globin requested a review from fpletz September 2, 2018 12:02
@cransom
Copy link
Contributor

cransom commented Sep 6, 2018

I could see this being an optional value but it being open to the user to customize. In a datacenter, pxebooting machines could declare some other options to the dhcp server for it to take some other action. But I agree with @xeji that the default shouldn't change.

@dasJ
Copy link
Member Author

dasJ commented Sep 6, 2018

@cransom That's not a new feature compared to the currently existing extraConfig functionality. I'll close this PR, users can add the appropriate line to their configuration if they wish.

@dasJ dasJ closed this Sep 6, 2018
@dasJ dasJ deleted the dhcpcd-vendor branch September 6, 2018 10:18
@vcunat
Copy link
Member

vcunat commented Sep 6, 2018

Explicit nixos options are easier to compose etc. but I suppose it's not a big deal in this case.

@dasJ
Copy link
Member Author

dasJ commented Sep 6, 2018

Totally agree. We should rather keep the options clean

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants