-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
dnssd.c: Enable service registration on loopback only #346
base: master
Are you sure you want to change the base?
dnssd.c: Enable service registration on loopback only #346
Conversation
47f6e5d
to
ab81728
Compare
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.
I'm not comfortable separating the "registration hostname" from the "listen hostname".
I support changing things to correctly advertise for the loopback interface but not for separating the hostname configurations.
Hmm... I meant registration hostname to be exactly the listen hostname - and we have to save the 'listen hostname' somewhere to have a way how to decide which net interface to use for advertising, because With your feedback, I guess you mean I can save the listen hostname into system struct in
It currently requires changing machine's hostname to change the scope where the service will be advertised (which is IMHO undesired) - IMO we have to separate server hostname and listen hostname configurations (if you meant them) and have a way where to save listen hostname, or remove the server hostname functionality to change machine's hostname (keep the hostname only internally and do not change /etc/hostname). WDYT? |
8e03879
to
ccaa1de
Compare
I've removed public accessors and renamed reghost to listen_hostname - it is set only when I set the PR to [WIP], until it is clear how I should change it. |
@michaelrsweet ad saving listen-hostname - Or did you mean to save listen-hostname into |
e86401c
to
78c654e
Compare
Ignore the review request for now, I rewrite it to use |
a2224e9
to
f39ba2c
Compare
Ready for review. |
In case users would like to prevent sharing services from printer applications to local network, restrict it to localhost and let CUPS do the sharing. This can be done by setting system hostname in PAPPL API - this prevents accessing the public addresses, but the service is still published on those public addresses. The PR does the following: - set/reset system hostname in `papplSystemAddListeners()` directly, not via API. It prevents changing machine' hostname. - dnssd function will check the hostname and if it is localhost or matching IP address, it will use loopback index, - in case of Avahi it passes NULL as hostname to let Avahi decide what hostname to use (in case of hostname conflicts - and Avahi forbids using localhost if it is not FQDN). The result is that if system hostname is set to localhost internally, the service is published on .local address, but resolved to loopback because CUPS uses DNS-SD names in URIs. PAPPL 2.x version requires CUPS PR OpenPrinting/cups#902 to have it working.
a58282b
to
b1d529e
Compare
@michaelrsweet ping |
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.
Needs work.
@@ -51,12 +51,12 @@ _papplPrinterRegisterDNSSDNoLock( | |||
const char *papermax; // PaperMax string value (legacy) | |||
|
|||
|
|||
if (!printer->dns_sd_name || !printer->system->is_running || (printer->system->options & PAPPL_SOPTIONS_NO_DNS_SD)) | |||
if (!printer->dns_sd_name || !printer->system->is_running || (printer->system->options & PAPPL_SOPTIONS_NO_DNS_SD) || !system->hostname) |
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.
system->hostname
cannot be NULL
.
// | ||
|
||
bool | ||
_papplDNSSDIsLoopback(const char *name) |
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.
I'm still not clear why we need a function for this...
@@ -141,6 +141,13 @@ papplSystemAddListeners( | |||
if (ret) | |||
system->port = port; | |||
} | |||
|
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.
This isn't where I'd save the hostname... You can have multiple listeners.
return (false); | ||
|
||
papplLogPrinter(printer, PAPPL_LOGLEVEL_DEBUG, "Registering DNS-SD name '%s' on '%s'", printer->dns_sd_name, printer->system->hostname); | ||
|
||
if_index = !strcmp(system->hostname, "localhost") ? CUPS_DNSSD_IF_INDEX_LOCAL : CUPS_DNSSD_IF_INDEX_ANY; | ||
if_index = _papplDNSSDIsLoopback(system->hostname) ? CUPS_DNSSD_IF_INDEX_LOCAL : CUPS_DNSSD_IF_INDEX_ANY; |
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.
I'm thinking we should examine the listeners or keep a boolean to track whether we are listening on an externally accessible address.
@@ -677,7 +677,7 @@ papplSystemRun(pappl_system_t *system) // I - System | |||
bool force_dns_sd = system->dns_sd_host_changes != dns_sd_host_changes; | |||
// Force re-registration? | |||
|
|||
if (force_dns_sd) | |||
if (!_papplDNSSDIsLoopback(system->hostname) && force_dns_sd) |
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.
It isn't about being on the loopback interface since we still need to advertise on localhost.
In case users would like to prevent sharing services from printer
applications to local network, restrict it to localhost and let CUPS
do the sharing.
This can be done by setting listen-hostname in PAPPL API - this prevents accessing the public addresses, but the service is still published on those public addresses. This can be prevented if the machine hostname is changed to localhost, but that's not desired on machines IIUC.
The PR does the following:
The result is that if reghost is set to localhost, the service is published on .local address, but resolved to loopback because CUPS uses DNS-SD names in URIs.
PAPPL 2.x version requires CUPS PR OpenPrinting/cups#902 to have it working.