Stdlib::Fqdn type accepts ipv4 addresses, also allows truncated addresses #1282

canihavethisone · 2022-11-13T10:45:19Z

Describe the Bug

Specifying a variable as type Stdlib::Fqdn allows ipv4 addresses. This issue is compounded when using Stdlib::Host as with the inclusion of Fqdn it allows truncated ipv4 addresses such as 10.10.10.10.10.

Expected Behavior

IPv4 addresses should be validated when using Stdlib::Host and not accepted within the Stdlib:Fqdn type

Steps to Reproduce

Steps to reproduce the behavior:

Create a variable with type Stdlib::Fqdn
Submit valid or truncated ipv4 address, which passes (the regex)
Create a variable with type Stdlib::Host
Submit a truncated ipv4 address, which passes (the regex)

Environment

stdlib v8.4.0
Puppet 6x

Additional Context

Unsure if this is a flaw in catering for numerical fqdn's, and how it can be addressed. TL:DR is that using Stdlib::Host does not ensure ipv4 addresses are valid

The text was updated successfully, but these errors were encountered:

ekohl · 2022-11-13T13:03:07Z

Unsure if this is a flaw in catering for numerical fqdn's, and how it can be addressed. TL:DR is that using Stdlib::Host does not ensure ipv4 addresses are valid

My bet would indeed be on the numberical FQDNs because Stdlib::Host has the problem already as shown in #1283.

canihavethisone · 2022-11-13T22:14:11Z

Interesting problem to solve. I'm wondering if a possible solution should include a test for invalid ipv4, such as 10.10.10.300.

ekohl · 2022-11-15T11:32:38Z

I'd suspect that the host definition should check that the last part (TLD) should be non-numeric. That should catch it.

@ekohl

