plugin/bind: add zone for link-local IPv6 instead of skipping #6547
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6547 +/- ##
==========================================
+ Coverage 55.70% 59.24% +3.54%
==========================================
Files 224 252 +28
Lines 10016 13477 +3461
==========================================
+ Hits 5579 7984 +2405
- Misses 3978 4901 +923
- Partials 459 592 +133 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Till Riedel <riedel@teco.edu>
|
Should have fixed the gofmt now. No clue how to put the relevant part into the test coverage. Would require the CI to bring up an interface with a link-local address. I tested just tested on windows 11 and ubuntu 22.4 now. |
riedel
changed the title
plugin/bind/setup.go
Outdated
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if !isIface { | ||
| if net.ParseIP(a) == nil { | ||
| _, err := net.ResolveIPAddr("ip", a) |
There was a problem hiding this comment.
From https://pkg.go.dev/net#ResolveIPAddr ...
ResolveIPAddr returns an address of IP end point.
The network must be an IP network name.
If the host in the address parameter is not a literal IP address, ResolveIPAddr resolves the address to an address of IP end point. Otherwise, it parses the address as a literal IP address. The address parameter can use a host name, but this is not recommended, because it will return at most one of the host name's IP addresses.
Am I reading the above go doc correctly that net.ResolveIPAddr will resolve hostnames to IPs (i.e. via system DNS resolver)? If so, it could have weird consequences if for example a user typo's an interface name. The interface name would not be a known interface, so it would fall through to here and potentially get resolved to a foreign ip address instead of returning an error.
There was a problem hiding this comment.
Thanks for catching this.
Hadn't thought of this.
My suggestion: I revert that second part, because if an interface is given this can be guaranteed.
I am using this function only because it maintains the "zone" (aka scope). And then the problem can be split off into another PR, as this is actually independent of the first part.
However, I wonder what the adverse effects could be (Attack scenario: provide a hostname that mimics an interface but provide a different IP), as the bind should fail and this is only a function for error handling. The only alternative I think would be to reimplement the scope parsing completely?!
ideas:
- I could limit the impact if I only resort to ResolveIPAddr if ParseIP fails.
- I could stringify the address again and do an equality check to make sure no resolving happens.
- Find a better go function? Any ideas?
There was a problem hiding this comment.
Or declare it a feature that you now can also bind "localhost" ;) and "fix" the documentation (actually checked: it works with the downstream bind).
Signed-off-by: Till Riedel <riedel@teco.edu>
|
I reverted the unrelated error handling and will create a new PR for that |
|
@chrisohaver: any more doubts after I removed the ResolveIPAddr from the error handler? Should now just behave like before except for binding all addresses of an interface (not skipping link local) |
This rewrites #4531 to actually not skip link-local IPs. See #6536.