Skip to content
GitLab
Projects Groups Topics Snippets
  • /
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Register
  • Sign in
  • Knot DNS Knot DNS
  • Project information
    • Project information
    • Activity
    • Labels
    • Members
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributor statistics
    • Graph
    • Compare revisions
  • Issues 19
    • Issues 19
    • List
    • Boards
    • Service Desk
    • Milestones
  • Merge requests 6
    • Merge requests 6
  • CI/CD
    • CI/CD
    • Pipelines
    • Jobs
    • Schedules
  • Deployments
    • Deployments
    • Environments
    • Releases
  • Packages and registries
    • Packages and registries
    • Container Registry
  • Monitor
    • Monitor
    • Incidents
  • Analytics
    • Analytics
    • Value stream
    • CI/CD
    • Repository
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Activity
  • Graph
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
Collapse sidebar
  • Knot projects
  • Knot DNSKnot DNS
  • Issues
  • #780
Closed
Open
Issue created Jan 06, 2022 by Robert Edmonds@edmondsReporter

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 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 #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!

Assignee
Assign to
Time tracking