dns/dnscrypt-proxy-ng: re-write

[agh1467]

This is an update to the dnscrypt-proxy plugin to support dnscrypt-proxy v2.1.0+.

I'm not a developer by trade, and don't have a strong background in the languages used here. So, sorry if I did something stupid, or got something wrong. I changed a lot of things with the intent to be more clear, faster, portable, aesthetically pleasing, dynamic, etc. That being said, all of that is only my opinion, and I'm eager to make any corrections if someone can provide guidance in the right directions.

There is far too much to talk about here. There is a README which has a description of all of the features of the plugin in detail. There are various notes about the functionality contained therein. There is more discussion about design decisions and other stuff that I've included in a separate file called DEVELOPMENT. There is a new file which is a MANIFEST of all of the relevant files in the plugin and a description of each file for reference.

There is additional documentation included in PHP DocBlocks, in the volt templates in a similar style, and in comments throughout almost every file. The compiled PHP DocBlocks are located here.

In the previous pull request (#2346) @fichtner expressed concerns about spiting DNSBL out to its own plugin. I'd like to understand more about this concern, as I don't have the necessary awareness to see the reliances. My primary reasoning for spiting it was for three reasons.

1. The majority of updates to the dnscrypt-proxy plugin have been to update the DNS black list (5 of 7)
2. The DNSBL functionality has nothing to do directly with dnscrypt-proxy, the output of DNSBL is only consumed by dnscrypt-proxy.
3. The DNSBL can be used by other services, and would be better served standing alone rather than relying on dnscrypt-proxy being installed.

The underlying functionality of DNSBL, such as performing updates, and creation of the list itself is still performed by the bash script. The changes I've included here relocate it to a new location in the menu, and output to a new more universally located /usr/local/etc/dnsbl/blacklist.txt. Reliance on the location /usr/local/etc/dnscrypt-proxy/blacklist.txt should be able to be accommodated with a symlink until the reliant services can update to the new location.

I've built this plugin (and DNSBL) into a package, and successfully test installed it via pkg. It's gone through a lot of feature testing while I was building it, but I don't have any automated tests written for it. Any guidance in that area is welcome.

There are several places where I wasn't sure of the best way to do a thing, I tried to note these in comments. Any pointers in those areas are welcome as well.

This plugin uses the config mount point of //OPNsense/dnscrypt-proxy instead of //OPNsense/dnscryptproxy/ so there shouldn't be any collisions or interference in the OPNsense config. It does use the same configuration directory /usr/local/etc/dnscrypt-proxy, and log directory /var/log/dnscrypt-proxy though, so the original dnscrypt-proxy plugin should be uninstalled while looking at this one to prevent it from clobbering the configuration files.

I did look into settings migration, but couldn't make any sense of it, and I think the usage of a different mounting point in the config may preclude that capability.

I tried to explain my approach for many things in the documentation, and the comments, but not everything got into the final cut so some things may need further explanation. I will be hanging out in the IRC channel to address any questions, or feel free to ask them here as well.

Incomplete list of features/changes in this commit:

• Added documentation (PHP, README, etc.)
  • Development Discussion
  • Manifest
• Refactored Jinja2 templates
  • New structures
  • Heavy usage of variables for names
• Refactored all Volt templates
  • Heavy usage of macros, and layout_parials
  • New layouts added
• Refactored all XML forms
  • New structure
  • Includes bootgrids, and other features
• Refactored all PHP Classes/Functions
  • Consolidated many into a single class
  • New function for supporting bootgrids
  • New classes/functions for doing import/export
• Additional UI functionality to support features
• Import/Export of several lists types
• Support for all lists
  • Allowed Names/IPs
  • Blocked Names/IPs
• Support for all settings in dnscrypt-proxy
  • All settings represented in the UI
  • Dropdown/interactive lists used where applicable
• Updates for Phalcon4
  • Symlink for log directory
• Added POST_INSTALL/DEINSTALL scripts
• Added Diagnostics tab
  • Resolve hostname
  • Show DOH Certificates
  • Configuration Check
  • Version
• Consolidated Logs tab including:
  • Main
  • Query
  • NX
  • Blocked Names
  • Blocked IPs
  • Allowed Names
  • Allowed IPs
• Added new configd actions for backend functions
• Add bootgrid function which supports all API functions
• General style cleanup (whitespace, long lines),
  and consistent formatting throughout
• Added first-time setup, and blocking modal while loading settings
• make style-fix, sweep

[L1ghtn1ng]

@mimugmail can you have a look at this please?

[Karlson2k]

@agh1467 Wow! Huge work, impressive!

Why do you limit disabled_server_names names only to set of the currently available servers? Some servers may periodically appear and disappear in the list. With this implementation user will need to catch the right moment to be able to add some particular server to the disabled list.

[agh1467]

@agh1467 Wow! Huge work, impressive!

Why do you limit disabled_server_names names only to set of the currently available servers? Some servers may periodically appear and disappear in the list. With this implementation user will need to catch the right moment to be able to add some particular server to the disabled list.

Thanks for this input! The intent wasn't to specifically limit the input, but was certainly the outcome. The intent was to provide the user a list of valid servers for selection, with the benefit of not having to type the names (or really know the names exactly). The input style selectpicker happens to not offer the option to enter arbitrary values. Therefore the user can only select what is listed in the dropdown.

You bring up a scenario that I did not think of, however, is a completely valid scenario, and I think should be accommodated. Accommodating it will be a challenge while accomplishing the other goals that I had though. With some quick experimenting here are a couple of challenges that I encountered:

1. The dropdown will not include arbitrary entries from the user as this is sourced from dnscrypt-proxy's list of current resolvers. The entries from the user will have to be injected into the list for them to appear.
2. The input box style selectpicker doesn't support arbitrary text entry to enable the user to input a server name.

I did try the tokenizer style which does support arbitrary text entry via the allownew element, however, the dropdown menu doesn't function correctly, only displaying the first few options available for selection. Another drawback of tokenizer is it expands the size of the input box as large as necessary to display all of the tokens, which can be very large with a lot of entries selected.

I did a quick search around, and it looks like it should be possible to add an option to the list, but the solution will be a bolt on, and not supported by OPNsense Core's bootstrap-select.js. That may be the only option to get the UI to work that way.

Another option, that I don't like, is having a secondary field for arbitrary server names, which would be tokenized, and combine the fields in the configuration file. It's definitely not ideal since it would be using two fields to store the same data. It's a compromise allowing the user to both select from the list, and also input server names as text.

FYI, I created an issue ticket in my repo for this.

[L1ghtn1ng]

@fichtner @AdSchellevis can one of you review this please?

[AdSchellevis]

@L1ghtn1ng We can't really accept this in the current state, these are multiple packages in a huge PR (I think that was @fichtner's previous concern as well), with overlapping functionality for an already existing plugin (dnscrypt-proxy). It would be better to work with @mimugmail to gradually extend the existing plugin (if possible) to prevent rewrites on periodic bases in case someone is seeking a new feature. Reviewing this would cost a significant amount of time, which is better spend elsewhere at the moment.

