#5680 closed enhancement (duplicate)

kea-dhcp4 server needs to support configurable client hostname sanitization

Reported by: tmark Owned by: tmark
Priority: high Milestone: Kea1.5
Component: dhcp4 Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by tmark)

The Infoblox SOW requires the ability to sanitize client host name values, sent by DHCPv4 clients, prior to using them to form the FQDN for DNS updates. Specifically, there should be two configuration parameters:

  1. A regular expression (e.g. [A-za-z0-9]-) which describes the allowed characters
  2. A replacement for characters that are disallowed (e.g. _ by -)

Does not apply to v6, or to v4 FQDN option values, as per RFC they cannot contain non-compliant characters.

Subtickets

Change History (14)

comment:1 Changed 17 months ago by tmark

  • Owner set to tmark
  • Status changed from new to assigned

comment:2 Changed 17 months ago by fdupont

  • Description modified (diff)

comment:3 Changed 17 months ago by fdupont

BTW bind9 has at least the 1 in src/lib/name.c dns_name_ishostname()... 2 is more challenging because hostname allowed character set is *very* limited (e.g. no ?, - illegal in first position, etc).

comment:4 Changed 17 months ago by fdupont

  • Summary changed from kea-dhcp4/6 servers need to support confiugrable client hostname sanitization to kea-dhcp4/6 servers need to support configurable client hostname sanitization

comment:5 Changed 17 months ago by tmark

  • Component changed from Unclassified to dhcp4
  • Description modified (diff)
  • Summary changed from kea-dhcp4/6 servers need to support configurable client hostname sanitization to kea-dhcp4 server needs to support configurable client hostname sanitization

comment:6 Changed 17 months ago by tmark

  • Owner changed from tmark to UnAssigned
  • Status changed from assigned to reviewing

Ticket is ready for review.

Note that kea-dhcp6 parsing recognizes the new parameters but the server does not use them. Initially I added them, before realizing this is a v4 feature only. However, to remove them such that v6 parsing will reject them, means making D2ClientConfig aware whether or not it belongs to a v6 client.
This is something we may eventually need for other reasons but for now, it is protocol family agnostic. Reviewer(s) should weigh in on this.

Requirements are here: http://kea.isc.org/wiki/HostnameSanitizer.

Proposed ChangeLog?:

14xx.   [func]      tmark
    Added two new configuration paramters to kea-dhcp4's DhcpDdns
    section, 'hostname-char-set' and 'hostname-char-replacment'.
    These values (when not empty) are used by the server to sanitize
    host names sent by DHCP clients prior to using them in DNS updates.
    (Trac #5680, git TBD)

comment:7 Changed 17 months ago by tmark

  • Owner changed from UnAssigned to tmark
  • Status changed from reviewing to accepted

comment:8 Changed 17 months ago by tmark

Infoblox in an email says it should also apply to FQDN option(s). This might warrant some discussion as FQDNs, per RFC should be compliant already.

comment:9 Changed 16 months ago by wlodekwencel

sanitization of option 12 tested, didn't find any issue. I assume that option 81 is not yet implemented.

One thing worth mentioning in the doc is.. when omitting signs it can be possible that multiple clients end up with the same hostname (which will be rejected by DNS), people that will use that feature probably are aware of this, but still warning should be added to docs:
e.g.:
hostname-char-set "[A-Za-z.-]"
hostname-char-replacement ""

and:
client1.example.com
client2.example.com
will end up with the same hostname:
client.example.com

comment:10 Changed 16 months ago by tmark

  • Owner changed from tmark to UnAssigned
  • Status changed from accepted to reviewing

Extended to cover DHCPv4 and V6 FQDN options per Infoblox.
Updated admin guide.

ChangeLog?:

14xx.   [func]      tmark
    Added two new configuration paramters to kea-dhcp4 and kea-dhcp6
    DhcpDdns sections: 'hostname-char-set' and 'hostname-char-replacment'.
    These values (when not empty) are used by the server to sanitize
    host name and FQDN domain names sent by clients prior to using them
    to construct DNS names.
    (Trac #5680, git TBD)

comment:11 Changed 16 months ago by tomek

  • Owner changed from UnAssigned to tomek

comment:12 follow-up: Changed 16 months ago by tomek

  • Owner changed from tomek to tmark

I think defining illegal characters is not the best
approach and doing the opposite would be better operationally.
The reason is that if you define a valid set, you're
ready to go, no matter what crazy stuff your users may come up with.
With invalid characters, there will be race to the bottom
between sysadmins and users with clever ideas. Anyway, I should
have raised this point couple weeks ago. It's too late to have
second thoughts about this. And regexps provide negation operator,
so the logic can be flipped (as you pointed out in the docs).

New parameters should be showcased in one v4 and one 6 example
configs. Added.

dhcp4/fqdn_unittest.cc

sanitizeHost test: Since we migrated to C++11, we can use its for
(auto name : container) syntax. Also, the was one indent level
missing in the innermost scope. Updated both. Please pull and
review.

util/strutil.h
The comment mentioned pre gnu 4.9.0. Did you mean gcc or g++
compiler?

strutil_unittest.cc
Small corrections in comments. Please pull and review.

labelsequence_unittest.cc
The tests covered here are little light. Added couple more tests.
I suppose it's ok for now.

d2_client_unittest.cc
The scenarios structure had weird indentation. It's fixed now.


Code compiled, unit tests passed on ubuntu 18.04.

Your ChangeLog? looks ok. Since you're on PTO today, I'll merge
the code today, but please take a quick look at my changes when
you get back.


I'm in process of merging and pushing those changes to master.

comment:13 in reply to: ↑ 12 Changed 16 months ago by tmark

Replying to tomek:

I think defining illegal characters is not the best
approach and doing the opposite would be better operationally.
The reason is that if you define a valid set, you're
ready to go, no matter what crazy stuff your users may come up with.
With invalid characters, there will be race to the bottom
between sysadmins and users with clever ideas. Anyway, I should
have raised this point couple weeks ago. It's too late to have
second thoughts about this. And regexps provide negation operator,
so the logic can be flipped (as you pointed out in the docs).

It is really a matter of semantics, so whether you express it as what is valid
or what is invalid is a matter of taste. To me it is more intuitive and easier
to list it as "anything BUT these", replace it. The subset of legal characters
is typically much smaller and easier to define in an expression.

New parameters should be showcased in one v4 and one 6 example
configs. Added.

dhcp4/fqdn_unittest.cc

sanitizeHost test: Since we migrated to C++11, we can use its for
(auto name : container) syntax. Also, the was one indent level
missing in the innermost scope. Updated both. Please pull and
review.

util/strutil.h
The comment mentioned pre gnu 4.9.0. Did you mean gcc or g++
compiler?

g++

strutil_unittest.cc
Small corrections in comments. Please pull and review.

labelsequence_unittest.cc
The tests covered here are little light. Added couple more tests.
I suppose it's ok for now.

d2_client_unittest.cc
The scenarios structure had weird indentation. It's fixed now.

Probably too much coffee.


Code compiled, unit tests passed on ubuntu 18.04.

Your ChangeLog? looks ok. Since you're on PTO today, I'll merge
the code today, but please take a quick look at my changes when
you get back.


It all looks good.

I'm in process of merging and pushing those changes to master.

Thanks. Do I have to pay you for this?

comment:14 Changed 15 months ago by tomek

  • Resolution set to duplicate
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.