Add Nmap Metadata#1527
Conversation
WalkthroughAdds programmatic Nmap scanning via the Ullaakut nmap Go library, maps Nmap service and OS fingerprint data into Naabu's host and port results (including populating Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Runner
participant NmapLib
participant Results
participant Output
User->>Runner: Start scan (RunEnumeration)
Runner->>Results: Populate initial HostResults (discovered ports)
alt NmapCLI provided
Runner->>NmapLib: Run grouped Nmap scans (targets, ports, args)
NmapLib-->>Runner: Return nmap.Run (hosts, ports, services, OS)
Runner->>Results: integrateNmapResults (merge ports + service metadata)
Runner->>Results: UpdateHostOS (OS fingerprint per IP)
end
Runner->>Output: handleOutput → WriteJSONOutput / CSV / console using enriched data
Output-->>User: Final JSON/CSV/console results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)pkg/runner/runner.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Mzack9999
added
the
Type: Enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
pkg/runner/output.go (1)
231-254: CSV output is missing service metadata fieldsThe
WriteCsvOutputfunction doesn't populate the service metadata fields in theResultstruct, even though these fields have CSV tags. This creates an inconsistency where JSON output includes service metadata but CSV output doesn't.Add service field population similar to JSON output:
for _, p := range ports { data.Port = p.Port data.Protocol = p.Protocol.String() //nolint data.TLS = p.TLS + if p.Service != nil { + data.DeviceType = p.Service.DeviceType + data.ExtraInfo = p.Service.ExtraInfo + // ... copy other service fields + } writeCSVRow(data, encoder, excludedFields) }
🧹 Nitpick comments (8)
cmd/naabu/main.go (1)
44-60: Consider extracting service mapping to a helper methodThe manual field-by-field mapping from
runner.Resulttoport.Servicecould be refactored into a helper method to improve maintainability and reusability.+func resultToService(r *runner.Result) *port.Service { + return &port.Service{ + DeviceType: r.DeviceType, + ExtraInfo: r.ExtraInfo, + HighVersion: r.HighVersion, + Hostname: r.Hostname, + LowVersion: r.LowVersion, + Method: r.Method, + Name: r.Name, + OSType: r.OSType, + Product: r.Product, + Proto: r.Proto, + RPCNum: r.RPCNum, + ServiceFP: r.ServiceFP, + Tunnel: r.Tunnel, + Version: r.Version, + Confidence: r.Confidence, + } +} // In the loop: - service := &port.Service{ - DeviceType: r.DeviceType, - ExtraInfo: r.ExtraInfo, - HighVersion: r.HighVersion, - Hostname: r.Hostname, - LowVersion: r.LowVersion, - Method: r.Method, - Name: r.Name, - OSType: r.OSType, - Product: r.Product, - Proto: r.Proto, - RPCNum: r.RPCNum, - ServiceFP: r.ServiceFP, - Tunnel: r.Tunnel, - Version: r.Version, - Confidence: r.Confidence, - } + service := resultToService(&r)pkg/runner/runner.go (3)
205-208: Consider documenting the deferred output behaviorWhen
NmapCLIis specified, JSON/CSV output is deferred until after nmap integration completes. This behavioral change should be documented to avoid user confusion about why real-time output stops working with nmap enabled.Consider adding a log message to inform users that output is being deferred:
// Skip immediate JSON/CSV output if nmap CLI is specified to postpone until after nmap integration if r.options.NmapCLI != "" && (r.options.JSON || r.options.CSV) { + gologger.Debug().Msgf("Deferring JSON/CSV output until after nmap integration completes") return }
471-477: Consider extracting the repeated nmap/output patternThe pattern of calling
handleNmap()followed byhandleOutput()is repeated in three locations. Consider extracting this into a helper method to reduce duplication and ensure consistency.+func (r *Runner) finalizeResults() error { + // handle nmap first to integrate service information + if err := r.handleNmap(); err != nil { + return err + } + // then handle output with enhanced service information + r.handleOutput(r.scanner.ScanResults) + return nil +} // Then replace the three occurrences with: - // handle nmap first to integrate service information - if err := r.handleNmap(); err != nil { - return err - } - // then handle output with enhanced service information - r.handleOutput(r.scanner.ScanResults) - return nil + return r.finalizeResults()Also applies to: 641-648, 707-714
1056-1073: Consider reusing the service mapping helperSimilar to the suggestion in
cmd/naabu/main.go, the manual field-by-field copying fromport.ServicetoResultcould benefit from a helper method to reduce duplication and improve maintainability.+func copyServiceToResult(service *port.Service, result *Result) { + if service == nil { + return + } + result.DeviceType = service.DeviceType + result.ExtraInfo = service.ExtraInfo + result.HighVersion = service.HighVersion + result.Hostname = service.Hostname + result.LowVersion = service.LowVersion + result.Method = service.Method + result.Name = service.Name + result.OSType = service.OSType + result.Product = service.Product + result.Proto = service.Proto + result.RPCNum = service.RPCNum + result.ServiceFP = service.ServiceFP + result.Tunnel = service.Tunnel + result.Version = service.Version + result.Confidence = service.Confidence +} // Then use it: - // copy service information if available - if p.Service != nil { - data.DeviceType = p.Service.DeviceType - data.ExtraInfo = p.Service.ExtraInfo - data.HighVersion = p.Service.HighVersion - data.Hostname = p.Service.Hostname - data.LowVersion = p.Service.LowVersion - data.Method = p.Service.Method - data.Name = p.Service.Name - data.OSType = p.Service.OSType - data.Product = p.Service.Product - data.Proto = p.Service.Proto - data.RPCNum = p.Service.RPCNum - data.ServiceFP = p.Service.ServiceFP - data.Tunnel = p.Service.Tunnel - data.Version = p.Service.Version - data.Confidence = p.Service.Confidence - } + // copy service information if available + copyServiceToResult(p.Service, data)pkg/runner/output.go (1)
98-114: Replace manual field copying with a more maintainable approachThe manual copying of 15 service fields is error-prone and difficult to maintain. When new fields are added to the
Servicestruct, they must be manually added here as well.Consider using reflection or a mapping library to automate the field copying:
// Using a helper function with reflection func copyServiceFields(src, dst interface{}) { // Implementation using reflection to copy matching fields }pkg/runner/nmap.go (3)
36-45: Define constants for port count thresholdsThe hardcoded values for grouping hosts by port count should be defined as constants for better maintainability and clarity.
const ( smallPortRange = 100 mediumPortRange = 1000 largePortRange = 10000 ) switch { case length > smallPortRange && length < mediumPortRange: index = 1 case length >= mediumPortRange && length < largePortRange: index = 2 case length >= largePortRange: index = 3 default: index = 0 }
169-190: Consider optimization for large result setsThe method correctly handles both updating existing ports and adding new ones. However, the nested iteration through all host results could be inefficient for large scans.
For better performance with large result sets, consider maintaining an index structure (e.g., map[string]map[int]*port.Port) for O(1) lookups instead of O(n) iteration.
212-229: Service metadata might be lost if name is emptyThe service information is only created when
nmapPort.Service.Nameis not empty. This could result in losing other valuable service metadata (version, product, etc.) if the service name wasn't detected but other fields were populated.Consider creating the Service struct if any service field has data:
if nmapPort.Service.Name != "" || nmapPort.Service.Product != "" || nmapPort.Service.Version != "" || nmapPort.Service.ExtraInfo != "" { naabuPort.Service = &port.Service{ // ... copy all fields } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
.github/workflows/build-test.ymlis excluded by!**/*.yml.github/workflows/lint-test.ymlis excluded by!**/*.yml.github/workflows/release-binary.ymlis excluded by!**/*.yml.github/workflows/release-test.ymlis excluded by!**/*.ymlgo.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
cmd/naabu/main.go(2 hunks)go.mod(1 hunks)pkg/runner/nmap.go(1 hunks)pkg/runner/nmap_test.go(1 hunks)pkg/runner/options.go(1 hunks)pkg/runner/output.go(4 hunks)pkg/runner/runner.go(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
cmd/naabu/main.go (1)
pkg/port/port.go (1)
Service(35-51)
pkg/runner/nmap.go (5)
pkg/runner/runner.go (1)
Runner(46-58)pkg/result/results.go (1)
HostResult(13-18)pkg/port/port.go (2)
Port(10-17)Service(35-51)pkg/scan/scan.go (1)
Scanner(77-95)pkg/protocol/protocol.go (3)
Protocol(7-7)TCP(10-10)UDP(11-11)
pkg/runner/output.go (2)
pkg/runner/banners.go (1)
Version(23-23)pkg/port/port.go (1)
Service(35-51)
pkg/runner/runner.go (2)
pkg/runner/output.go (1)
Result(24-51)pkg/port/port.go (2)
Port(10-17)Service(35-51)
🔇 Additional comments (3)
pkg/runner/options.go (1)
43-43: Documentation update looks goodThe clarified comment accurately reflects the new behavior where nmap detailed scan is conditional on the
NmapCLIoption being provided.pkg/runner/nmap_test.go (1)
13-95: Excellent test coverage for the new Nmap integrationThe test suite properly covers:
- Behavior when no Nmap CLI is specified (should skip)
- Behavior with custom Nmap CLI arguments
- Service information update functionality
The tests are well-structured and follow good testing practices.
pkg/runner/nmap.go (1)
127-166: LGTM!The method properly handles nil checks, integrates nmap results cleanly, and provides informative logging of the enhanced service information.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/result/results.go(2 hunks)pkg/runner/nmap.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/result/results.go (3)
pkg/port/port.go (1)
Port(10-17)pkg/result/confidence/confidence.go (1)
ConfidenceLevel(3-3)pkg/runner/output.go (1)
Result(24-51)
pkg/runner/nmap.go (5)
pkg/runner/runner.go (2)
Runner(46-58)Target(60-65)pkg/result/results.go (2)
HostResult(21-27)OSFingerprint(13-19)pkg/port/port.go (2)
Port(10-17)Service(35-51)pkg/scan/scan.go (1)
Scanner(77-95)pkg/protocol/protocol.go (3)
Protocol(7-7)TCP(10-10)UDP(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: release-test-mac
- GitHub Check: release-test-windows
- GitHub Check: Functional Test (ubuntu-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
pkg/result/results.go (2)
13-19: LGTM! Clean struct definition for OS fingerprinting data.The
OSFingerprintstruct provides appropriate fields for capturing OS detection metadata from Nmap scans.
26-26: LGTM! Appropriate use of pointer for optional OS data.Using
*OSFingerprintallows the OS field to be nil when OS fingerprinting data is not available.pkg/runner/nmap.go (9)
15-18: LGTM! Appropriate early return for empty CLI arguments.The early return when no custom nmap CLI arguments are provided is logical and efficient.
20-29: LGTM! Clean host collection logic.The code properly collects hosts with open ports and handles the case when no hosts are available for scanning.
31-47: Good optimization strategy for grouping hosts by port count.The port-count-based grouping approach is smart for efficient scanning, as it allows nmap to be run with appropriate parameters for different scan sizes.
54-76: LGTM! Efficient port and IP aggregation.The logic correctly aggregates unique ports and IPs for each group, avoiding duplicates using a map.
79-103: LGTM! Proper nmap scanner configuration.The scanner setup correctly handles custom arguments and removes the "nmap" prefix when present. Error handling is appropriate.
105-121: LGTM! Good error handling and warning management.The scan execution includes proper error handling and warning logging, with graceful continuation on failures.
127-148: LGTM! Comprehensive OS fingerprint conversion.The
nmapOS2Fingerprintfunction properly handles the conversion from nmap OS data to the internal OSFingerprint struct, with appropriate null checks.
151-195: LGTM! Well-structured result integration.The
integrateNmapResultsfunction properly processes nmap results and integrates them with existing scan data, including both service information and OS fingerprinting.
222-261: LGTM! Comprehensive port conversion with proper type handling.The
convertNmapPortToNaabuPortfunction correctly handles protocol conversion and comprehensive service data mapping. The uint16 to int conversion is handled appropriately with a comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/runner/output.go (1)
34-50: Service field duplication already flagged in past reviews.The duplication of service fields between
ResultandjsonResultstructs, and the duplicated copying logic, have been identified in previous review comments. These concerns remain valid.Also applies to: 66-82, 98-113, 207-224
🧹 Nitpick comments (1)
pkg/runner/runner.go (1)
1082-1088: Inefficient CSV writer creation inside loop.A new
csv.Writeris created for each port iteration (line 1082), and another one is created just to flush at line 1095. This is inefficient.Create the writer once before the port loop and reuse it:
// console output if r.options.JSON || r.options.CSV { data := &Result{IP: hostResult.IP, TimeStamp: time.Now().UTC()} + var writer *csv.Writer + if r.options.CSV { + writer = csv.NewWriter(&buffer) + } if r.options.OutputCDN { data.IsCDNIP = isCDNIP data.CDNName = cdnName } if host != hostResult.IP { data.Host = host } for _, p := range hostResult.Ports { data.Port = p.Port data.Protocol = p.Protocol.String() data.TLS = p.TLS // copy service information if available if p.Service != nil { copyServiceFields(data, p.Service) } if r.options.JSON { b, err := data.JSON(r.options.ExcludeOutputFields) if err != nil { continue } buffer.Write([]byte(fmt.Sprintf("%s\n", b))) } else if r.options.CSV { - writer := csv.NewWriter(&buffer) if csvFileHeaderEnabled { writeCSVHeaders(data, writer, r.options.ExcludeOutputFields) csvFileHeaderEnabled = false } writeCSVRow(data, writer, r.options.ExcludeOutputFields) } } } if r.options.JSON { gologger.Silent().Msgf("%s", buffer.String()) } else if r.options.CSV { - writer := csv.NewWriter(&buffer) + writer.Flush() - writer.Flush() gologger.Silent().Msgf("%s", buffer.String()) }Also applies to: 1095-1097
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.github/workflows/release-binary.ymlis excluded by!**/*.yml.github/workflows/release-test.ymlis excluded by!**/*.ymlgo.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.mod(1 hunks)pkg/runner/options.go(1 hunks)pkg/runner/output.go(4 hunks)pkg/runner/runner.go(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/runner/options.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/runner/output.go (1)
pkg/port/port.go (1)
Service(35-51)
pkg/runner/runner.go (2)
pkg/runner/output.go (1)
Result(24-51)pkg/port/port.go (2)
Port(10-17)Service(35-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: release-test-windows
- GitHub Check: Functional Test (ubuntu-latest)
- GitHub Check: release-test-mac
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
pkg/runner/runner.go (1)
470-477: LGTM! Consistent nmap integration ordering.The pattern of calling
handleNmap()first (with error handling) followed byhandleOutput()is consistently applied across all execution paths, ensuring service metadata is enriched before final output.Also applies to: 641-648, 707-713
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.github/workflows/build-test.ymlis excluded by!**/*.yml.github/workflows/release-binary.ymlis excluded by!**/*.yml.github/workflows/release-test.ymlis excluded by!**/*.ymlgo.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.mod(1 hunks)pkg/runner/options.go(1 hunks)pkg/runner/output.go(4 hunks)pkg/runner/runner.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- pkg/runner/output.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/runner/runner.go (2)
pkg/runner/output.go (1)
Result(24-51)pkg/port/port.go (2)
Port(10-17)Service(35-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Functional Test (ubuntu-latest)
- GitHub Check: Lint
- GitHub Check: release-test-linux
- GitHub Check: release-test-windows
- GitHub Check: Analyze (go)
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
3 participants
Closes #1525
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation