dns-client-mirage udp #322
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds support for either all udp or all tcp|tls resolvers. At a later stage, we can of course support mixed sets of resolvers (especially with the previous commit paving this path).
…e.connect This avoids the need to unmarshal the arguments for a Dns_stub.t instantiation, and makes the surface more uniform.
hannesm
commented
Oct 7, 2022
similar to qubes-mirage-firewall (thanks @palainp), at initialization time a single udp_port is reserved as last resort. In general, the UDP source port is randomized, and UDP.listen/unlisten are executed on that port (which is as well registered / unregistered). If the port allocation fails, the last_udp_port is used, which is always listened to.
|
@reynir if you could take a look at this, that'd be great :) |
reynir
reviewed
Oct 17, 2022
There was a problem hiding this comment.
I am a little concerned that generate_udp_port doesn't check that nothing is listening on the generated port. The interface of UDP.listen always replaces an existing listener. An application that uses DNS and listens on UDP may eventually break because a dns lookup triggered generate_udp_port to generate the application port. However, with the current interface I can't see how we can do better.
Co-authored-by: Reynir Björnsson <reynir@reynir.dk>
Co-authored-by: Reynir Björnsson <reynir@reynir.dk>
…gth was received (as reviewed by @reynir)
|
thanks for your review -- I addressed your comments :) |
|
the interface of UDP.S with listen and unlisten is not optimal, indeed -- and should be enhanced -- maybe as simple as providing a |
previously, the source port of the remote (usually 53) was checked against our source port (some random ephemeral), leading to no accepted reply. the mirage udp interface does not provide the destination port in the callback (report as mirage/mirage-tcpip#497).
reynir
reviewed
Oct 24, 2022
reynir
approved these changes
Oct 24, 2022
hannesm
added a commit
to hannesm/opam-repository
that referenced
this pull request
Oct 24, 2022
…er, dns-mirage, dns-client, dns-cli and dns-certify (6.4.0) CHANGES: * dns-client: demote log level of response to debug (mirage/ocaml-dns#317 @hannesm) * dns-client: use DNS-over-TLS for uncensoreddns.org only (mirage/ocaml-dns#320 @hannesm) * API: dns-client: connect returns the protocol (UDP/TCP), allowing mixed UDP and TCP namerservers being used (mirage/ocaml-dns#322 @hannesm) * dns-client-mirage: allow hostname in authenticator, improve error message and documentation (mirage/ocaml-dns#319 mirage/ocaml-dns#322 @hannesm) * dns-client-mirage: support UDP nameservers as "udp:<IP>" in nameserver_of_string (mirage/ocaml-dns#322 @reynir @hannesm) * API: dns-client, dns-stub, dns-resolver: ?size is now ?cache_size (mirage/ocaml-dns#322 @hannesm, suggested by @reynir)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.