proxy: add support for HTTP-only listeners for DoH #302
Conversation
| @@ -49,6 +49,9 @@ type Options struct { | |||
| // Server listen ports | |||
| ListenPorts []int `yaml:"listen-ports" short:"p" long:"port" description:"Listening ports. Zero value disables TCP and UDP listeners"` | |||
|
|
|||
| // HTTP listen ports | |||
| HTTPListenPorts []int `yaml:"http-port" short:"i" long:"http-port" description:"Listening ports for DNS-over-HTTP"` | |||
There was a problem hiding this comment.
I couldn't find a better letter here, -i.
| @@ -216,7 +219,7 @@ func main() { | |||
| if flagsErr, ok := err.(*goFlags.Error); ok && flagsErr.Type == goFlags.ErrHelp { | |||
| os.Exit(0) | |||
| } else { | |||
| os.Exit(1) | |||
| log.Fatalf("failed to parse args: %v", err) | |||
There was a problem hiding this comment.
This is an unrelated fix but handy to have.
| @@ -37,6 +37,21 @@ func (p *Proxy) listenHTTP(addr *net.TCPAddr) (laddr *net.TCPAddr, err error) { | |||
| return tcpListen.Addr().(*net.TCPAddr), nil | |||
| } | |||
|
|
|||
| // listenHTTP creates instances of TCP listeners that will be used to run an | |||
| // H1 server. Returns the address the listener actually listens to (useful | |||
| // in the case if port 0 is specified). | |||
There was a problem hiding this comment.
The result is not really used, since there is no H3 support.
There was a problem hiding this comment.
@gdm85 thank you for the contribution!
However, I'd prefer it to be made simpler. I don't think anyone will need to run both plain HTTP and HTTPS listeners at the same time. Internally, dnsproxy allows running DoH server with no TLS certificate. Could you please instead just expose this option via the command-line interface and not modify the internals?
Here's what I suggest:
- Add
--http-portcommand-line argument and allow running dnsproxy with no TLS configuration when it's specified. - Validation: do not allow specifying both
--http-portand--https-port.
And one more thing, please update the README.md as well.
|
Any update? |
|
Is h2c supported here? I would like to see that. Nginx does not support it but Apache HTTP Server does that and would be something good for internal connections in load balancing. Unsure if Caddy supports it too. |
So exposing this option requires modifying the internals, not just on the command-line interface. If need be, I will try it. |
|
Yes, let's modify the internals to allow plain HTTP server to run. |
It is desirable to use the DoH endpoint even without TLS termination, for example in case of an external proxy (nginx) taking care of termination. This change allows to specify independent --http-port ports which use simple TCP listeners. It also contains a separate fix to print the arguments parsing error instead of exiting with exit code 1 and no output.
|
@gdm85 I see you pushed new commits, are you actively working on this feature? @ameshkov Looking at the code, it seems if we try to keep changes to internals to a minimum we end up with a lot of references to "https" when it could be http or https. For example, there's multiple log messages and function names that explicitly mention https in some form. If we want those to be accurate, I think we end up touching internals nonetheless. Another thing to take care of is handling HTTP/3 & plain HTTP. As in, you can't mix the two, but the code needs to be aware of it, meaning in validation it needs to check if the combination of those two options is specified. On the other hand, if we allow both HTTPS and HTTP to mix then we can avoid that issue. I understand that if it it was as simple as just specifying one option or the other then it would be much cleaner, but it might be we're introducing more complexity into the code, at least from what I could see so far. What do you think? |
I rebased it, and then noticed the request for changes which I had unattended for more than a year (my bad). I was trying to figure out how to make the requested changes, and then I would post a reply. |
I tend to agree; it might sound silly to run both, but there are use-cases where it might be handy, for example internal LAN traffic which can use DoH without TLS while at the same time having an external TLS-protected listener. I also prefer simpler setups for security reasons but - as far as the log lines can be made explicit about which endpoint was hit - I see no reason to limit the feature. I will wait for this discussion to continue before altering the PR in a more radical way. |
|
Thanks for your work! I've already deployed some instances with http listener patch applied :)
But I'll not agree with this statement, as DoH is explicitly requiring secure connection by its specification, currently no such client exists that will accept http doh endpoint (curl, browsers, ...) at all, AFAIK. |
It is desirable to use the DoH endpoint even without TLS termination, for example in case of an external proxy (nginx) taking care of termination. This change allows to specify independent --http-port ports which use simple TCP listeners.
It also contains a separate fix to print the arguments parsing error instead of exiting with exit code 1 and no output.
Fixes #146