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

kresd service: switch .listenDoH to new implementation #103633

Merged
merged 3 commits into from Nov 17, 2020

Conversation

vcunat
Copy link
Member

@vcunat vcunat commented Nov 12, 2020

Motivation for this change

Switch the option to a better implementation.

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
  • N/A Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • N/A 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)
  • N/A Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

I don't use DoH normally, but I briefly tested that it looks OK.

There's this extraFeatures caveat (though very minor IMO), so I wanted to wait a while for feedback before merging.

@vcunat
Copy link
Member Author

vcunat commented Nov 12, 2020

I considered also adding .listenXDP, but the technology seems useful only for relatively special performance-focused deployments, so I suspect such easy option wouldn't be useful anyway.

Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general. Have not tested.

By default.  I'm not a DoH fan, but the difference in runtime closure
is really tiny (216 KiB by du).  I somehow forgot this during update.

Some of the newly running tests were failing and got disabled.
Beware: extraFeatures are not needed *for this* anymore,
but their removal may still cause a regression in some configs
(example: prefill module).
By default.  I forgot to add this a long time ago.
The difference in runtime closure is really tiny (232 KiB by du).
vcunat added a commit that referenced this pull request Nov 17, 2020
... to new implementation - and a couple other improvements.
@vcunat vcunat merged commit cd5c7c0 into NixOS:master Nov 17, 2020
@vcunat vcunat deleted the p/kresd-doh2 branch November 17, 2020 19:11
@Mic92
Copy link
Member

Mic92 commented Nov 23, 2020

There is only one caveat when upgrading. It's no longer possible to put kresd/http2 behind an nginx reverse proxy. nginx only supports http1.1 to talk to the backend. Proxies like nginx are required if port 443 is already in use otherwise i.e. for normal https traffic. It is possible to use nginx stream module to match on the hostname but than it gets awkward to support normal https vhosts.

@Mic92
Copy link
Member

Mic92 commented Nov 23, 2020

In my deployment I can work around it by using sslh to match on the SNI hostname. However how do get new letsencrypt certificates in this case? There is no acme module available for kresd.

@Mic92
Copy link
Member

Mic92 commented Nov 23, 2020

I am using the web management backend for the time being: Mic92/dotfiles@d5164e9

@vcunat
Copy link
Member Author

vcunat commented Nov 23, 2020

Thanks for the feedback; I'll post more details later.

Detail: kind = 'doh' didn't go away in kresd 5.2.0, so it's possible to keep using that (i.e. it's possible to just revert this PR - or avoid using .listenDoH and configure it in .extraConfig).

@vcunat
Copy link
Member Author

vcunat commented Nov 24, 2020

If you're interested in auto-reloading the certificate (which should be a fairly reliable methon, I expect), there's a way. Note that it uses lua cqueues, so you'd still need it to add e.g. through extraFeatures = true (though that option's a little over-kill for that case).

Better upstream docs for explicit reloads will be incoming soon, but I'd personally think that's a little harder to integrate.

@vcunat
Copy link
Member Author

vcunat commented Nov 24, 2020

Honestly, the non-support in nginx is a surprise. I understand their arguments, but... fortunately other proxies seem to usually support /2.

@Mic92
Copy link
Member

Mic92 commented Nov 24, 2020

If you're interested in auto-reloading the certificate (which should be a fairly reliable methon, I expect), there's a way. Note that it uses lua cqueues, so you'd still need it to add e.g. through extraFeatures = true (though that option's a little over-kill for that case).

Better upstream docs for explicit reloads will be incoming soon, but I'd personally think that's a little harder to integrate.

This solves reload at least. Is it possible to serve the acme validation directory somehow with doh2? Than all the common ACME tools work again.

@vcunat
Copy link
Member Author

vcunat commented Nov 24, 2020

No, doh2 can only do DoH, no other HTTP stuff. It's possible to use doh2 for DoH and additionally http module on some other IP-port combinations (to serve whatever... probably not hard).

@Mic92
Copy link
Member

Mic92 commented Nov 24, 2020

No, doh2 can only do DoH, no other HTTP stuff. It's possible to use doh2 for DoH and additionally http module on some other IP-port combinations (to serve whatever... probably not hard).

Different port won't work. Doh2 needs to be on port 443 because this where clients try to reach it.

@vcunat
Copy link
Member Author

vcunat commented Nov 24, 2020

I meant that the http for ACME might be on a different IP, for example.

@Mic92
Copy link
Member

Mic92 commented Nov 24, 2020

I don't see how multiple IPs would solve the issue. I think acme checks multiple ip addresses. However a simple hack that could be implemented into doh2 is to redirect to a different hostname: i.e. dns.example.com/.well-known/acme-challenge/random-filename -> acme.dns.example.com/.well-known/acme-challenge/random-filename. Than it could be handled by a different ip address: https://community.letsencrypt.org/t/one-hostname-multiple-ips/104301/2

@vcunat
Copy link
Member Author

vcunat commented Nov 24, 2020

Right, you want to serve DoH on the IPs that are advertised in DNS.

Here's the docs around explicit reloads: https://gitlab.nic.cz/knot/knot-resolver/-/merge_requests/1098

@vcunat
Copy link
Member Author

vcunat commented Nov 24, 2020

I'm no good around ACME. By any chance, it's not possible to make it work just with port 80, right?

@Mic92
Copy link
Member

Mic92 commented Nov 24, 2020

I'm no good around ACME. By any chance, it's not possible to make it work just with port 80, right?

Oh, you are right. Why did I not thought about this! Indeed just opening port 80 is enough. This solves the ACME story. So the only thing remains is how marry nginx and kresd. In my deployment I will just use the sslh instance that is in front of my nginx and match on the SNI hostname. Non-SSLH users could achive the same with nginx: First use the stream module to either forward traffic to kresd or terminate the ssl and forward to the same unencrypted nginx server. Or use a different webserver...

@vcunat
Copy link
Member Author

vcunat commented Nov 30, 2020

For reference, my reading is that the HTTP challenge only uses port 80:

This request MUST be sent to TCP port 80 on the HTTP server.

https://tools.ietf.org/id/draft-ietf-acme-acme-18.html#rfc.section.8.3

In particular, "doh2" in kresd does not even implement insecure http, so it can't be used to affect this ACME challenge. (The answer over port 80 may be a redirect to https, but in that case you can surely redirect to a better place instead.)

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

2 participants