Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dhcp-server: T5414: improve bootfile-name constraint pattern #2118

Closed
wants to merge 1 commit into from

Conversation

etedor
Copy link

@etedor etedor commented Jul 28, 2023

Change Summary

This pull request expands the accepted character set for the bootfile-name option in the DHCP server configuration of VyOS. The original pattern [-_a-zA-Z0-9./]+ has been updated to [-_a-zA-Z0-9.:/~#@!$&*+,;=~%\\\\]+ to accommodate a broader range of input, including full URLs, file paths, bare filenames, and a subset of RFC 3886 reserved characters (excluding [] ' ()). This change allows DHCP server configurations in VyOS to support functions like zero-touch provisioning of network devices, which rely on the bootfile-name (Option 67) for fetching boot instructions. The new pattern also adds simple handling of "% encoding", and supports Windows paths used for services like Windows Deployment Services. This adjustment addresses the limitation that was preventing the system from accepting valid configurations and hindering the migration of existing (legacy) configurations during a VyOS upgrade.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

Component(s) name

dhcp-server

Proposed changes

Change the constraint regex pattern to: [-_a-zA-Z0-9.:/~#@!$&*+,;=~%\\\\]+

This adjustment expands the existing pattern to accommodate full URLs, file paths, and bare filenames. It also adds a subset of RFC 3886 "reserved" characters (gen-delims and sub-delims), excluding [] ' () , due to their rarity and negative interactions with the command line. Additionally, it adds simple handling of "% encoding".

Finally, the double-escaped \ enables support for Windows paths like those used for Windows Deployment Services (ex: boot\x64\wdsnbp.com). These land in dhcpd.conf as boot\\x64\\wdsnbp.com, which passes dhcpd -t. An explanation of string handling in ISC DHCP's documentation suggests that this is the correct format. See also: PXE Boot from WDS using ISC DHCPd (r/networking).

How to test

The following commands test the proposed pattern against input validation and against dhcpd -t upon commit:

configure

cp /opt/vyatta/share/vyatta-cfg/templates/service/dhcp-server/shared-network-name/node.tag/subnet/node.tag/bootfile-name/node.def /tmp/bootfile-name-node.def.bak

# manually edit the regex in node.def to [-_a-zA-Z0-9./:?#@!$&*+,;=~%\\\\]+
sudo vi /opt/vyatta/share/vyatta-cfg/templates/service/dhcp-server/shared-network-name/node.tag/subnet/node.tag/bootfile-name/node.def

sudo systemctl restart vyos-configd

set service dhcp-server shared-network-name TESTNET subnet 203.0.113.0/24 range 0 start 203.0.113.2
set service dhcp-server shared-network-name TESTNET subnet 203.0.113.0/24 range 0 stop 203.0.113.254

set service dhcp-server shared-network-name TESTNET subnet 203.0.113.0/24 bootfile-name "http://example.com/path/to/file.py"
commit
grep "shared-network TESTNET" -A9 /run/dhcp-server/dhcpd.conf

set service dhcp-server shared-network-name TESTNET subnet 203.0.113.0/24 bootfile-name "https://192.0.2.1/path/to/file.py"
commit
grep "shared-network TESTNET" -A9 /run/dhcp-server/dhcpd.conf

set service dhcp-server shared-network-name TESTNET subnet 203.0.113.0/24 bootfile-name "/pxelinux.0"
commit
grep "shared-network TESTNET" -A9 /run/dhcp-server/dhcpd.conf

set service dhcp-server shared-network-name TESTNET subnet 203.0.113.0/24 bootfile-name "boot\x64\wdsnbp.com"
commit
grep "shared-network TESTNET" -A9 /run/dhcp-server/dhcpd.conf

delete service dhcp-server shared-network-name TESTNET
commit

sudo mv /tmp/bootfile-name-node.def.bak /opt/vyatta/share/vyatta-cfg/templates/service/dhcp-server/shared-network-name/node.tag/subnet/node.tag/bootfile-name/node.def
sudo systemctl restart vyos-configd

exit

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@vyosbot vyosbot requested review from a team, dmbaturin, sarthurdev, zdc, jestabro, sever-sever and c-po and removed request for a team July 28, 2023 18:16
@@ -129,7 +129,7 @@
<properties>
<help>Bootstrap file name</help>
<constraint>
<regex>[-_a-zA-Z0-9./]+</regex>
<regex>[-_a-zA-Z0-9.:/~#@!$&*+,;=~%\\\\]+</regex>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use something like: <regex>[[:ascii:]]{0,256}</regex> to allow all ASCII characters? Makes it easier to read.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as VyOS supports that then this <regex>[[:ascii:]]{0,256}</regex> a better option since the source code of ISC DHCP defines that option as type "t" meaning just that:

https://github.com/isc-projects/dhcp/blob/master/common/tables.c#L165

{ "bootfile-name", "t",			&dhcp_universe,  67, 1 },
/* DHCP Option names, formats and codes, from RFC1533.
   Format codes:
...
   t - ASCII text

RFC1533 has been superseeded by RFC2132 according to IANA:

https://www.iana.org/assignments/bootp-dhcp-parameters/bootp-dhcp-parameters.xhtml

https://www.rfc-editor.org/rfc/rfc2132.html

And in RFC2132:

9.5 Bootfile name

   This option is used to identify a bootfile when the 'file' field in
   the DHCP header has been used for DHCP options.

   The code for this option is 67, and its minimum length is 1.

       Code  Len   Bootfile name
      +-----+-----+-----+-----+-----+---
      | 67  |  n  |  c1 |  c2 |  c3 | ...
      +-----+-----+-----+-----+-----+---

Since the above says that length must be at least 1 shouldnt the regex in question be this instead?

<regex>[[:ascii:]]{1,256}</regex>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use 1 as minimum length

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks -- I agree that [[:ascii:]] would make the pattern easier to read / maintain. The original proposal was meant to align with the established conventions for this and other value constraints. If we're OK with using the POSIX expression here, I agree that's preferable.

In that case, should we consider adjusting the byte limits to a minimum of 1 as previously mentioned, and a maximum of 253, to account for the maximum option length of 255 bytes, minus two for the option tag and length bytes?

This would result in the final pattern of [[:ascii:]]{1,253}. If this looks good, I'll go ahead and retest / resubmit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do so

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'[[:ascii:]]{1,253}' sounds good to me too.

As a spinoff there might be needed to add an additional task of verifying all other options available through the dhcp config in VyOS aswell.

That is '[[:ascii:]]{1,253}' should be used for all variables (dhcp options) defined as type "t" in https://github.com/isc-projects/dhcp/blob/master/common/tables.c#L43-L301

@c-po
Copy link
Member

c-po commented Aug 2, 2023

Fixed in 24528fd

@c-po c-po closed this Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants