search

mrheinen / lophiid

A distributed honeypot for monitoring large scale web attacks
GNU General Public License v2.0
6 stars 1 forks source link

Add IP event table and logic to send IP related events #21

Closed mrheinen closed 2 months ago

mrheinen commented 2 months ago

User description

This change adds an IP event table to the database. It also updates the backend to write events to the database in case when we know an IP is malicious because it send a payload that was marked malicious by VirusTotal. The content rules are expanded and now allow an Request Purpose to be specified (e.g ATTACK, CRAWL, ..) and this, depending on the purpose also causes an entry to be added to the IP event table.

The IP event table can then be used to query information about IPs and their roles throughout time.

PR Type

Enhancement, Bug fix

Description

Changes walkthrough 📝

Relevant files
Enhancement
backend_main.go
Integrate IP event manager in backend                                       
cmd/backend/backend_main.go
  • Added IP event manager initialization and integration
  • Updated VTBackgroundManager initialization with IP event manager
  • Fixed import path from 'loophid' to 'lophiid'
    		•  +10/-4   
    ip.go
    Implement IP event management system                                         
    pkg/analysis/ip.go
  • Implemented IpEventManager interface and IpEventManagerImpl struct
  • Added methods for adding, processing, and monitoring IP events
  • Implemented cache mechanism for IP events before database insertion
    		•  +129/-0 
    backend.go
    Integrate IP event manager in backend server                         
    pkg/backend/backend.go
  • Added ipEventManager to BackendServer struct
  • Updated NewBackendServer function to include ipEventManager
  • Modified HandleProbe and HandleFileUpload to use IP event manager
    		•  +57/-13 
    database.go
    Add IP event database structures and methods                         
    pkg/database/database.go
  • Added IpEvent struct and related constants
  • Updated DatabaseClient interface with new methods for IP events
  • Added SearchEvents method to KSQLClient
    		•  +76/-4   
    manager.go
    Integrate IP events with VirusTotal analysis                         
    pkg/vt/manager.go
  • Added GetEventsForDownload method to VTManager interface
  • Updated VTBackgroundManager to include ipEventManager
  • Modified GetFileAnalysis to generate IP events for
    malicious/suspicious files
    		•  +72/-13 
    EventsList.vue
    Add EventsList component for IP event display                       
    ui/src/components/container/EventsList.vue
  • Implemented new EventsList component for displaying IP events
  • Added pagination, search, and event details functionality
  • Integrated with backend API for fetching event data
    		•  +278/-0 
    database.sql
    Add IP event and request purpose database structures         
    config/database.sql
  • Added IP_EVENT_TYPE enum and ip_event table
  • Updated content_rule table with request_purpose column
  • Added REQUEST_PURPOSE enum
    		•  +20/-5   

    💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    github-actions[bot] commented 2 months ago

    Preparing PR description...

    github-actions[bot] commented 2 months ago

    Preparing review...

    codiumai-pr-agent-pro[bot] commented 2 months ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Race Condition
    The `ProcessRequest` function is modifying shared state (`s.reqsQueue`) without proper synchronization. This could lead to race conditions in a concurrent environment. Potential Memory Leak
    The `MonitorQueue` function runs in an infinite loop without a clear termination condition. This could potentially lead to a goroutine leak if not properly managed. Error Handling
    The `GetEventsForDownload` function ignores the error returned by `v.dbClient.GetRequestByID`. This could lead to silent failures and unexpected behavior.
    codiumai-pr-agent-pro[bot] commented 2 months ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Verify the correctness and consistency of the import path change across the project ___ **The importpath has been changed from "loophid/cmd/backend" to "lophiid/cmd/backend".
    Ensure that this change is intentional and consistent across the entire project. If
    it's a typo, correct it to maintain consistency.** [cmd/backend/BUILD.bazel [6]](https://github.com/mrheinen/lophiid/pull/21/files#diff-af999aed3fb706d3f0eed35e1896ed092c8b96ea71ff85a20198533f71edb397R6-R6) ```diff -importpath = "lophiid/cmd/backend", +importpath = "loophid/cmd/backend", ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 9 Why: This suggestion identifies a critical change that affects multiple files and could cause significant issues if inconsistent or incorrect.
    		9
    Extract event creation logic into separate methods for improved code organization and reusability ___ **Consider extracting the event creation logic into a separate method to improve code
    readability and reusability. This will also make it easier to add new event types in
    the future.** [pkg/vt/manager.go [180-207]](https://github.com/mrheinen/lophiid/pull/21/files#diff-a001ead65cd25a420c17947b16e7deaef08374a08839adf0a5d5edcec50bf5e4R180-R207) ```diff -evt := database.IpEvent{ - IP: dl.IP, - Type: analysis.IpEventHostedMalware, - RequestID: dl.RequestID, - Details: fmt.Sprintf("Hosted file that VirusTotal reported on (%d malicious, %d suspicious)", dl.VTAnalysisMalicious, dl.VTAnalysisSuspicious), +func (v *VTBackgroundManager) createHostedMalwareEvent(dl *database.Download) database.IpEvent { + evt := database.IpEvent{ + IP: dl.IP, + Type: analysis.IpEventHostedMalware, + RequestID: dl.RequestID, + Details: fmt.Sprintf("Hosted file that VirusTotal reported on (%d malicious, %d suspicious)", dl.VTAnalysisMalicious, dl.VTAnalysisSuspicious), + } + if dl.Host != dl.IP { + evt.Domain = dl.Host + } + return evt } -if dl.Host != dl.IP { - // TODO: consider resolving the domain and also adding any additional - // IPs that yields. - evt.Domain = dl.Host +func (v *VTBackgroundManager) createAttackedEvent(dl *database.Download, sourceIP string) database.IpEvent { + return database.IpEvent{ + IP: sourceIP, + Type: analysis.IpEventAttacked, + RequestID: dl.RequestID, + Details: fmt.Sprintf("Sent payload that VirusTotal reported on (%d malicious, %d suspicious)", dl.VTAnalysisMalicious, dl.VTAnalysisSuspicious), + } } -ret = append(ret, evt) +// In GetEventsForDownload method: +ret = append(ret, v.createHostedMalwareEvent(dl)) r, err := v.dbClient.GetRequestByID(dl.RequestID) if err != nil { slog.Error("unexpected error, cannot find request", slog.String("error", err.Error()), slog.Int64("request_id", dl.RequestID)) } else { - ret = append(ret, database.IpEvent{ - IP: r.SourceIP, - Type: analysis.IpEventAttacked, - RequestID: dl.RequestID, - Details: fmt.Sprintf("Sent payload that VirusTotal reported on (%d malicious, %d suspicious)", dl.VTAnalysisMalicious, dl.VTAnalysisSuspicious), - }) + ret = append(ret, v.createAttackedEvent(dl, r.SourceIP)) } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: This suggestion significantly improves code organization, reusability, and maintainability, making it easier to manage and extend the event creation logic.
    		8
    Type safety
    Use a typed enum for the RequestPurpose field instead of a string ___ **Consider using a constant or enum for the RequestPurpose field instead of a string.
    This can help prevent typos and ensure only valid values are used.** [pkg/database/database.go [100]](https://github.com/mrheinen/lophiid/pull/21/files#diff-1adb887d06a44193c36fc1c5708be385f3129cd59c2f2aa555faa065941ed877R100-R100) ```diff -RequestPurpose string `ksql:"request_purpose" json:"request_purpose" doc:"The purpose of the request (e.g. UNKNOWN, RECON, CRAWL, ATTACK)"` +RequestPurpose RequestPurpose `ksql:"request_purpose" json:"request_purpose" doc:"The purpose of the request"` ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Using a typed enum instead of a string for RequestPurpose improves type safety and prevents potential errors from invalid string values.
    		8
    Possible issue
    Verify the correct placement of the semver dependency in the Bazel build file ___ **The dependency for semver has been moved from the go_test rule to the go_library
    rule. Ensure that the semver package is actually used in the library code and not
    just in tests. If it's only used in tests, move the dependency back to the go_test
    rule.** [pkg/util/BUILD.bazel [15]](https://github.com/mrheinen/lophiid/pull/21/files#diff-ef0e6ade2ca273a7a53dbecd6c11afab762c26feb5bd907307158de5728745c2R15-R15) ```diff +deps = ["@com_github_blang_semver_v4//:semver"], - ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: This suggestion correctly identifies a potentially significant change in dependency management that could affect build and runtime behavior.
    		8
    Enhancement
    Refactor multiple if conditions into a switch statement for better readability and maintainability ___ **Consider using a switch statement instead of multiple if conditions for different
    event types. This will make the code more readable and easier to maintain,
    especially if more event types are added in the future.** [pkg/vt/manager.go [768-792]](https://github.com/mrheinen/lophiid/pull/21/files#diff-a001ead65cd25a420c17947b16e7deaef08374a08839adf0a5d5edcec50bf5e4R768-R792) ```diff if rule.RequestPurpose != database.RuleRequestPurposeUnknown { + var eventType analysis.IpEventType + var eventDetails string switch rule.RequestPurpose { case database.RuleRequestPurposeAttack: - s.ipEventManager.AddEvent(&database.IpEvent{ - IP: req.SourceIP, - Type: analysis.IpEventAttacked, - Details: fmt.Sprintf("rule %d indicated the IP attacked", rule.ID), - RequestID: dm.ModelID(), - }) + eventType = analysis.IpEventAttacked + eventDetails = "attacked" case database.RuleRequestPurposeCrawl: - s.ipEventManager.AddEvent(&database.IpEvent{ - IP: req.SourceIP, - Type: analysis.IpEventCrawl, - Details: fmt.Sprintf("rule %d indicated the IP crawled", rule.ID), - RequestID: dm.ModelID(), - }) + eventType = analysis.IpEventCrawl + eventDetails = "crawled" case database.RuleRequestPurposeRecon: - s.ipEventManager.AddEvent(&database.IpEvent{ - IP: req.SourceIP, - Type: analysis.IpEventRecon, - Details: fmt.Sprintf("rule %d indicated the IP reconned", rule.ID), - RequestID: dm.ModelID(), - }) + eventType = analysis.IpEventRecon + eventDetails = "reconned" } + s.ipEventManager.AddEvent(&database.IpEvent{ + IP: req.SourceIP, + Type: eventType, + Details: fmt.Sprintf("rule %d indicated the IP %s", rule.ID, eventDetails), + RequestID: dm.ModelID(), + }) } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: This suggestion enhances code readability and maintainability, making it easier to add new event types in the future.
    		7
    Encapsulate related parameters into a struct for improved function signature ___ **Consider using a struct to encapsulate the request and rule information instead of
    passing them as separate parameters. This will make the function signature cleaner
    and more maintainable.** [pkg/backend/backend.go [759]](https://github.com/mrheinen/lophiid/pull/21/files#diff-c65bcfe9bb457434c3e69ba3f0576d7669935f350d24e2c2c58b05b4f9c510b2R759-R759) ```diff -func (s *BackendServer) ProcessRequest(req *database.Request, rule database.ContentRule) error { +type RequestWithRule struct { + Request *database.Request + Rule database.ContentRule +} +func (s *BackendServer) ProcessRequest(reqWithRule RequestWithRule) error { + ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: The suggestion improves code readability and maintainability by grouping related parameters, but it's not addressing a critical issue.
    		6
    Add context parameter for better control flow and cancellation support ___ **Consider adding a context parameter to the ProcessNewEvent function to allow for
    cancellation and timeout handling.** [pkg/analysis/ip.go [115]](https://github.com/mrheinen/lophiid/pull/21/files#diff-24480f2fb374402d21f92840858fefced2bab4fd547efd664c76acbce047103bR115-R115) ```diff -func (i *IpEventManagerImpl) ProcessNewEvent(evt *database.IpEvent) error { +func (i *IpEventManagerImpl) ProcessNewEvent(ctx context.Context, evt *database.IpEvent) error { ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: Adding a context parameter can improve control flow and allow for cancellation, but it's not critical for this specific function and would require changes in the calling code.
    		6
    Error handling
    ✅ Add error handling for the cache entry replacement operation ___ **Add error handling for the Replace operation in the ProcessNewEvent function. If an
    error occurs during replacement, it should be logged or handled appropriately.** [pkg/analysis/ip.go [120-121]](https://github.com/mrheinen/lophiid/pull/21/files#diff-24480f2fb374402d21f92840858fefced2bab4fd547efd664c76acbce047103bR120-R121) ```diff -i.ipCache.Replace(cacheKey, *entry) +if err := i.ipCache.Replace(cacheKey, *entry); err != nil { + return fmt.Errorf("failed to replace cache entry: %w", err) +} return nil ``` `[Suggestion has been applied]`
    Suggestion importance[1-10]: 7 Why: Adding error handling for the cache replacement operation is a good practice to catch and handle potential errors, improving the robustness of the code.
    		7
    Security
    Reduce the cache expiration time for VirusTotal results to ensure more up-to-date security information ___ **Consider using a shorter cache expiration time for VirusTotal results. 96 hours (4
    days) might be too long, potentially leading to outdated security information. A
    shorter duration, such as 24 hours, could provide a better balance between quota
    conservation and up-to-date results.** [config/backend-config.yaml [56]](https://github.com/mrheinen/lophiid/pull/21/files#diff-422f5c2c60abdac2feda6fca8b0816bb571e902de5d73a2c5c9c430406dff9c5R56-R56) ```diff -cache_expiration_time: 96h +cache_expiration_time: 24h ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: The suggestion is valid and addresses a potential security concern. However, the optimal cache time depends on specific use cases and quota limitations.
    		7
    Performance
    Ensure the event queue channel is properly sized to prevent blocking ___ **Consider using a buffered channel for eventQueue to prevent potential blocking when
    adding events.** [pkg/analysis/ip.go [66]](https://github.com/mrheinen/lophiid/pull/21/files#diff-24480f2fb374402d21f92840858fefced2bab4fd547efd664c76acbce047103bR66-R66) ```diff eventQueue: make(chan *database.IpEvent, ipQueueSize), +// Consider increasing the buffer size if needed ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 5 Why: The suggestion is correct but the existing code already uses a buffered channel with a configurable size, so the improvement is minimal.
    		5
    mrheinen commented 2 months ago

    /describe

    github-actions[bot] commented 2 months ago

    Preparing PR description...

    codiumai-pr-agent-pro[bot] commented 2 months ago

    PR Description updated to latest commit (https://github.com/mrheinen/lophiid/commit/2d6845a94422f2f646682f1e699e9731d4819703)