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

Additional DHCP client hostname validations #2952

Closed
ainar-g opened this issue Apr 13, 2021 · 8 comments
Closed

Additional DHCP client hostname validations #2952

ainar-g opened this issue Apr 13, 2021 · 8 comments
Assignees
Milestone

Comments

@ainar-g
Copy link
Contributor

ainar-g commented Apr 13, 2021

In #2946 we've added some validations for clients' hostnames. Some additional validations that we would like to add are:

  1. Hostname uniqueness validation among our DHCP clients.

  2. I'm not sure if we want this, but probably we should validate that the hostnames that clients send are actually valid domain name labels (that is, my-example-machine) as opposed to domain names (that is, my.example.machine).

    We decided to instead normalise the names using the algorithm described in this comment below.

@ameshkov, what do you think? Anything else I forgot?

@ameshkov
Copy link
Member

are actually valid domain name labels

I am not fully sure about that, they may easily send just plain strings that have nothing to do with domain names.

Wouldn't it be better to accept everything and convert to valid hostnames?

@ainar-g
Copy link
Contributor Author

ainar-g commented Apr 13, 2021

@ameshkov

I am not fully sure about that, they may easily send just plain strings that have nothing to do with domain names.

Not according to the RFC:

This option specifies the name of the client.  The name may or may
not be qualified with the local domain name (see section 3.17 for the
preferred way to retrieve the domain name).  See RFC 1035 for
character set restrictions.

Then again, we've already seen in the original issue that some device manufactureres just don't care, heh. But changing spaces into hyphens seems harmless enough (although I still have some doubts). On the other hand, if we consider that clients can send any random data there and that we somehow have to make sense of it, then we will have to define an algorithm for doing that and also support that algorithm. Possibly reimplementing it in other products. Whether it's Punycode or something custom.

In my personal opinion, it's better (and simpler) to just be strict by default. But I'm not sure about the usability of such solution. Maybe I'll implement the uniqueness validation and leave the other part as is for now.

@ameshkov
Copy link
Member

Well, the simple algorithm we could use is lower-casing + replacing all unsupported characters with hyphens

@ainar-g
Copy link
Contributor Author

ainar-g commented Apr 14, 2021

@ameshkov, I'm afraid that that won't do. Consider a client sending something like xn!!d1acpjx3f, which we convert into xn--d1acpjx3f, which will then be interpreted as an IDNA for яндекс. Honestly, the more you analyse the problem or making sense of random strings as domain names, the sillier the solutions become. I'm still in favour or more strictness.

@ameshkov
Copy link
Member

But we already know about existing issues where strictness would do more harm than good.

@ainar-g
Copy link
Contributor Author

ainar-g commented Apr 15, 2021

@ameshkov

We could use a three-step process of first replacing every invalid character with a hyphen, then collapsing all sequences of more than one hyphen into one, and then lowercasing it. So that something like:

My @#$%^&* Phone

Becomes:

my-phone

I still have doubts about it, but in combination with the uniqueness check I proposed in the OP this could probably work.

@ameshkov
Copy link
Member

Yep, that looks good to me

@ainar-g ainar-g added this to the v0.107.0 milestone Apr 15, 2021
@ainar-g ainar-g modified the milestones: v0.107.0, v0.106.0 Apr 19, 2021
adguard pushed a commit that referenced this issue Apr 19, 2021
Updates #2952.
Updates #2978.

Squashed commit of the following:

commit 20e379b
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Mon Apr 19 15:58:37 2021 +0300

    all: imp naming

commit ed300e0
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Mon Apr 19 15:43:09 2021 +0300

    all: imp dhcp client hostname normalization
adguard pushed a commit that referenced this issue Apr 20, 2021
Updates #2952.

Squashed commit of the following:

commit 45afcab
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 20 15:02:34 2021 +0300

    all: imp docs

commit d844ce1
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 20 14:57:49 2021 +0300

    all: more code imp

commit eef08cb
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 20 14:52:33 2021 +0300

    all: imp code, docs

commit 20748f2
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 20 14:30:57 2021 +0300

    all: imp dhcp host normalization, validation
@ainar-g
Copy link
Contributor Author

ainar-g commented Apr 20, 2021

This is now merged. If anyone finds any bugs or other issues, please file new bugs. Thanks!

@ainar-g ainar-g closed this as completed Apr 20, 2021
heyxkhoa pushed a commit to heyxkhoa/AdGuardHome that referenced this issue Mar 20, 2023
Updates AdguardTeam#2952.
Updates AdguardTeam#2978.

Squashed commit of the following:

commit 20e379b
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Mon Apr 19 15:58:37 2021 +0300

    all: imp naming

commit ed300e0
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Mon Apr 19 15:43:09 2021 +0300

    all: imp dhcp client hostname normalization
heyxkhoa pushed a commit to heyxkhoa/AdGuardHome that referenced this issue Mar 20, 2023
Updates AdguardTeam#2952.

Squashed commit of the following:

commit 45afcab
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 20 15:02:34 2021 +0300

    all: imp docs

commit d844ce1
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 20 14:57:49 2021 +0300

    all: more code imp

commit eef08cb
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 20 14:52:33 2021 +0300

    all: imp code, docs

commit 20748f2
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 20 14:30:57 2021 +0300

    all: imp dhcp host normalization, validation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants