Skip to content

(PUP-10238) Change default value of strict_hostname_checking to true #7982

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

Merged
merged 1 commit into from
Feb 12, 2020

Conversation

justinstoller
Copy link
Member

Previously our default value of strict_hostname_checking was false which
allowed matching dotted segments of a nodes certname (its CN in its
certificate) as well as the segments of its fqdn fact, or hostname +
domain fact.

This was for compatibility when fact based classification within a
site.pp was a more common pattern and node declarations were much less
powerful than they are now.

With the ability to use regular expressions in a node declaration the
auto segmenting is no longer needed and with the ability to use facts
directly, to use fact interpetation in hiera lookups, or create a custom
external node classifier the injecting of facts into the nodes "name" is
unneeded.

The desire is to remove the setting completely in Puppet 7, while
leaving it in 6 so those that depend on this behavior have time to
re-write their site.pps to the newer styles.

strict_hostname_checking setting is not marked deprecated completely
because it will cause deprecation notices on setting access, which
happens as part of normal compilation for now. However it does mark
"node_name" setting as deprecated completely because it is now only
referenced in code that by default will not run (and will only run if
users change strict_hostname_checking back to false).

@justinstoller justinstoller requested review from a team February 11, 2020 22:52
Puppet[:node_name] = "cert"
@node = Puppet::Node.new("foo.domain.com",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed now? Looks like the same as the @node above?

Puppet[:strict_hostname_checking] = true
@node = Puppet::Node.new("foo.domain.com",
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I re-wrote the setup and teardown of these specs in an intermediate step and forgot they weren't needed when I cleaned them up.

Copy link
Member Author

Choose a reason for hiding this comment

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

pushed an update addressing your comments

Copy link
Contributor

@mwaggett mwaggett left a comment

Choose a reason for hiding this comment

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

@joshcooper
Copy link
Contributor

I reverted the whitespace change in 5.5.x, so now there's a conflict. @justinstoller can you resolve?

Previously our default value of strict_hostname_checking was false which
allowed matching dotted segments of a nodes certname (its CN in its
certificate) as well as the segments of its fqdn fact, or hostname +
domain fact.

This was for compatibility when fact based classification within a
site.pp was a more common pattern and node declarations were much less
powerful than they are now.

With the ability to use regular expressions in a node declaration the
auto segmenting is no longer needed and with the ability to use facts
directly, to use fact interpetation in hiera lookups, or create a custom
external node classifier the injecting of facts into the nodes "name" is
unneeded.

The desire is to remove the setting completely in Puppet 7, while
leaving it in 6 so those that depend on this behavior have time to
re-write their site.pps to the newer styles.

strict_hostname_checking setting is not marked deprecated completely
because it will cause deprecation notices on setting access, which
happens as part of normal compilation for now. However it does mark
"node_name" setting as deprecated completely because it is now only
referenced in code that by default will not run (and will only run if
users change strict_hostname_checking back to false).
@justinstoller
Copy link
Member Author

Wow, I sure screwed up that rebase. I think it's sorted now.

Copy link
Contributor

@joshcooper joshcooper left a comment

Choose a reason for hiding this comment

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

LGTM

@joshcooper joshcooper merged commit c08b9fd into puppetlabs:5.5.x Feb 12, 2020
@joshcooper
Copy link
Contributor

I'll merge this up to 6.4.x before tonight's run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants