Consider preserving QNAME case in knot_pkt_t 'wire' field
Hi,
Back in #773 (closed), I mentioned:
Personally I think it would be better for the
wire
field ofknot_pkt_t
to be left unmodified and represent the originally received wire message, and require all accesses of the QNAME to occur by callingknot_pkt_qname()
, which can be defined as always returning the QNAME in the canonical case (lower case).
And @dsalzman responded:
I understand your idea but QNAME is accessed from more places and always storing and lower-casing it doesn't sound better. It must be considered carefully.
(See also #777 (closed) for a bug that the initial fix to #773 (closed) caused.)
I considered this idea a little bit more and attempted to implement it here:
https://github.com/edmonds/knot-dns/tree/edmonds/orig-qname
This branch attempts to make the wire
field of knot_pkt_t
represent the original wire format packet buffer as received off the wire (if the packet contents were received off the wire). It adds a lower_qname
field to knot_pkt_t
that stores an allocated domain name (knot_dname_t *
) that contains the lowercased version of the packet's QNAME. The lower_qname
field needs to be updated from knot_pkt_parse_question()
, knot_pkt_init_response()
, and knot_pkt_put_question()
, and is freed from payload_clear()
in pkt.c
.
knot_pkt_qname()
is updated to return the lower_qname
field of a knot_pkt_t
object (rather than a pointer to inside the wire buffer as before), and also returns it as a const knot_dname_t *
rather than a plain knot_dname_t *
to try to catch callers that try to modify it. A separate function to return a const knot_dname_t *
to the QNAME in the wire format buffer is introduced, knot_pkt_wire_qname()
. So if a caller needs to access the QNAME in canonicalized (downcased) form, it should call knot_pkt_qname()
on the knot_pkt_t
object (as before), but if it needs to access the original QNAME off the wire (which could include both upper and lower case characters), it should call the new knot_pkt_wire_qname()
function.
These changes allowed removing a bunch of ad hoc calls that modify the QNAME inside the wire packet buffer. The process_query_qname_case_lower()
and process_query_qname_case_restore()
functions and their calls were removed from the nameserver. The store_original_qname()
function was removed from src/knot/events/handlers/update.c
. The dnsproxy and dnstap modules could have their own QNAME case patching code removed (reverting 7f3aa3f6, 4a3eb76b, and 55663f09).
After these changes I was then able to remove the orig_qname
field from knotd_qdata_extra_t
, since there were no more users of it. I also reviewed kdig's use of knot_pkt_qname()
and changed a few calls to knot_pkt_orig_qname()
instead.
I also looked for instances where the QNAME inside the wire packet buffer was accessed directly (git grep 'KNOT_WIRE_HEADER_SIZE'
) without using knot_pkt_qname()
, but I believe the only instances are in the implementation described above.
Overall this deletes more code than was added (unless I missed a bunch of places where the new lower_qname
field needs to be updated) and it retires a number of workarounds spread throughout the code base that were caused by the in-place downcasing of the QNAME in the wire format buffer. It should also prevent similar workarounds from being added in the future :-)
Thanks for considering!