opnsense / core

OPNsense GUI, API and systems backend
https://opnsense.org/
BSD 2-Clause "Simplified" License
3.37k stars 757 forks source link

Unable to set NTP Pools other than subdomains of pool.ntp.org #6923

Open CJ-Ross opened 1 year ago

CJ-Ross commented 1 year ago

Important notices

Before you add a new report, we ask you kindly to acknowledge the following:

Describe the bug

This commit introduced the ability of using ntp pools but restricts it to subdomains of pool.ntp.org.

https://github.com/opnsense/core/commit/7366d785a62d86de5f7eb7bfab15039492eea9bf

https://github.com/opnsense/core/issues/5569

To Reproduce

Steps to reproduce the behavior:

  1. Go to Network Time Service General page
  2. Add a pool such as pool.ntp.org or time.nist.gov and save
  3. Go to Network Time Service Status page
  4. See that added entry resolves to just one server.

Expected behavior

Ability to designate time servers as pools.

Describe alternatives you considered

Add a server/pool toggle to the time server UI along with the existing Prefer, Iburst, and Do not use.

daerSeebaer commented 3 months ago

Hi, so I tried to implement this as a checkbox that switches a given time server from "server" to "pool". The plan was to take the checkbox value, if the checkbox is set the config line begins with "pool", if not it begins with "server".

So far I have managed to implement the checkbox itself, and if it is set the server gets written into the OPNSense config file unter ntpd->pool as expected:

<ntpd>
    <logsys>on</logsys>
    <iburst>pool.ntp.org</iburst>
    <pool>pool.ntp.org</pool>
  </ntpd>

However, when the code tries to load the config entry, it crashes and claims that it got an array instead of a string: Fatal error: Uncaught TypeError: explode(): Argument #2 ($string) must be of type string, array given in /usr/local/etc/inc/plugins.inc.d/ntpd.inc:332

The offending lines (the "$pool" line is 332):

    $noselect = isset($config['ntpd']['noselect']) ? explode(' ', $config['ntpd']['noselect']) : [];
    $pool = isset($config['ntpd']['pool']) ? explode(' ', $config['ntpd']['pool']) : [];
    $prefer = isset($config['ntpd']['prefer']) ? explode(' ', $config['ntpd']['prefer']) : [];
    $iburst = isset($config['ntpd']['iburst']) ? explode(' ', $config['ntpd']['iburst']) : [];

I tried to debug this but only got to the point where I could confirm that $config['ntpd']['pool'] is in fact an array. I don't understand this, since the other options are written the same way, but result in a string. The configuration is written in /usr/local/www/services_ntpd.php:

$a_ntpd = &config_read_array('ntpd');

    $pconfig = $_POST;

        $config['system']['timeservers'] = trim(implode(' ', $pconfig['timeservers_host']));
        $a_ntpd['noselect'] = !empty($pconfig['timeservers_noselect']) ? trim(implode(' ', $pconfig['timeservers_noselect'])) : null;
        $a_ntpd['pool'] = !empty($pconfig['timeservers_pool']) ? trim(implode(' ', $pconfig['timeservers_pool'])) : null;
        $a_ntpd['prefer'] = !empty($pconfig['timeservers_prefer']) ? trim(implode(' ', $pconfig['timeservers_prefer'])) : null;
        $a_ntpd['iburst'] = !empty($pconfig['timeservers_iburst']) ? trim(implode(' ', $pconfig['timeservers_iburst'])) : null;
        $a_ntpd['interface'] = !empty($pconfig['interface']) ? implode(',', $pconfig['interface']) : null;

        write_config("Updated NTP Server Settings");

From my understanding, the options get passed in the POST request as arrays, then imploded into strings, those strings get stored in the config, and afterwards "exploded" back into arrays when the information is needed. All the checkboxes are handled exactly the same.

Some help with this, possible explanations of this behavior or hints about how OPNSense handles it's configuration in the background would be appreciated. This will be my first contribution, and while I have solid knowledge about operating an OPNSense, I'm not yet familiar with the codebase.