The 3.x function rely on is_domain_name() which is deprecated. Rewrite it using the more modern puppet 4.x function to rely on data types for better parameters validation. While here, adjust the Stdlib::Fqdn data type to ensure the last component (TLD) is non-numeric as [suggested by @ekohl](#1282 (comment)), and add a Stdlib::Dns_zone data type that is basically the same but with a trailing dot.

@ekohl

The 3.x function rely on is_domain_name() which is deprecated. Rewrite it using the more modern puppet 4.x function to rely on data types for better parameters validation. While here, adjust the Stdlib::Fqdn data type to ensure the last component (TLD) is non-numeric as [suggested by @ekohl](#1282 (comment)), and add a Stdlib::Dns_zone data type that is basically the same but with a trailing dot.

b4ldr · 2023-04-28T11:58:30Z

it allows truncated ipv4 addresses such as 10.10.10.10.10

FTR this is a valid FQDN

a host domain name is now allowed to begin with a digit and could legally be entirely numeric

re: rfc1123 Section 2

I'd suspect that the host definition should check that the last part (TLD) should be non-numeric. That should catch it.

There are two issues with this

A fqdn can contain just one label i.e. com, net, google are all valid FQDN's as such this type can and will be used to validated hostnames (FQDN's with a single label) which could contain a numbers e.g. webserver1001
This only assumes the IANA DNS it doesn't account for:
- private TLDs e.g. 7up may have there own private DNS with a 7up. tld
- Pseudo TLD's e.g. i2p

Arguably the IANA DNS is the one most users would want to check however i think we should add a new type for this e.g. Stdlib::DNS::IANA

ekohl · 2023-04-28T12:04:12Z

re: rfc1123 Section 2

This part looks relevant:

Whenever a user inputs the identity of an Internet host, it SHOULD
be possible to enter either (1) a host domain name or (2) an IP
address in dotted-decimal ("#.#.#.#") form. The host SHOULD check
the string syntactically for a dotted-decimal number before
looking it up in the Domain Name System.

That reads to me as an XOR: either a domain name or an IP. It should prefer it as an IP. So I think a fully numeric FQDN is invalid.

You can catch that with [a-z0-9]*[a-z]+[a-z0-9]* for the last domain component.

b4ldr · 2023-04-28T12:09:20Z

That reads to me as an XOR: either a domain name or an IP. It should prefer it as an IP.

I think that's valid if it matches (loose) pattern \d{3}\.\d{3}\.\d{3}\.\d{3} then it should be treated as an ip address however

So I think a fully numeric FQDN is invalid.

100
100.100
100.100.100
100.100.100.100.100

should all be treated as DNS names

smortex · 2023-04-28T19:36:07Z

Thank you for these insightful information and links.

The rfc1123 Section 2 quoted above make me think we can accept an IP address as a FQDN: in an ideal world we would not, but this would need to happen in puppet with a custom data type that tries to parse the value as an IP address, raise an error if it succeed and does the actual test otherwise. I do not think we can do this at the stdlib level. So accepting IP addresses here looks like an acceptable compromise.

I will update #1345 to take this into account.

Thank you 💯 💯 💯

@ekohl

The 3.x function rely on is_domain_name() which is deprecated. Rewrite it using the more modern puppet 4.x function to rely on data types for better parameters validation. While here, adjust the Stdlib::Fqdn data type to ensure the last component (TLD) is non-numeric as [suggested by @ekohl](#1282 (comment)), and add a Stdlib::Dns_zone data type that is basically the same but with a trailing dot.

This PR introduces a new set of types for validating FQDN's. it creates a type for the more loose rfc definitions of domain names and a stricter and likely more useful iana type. This allows us to create a new type that more does to what most users expect i.e. that a dns name is one that works on the internt, without breaking current uses for users that may be using the Stdlib::Fqdn to validate validate DNS names that don't work with the IANA roots. The intention of this patch would be to deprecate the currnet Stdlib::Fqdn type and encourage users to move to the appropriate Stdlib::DNS::* type which for most users will likely be the stricter Stdlib::DNS::Fqdn type Note: this PR is intentionally a bit rough to first garner thoughts as to if this is the correct direction Fixes puppetlabs#1282 (not sure it fixes but want it tagged)

b4ldr · 2023-04-28T21:16:59Z

FTR i don't think that that the current type is what most people would want or expect but don't want to break things for current users. i created #1347 as a possible way forward let me know what you think and ill clean up the patch if its seems like a good direction

@ekohl

The 3.x function rely on is_domain_name() which is deprecated. Rewrite it using the more modern puppet 4.x function to rely on data types for better parameters validation. While here, adjust the Stdlib::Fqdn data type to ensure the last component (TLD) is non-numeric as [suggested by @ekohl](#1282 (comment)), and add a Stdlib::Dns_zone data type that is basically the same but with a trailing dot.

@ekohl

The 3.x function rely on is_domain_name() which is deprecated. Rewrite it using the more modern puppet 4.x function to rely on data types for better parameters validation. While here, adjust the Stdlib::Fqdn data type to ensure the last component (TLD) is non-numeric as [suggested by @ekohl](#1282 (comment)), and add a Stdlib::Dns_zone data type that is basically the same but with a trailing dot.

This PR introduces a new set of types for validating FQDN's. it creates a type for the more loose rfc definitions of domain names and a stricter and likely more useful iana type. This allows us to create a new type that more does to what most users expect i.e. that a dns name is one that works on the internt, without breaking current uses for users that may be using the Stdlib::Fqdn to validate validate DNS names that don't work with the IANA roots. The intention of this patch would be to deprecate the currnet Stdlib::Fqdn type and encourage users to move to the appropriate Stdlib::DNS::* type which for most users will likely be the stricter Stdlib::DNS::Fqdn type Note: this PR is intentionally a bit rough to first garner thoughts as to if this is the correct direction Fixes puppetlabs#1282 (not sure it fixes but want it tagged)

ia-content added the community label Nov 13, 2022

canihavethisone changed the title ~~Stdlib::Fqdn type accepts ipv4 addresses~~ Stdlib::Fqdn type accepts ipv4 addresses, also allows truncated addresses Nov 13, 2022

ekohl mentioned this issue Nov 13, 2022

Add test with fully numerical hostname #1283

Closed

smortex mentioned this issue Apr 28, 2023

Rewrite validate_domain_name() as a Puppet 4.x function #1345

Merged

smortex linked a pull request Apr 28, 2023 that will close this issue

Rewrite validate_domain_name() as a Puppet 4.x function #1345

Merged

b4ldr mentioned this issue Apr 28, 2023

types - dns: add a new dns type with more specific fqdn definitions #1347

Open

binford2k closed this as completed in #1345 May 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stdlib::Fqdn type accepts ipv4 addresses, also allows truncated addresses #1282

Stdlib::Fqdn type accepts ipv4 addresses, also allows truncated addresses #1282

canihavethisone commented Nov 13, 2022 •

edited

Loading

ekohl commented Nov 13, 2022

canihavethisone commented Nov 13, 2022

ekohl commented Nov 15, 2022

b4ldr commented Apr 28, 2023 •

edited

Loading

ekohl commented Apr 28, 2023

b4ldr commented Apr 28, 2023 •

edited

Loading

smortex commented Apr 28, 2023

b4ldr commented Apr 28, 2023

Stdlib::Fqdn type accepts ipv4 addresses, also allows truncated addresses #1282

Stdlib::Fqdn type accepts ipv4 addresses, also allows truncated addresses #1282

Comments

canihavethisone commented Nov 13, 2022 • edited Loading

Describe the Bug

Expected Behavior

Steps to Reproduce

Environment

Additional Context

ekohl commented Nov 13, 2022

canihavethisone commented Nov 13, 2022

ekohl commented Nov 15, 2022

b4ldr commented Apr 28, 2023 • edited Loading

ekohl commented Apr 28, 2023

b4ldr commented Apr 28, 2023 • edited Loading

smortex commented Apr 28, 2023

b4ldr commented Apr 28, 2023

canihavethisone commented Nov 13, 2022 •

edited

Loading

b4ldr commented Apr 28, 2023 •

edited

Loading

b4ldr commented Apr 28, 2023 •

edited

Loading