ban DNS lookups with forbidden apis #15969
Conversation
followup to elastic#15851 In general, we should avoid InetAddress.getByName under most circumstances: * If you really want to do a lookup, why not getAllByName (why an arbitrary address) * if you really just want to parse an address, we have a separate method for that in InetAddresses
| @@ -210,6 +211,7 @@ public InetAddress resolvePublishHostAddresses(String publishHosts[]) throws IOE | |||
| } | |||
|
|
|||
| /** resolves a single host specification */ | |||
| @SuppressForbidden(reason = "does DNS lookup when bind/publish host is specified as hostname") | |||
There was a problem hiding this comment.
Maybe extract the part the must use the forbidden method into a little method so you don't have to annotate the whole thing? It'd be nice to be able to be sure that we're not doing dns lookups when we try to get the loopback address, for instance.
There was a problem hiding this comment.
i don't think we should make code more complex for this reason. the method's name is resolveInternal and i think its clear that it does DNS resolution.
|
I'm closing this, its not worth it to me if it results in more complex code. I just want to prevent mistakes. |
|
I opened this to try to improve the situation: policeman-tools/forbidden-apis#96 I definitely think we should not restructure our real code in unnatural ways (e.g. factoring out strange methods) to try to satisfy a static analysis tool. The code should be as simple as possible and structured in the way that makes it easiest to understand. I've seen this a few times lately and we gotta do something about it, we already have too many things tugging at the quality of the code, we don't need forbidden apis added to that too. |
| // only one address: because we explicitly ask for only one via the GceHostnameType | ||
| return new InetAddress[] { InetAddress.getByName(metadataResult) }; | ||
| // really should be only one address: because we explicitly ask for only one via the Ec2HostnameType | ||
| // but why do we even allow configuring this by hostname... |
There was a problem hiding this comment.
I totally agree with you. Actually when I wrote the code, I did the same thing which was done for EC2 plugin.
I think that we can totally remove that and only support IP address.
It can be done in a follow up issue though. I'd add a TODO here or open an issue.
|
Even if you close it, I added a few comments. I think we should do it. |
|
I am just unhappy with the current solution. Honestly, I was on the fence to even make a pull request about it, because its a tricky thing to contain. So when nik didn't like it either, it just convinced me, you know with forbidden-apis as it is now, this isn't a good tradeoff to make. Maybe we can better fine-grain the annotation and revisit. But ive seen strange code recently that was factored in a way to reduce the forbidden-apis stuff, hell i know i've even suggested it before, but in general I really think its a bad tradeoff. |
followup to #15851
The idea here is that only a very few places really need to do DNS lookups, we should prevent it happening by accident. Abuse cases of InetAddress.getByName for other purposes (e.g. parsing addresses, getting the loopback address) can be done instead with other safer methods.
Especially parsing addresses, its better to use
InetAddresses.forString, which won't do a lookup but just throw an exception if the address is wrong.This cleans up the tests around this for the most part. There are some exceptions where we just
SuppressForbidden, such as tests for InetAddresses.forString itself, which intentionally do round-trip testing against InetAddress, etc.