[L1ghtn1ng]

@agh1467 please work with @mimugmail to extend the existing package. Please see @AdSchellevis comment

[greggitter]

In addition to a rewrite, it would be nice to relax the cache min and max TTL values such that they can be set greater than the arbitrary (?) 3600 seconds unless there is some technical reason this is max allowed. I don't see any limits in DNSCrypt-proxy. Looking forward to the new interface and additional functions....thanks @agh1467 and of course @mimugmail. Cheers. :)

[L1ghtn1ng]

@greggitter As it stands this PR is not going to get merged as it is a new plugin where as OPNsense devs want it to be part of the current plugin which they have stated in a previous comment

[greggitter]

Thanks @L1ghtn1ng, I read through the thread and I guess I meant that some/most/all of the additional enhancements would or might be able to be merged into the existing add-on. And I should say thanks to all devs here...all the work is truly appreciated.

[agh1467]

In addition to a rewrite, it would be nice to relax the cache min and max TTL values such that they can be set greater than the arbitrary (?) 3600 seconds unless there is some technical reason this is max allowed. I don't see any limits in DNSCrypt-proxy. Looking forward to the new interface and additional functions....thanks @agh1467 and of course @mimugmail. Cheers. :)

Absolutely, the choice was entirely arbitrary. Looking through the dnscrypt-proxy code, I didn't see where a range is defined for this, though I've got no experience with Go. I do see that it's using a uint32 (unsigned 32-bit integer) which apparently has a valid range of 0 to 4294967295. So I don't see a problem with using that as the validation range. I'll check some of the other values as well.

Also closing this since there is no longer a point in keeping it open.