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/dnscrypt-wrapper: use dnscrypt-proxy1 #85900

Merged
merged 3 commits into from May 27, 2020
Merged

nixos/dnscrypt-wrapper: use dnscrypt-proxy1 #85900

merged 3 commits into from May 27, 2020

Conversation

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Apr 23, 2020

Motivation for this change

Fix the dnscrypt-wrapper module. I didn't realise dnscrypt-proxy was completely removed from nixpkgs. This PR add it back in the attribute dnscrypt-proxy1 and also a test for dnscrypt-wrapper, the blame is on me for not having written one before.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test (nixosTests.dnscrypt-wrapper)
  • Tested compilation of all pkgs that depend on this change (dnscrypt-proxy1)
  • Tested execution of all binary files (usually in ./result/bin/)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@rnhmjoj rnhmjoj requested a review from worldofpeace Apr 23, 2020
@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Apr 23, 2020

@GrahamcOfBorg test dnscrypt-wrapper

@emilazy
Copy link
Member

emilazy commented Apr 23, 2020

FWIW, this was removed in #78543.

Personally, I'm not sure how I feel about re-packaging a security-sensitive codebase in a memory-unsafe language that's been abandoned by the original owner and hasn't been actively maintained since; a single flick of the scrollbar on the forked repository gets you back into commits from the original author in 2017, and at least one of the security fixes since seems to have only happened because the original author dropped in to patch them, so I don't think it's a great idea to encourage using it over the new version.

It seems it's only used for key rotation; would it be difficult to port this to using dnscrypt-proxy2 instead?

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Apr 24, 2020

It seems it's only used for key rotation; would it be difficult to port this to using dnscrypt-proxy2 instead?

That's correct: it's only used to make a connection to localhost so I can't think of any security issue.

Unfortunately yes, It's no possibile to use dnscrypt-proxy2 because the function is simply not implemented. It was added at some point but reverted soon after. Apparently the author doesn't care about dnscrypt that much anymore.

If you are worried about users picking this up over v2 I guess I could move the attribute out of all-packages.nix.

@emilazy
Copy link
Member

emilazy commented Apr 25, 2020

Fair enough; seeing as dnscrypt-wrapper itself is based on the dnscrypt-proxy 1.x code, I don't think it makes much difference security-wise to use it here, though it would be nice in the long run if there was a standalone tool or dnscrypt-wrapper had something upstream, rather than having to keep the package around just for key rotation. I'll let people who actually have commit access decide what they think about the maintenance burden of that, though.

@rnhmjoj rnhmjoj requested review from Infinisil and flokli May 7, 2020
@flokli
Copy link
Contributor

flokli commented May 11, 2020

Huuuh. I'm also not a fan of introducing a package with known security issues, even if it's just used in a localhost context.

@rnhmjoj Has there been any discussion with upstream about why DNSCrypt/dnscrypt-proxy@519af2e#diff-882438215b589ae5bb3f1b4ab0f2f512 has happened?

Maybe you could explain your usecase, and there's another way, instead of using an unmaintained older package version.

@emilazy
Copy link
Member

emilazy commented May 11, 2020

Honestly, while packaging dnscrypt-proxy1 or not is its own decision, its codebase is probably not worth worrying about too much in the context of a dnscrypt-wrapper module, because dnscrypt-wrapper is itself a fork of the dnscrypt-proxy 1.x code that has barely been touched in years; the fork of dnscrypt-proxy 1.x re-added in this commit is not really any less active and I wouldn't be surprised if it actually has security fixes that dnscrypt-wrapper hasn't inherited.

Personally, I think this mostly means that it might not be the greatest idea to have integration with dnscrypt-wrapper upstream, but I feel bad about responding "maybe we should just remove this" when someone has gone out of their way to fix it 😅

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented May 11, 2020

@rnhmjoj Has there been any discussion with upstream about why DNSCrypt/dnscrypt-proxy@519af2e#diff-882438215b589ae5bb3f1b4ab0f2f512 has happened?

No, I don't think so.

Maybe you could explain your usecase, and there's another way, instead of using an unmaintained older package version.

Yes, I could open an issue but the author expressed the -verify feature would better be in an external tool so it doesn't look like there's going to be one in dnscrypt-proxy 2. It's probably better to open an issue in dnscrypt-wrapper, which is directly affected.

In any case I'm pretty sure the only option is to write a tool to verify the keys. I'm probably going to write it myself but it will take time.

dnscrypt-wrapper is itself a fork of the dnscrypt-proxy 1.x code that has barely been touched in years;

Yeah, the world seems to have chosen (once again) the technically messier solution (DNS over HTTP/TLS). I may be the only one still using this but given the program has not been officially deprecated and is still working I don't see why it should be removed.

I'll move the definition of dnscrypt-proxy1 out of all-packages.nix to make it only available to dnscrypt-wrapper.

rnhmjoj added 3 commits Apr 23, 2020
@rnhmjoj rnhmjoj force-pushed the rnhmjoj:dnscrypt branch from c8be03f to cb8975f May 25, 2020
@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented May 25, 2020

  • Moved dnscrypt-proxy1 inside the dnscrypt-wrapper module
  • Rebased and fixed conflict
  • Opened an issue at cofyc/dnscrypt-wrapper#171
@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented May 27, 2020

I'm going to merge this since we have agreed that dnscrypt-proxy1 limited to this module is not a security concern and no other solution exists.

I'll also backport the change to 20.03 since the module is broken there.

@rnhmjoj rnhmjoj merged commit a4f9e8b into NixOS:master May 27, 2020
14 checks passed
14 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="cb8975f"; rev="cb8975f5b0f4a04b26620f078700d92d904d577a"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="cb8975f"; rev="cb8975f5b0f4a04b26620f078700d92d904d577a"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="cb8975f"; rev="cb8975f5b0f4a04b26620f078700d92d904d577a"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="cb8975f"; rev="cb8975f5b0f4a04b26620f078700d92d904d577a"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="cb8975f"; rev="cb8975f5b0f4a04b26620f078700d92d904d577a"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="cb8975f"; rev="cb8975f5b0f4a04b26620f078700d92d904d577a"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="cb8975f"; rev="cb8975f5b0f4a04b26620f078700d92d904d577a"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@rnhmjoj rnhmjoj deleted the rnhmjoj:dnscrypt branch Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.