Skip to content
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

Fixing "Error handling difficult and not reliable #38" #39

Merged
merged 12 commits into from
Nov 23, 2019
Merged

Fixing "Error handling difficult and not reliable #38" #39

merged 12 commits into from
Nov 23, 2019

Conversation

noneymous
Copy link
Contributor

@noneymous noneymous commented Nov 20, 2019

Here comes the implementation of my suggestion to fix #38. Please have a look and see how this works for you. I tested it against some scan targets and am still running it against even more. You are probably using different Nmap arguments than me, so I'm curious about your experiences.

Result processing would be like:

  1. Check if error is returned (includes package errors and nmap errors). If error is returned, something went most probably wrong and you can abort. Warning's might give you more details/flexibility. Most returned errors are defined as error constants in errors.go. Have a look there to see what issues might come back to handle them individually
  2. OptionA: Check warnings for things that might be an issue for you and abort or proceed
    OptionB: Ignore warnings at all and just work with the returned XML. It already contains a lot of details

Here is sample usage code:

// Prepare Nmap scan
proc, err := nmap.NewScanner(options...)
if err != nil {
    fmt.Println("Could not initialize Nmap")
    return
}

// Execute Nmap scan
result, warnings, err := proc.Run()

// Check for nmap errors and optionally handle them individually
if err != nil {
	switch err {
		case nmap.ErrScanTimeout:
        	fmt.Println("ERROR: Nmap scan ran into timeout")
    	case nmap.ErrParseOutput:
        	fmt.Printf("ERROR: Nmap output could not be parsed: %w\n", warnings[len(warnings)-1])
    	case nmap.ErrExcludeList:
        	fmt.Println("ERROR: Nmap could not resolve host(s) on exclude list")
    	default:
        	fmt.Printf("ERROR: Nmap scan failed with unexpected error: %w\n", err)
	}
	return
}

// Check for nmap warnings that might be relevant for you
for _, warning := range warnings {
    switch {
 		case strings.Contains(warning, "Failed to resolve"):
        	fmt.Println("A target could not be resolved.") 
    	case strings.Contains(warning, "No targets were specified")
			fmt.Println("The only target is blacklisted.")
    	case strings.Contains(warning, "TTVAR has grown")
        	fmt.Println("Just some TTVAR change notification.")
    	default:
        	fmt.Println(fmt.Sprintf("Other warning: %s", warning))
    }
}

// Process result
fmt.Println(result)

- Run() does now return a result, optional warnings and an error if something critical went wrong. The error might come from the package or Nmap
- Issues from Nmap's stderr are all treated as warnings. If something critical went wrong, it can be found in the Nmap result XML (and will also be read from there raising an error)
- All currently experienced Nmap errors are added as error constants in errors.go. People can use these error constants to handle individual errors individually. This should be extended as more new errors are found
- Removed 'NmapErrors' from xml.go again, as this was only required with the previous Nmap result error fix
moved to utils.go
examples were missed
In a previous version duplicate messages from Nmap stderr got removed. As we understand these messages as warnings now, I think it's not good to alter/clean these messages anymore, in order to give full control to the developer. The developer still could do a "unique" on the returned list of messages on his own. In other cases he might want to know how often a certain warning happened, etc...
Copy link
Owner

@Ullaakut Ullaakut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for this contribution @noneymous! This is great.

Looks solid so far! I have a suggestion that impacts a few little things, and also it will require unit tests before it can be merged. I didn't test it yet but I will as soon as I have some time 🙏

nmap.go Outdated Show resolved Hide resolved
nmap.go Outdated Show resolved Hide resolved
nmap.go Outdated Show resolved Hide resolved
nmap_test.go Outdated Show resolved Hide resolved
@Ullaakut Ullaakut requested a review from TerminalFi November 20, 2019 16:15
@Ullaakut Ullaakut added the enhancement New feature or request label Nov 20, 2019
@noneymous
Copy link
Contributor Author

Good idea, I introduced named return values and it's already commited.

Just the warning unit test outstanding. Maybe tomorrow.

@noneymous
Copy link
Contributor Author

Sorry, I'm in the wrong environment to get the tests running. I feel this would take me too much time but might be a simple thing for you...

@TerminalFi
Copy link
Collaborator

Awaiting Unit Tests

Copy link
Collaborator

@TerminalFi TerminalFi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks promising and well laid out. I agree, we need unit tests as this is a pretty big change across many files.

@Ullaakut
Copy link
Owner

I'll test it this weekend :) It's on my calendar now:

Screenshot 2019-11-22 at 10 20 42 AM

Copy link
Owner

@Ullaakut Ullaakut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@noneymous
Copy link
Contributor Author

Thank you guys

@noneymous
Copy link
Contributor Author

What's the plan, when do you intend to merge?

@TerminalFi
Copy link
Collaborator

@noneymous we will merge once tests have been completed. We thank you for your patience. Both myself and @Ullaakut have busy schedules at times.

@Ullaakut
Copy link
Owner

I'll wait for @TheSecEng to test it as well and once it's fine for him, we can merge the PR :)

@TerminalFi
Copy link
Collaborator

Cleared.

@TerminalFi TerminalFi merged commit 36f7fbb into Ullaakut:master Nov 23, 2019
@Ullaakut Ullaakut mentioned this pull request Nov 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error handling difficult and not reliable
3 participants