-
Notifications
You must be signed in to change notification settings - Fork 759
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
add WPAD to dhcp server #2088
add WPAD to dhcp server #2088
Conversation
src/www/services_dhcp.php
Outdated
| <td><i class="fa fa-info-circle text-muted"></i> <?=gettext("WPAD");?> </td> | ||
| <td> | ||
| <input name="wpad" id="wpad" type="checkbox" value="yes" <?=!empty($pconfig['wpad']) ? "checked=\"checked\"" : ""; ?> /> | ||
| <strong><?= sprintf(gettext("Enable Web Proxy Auto Discovery on the %s interface"),!empty($config['interfaces'][$if]['descr']) ? htmlspecialchars($config['interfaces'][$if]['descr']) : strtoupper($if));?></strong> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Enable Web Proxy Auto Discovery on this interface", the rest is too confusing (and there is a double space in front of %s anyway)
|
|
||
|
|
||
| // add pac url if it applies | ||
| if (isset($dhcpifconf['wpad']) && !empty($config['system']['hostname']) && !empty($config['system']['domain'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we set host names / domains per interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theoretically yes but is there a reason to? It must point to an IP of the Firewall and the system hostname makes that always true. On that interface the wpad.dat file must be served.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true
| if (isset($dhcpifconf['wpad']) && !empty($config['system']['hostname']) && !empty($config['system']['domain'])) { | ||
| $protocol = !empty($config['system']['webgui']['protocol']) ? $config['system']['webgui']['protocol'] : 'https'; | ||
| // take hostname from system settings - it can be used to be resolved to anything based on client IP | ||
| $host = implode('.', array('wpad', $config['system']['domain'])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hostname is checked in first if, but not added here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hosts try the hostname wpad in the domain. which is created here. This one needs DNS support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but why the sanity check of !empty($config['system']['hostname']) ? it should never be empty and if it is WPAD not working is the least of our issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hostname is used for the CNAME in Unbound:
https://github.com/opnsense/core/pull/2097/files#diff-a89985242e1eea6a91d3e103e3353d5cR586
src/etc/inc/services.inc
Outdated
| $protocol = !empty($config['system']['webgui']['protocol']) ? $config['system']['webgui']['protocol'] : 'https'; | ||
| // take hostname from system settings - it can be used to be resolved to anything based on client IP | ||
| $host = implode('.', array('wpad', $config['system']['domain'])); | ||
| $port = !empty($config['system']['webgui']['port']) ? $config['system']['webgui']['port'] : 443; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
443 is only true for https :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
Merged, thanks! |
It deploys the default hostname: