Admin message

Self sign-up has been disabled due to increased spam activity. If you want to get access, please send an email to a project owner (preferred) or at gitlab(at)nic(dot)cz. We apologize for the inconvenience.

Consider preserving QNAME case in knot_pkt_t 'wire' field
Hi, Back in https://gitlab.nic.cz/knot/knot-dns/-/issues/773, I mentioned: > Personally I think it would be better for the `wire` field of `knot_pkt_t` to be left unmodified and represent the originally received wire message, and require all accesses of the QNAME to occur by calling `knot_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 https://gitlab.nic.cz/knot/knot-dns/-/issues/777 for a bug that the initial fix to https://gitlab.nic.cz/knot/knot-dns/-/issues/773 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 7f3aa3f6a36b26bbb01f291684d6067077c62fa1, 4a3eb76be83b99b100a8289253205d6e7559821f, and 55663f09360c34c5daac5f317bf8e186e581fc29). 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!
issue