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

Accept non-conventional ports from addresses resolved through DNS resolver or resolve method #2413

Closed
Nuhvi opened this issue Sep 10, 2024 · 9 comments

Comments

@Nuhvi
Copy link
Contributor

Nuhvi commented Sep 10, 2024

`Warning

Since the DNS protocol has no notion of ports, if you wish to send traffic to a particular port you must include this port in the URL itself, any port in the overridden addr will be ignored and traffic sent to the conventional port for the given scheme (e.g. 80 for http).
`

While this is generally true, the new HTTPS and SVCB records do support ports.

Would it be possible to change the current behavior to accept these ports?

@seanmonstar
Copy link
Owner

To be sure I understand, you're referring to a custom resolver that asks for those records instead of A or AAAA, and reqwest doesn't know that it's doing that? Then the port could have actually been specified...

Using the port in reqwest probably isn't too hard naively, but so far since it has been ignored, could using it cause surprising behavior if the port returned was junk?

@Nuhvi
Copy link
Contributor Author

Nuhvi commented Sep 12, 2024

@seanmonstar

To be sure I understand, you're referring to a custom resolver that asks for those records instead of A or AAAA, and reqwest doesn't know that it's doing that? Then the port could have actually been specified...

More or less. Usually recursive resolvers will also get the A and AAAA records that corresponds to the the target of the HTTPS records.

could using it cause surprising behavior if the port returned was junk

I don't imagine why would a resolver set a junk port, and since the default is 0 and since 0 usually means I don't know, it is safe to override 0 port, and respect everything else.

I dived more into the code and I think hyper_util will have to do the same as the h3_client here, so more.

So there is an opportunity here to also extend Resolve trait so it has a resolve_service_binding or resolve_endpoint method, that has a default implementation (hopefully that is enough for backward compatibility):

fn resolve_endpoint(&mut self, name: Name) -> Future<Output = Result<Option<Endpoint>, Self::Error>> {
    None
}

Where Endpoint is a struct that is very similar to HTTPS minus the lifetimes, and converting stuff like . to the actual domain.

Why bother with all of this? well because then reqwest can call resolver.resolve_endpoint(name) earlier in the request life time, and hopefully get early useful informations like alpn, ech, and maybe use ipv4hint and ipv6hint to make early connections that can be dropped later if resolve() returned authoritative addresses from A or AAAA.

I would be happy to work on that if you have some patience for questions.

Disclosure 1: I am not a Rust expert, and I get scared of Send and Sync, but I would like to get over that.

Disclosure 2: My main agenda here is to enable Reqwest to make requests to Pkarr domains, and eventually I will try to extend Rustls to also support certificates signed by Pkarr keys. But since this specific addition to reqwest is useful for more than just my use case, I thought it wouldn't be a waste of your time to suggest it.

seanmonstar pushed a commit to hyperium/hyper-util that referenced this issue Sep 17, 2024
If a resolved address sets the port number to something besides `0`, and the port isn't otherwise explicitly asked for, the `HttpConnector` will now use that port. This allows custom resolvers that might lookup SRVB or HTTPSrr records that include a port number.

cc seanmonstar/reqwest#2413
@blind-oracle
Copy link

A bit sad that this breaking change got into a patch-only hyper-util release which broke some of our workflow.

@blind-oracle
Copy link

Also probably the documentation here https://docs.rs/reqwest/latest/reqwest/struct.ClientBuilder.html#method.resolve_to_addrs should be updated since it clearly states that the port from the SocketAddr is not used.

@seanmonstar
Copy link
Owner

A bit sad that this breaking change got into a patch-only hyper-util release which broke some of our workflow.

Can you say anymore besides it being sad? I'm sorry it caused you problems, but it's also not a very actionable comment... Do you have a custom resolver that was picking a non-zero port that wasn't the real port? Was there a reason that I didn't think of?

@blind-oracle
Copy link

Sorry for not being specific.

We had some SocketAddrs passed to .resolve_to_addrs() overrides that contained non-zero ports.

And since for overrides it uses a custom resolver internally - the calls using those started picking ports from SocketAddrs instead of the usual URL schema.

That's why I think it is a breaking change.

@Nuhvi
Copy link
Contributor Author

Nuhvi commented Oct 10, 2024

@blind-oracle I am curious to know the source of that SocketAddrs and why is the port set to non-zero if not intended to be used?

@blind-oracle
Copy link

blind-oracle commented Oct 11, 2024

@Nuhvi well that's another story, they come from different workflow and since the docs state that

Since the DNS protocol has no notion of ports, if you wish to send traffic to a particular port you
must include this port in the URL itself, any port in the overridden addresses will be ignored and
traffic sent to the conventional port for the given scheme (e.g. 80 for http).

we didn't sanitize them to set the ports to zero...

@flub
Copy link

flub commented Nov 7, 2024

Oh, we also got caught out by the docs clearly stating that the port is ignored. I put 80 in as it seemed safer than 0, but now it seems 0 is the safer port to put in.

The docs in question:
https://docs.rs/reqwest/latest/reqwest/struct.ClientBuilder.html#method.resolve
https://docs.rs/reqwest/latest/reqwest/struct.ClientBuilder.html#method.resolve_to_addrs

It would be great if this could document the exact behaviour.

github-merge-queue bot pushed a commit to n0-computer/iroh that referenced this issue Nov 7, 2024
## Description

A non-zero port is interpreted as overriding the port specified or
implied by the URL.  Despite the docs not telling you this.  That's
not really what we want here.

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions


seanmonstar/reqwest#2413 (comment)

## Change checklist

- [x] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [x] Tests if relevant.
- [x] All breaking changes documented.
@Nuhvi Nuhvi closed this as completed Nov 15, 2024
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

No branches or pull requests

4 participants