Update http_proxy validation to support IPs too #9
Conversation
| if is_ipv4_address($http_proxy) or is_domain_name($http_proxy) { | ||
| $proxy_is_valid = true | ||
| } elsif is_ipv6_address($http_proxy) { # IPv6 needs to be enclosed in [] | ||
| $http_proxy = enclose_ipv6($http_proxy) |
There was a problem hiding this comment.
puppet does not allow you to reassign a variable. You will need to use another variable like $_http_proxy and then use that below.
| if $http_proxy_port { validate_re($http_proxy_port, '^\d+$') } | ||
| # Validates that $http_proxy is a valid, usable domain name or IP address | ||
| if $http_proxy and size($http_proxy) > 3 { # '.' is an RFC-valid domain name, but not usable in this context | ||
| if is_ipv4_address($http_proxy) or is_domain_name($http_proxy) { |
There was a problem hiding this comment.
These functions were not introduced untill stdlib version 4.12.0.
You will need to require a minimum of that version here: https://github.com/MiamiOH/puppet-httpproxy/blob/master/metadata.json#L12
| # Invalid proxy specified - could also 'undef' the value an continue?? | ||
| $proxy_is_valid = 'No, the proxy is not valid' | ||
| } | ||
| validate_bool($proxy_is_valid) |
There was a problem hiding this comment.
We are not really validating a boolean here. I feel like this is kind of a bad way to do this.
I think a better approach would be to just use the fail function in the else with a nice message about why the input is not valid (Its not a valid domain or ip.)
Something like this: https://github.com/MiamiOH/puppet-pam_access/blob/master/manifests/params.pp#L18
There was a problem hiding this comment.
I was looking for the function to do that! Thank you. I'll get rid of that ugly hack.
| if is_ipv4_address($http_proxy) or is_domain_name($http_proxy) { | ||
| $_http_proxy = $http_proxy | ||
| } elsif is_ipv6_address($http_proxy) { # IPv6 needs to be enclosed in [] | ||
| $_http_proxy = enclose_ipv6($http_proxy) |
There was a problem hiding this comment.
A couple of other files reference this var too:
I almost missed these...
puppet-httpproxy/manifests/package/rpm.pp
Line 13 in d785d5d
There was a problem hiding this comment.
I noticed those, but both looked like the underlying tooling handled host & port independently. For rpm, this is the case. I'll fix it up for APT, since the underlying puppet module concatenates the host and port into a string in the erb template.
There was a problem hiding this comment.
fixed APT handling in 2b8ae10
And THANK YOU for taking the time to review and help me improve the PR!!
There was a problem hiding this comment.
I'm not sure I understand your reasoning for why the rpm class does not need the same thing?
If you don't change it in the rpm class then your ipv6 fix will not be applied to the rpm config file...
Can you help me understand why you think that one is not necessary?
There was a problem hiding this comment.
In the rpm case, rpm.pp creates a file which contains a line for %_httpport and a line for %_httpproxy, and RPM/YUM builds the full proxy RUL on the fly, handling enclosing the IPv6 address in brackets if needed.
For apt configurations, the parameters are passed into the '::apt' class's proxy hash (package/apt.pp#L9) . In the apt module, they're used to build a single string which requires that the IPv6 address be wrapped in brackets (init.pp#L76 and proxy.erb#L1). In all likelihood, it's probably more accurate to say that the apt module has a bug - it does not properly handle IPv6 addresses when using them in the template; this would qualify as a work-around for that bug.
Despite the appearance to the contrary, the puppetlabs/apt module uses the 'host' and 'port' to form a proxy URL string. Because of this, we need to use the _http_proxy variable, in case the value holds an IPv6 address which needs to be proprely enclosed. Signed-off-by: Mr. Secure <ben.github@mrsecure.org>
|
Created upstream PR for IPv6 handling - puppetlabs/puppetlabs-apt#645 |
|
Fixed in #10 |
Update validation of http_proxy & http_proxy_port to be a bit more strict, as well as to allow IPv4 and IPv6 address as the http_proxy.