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

Significant changes for Proxy DNS and SVCB request/response handling : try 2 #9

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

enygren
Copy link
Contributor

@enygren enygren commented Mar 30, 2021

No description provided.

@enygren enygren marked this pull request as ready for review March 30, 2021 18:48
@enygren
Copy link
Contributor Author

enygren commented Mar 30, 2021

Hopefully this should actually be able to be merged?

Copy link

@MikeBishop MikeBishop left a comment

Choose a reason for hiding this comment

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

This is a good start, with mostly nits and comments on issues that can be deferred from this PR.

I do feel like we don't have a good story (or perhaps merely a good example?) for cases which involve a mix of CNAMEs and AliasMode entries, though perhaps that's alleviated by the current design of telling the proxy not to consider SVCB records in performing the actual connection.

Comment on lines +377 to +383
*TODO*: Do we need the "o=" parameter or is it redundant?
If we include it, should it be a MUST or MAY?

*TODO*: Do we need to cover DNAME as well?

*TODO*: Should a future version be able to include NS record
and DNSSEC information?

Choose a reason for hiding this comment

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

At first glance, it appears to be redundant. Is there a situation in which the owner is not simply the original hostname (for the first entry) or the preceding hostname (for subsequent entries)?

However, if we want to be able to include additional information in the future, it might make sense to swap the format around; rather than including a list of CNAME(s) and/or IP(s) and giving an owner for each, list the owner first and an inner list of useful records for each.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The redundancy is why I'm include to leave it optional or just get rid of it. It could be helpful for cases such as DNSSEC in the future, or perhaps if we include NS records or other things,

Comment on lines +388 to +392
Proxy servers MUST NOT take action based on SVCB records.
In particular, the ipv4hint and ipv6hint SvcParams MUST NOT
be used by proxies for making connections.

*TODO*: Discuss if this is too restrictive

Choose a reason for hiding this comment

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

I think this probably is too restrictive, especially in the case of SVCB-naive clients. It means a high probability of the client having to open a new tunnel after processing the records, even though avoiding that is something this spec states as a goal.

It still seems reasonable for the proxy to chase AliasMode records, use hints, etc. to choose the target to connect to. The flip side, I suppose, would be if the proxy chooses a host whose ALPN list doesn't reflect the client's support, but that could be solved by requiring the server to choose based on the transport protocol implied by the method, and then preferring the default ALPN within that. Alternatively, the client could include an ALPN list in the request.

However, it's also totally reasonable to save that for a separate PR if we decide to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Part of why I'm wary of this is that it changes the Proxy behavior and semantics substantially. Rather than the proxy just returning additional information, here we start fundamentally changing behavior. (Would doing this count as an "Update" to any of the RFCs defining CONNECT, for example?)

Comment on lines +397 to +399
Clients that are SVCB-required ({{SVCB}} Section 3) MUST perform a DNS
resolution prior to making a CONNECT* request, as they will need to
obtain SVCB records and a TargetName.

Choose a reason for hiding this comment

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

Given the problems of doing such a resolution noted above, how would they do that? It might make more sense to say that a client which is SVCB-required must abandon the tunnel if it doesn't receive SVCB records from the proxy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They could do DoH through the proxy, for example. Or use ODoH. Or do something else. We could mention this in security considerations, but it seems like an implementation detail we don't want to get into? This is already covered in the core SVCB draft in some detail.

Comment on lines 494 to 499
*TODO*: Do we want to include text on how clients
can also use Proxy-DNS-Used for connection coalescing?
(One thing missing there is the full set of IP addresses
to look for overlaps within an address family. I'm not
sure this is worth the complexity. Or do we remove this
sub-section entirely?)

Choose a reason for hiding this comment

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

My vote is for removal. Coalescing based on IP address is not a battle we've been winning, and I don't think this draft is the venue to continue fighting it.

Incorporating suggestions from Mike Bishop

Co-authored-by: Mike Bishop <[email protected]>
Start an Acknowledgements section before we forget.
tfpauly added a commit that referenced this pull request Apr 25, 2021
Test #9
@Smithad499
Copy link

eaffef1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants