feat: TCP, DoT, DoH support for DNS monitor #5940
Conversation
![github-advanced-security[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Similar to hostNameRegexPattern, except the hostname pattern | ||
| // can also match root (.) and top-level domains (.com, .org), | ||
| // and may contain underscores (_) | ||
| const dnsNamePattern = /^(\.|(\.?[a-zA-Z0-9\-_]+)+)$/; |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
| */ | ||
| export function urlPathRegexPattern(qstr = false, tmpl = false) { | ||
| // Matches any URL path, including empty string | ||
| const pathRegexPattern = /^\/?(([a-zA-Z0-9\-_%])+\/)*[a-zA-Z0-9\-_%]*(\?([a-zA-Z0-9\-_%]+=[a-zA-Z0-9\-_%]*&?)+)?$/; |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
| // Matches any URL path, including empty string | ||
| const pathRegexPattern = /^\/?(([a-zA-Z0-9\-_%])+\/)*[a-zA-Z0-9\-_%]*(\?([a-zA-Z0-9\-_%]+=[a-zA-Z0-9\-_%]*&?)+)?$/; | ||
| // Ensures a URL path follows query string format | ||
| const queryRegexPattern = /^\/?(([a-zA-Z0-9\-_%])+\/)*[a-zA-Z0-9\-_%]*\?([a-zA-Z0-9\-_%]+=[a-zA-Z0-9\-_%]*&?)+$/; |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
There was a problem hiding this comment.
Could you split this PR up into multiple PRs?
The PR changes 8 things as noted by you.. We can group related things, but +900 -90 lines (discounting tests/..) is not omething that I can review.
For such large PRs, it is waaay to simple to miss bugs/...
I looked over the changes and found the following:
- the inital migration should not change. Please use a migration instead
- naming for migrations follows YYYY-MM-DD format.
- Removal of the
recordConditionVariablelikely breaks existing monitors. Adding variables is fine, but for replacement, you need to add a migration - Regarding changging of the hostname regex, I think we need to change this fundamentally. See #3929 (comment)
CommanderStorm
added
the
pr:please address review comments
|
ACK. Let me work on making this PR exclusively for the new DNS transport methods and related features. Changes to regex and condition variables/logic can get separate PRs. So question about the database and migrations. Do you mean that I need to add new columns in a migration, and leave the |
|
Yes, correct. The migration is already in |
2 participants
📋 Overview
Provide a clear summary of the purpose and scope of this pull request:
What problem does this pull request address?
What features or functionality does this pull request introduce or enhance?
DoT), and DNS over HTTPS (DoH).DoHquery URLDoTandDoH.DNSSECvalidation. Before, node DNS client hadDNSSECsupport bit unset.DoHtransport method to useGETorPOSTmethod, and send requests usingHTTP/2.A,AAAA,NS, andMXare evaluated as an array in conditions.CAAandSOAnow support evaluating against rrtype-specific fields in conditions.🔄 Changes
🛠️ Type of change
🔗 Related Issues
📄 Checklist *
📷 Screenshots or Visual Changes
This is the EditMonitor page before changes
Before
This is the EditMonitor page after changes, with new components in red:
After
ℹ️ Additional Context
Provide any relevant details to assist reviewers in understanding the changes.
Click here for more details:
This PR is primarily intended to bring DNS over TLS and DNS over HTTPS support to the DNS monitor. I found some areas for improvement along the way that I will describe as best I can.
The built-in node
dnsresolver inserver/util-server.jshas been replaced withdgram,Socket,https, andhttp2built-in node modules. I would have loved to bring in support for DNS over QUIC as well, but opted not to because there is no built-in node module for QUIC. I am using the external moduledns-packetfor encoding/decoding DNS packet data because it is very easy to manipulate packet-level data without needing to make adjustments for each transport type. I settled on it after finding its use in other projects includingdohdecandtangerine. I'm also usingip-addressfor identifying and manipulating IP addresses.The biggest changes besides the transport methods are the condition variables and logic. Excluding
TXT, the RRTYPES that are expected to return more than one record are passed to the expression evaluator as an array. This allows for more meaningful and/or logic in conditions, but also requires to specify the full value of each record that should match. I did it this way because usingArray.some()in the DNS monitor was conflicting with the evaluator's ability to correctly evaluate multiple conditions. For example, before if I configured a query forone.one.one.onefor typeA, and a condition for the records to contain values1.1.1.1and1.0.0.1, it would always evaluate the condition tofalseno matter what the answers were. The solution I came up with was to pass the list of answers to the evaluator and let it decide if it contained the exact value (or not).Due the reason for changes made in PR #5768, I configured the answers for
TXTrecords to be passed to the evaluator as a pipe-separated string (txt_record_1|txt_record_2), so the string operators could still be used in this respect. The reason theTXTrecord answers needed to be flattened in the first place is because they use strings that have a 255 character limit (RFC 1035). In order to overcome this limitation on values longer than 255 characters, the value is split into 255-character length strings that are later reconstructed by the DNS client. There can be multipleTXTrecords for a DNS name, and eachTXTrecord can have multiple strings associated to it. Flattening the answers will not match properly in conditions for records that exceed 255 characters and thus use multiple strings. In the code for this PR, I've made sure theTXTanswers are properly reconstructed before sending to the evaluator.Record types
CAAandSOAare expected to return a single answer. This allows for using any of those record types' RDATA fields for condition variables.I updated the regular expressions in
src/util-frontend.jsthat are used for validating hostnames and IP addresses. This is because DNS names have a bit more flexibility than normal hostnames and FQDNs, plus I wanted to require IP for some DNS transport methods, and hostnames for others. I first converted the existing patterns to JSRegExpenclosed in forward slash (/) for proper linting. To get these to work with the Vue:patterndirective, I used theRegExp.sourceattribute which provides the pattern in an escaped string. Then I updated the patterns for hostname, IPv4, and IPv6 to match what I needed in the DNS monitor form. Finally, I created some parameters that can be used to switch between the different combinations of patterns for different scenarios, retaining the functionality for the MQTT monitor. There are a couple places where I needed to tell ESLint to ignore escapes (\) that are actually necessary for the pattern to work.Smaller changes and improvements I made include:
💬 Requested Feedback
Given the changes made, I would like to know how current users feel about them and if any new documentation should be created. Personally, I feel that the descriptions on the EditMonitor form are adequate for guidance on configuring the different transport methods. However, the changes to conditions might warrant a quick guide on how to configure the logic for different record types.
I also need some assistance confirming that IPv6 is working for the different transport methods. I don't have IPv6 with my ISP, so I have not been able to test it extensively against IPv6 resolvers.