search

keepsimple1 / mdns-sd

Rust library for mDNS based Service Discovery
Apache License 2.0
89 stars 38 forks source link

Trouble with query_missing_srv() #156

Closed lyager closed 6 months ago

lyager commented 7 months ago

I've experienced some trouble in the latest release with the above function, and just wondering if additional querying of SRV entries are within spec?

What happens is that the SRV query within the function above is very aggressive, and if a SRV never comes it will spam the network.

There should probably be a stopping mechanism if no answer hits the cache?

keepsimple1 commented 7 months ago

I've experienced some trouble in the latest release with the above function, and just wondering if additional querying of SRV entries are within spec?

May I ask what kind of trouble? (never mind, I think you mentioned that in the next paragraph.) More details will be helpful for fixing this issue.

The spec says a service should send SRV records along with PTR records. But in reality some service don't do that. Hence this function was added. I cannot find spec forbidding such queries. Let me know if I missed some part of the spec.

What happens is that the SRV query within the function above is very aggressive, and if a SRV never comes it will spam the network.

One thing I'd like to do is to limit such query only for the domains being queried, something like the refreshing logic:

https://github.com/keepsimple1/mdns-sd/blob/95e411b5d922c332bd78e7fd34f1c70993c562fb/src/service_daemon.rs#L504

This will avoid querying random services on the network and reduce potential security issues. We can also adjust the wait time before querying.

There should probably be a stopping mechanism if no answer hits the cache?

We can limit the number of such queries for a given service, say up to twice.

keepsimple1 commented 7 months ago

I've opened a PR to solve this issue. If possible, please let me know if the patch would work for you. Thanks!

lyager commented 7 months ago

Sorry for the rather short reporting. I think querying for SRV is fine. But your observation is correct, it might not be fruitful to do so when it's not within the TY that we're browsing for.

Also, what if the device that fails to initially send a SRV response never does?

I wont be able to properly test the PR this year, due to being out of office, but I'm pretty sure that the PR is a good shot, and what is expected of the package.

keepsimple1 commented 6 months ago

Sorry for the rather short reporting. I think querying for SRV is fine. But your observation is correct, it might not be fruitful to do so when it's not within the TY that we're browsing for.

no problem.

Also, what if the device that fails to initially send a SRV response never does?

With the PR, we will stop querying for SRV after two attempts, and the service instance will not be resolved.

I wont be able to properly test the PR this year, due to being out of office, but I'm pretty sure that the PR is a good shot, and what is expected of the package.

Thanks for reviewing the PR. No problem, I will also try to add a test case.

keepsimple1 commented 6 months ago

refactored the patch and did manual tests. Merged the patch. Take it for a spin when you have time in new year ;-)