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

Don't return an empty label from to_domain_name #54

Closed
wants to merge 1 commit into from

Conversation

lcdunstan
Copy link

Fixes #53

@dsheets
Copy link
Member

dsheets commented Aug 11, 2015

Is this really an issue with this library or the ocaml-dns consumer? If this library, the documentation will need updating as well.

I'm inclined to believe that the current implementation is correct and the consumer should be changed but maybe not. We need an IDN library.

@lcdunstan
Copy link
Author

I did consider that, and yes Dns.Name.of_ipaddr could be changed to something like:

let of_ipaddr ip = of_string_list (Ipaddr.to_domain_name ip |> List.filter (fun s -> s <> ""))

(that was just a quick hack that did fix gethostbyaddr)
Or strip it inside of_string_list.

But I didn't find code anywhere in ocaml-dns that supports the convention that a list of strings ending in "" signifies an FQDN, so that's why I filed it here.

By documentation update, do you mean:

  (** [to_domain_name ipv4] is the domain name label list for reverse
      lookups of [ipv4]. This includes the [.in-addr.arpa] suffix. *)

?

@dsheets
Copy link
Member

dsheets commented Aug 11, 2015

That is what I mean by documentation update, yes.

I think ocaml-dns should be made compatible with FQDNs in general and, if that is not practical in the short term, should be made to deal with this interface. Is that off-the-mark? Maybe the FQDN/PQDN distinction is irrelevant here and we should drop the trailing "" after all? Nicely abstracted (shared) types representing domain names of various kinds would help with this.

@lcdunstan
Copy link
Author

Yeah maybe ocaml-dns should also be changed to prevent creation of a bad Name.t. Maybe others can upvote/downvote this PR?

@avsm
Copy link
Member

avsm commented May 11, 2017

see mirage/ocaml-dns#137 where this issue indeed comes up again

@hannesm
Copy link
Member

hannesm commented Jun 17, 2018

if anyone is still listening, i just factored out my domain-name library https://github.com/hannesm/domain-name (not released yet, happy to hear feedback on the API) which uses few dependencies (fmt, astring) and could be used in ipaddr. (NB: it does not accept strings with trailing .)

@avsm
Copy link
Member

avsm commented Sep 24, 2018

the domain-name interface looks good to me. Only query that came up when I read it was why you need a case_sensitive option on the equals. Aren't they always case-insensitive when it comes to domain names?

@cfcs
Copy link

cfcs commented Sep 24, 2018

@avsm AFAI understand, case-sensitive equal is a hack to get more entropy in DNS queries exploiting the fact that DNS query names should return the exact query (it's not allowed to change the case).
Thus by arbitrarily flipping the lowercase bit, you can get a few extra bits of entropy.
In all other cases, AFAIK, the matching should be case-insensitive.

@hannesm
Copy link
Member

hannesm commented Sep 24, 2018

@avsm as @cfcs mentioned, entropy in DNS queries is pretty low -- 16 bit from DNS transaction ID, which means you need 65k packets for a collision -- so people use 1 bit entropy for each alphabetic character in the query, and servers are supposed to send the same domain-name back to the client (i.e. mIrAgE.iO will be echoed) -- this way there's more entropy. now, this is only relevant for a client issuing a DNS request, and awaiting a reply, and it is brittle since some DNS servers ignore casing and respond with all uppercase (or all lowercase) letters.

the result is: it is a good idea to provide both comparisons in a domain-name library, and maybe even a function "randomize_casing" (which then requires a RNG) from Domain_name.t -> Domain_name.t.

@avsm
Copy link
Member

avsm commented Jul 9, 2019

Fixed via #88

@avsm avsm closed this Jul 9, 2019
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.

Ipaddr.to_domain_name returns an empty label
5 participants