New Plugin: QUIC proxy [RFC9250] #6245

jaehnri · 2023-08-08T04:46:01Z

1. Why is this pull request needed and what does it do?

This PR adds support for QUIC proxying in CoreDNS

2. Which issues (if any) are related?

Closes #5583

3. Which documentation changes (if any) need to be made?

README documentation for the plugin was added, but it is possible that there are more places to update.

4. Does this introduce a backward incompatible change or deprecation?

No.

chrisohaver · 2023-08-31T13:50:49Z

plugin/quic/quic.go

+			// reached the end of list without any answer
+			if ret != nil {
+				// write empty response and finish
+				w.WriteMsg(ret)


I think that writing an empty dns message to client would result in client-side parsing/validation errors. I think the proper response id with a SERVFAIL response code, with question correctly set. Returning a servfail and error instead of writing a response would work too (base server code writes an error response to client when ServeDNS returns a SERVFAIL code).

Suggested change

w.WriteMsg(ret)

return dns.RcodeServerFailure, <some-error>

Agreed and fixed. I got this code from the grpc plugin, should we port this fix there too?

coredns/plugin/grpc/grpc.go

Lines 51 to 54 in b5e6291

// reached the end of list without any answer

if ret != nil {

// write empty response and finish

w.WriteMsg(ret)

should we port this fix there too?

Probably, but not in this PR.

jaehnri · 2023-09-18T06:05:02Z

plugin/quic/policy.go

@@ -0,0 +1,68 @@
+package quic


This policy selection code has been repeated across several plugins now (forward, grpc, quic). Should we unify it? If so, where should it be? plugin/pkg?

That would be a good place. Thanks!

jaehnri · 2023-09-18T06:14:46Z

test/quic_test.go

@@ -117,6 +121,38 @@ func TestQUICProtocolError(t *testing.T) {
 	}
 }

+func TestQUICPlugin(t *testing.T) {


The plugin integration test ended up in the test folder. Unlike forward, setting up a mock quic server to answer a simple DNS query is not easy because you need not only to create a listener but also accept connections, streams, write, and close to the stream, all manually.

The best way I found to make it work and avoid flakiness was to use the CoreDNSServerAndPorts function to spin up a CoreDNS QUIC server and test the plugin against it.

Signed-off-by: João Henri <joao.rocha@suse.com>

Also add relevant documentation to QUIC proxy.go Signed-off-by: João Henri <joao.rocha@suse.com>

Signed-off-by: João Henri <joao.rocha@suse.com>

Signed-off-by: jaehnri <joao.henri.cr@gmail.com>

jaehnri · 2023-09-18T06:29:04Z

plugin/quic/setup.go

+	if q.tlsServerName != "" {
+		q.tlsConfig.ServerName = q.tlsServerName
+	} else {
+		q.tlsConfig.InsecureSkipVerify = true


I'll have to find a way to circumvent this. quic-go doesn't work without tls.Config and I couldn't set the tls_servername using plugin/tls test certificates.

@jaehnri the plugin/dns test certs common name is empty, therefore the hostname cannot match. My suggestion would be to drop the tls_servername option, because it should never be needed (the tls package checks if the common name, subject alternative names or ip addresses contains the hosts name) and instead introduce the option tls_skip_verify. For test cases u can set the common name manually via the tlsConfig

codecov · 2023-09-18T06:29:22Z

Codecov Report

Attention: Patch coverage is 32.13213% with 226 lines in your changes are missing coverage. Please review.

Project coverage is 57.96%. Comparing base (93c57b6) to head (3fa11dc).
Report is 1152 commits behind head on master.

Files	Patch %	Lines
plugin/quic/proxy.go	12.82%	102 Missing ⚠️
plugin/quic/quic.go	6.94%	67 Missing ⚠️
plugin/quic/setup.go	66.96%	29 Missing and 8 partials ⚠️
plugin/quic/policy.go	40.00%	18 Missing ⚠️
core/dnsserver/server_quic.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6245      +/-   ##
==========================================
+ Coverage   55.70%   57.96%   +2.26%     
==========================================
  Files         224      256      +32     
  Lines       10016    16868    +6852     
==========================================
+ Hits         5579     9777    +4198     
- Misses       3978     6494    +2516     
- Partials      459      597     +138

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chrisohaver · 2024-03-01T12:52:28Z

@jaehnri, What is the state of this PR. Is it pending a final review, or are there parts yet to complete?

jaehnri force-pushed the quic-plugin branch 3 times, most recently from 78c08a5 to 588da60 Compare August 9, 2023 02:29

jaehnri marked this pull request as ready for review August 25, 2023 12:58

jaehnri requested review from miekg, chrisohaver, johnbelamaric, yongtang, stp-ip, bradbeam, dilyevsky, jameshartig, greenpau, Isolus, pmoroney, rajansandeep, SuperQ and Tantalor93 as code owners August 25, 2023 12:59

jaehnri force-pushed the quic-plugin branch from ddfbb34 to 7ec9994 Compare August 29, 2023 02:48

chrisohaver reviewed Sep 12, 2023

View reviewed changes

jaehnri commented Sep 18, 2023

View reviewed changes

jaehnri and others added 9 commits September 18, 2023 03:15

Add first funciontal version of QUIC plugin

62b5df3

Signed-off-by: João Henri <joao.rocha@suse.com>

Refactor QUIC proxy to reuse server code

30e8d97

Also add relevant documentation to QUIC proxy.go Signed-off-by: João Henri <joao.rocha@suse.com>

Handle expected errors when querying proxy

add4efe

Signed-off-by: João Henri <joao.rocha@suse.com>

Cover quic plugin setup with unit tests

57aeb3a

Signed-off-by: João Henri <joao.rocha@suse.com>

Remove unnecessary tls_servername parameter from quic.Proxy

1c52e97

Signed-off-by: jaehnri <joao.henri.cr@gmail.com>

Add unit test coverage to quic/proxy

9ffed7e

Signed-off-by: jaehnri <joao.henri.cr@gmail.com>

Return SERVFAIL instead of writing empty message on quic

766e146

Signed-off-by: jaehnri <joao.henri.cr@gmail.com>

Add README to QUIC plugin

66ffa9b

Signed-off-by: jaehnri <joao.henri.cr@gmail.com>

Add unit and integration test coverage to QUIC plugin

3fa11dc

Signed-off-by: jaehnri <joao.henri.cr@gmail.com>

jaehnri force-pushed the quic-plugin branch from 262007b to 3fa11dc Compare September 18, 2023 06:19

github-advanced-security bot found potential problems Sep 18, 2023

View reviewed changes

chrisohaver changed the title ~~[RFC9250]: QUIC plugin~~ New Plugin: QUIC proxy [RFC9250] Mar 7, 2024

chrisohaver added the help wanted label Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Plugin: QUIC proxy [RFC9250] #6245

New Plugin: QUIC proxy [RFC9250] #6245

jaehnri commented Aug 8, 2023 •

edited

chrisohaver Aug 31, 2023

jaehnri Sep 17, 2023

chrisohaver Nov 1, 2023

jaehnri Sep 18, 2023

chrisohaver Nov 1, 2023

jaehnri Sep 18, 2023 •

edited

jaehnri Sep 18, 2023

iFrozenPhoenix Jan 8, 2024

codecov bot commented Sep 18, 2023 •

edited

chrisohaver commented Mar 1, 2024

	// reached the end of list without any answer
	if ret != nil {
	// write empty response and finish
	w.WriteMsg(ret)

New Plugin: QUIC proxy [RFC9250] #6245

Are you sure you want to change the base?

New Plugin: QUIC proxy [RFC9250] #6245

Conversation

jaehnri commented Aug 8, 2023 • edited

1. Why is this pull request needed and what does it do?

2. Which issues (if any) are related?

3. Which documentation changes (if any) need to be made?

4. Does this introduce a backward incompatible change or deprecation?

chrisohaver Aug 31, 2023

Choose a reason for hiding this comment

jaehnri Sep 17, 2023

Choose a reason for hiding this comment

chrisohaver Nov 1, 2023

Choose a reason for hiding this comment

jaehnri Sep 18, 2023

Choose a reason for hiding this comment

chrisohaver Nov 1, 2023

Choose a reason for hiding this comment

jaehnri Sep 18, 2023 • edited

Choose a reason for hiding this comment

jaehnri Sep 18, 2023

Choose a reason for hiding this comment

iFrozenPhoenix Jan 8, 2024

Choose a reason for hiding this comment

codecov bot commented Sep 18, 2023 • edited

Codecov Report

chrisohaver commented Mar 1, 2024

jaehnri commented Aug 8, 2023 •

edited

jaehnri Sep 18, 2023 •

edited

codecov bot commented Sep 18, 2023 •

edited