Page MenuHomeVyOS Platform
Create Task
Maniphest T6123

Limit NTP allow-client config to internal addresses by default
Needs testing, LowPublic
Actions

Description

Opening this tasked based on discussion from this forum thread: https://forum.vyos.io/t/default-ntp-settings/13903

Recommendation is to change default:

ntp {
    allow-client {
        address "0.0.0.0/0"
        address "::/0"
    }

To:

ntp {
    allow-client {
        address "127.0.0.0/8"
        address "::1/128"
        address "10.0.0.0/8"
        address "172.16.0.0/12"
        address "192.168.0.0/16"
        address "169.254.0.0/16"
        address "fc00::/7"
        address "fe80::/10"
    }

This would restrict access to NTP to internal use IPs.

I'd be happy to submit PR but would need one of the experts to chime in on whether the default values are configured from the ntp migration script here:
https://github.com/vyos/vyos-1x/blob/f237e75e9fd156c2e4ce15dd6ca8fd4d5d8790cc/src/migration-scripts/ntp/1-to-2

Removing:

config.set(new_base_path + ['allow-client', 'address'], value='0.0.0.0/0', replace=False)
config.set(new_base_path + ['allow-client', 'address'], value='::/0', replace=False)

Adding:

config.set(new_base_path + ['allow-client', 'address'], value='127.0.0.0/8', replace=False)
config.set(new_base_path + ['allow-client', 'address'], value='10.0.0.0/8', replace=False)
config.set(new_base_path + ['allow-client', 'address'], value='172.16.0.0/12', replace=False)
config.set(new_base_path + ['allow-client', 'address'], value='192.168.0.0/16', replace=False)
config.set(new_base_path + ['allow-client', 'address'], value='169.254.0.0/16', replace=False)
config.set(new_base_path + ['allow-client', 'address'], value='fd00::/8', replace=False)

Details

Difficulty level
Unknown (require assessment)
Version
VyOS 1.4.0-epa1
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Unspecified (possibly destroys the router)
Issue type
Improvement (missing useful functionality)

Related Objects

Event Timeline

Giggum created this task.Mar 15 2024, 12:43 AM
Giggum created this object in space S1 VyOS Public.
Giggum changed Version from - to VyOS 1.4.0-epa1.Mar 15 2024, 1:00 AM
pasik added a subscriber: pasik.Mar 15 2024, 8:38 AM
Apachez added a subscriber: Apachez.Mar 15 2024, 10:36 AM
matthewr added a subscriber: matthewr.Mar 16 2024, 2:03 PM
Giggum added a comment.EditedMar 26 2024, 5:07 PM

Have installed 1.5-rolling-202403200018 in a VM as a clean install (no prior configs) to dig at this but have hit a block.

By default 0.0.0.0/0 and ::/0 are being allowed and I can see that this comes from the 'ntp' class object in service_ntp.py* which is defaulting to include those address values {'address' : ['0.0.0.0/0', '::/0']}.

Can someone with an understanding of class Config nudge me in the right direction on which function is assigning these values within that class?

*service_ntp.py: https://github.com/vyos/vyos-1x/blob/0fcab52921f27f8d2df9038a193be66c2ba18d11/src/conf_mode/service_ntp.py#L40

Giggum added a comment.Tue, Apr 2, 8:23 PM

Related to https://vyos.dev/T6080

Giggum updated the task description. (Show Details)Tue, Apr 9, 12:18 AM
Giggum updated the task description. (Show Details)Tue, Apr 9, 12:36 AM
Giggum mentioned this in T5694: NTP should always be allowed from localhost and bindaddress/binddevice can only exist once.Wed, Apr 10, 2:07 AM
Giggum added a comment.EditedThu, Apr 11, 3:33 PM

Editing config.boot.default has addressed this. Pull request opened for comments/integration here: https://github.com/vyos/vyos-build/pull/559

Apachez added a comment.Sun, Apr 14, 3:54 AM

Will a migrationsscript be included so that users who used the default of:

ntp {
    allow-client {
        address "0.0.0.0/0"
        address "::/0"
    }

would have that adjusted into this?

ntp {
    allow-client {
        address 127.0.0.0/8
        address 169.254.0.0/16
        address 10.0.0.0/8
        address 172.16.0.0/12
        address 192.168.0.0/16
        address ::1/128
        address fe80::/10
        address fc00::/7
    }

Otherwise this fix will only be valid for new deployments while current installations (who used the default) wont be fixed and still being exposed.

Viacheslav changed the task status from Open to Needs testing.Tue, Apr 16, 1:03 PM
Giggum added a subscriber: Viacheslav.Mon, Apr 22, 8:00 PM

@Viacheslav or another of the Maintainers:

This comment is in the 1-to-2 migration script for NTP ... should it remain as is for 1.3x in light of this Maniphest ticket or are we permitted to ammend and therefore change "fully backwards compatible" to best effort between compatibility and a sane default?

# By default VyOS 1.3 allowed NTP queries for all networks - in chrony we
# explicitly disable this behavior and clients need to be specified using the
# allow-client CLI option. In order to be fully backwards compatible, we specify
# 0.0.0.0/0 and ::/0 as allow networks if not specified otherwise explicitly.