ZAP parser: do not resolve hostname to IP address during endpoint creation (#2284)

[AlexanderTyutin]

#2284

ZAP Parser:

• disabled fqdn to ip resolving;
• changed main endpoint visualization from IPport to scheme://fqdn:port

[valentijnscholten]

I think it's a good improvement, but I am wondering if we should make it optional to use the old behaviour? Someone has experience with ZAP scans/imports? Changing endpoint value may also impact dedupe.

[AlexanderTyutin]

@valentijnscholten Any way except creating new scanner entry like "ZAP Scan with Hostnames"?
We can think about adding a parameter "use_hostnames" with default value "false" if not set, for example. But it will take more efforts I think.
Also really this PR doesn't change the behavior if there was an IP address used as host address.

So, it your target was https://10.0.0.1:8443 you will have exactly the same endpoint. I just removed enforcing of converting from hostname to ip address. I.e. the endpoint is just a real from the report (now endpoint is changing from original by importing tool).

I think this option (to resolve hostname) was useful for the company who wrote this import in 2013 (see the copyright note). But it is obsoleted now.

[AlexanderTyutin]

Just look: with this fix we have clear value of the endpoint while without it we have something like XX.XXX.XXX.XXXYYYY (X - IP address, Y - port number)

And today having XX.XXX.XXX.XXXXYYYY is not so useful as 7 years ago because one ip is used for many sites (thanks virtual hosts parameter :) )

[valentijnscholten]

I think it would have to be a setting in settings.py. But you may be right that it is not needed. I have no experience with endpoints at all, so I'll have to leave it to others to decide.

[AlexanderTyutin]

A new challenge to learn DefectDojo closer :-D 👍 Really I forgot about settings.py

[mtesauro]

I've only used endpoints a bit - typically they're not as important as the product name + test environment especially for AppSec work.

That said, I think it's a good idea to default to this behaviour (not resolving hostnames to IPs) and to put an boolean in settings.py for those that really want the previous style of behaviour. Heck, you can control how this works by using the IP or hostname in Zap so it's upstream of DD's import.

TBH, when I've needed/wanted to change the behaviour of a scanner's import, I've just written either a pre or post importing tools that either changes the file before importing or updated findings using the API - so those options will still exist after this PR is merged for those that have special reasons to save scanner data stored in a certain way.

[madchap]

Based on @mtesauro's feedback, a small flag in settings.dist.py to dictate the behavior as well as the needed logic in code would probably be best and suit everyone.

[AlexanderTyutin]

Thanks, @mtesauro @madchap. Will try :)

[valentijnscholten]

@mtesauro @devGregA I suggest we go with this PR and remove the DNS resolution stuff as it seems out of place anyway. No other scanner importer does this. So no settings flag for the old behaviour, just remove it. WDYT?

[Maffooch]

Agreed that removing this the host resolving is a good move. Since the user can choose either the host or IP at the ZAP level then there is no need to force it here.

[valentijnscholten]

2 approvals