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/cfdyndns: add apikeyFile option #102376

Merged
merged 1 commit into from
Nov 10, 2020

Conversation

felschr
Copy link
Member

@felschr felschr commented Nov 1, 2020

Motivation for this change

Making it possible to specify cfdyndns api key by referencing a file.
See #24288.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@aanderse
Copy link
Member

aanderse commented Nov 1, 2020

Maybe you should remove the insecure option while at it. What do you think?

@felschr
Copy link
Member Author

felschr commented Nov 2, 2020

Seeing that it would break backwards compatibility I don't think we should do that yet.
Most of the other services where an option like this has been added also haven't removed or deprecated the non-file option yet as there's still some discussion about how this should be handled: #24288

@aanderse
Copy link
Member

aanderse commented Nov 2, 2020

Seeing that it would break backwards compatibility

That was my point... 😄 We should inform users that they are leaking sensitive data on their systems and push them as hard as we can to stop. The user who for some reason wishes to continue to push secrets into the nix store is still able to do so with something like:

services.cfdyndns.apikey = pkgs.writeText "insecure-file-this-is-a-bad-idea.txt" "i-cant-stress-enough-this-is-a-really-bad-idea";

Deprecated options can use mkRemovedOptionModule to let the user know that the insecure option was removed and they should migrate to the secure option.


This is merely my personal opinion, though. This PR as is is a great addition and I would merge it as is if you are absolutely convinced we must continue to provide the apikey option for now. Let me know and I can hit merge 👍

@felschr
Copy link
Member Author

felschr commented Nov 2, 2020

I guess I was just a little cautious because I haven't introduced any breaking changes before.
You're absolutely right, though. It's way too simple to accidentally add secrets into the nix store at the moment.
I've added a new commit to remove the option now. If I should combine the commits or there's anything else let me know.

@aanderse
Copy link
Member

aanderse commented Nov 8, 2020

Sorry for the delay @felschr. Squash these commits and you have yourself a merge. Thank you for contributing this PR! 🎉

nixos/cfdyndns: remove apikey option
@felschr
Copy link
Member Author

felschr commented Nov 10, 2020

@aanderse and done :)

@aanderse aanderse merged commit e419de3 into NixOS:master Nov 10, 2020
@felschr felschr deleted the feat/cfdyndns-password-file branch November 11, 2020 09:07
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.

2 participants