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.

Sign up for GitHub

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

Jump to bottom

Enabling 0-RTT for QUIC/H3 clients #387

Merged
merged 5 commits into from
May 9, 2024
Merged

Enabling 0-RTT for QUIC/H3 clients #387

merged 5 commits into from
May 9, 2024

Conversation

LeonardWalter
Copy link
Contributor

Updated the DoQ and DoH QUIC client to enable 0-RTT based on the guide from: https://quic-go.net/docs/http3/client/#using-0-rtt

@LeonardWalter LeonardWalter mentioned this pull request May 6, 2024
Open
folbricht
folbricht reviewed May 7, 2024
View reviewed changes
Copy link
Owner

@folbricht folbricht left a comment

Choose a reason for hiding this comment

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

Much appreciated

dohclient.go Outdated
@@ -181,7 +181,12 @@ func (d *DoHClient) ResolveGET(q *dns.Msg) (*dns.Msg, error) {
ctx, cancel := context.WithTimeout(context.Background(), d.opt.QueryTimeout)
defer cancel()

req, err := http.NewRequestWithContext(ctx, "GET", u, nil)
method := http.MethodGet
if d.opt.Transport == "quic" {
Copy link
Owner

Choose a reason for hiding this comment

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

Since 0-RTT affects the security-properties, what do you think about adding a flag to config like enable-0rtt or so to enable it. In the docs we could then link to https://datatracker.ietf.org/doc/html/rfc8446#section-8 so users know and can decide if they want it anyway.

@LeonardWalter
Copy link
Contributor Author

I updated the DoQ client to use DialEarly. The implementation looks a bit weird but others (SSH3 for example) are also using net.ListenUDP(netString, nil) for this purpose.
In my testbed with adguard's dnsproxy DoQ server, I can observe 0-RTT packets with DoQ.

folbricht
folbricht reviewed May 8, 2024
View reviewed changes
Copy link
Owner

@folbricht folbricht left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this. As for docs, would you mind adding a note about the new flag to the documentation in https://github.com/folbricht/routedns/blob/master/doc/configuration.md#DNS-over-QUIC-Resolver ? If not I can do that later too though

doqclient.go Outdated
}

// Use quic.DialEarly to attempt to use 0-RTT DNS queries for lower latency
s.EarlyConnection, err = quic.DialEarly(context.TODO(), s.udpConn, udpAddr, s.tlsConfig, s.config)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm surprised that this change was even necessary. quicDial itself also calls quic.DialEarly. And it basically does the same thing.

Also there's a bit of a filehandle leak here when this fails. s.udpConn will not get closed. See the comment in https://github.com/folbricht/routedns/blob/master/dohclient.go#L427-L428

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, I tested it again and it works without the changes to use DialEarly. I will revert this

@LeonardWalter
reverted the doqcliet DialEarly changes as they were not needed and h…
9934842 
…ad a bug.

Renamed the 0RTT toggle and updated the documentation.
@mattkeenan
Copy link
Collaborator

@LeonardWalter you mentioned in your commit message that there was a bug in the previous doqclient code; what was the bug?

@LeonardWalter
Copy link
Contributor Author

LeonardWalter commented May 8, 2024 •
edited
Loading

The problem was what Frank mentioned with the updConn not being closed in my earlier commit (4d4f1a1)
If the query failed due to timeout or if the server rejects the query, the udpConn would remain open for some time causing new queries to fail.
This could have been avoided if I added a simple if statement that closes the udpconn again after a failed request.
As the change to DialEarly was not needed, opening the UdpConn was also redundant.
So I just reverted it.

@mattkeenan
Copy link
Collaborator

@LeonardWalter to be clear it wasn't a criticism of the commit, i wasn't sure i understood it correctly; apologies. also, so i understand correctly, dohclient.go will do the correct thing for http2 / TCP?

folbricht
folbricht reviewed May 8, 2024
View reviewed changes
dohclient.go Outdated
@@ -181,7 +186,12 @@ func (d *DoHClient) ResolveGET(q *dns.Msg) (*dns.Msg, error) {
ctx, cancel := context.WithTimeout(context.Background(), d.opt.QueryTimeout)
defer cancel()

req, err := http.NewRequestWithContext(ctx, "GET", u, nil)
method := http.MethodGet
if d.opt.Use0RTT {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if d.opt.Use0RTT {
if d.opt.Use0RTT && opt.Transport == "quic" {

Should we add this? To handle a mis-configuration where Use0RTT is used but it's not actually quic, but http2

@LeonardWalter
Copy link
Contributor Author

@mattkeenan
It is good that you mention this. The changes don't affect DoH much, but introducing the session cache (tlsConfig.ClientSessionCache = tls.NewLRUClientSessionCache(100)) does change the behavior.
Session resumption should also be used for HTTP/2, but I have not tested it.

I looked at the DoH RFC

TLS-based implementations often achieve better handshake performance
through the use of some form of session resumption mechanism, such as
Section 2.2 of [RFC8446]. Session resumption creates trivial
mechanisms for a server to correlate TLS connections together.

AFAIK there are countermeasures to this in TLS1.3 but don't quote me on this I am no expert. Other DoH implementations also it.
From the DNS over QUIC RFC but this also counts for DoH when using session resumption/pre shared keys

Session resumption and 0-RTT data transmission create privacy risks
detailed in Sections 7.1 and 7.2. The following recommendations are
meant to reduce the privacy risks while enjoying the performance
benefits of 0-RTT data, subject to the restrictions specified in
Section 4.5.

Clients SHOULD use resumption tickets only once, as specified in
Appendix C.4 of [RFC8446]. By default, clients SHOULD NOT use
session resumption if the client's connectivity has changed.

@LeonardWalter
Copy link
Contributor Author

Also, I really don't like how I disable 0-RTT for DoQ

if opt.Use0RTT { tlsConfig.ClientSessionCache = tls.NewLRUClientSessionCache(100) }
Session caching should not be disabled, instead there should be some other way to handle this.

@folbricht
Copy link
Owner

Let's keep the session cache enabled in all cases. It's a good thing to have regardless of config.

@LeonardWalter
Copy link
Contributor Author

Do you know of a flag or something to disable 0-RTT. I know Allow0RTT is only used for the server side. So far I haven't discovered a good way to toggle 0-RTT on and off

@folbricht
Copy link
Owner

Server side? Is it needed? Seems fine to support it in all cases

@folbricht folbricht merged commit f2a08d6 into folbricht:master May 9, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@folbricht folbricht folbricht approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LeonardWalter @mattkeenan @folbricht