HIDS/HIPS Architecture Implementation#2152
Open
sisyphuspulpfiction wants to merge 2 commits intosafing:developmentfrom
Open
HIDS/HIPS Architecture Implementation#2152sisyphuspulpfiction wants to merge 2 commits intosafing:developmentfrom
sisyphuspulpfiction wants to merge 2 commits intosafing:developmentfrom
Conversation
- Phase 1: Injected telemetry hook in Go network connection teardown to stream data to Unix socket - Phase 2: Added Python/PyTorch sidecar daemon to listen for telemetry, run anomaly detection, and fire alert endpoints - Phase 3: Added Go API endpoints for alerts and app quarantining via Profile configuration defaults - Scaffolded basic Vue3 WarningCard components to interact with the quarantine API Co-authored-by: sisyphuspulpfiction <222319458+sisyphuspulpfiction@users.noreply.github.com>
…7782092985171259 HIDS/HIPS Architecture Implementation
|
|
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a complete HIDS (Host-Based Intrusion Detection System) UI stack: a Vue 3 frontend for displaying and quarantining suspicious processes, two new backend API endpoints for alerts and quarantine operations, telemetry collection infrastructure in the core service, and a Python-based ML anomaly detection sidecar using a tabular autoencoder model. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Frontend)
participant Frontend as portmaster-ui
participant Backend as Service API
participant DB as Profile DB
User->>Frontend: Click "Quarantine App" button
Frontend->>Frontend: Build FormData (profile=processName)
Frontend->>Backend: POST /api/v1/hids/quarantine
Backend->>DB: GetLocalProfile(processName)
DB-->>Backend: Profile object
Backend->>Backend: Set filter/defaultAction to "block"
Backend->>DB: prof.Save()
DB-->>Backend: Success
Backend-->>Frontend: {status:"quarantined", profile:id}
Frontend->>Frontend: Set quarantined=true, disable button
Frontend->>User: Display "App Quarantined"
sequenceDiagram
participant Service as Portmaster Service
participant Telemetry as UDS Socket
participant Sidecar as InferenceEngine
participant Model as TabularAutoencoder
participant Alert as /hids/alert Endpoint
Service->>Service: Connection finalized (delete)
Service->>Service: Build ConnectionTelemetry struct
Service->>Service: Marshal to JSON
Service->>Telemetry: Async dial & write JSON+newline
Sidecar->>Telemetry: Accept connection, read JSON
Sidecar->>Sidecar: Parse telemetry fields
Sidecar->>Sidecar: Build 3-feature tensor
Sidecar->>Model: Forward pass (encoder→decoder)
Model-->>Sidecar: Reconstructed output, MSE loss
Sidecar->>Sidecar: Compare loss vs ANOMALY_THRESHOLD
alt Loss > Threshold
Sidecar->>Alert: POST {pid, binaryPath, score}
Alert-->>Sidecar: {status:"alert_received"}
end
Telemetry-->>Service: Done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
.gitignore (1)
70-71: Optional: Line 71 is partially redundant with the existingdist/pattern at line 3.The root
.gitignorealready containsdist/at line 3, which will matchportmaster-ui/dist/when evaluated from the repository root. Line 71 (portmaster-ui/dist/) is explicit but not strictly necessary.The current setup works correctly and being explicit can improve clarity, so this is purely optional feedback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 70 - 71, The .gitignore contains a redundant entry "portmaster-ui/dist/" because a root-level "dist/" already ignores that path; remove the specific "portmaster-ui/dist/" line (or alternatively remove the global "dist/" if you prefer explicit per-package ignores) so entries are not duplicated—look for the "dist/" and "portmaster-ui/dist/" patterns in .gitignore and keep only the form your project prefers.portmaster-ui/package.json (1)
1-4: Mark this UI package as private.Add
"private": trueto avoid accidental npm publication of an internal app package.Suggested change
{ "name": "portmaster-ui-vue", + "private": true, "version": "1.0.0", "description": "Portmaster UI Transformation - HIDS/HIPS",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portmaster-ui/package.json` around lines 1 - 4, Add the npm private flag to the package manifest so the UI package cannot be accidentally published: update package.json (the manifest containing "name": "portmaster-ui-vue") to include the top-level property "private": true alongside "name", "version", and "description".sidecar/requirements.txt (1)
1-2: Pin sidecar dependency versions.
torchandrequestsare unversioned insidecar/requirements.txt, making builds non-reproducible and vulnerable to breaking or security regressions at install time. Pin tested versions (e.g.,torch==2.x.x,requests==2.x.x) or enforce via aconstraints.txtfile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sidecar/requirements.txt` around lines 1 - 2, The requirements file lists unpinned packages "torch" and "requests" which makes builds non-reproducible; update sidecar/requirements.txt to pin each package to tested versions (e.g., replace "torch" with a specific version like "torch==2.x.y" and "requests" with "requests==2.x.y") or add a constraints file and reference it so installs are deterministic; ensure the chosen versions match CI/tested matrix and update any build/dev docs or CI that install these deps.sidecar/inference.py (2)
52-55: Broad exception handling is acceptable for resilience but consider logging exception type.The static analysis flags (BLE001) broad
except Exceptionclauses. While this is appropriate for a long-running daemon to prevent crashes, consider logging the exception type for better debugging:except Exception as e: logging.error(f"Error handling connection: {type(e).__name__}: {e}")Also applies to: 77-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sidecar/inference.py` around lines 52 - 55, The except blocks in the connection-handling code (the broad "except Exception as e" in the function handling connections) should log the exception type as well as the message for better debugging; update the logging.error calls in that handler (and the similar broad except at the other site around lines 77-78) to include type(e).__name__ alongside e, preserving the finally conn.close() behavior and not changing the exception handling semantics.
13-13: Acknowledged:/tmppath flagged by static analysis (S108).Using
/tmpfor the Unix socket is a potential security concern as the directory is world-writable. A malicious process could pre-create the socket to intercept telemetry.For production, consider using a more restricted path like
/run/portmaster/or a path within the Portmaster data directory with appropriate permissions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sidecar/inference.py` at line 13, UDS_SOCKET_PATH set to "/tmp/portmaster_telemetry.sock" is insecure; change it to a restricted, configurable runtime path (e.g. use XDG_RUNTIME_DIR or "/run/portmaster/portmaster_telemetry.sock") and ensure the directory is created with secure permissions before binding. Update the UDS_SOCKET_PATH constant in inference.py to read from configuration/env (fallback to a secure default), and add code to create the parent directory with os.makedirs(..., exist_ok=True) and set permissions (os.chmod) or use a umask so only the service user can write the socket.service/network/api.go (2)
83-94: Consider whetherapi.PermitUseris the appropriate permission level for quarantine.The
hids/quarantineendpoint can block all network traffic for an application by modifying its profile configuration. This is a high-impact action that could be abused or triggered accidentally.Consider whether
api.PermitAdminwould be more appropriate, or adding a confirmation mechanism.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/network/api.go` around lines 83 - 94, The hids/quarantine endpoint is currently registered with api.PermitUser; update the RegisterEndpoint call for the Endpoint with Path "hids/quarantine" so its Write permission uses api.PermitAdmin instead of api.PermitUser, and optionally add a required confirmation parameter (e.g., a boolean "confirm" Parameter or require an additional confirmation middleware) to the Endpoint.Parameters to ensure explicit confirmation before applying the profile change.
99-108: Consider validating and parsing input parameters.
handleHidsAlertaccepts raw form values without validation:
pidshould be validated as a positive integerscoreshould be parsed as a float for structured logging or future processingCurrently this only logs, but if extended to store alerts or broadcast to UI, malformed data could cause issues.
Optional validation
func handleHidsAlert(ar *api.Request) (i interface{}, err error) { - pid := ar.Request.FormValue("pid") + pidStr := ar.Request.FormValue("pid") + pid, err := strconv.Atoi(pidStr) + if err != nil || pid <= 0 { + return nil, fmt.Errorf("invalid pid parameter") + } binaryPath := ar.Request.FormValue("binaryPath") - score := ar.Request.FormValue("score") + scoreStr := ar.Request.FormValue("score") + score, err := strconv.ParseFloat(scoreStr, 64) + if err != nil { + return nil, fmt.Errorf("invalid score parameter") + } - log.Warningf("HIDS ALERT: Suspicious activity detected for PID %s (%s) with score %s", pid, binaryPath, score) + log.Warningf("HIDS ALERT: Suspicious activity detected for PID %d (%s) with score %.4f", pid, binaryPath, score) return map[string]string{"status": "alert_received"}, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/network/api.go` around lines 99 - 108, handleHidsAlert currently reads raw form values (pid, binaryPath, score) without validation; update the function to validate pid is a positive integer (parse pid into an int and ensure >0), parse score into a float64 (handle strconv.ParseFloat errors), and treat binaryPath as a non-empty string; on parse/validation failure return a clear error (or a structured bad-request response) and log the normalized parsed values (use the parsed pid and score in log messages) so downstream storage/broadcast code receives typed data; reference function handleHidsAlert and variables pid, score, binaryPath and ensure any errors from parsing are handled and returned instead of proceeding with raw strings.portmaster-ui/src/App.vue (1)
4-10: Consider using a unique identifier forv-forkey instead ofpid.PIDs can be recycled by the operating system after a process terminates. If two alerts from different processes happen to share the same PID (one historical, one current), Vue's keyed rendering may incorrectly reuse DOM elements or skip updates.
Consider adding a unique alert ID (e.g., timestamp + pid combination, or a UUID) when alerts are received.
Example approach
- v-for="alert in alerts" - :key="alert.pid" + v-for="alert in alerts" + :key="alert.id"Where
alert.idcould be generated as${alert.pid}-${Date.now()}or a server-provided unique identifier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portmaster-ui/src/App.vue` around lines 4 - 10, The v-for key currently uses alert.pid which can collide due to PID reuse; change the key to a stable unique identifier (e.g., alert.id) and ensure that each alert item gets a generated unique id when it is created/received (server-provided UUID or combine pid with timestamp/UUID) so Vue can reliably track WarningCard instances; update the template's v-for to use :key="alert.id" and populate alert.id in the code path that constructs or normalizes the alerts array.portmaster-ui/src/components/WarningCard.vue (1)
44-44: Hardcoded API URL may not match runtime configuration.The URL
http://127.0.0.1:817is hardcoded, but perservice/core/base/module.go, this is a configurable default. If the API listen address is changed at runtime, this will fail.Consider using a configuration value or environment variable, or making the base URL a prop/inject from the parent application.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portmaster-ui/src/components/WarningCard.vue` at line 44, The fetch call in WarningCard.vue currently hardcodes the base URL ("http://127.0.0.1:817"), which will break if the service listen address is changed; change the component to accept the API base as a configurable value (e.g., a prop like apiBaseUrl or via Vue provide/inject) and build the request URL by concatenating `${apiBaseUrl}/api/v1/hids/quarantine` where the current fetch is used (the line with fetch('http://127.0.0.1:817/api/v1/hids/quarantine')), defaulting to the known default if the prop/env is not provided (or read from an environment variable such as process.env.VUE_APP_API_BASE) so runtime configuration is respected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@portmaster-ui/src/components/WarningCard.vue`:
- Around line 39-47: The UI is sending the binary path (this.processName) as the
'profile' field which causes handleHidsQuarantine/profile.GetLocalProfile to
fail; update WarningCard (and the telemetry flow) to send the actual profile ID:
add a profileId prop to WarningCard, change the form payload to
append('profile', this.profileId) instead of this.processName in the POST to
/api/v1/hids/quarantine, and ensure the backend-facing telemetry/alert pipeline
(Python sidecar and Go telemetry sender) is updated to include profileId
alongside binaryPath so consumers calling profile.GetLocalProfile receive a
valid profile identifier.
In `@service/network/connection.go`:
- Around line 842-844: Telemetry is currently emitted unconditionally via
sendTelemetry(conn) during deletion, causing DNS and incomplete records to be
sent; wrap the sendTelemetry(conn) call with a guard that only allows emission
for finished IP connections (e.g., check the connection object for an IP-type
and a terminal/complete state such as conn.Type == IP or conn.IsIP() and
conn.State/Status == Complete/Closed or conn.IsComplete()), or use any existing
helper like conn.IsIP() or conn.IsComplete() if present; place this conditional
immediately before the sendTelemetry(conn) invocation so only complete IP
connections are streamed to the HIDS sidecar.
In `@service/network/telemetry.go`:
- Around line 24-42: sendTelemetry currently reads mutable Connection fields
(Entity, Ended, BytesSent, BytesReceived, etc.) without holding the connection
lock, causing data races; fix by either acquiring the connection's lock inside
sendTelemetry (lock/unlock around reads of conn.Entity, conn.Ended,
conn.BytesSent, conn.BytesReceived and any other mutable fields before building
ConnectionTelemetry) or change the API to accept a pre-populated
ConnectionTelemetry (or explicit values) that callers populate while holding the
connection lock; update signatures and call sites accordingly (referencing
sendTelemetry, Connection, ConnectionTelemetry and the fields
Entity/Ended/BytesSent/BytesReceived) so all mutable reads are protected.
In `@sidecar/inference.py`:
- Around line 87-92: The requests.post call currently sends JSON
(requests.post(url, json=payload)) but the Go handler handleHidsAlert reads form
values via Request.FormValue(), so change the send to use form-encoded data
(requests.post(url, data=payload, timeout=2)) and ensure payload values (e.g.,
payload['pid'], payload['score'], payload['binaryPath']) are strings (convert
numeric score/pid to str) before sending; keep the same try/except around the
call and continue to log success/failure using self.api_port, payload and score
identifiers.
In `@sidecar/main.py`:
- Around line 9-15: Change the CLI so --api-port is not always required: make
parser.add_argument("--api-port", ..., required=False) and after args =
parser.parse_args() add a validation that raises parser.error when
args.test_mode is False and args.api_port is None (e.g., if not args.test_mode
and args.api_port is None: parser.error("the --api-port argument is required
unless --test-mode is used")). Keep passing api_port=args.api_port into
InferenceEngine so test_mode can run without supplying a port.
---
Nitpick comments:
In @.gitignore:
- Around line 70-71: The .gitignore contains a redundant entry
"portmaster-ui/dist/" because a root-level "dist/" already ignores that path;
remove the specific "portmaster-ui/dist/" line (or alternatively remove the
global "dist/" if you prefer explicit per-package ignores) so entries are not
duplicated—look for the "dist/" and "portmaster-ui/dist/" patterns in .gitignore
and keep only the form your project prefers.
In `@portmaster-ui/package.json`:
- Around line 1-4: Add the npm private flag to the package manifest so the UI
package cannot be accidentally published: update package.json (the manifest
containing "name": "portmaster-ui-vue") to include the top-level property
"private": true alongside "name", "version", and "description".
In `@portmaster-ui/src/App.vue`:
- Around line 4-10: The v-for key currently uses alert.pid which can collide due
to PID reuse; change the key to a stable unique identifier (e.g., alert.id) and
ensure that each alert item gets a generated unique id when it is
created/received (server-provided UUID or combine pid with timestamp/UUID) so
Vue can reliably track WarningCard instances; update the template's v-for to use
:key="alert.id" and populate alert.id in the code path that constructs or
normalizes the alerts array.
In `@portmaster-ui/src/components/WarningCard.vue`:
- Line 44: The fetch call in WarningCard.vue currently hardcodes the base URL
("http://127.0.0.1:817"), which will break if the service listen address is
changed; change the component to accept the API base as a configurable value
(e.g., a prop like apiBaseUrl or via Vue provide/inject) and build the request
URL by concatenating `${apiBaseUrl}/api/v1/hids/quarantine` where the current
fetch is used (the line with
fetch('http://127.0.0.1:817/api/v1/hids/quarantine')), defaulting to the known
default if the prop/env is not provided (or read from an environment variable
such as process.env.VUE_APP_API_BASE) so runtime configuration is respected.
In `@service/network/api.go`:
- Around line 83-94: The hids/quarantine endpoint is currently registered with
api.PermitUser; update the RegisterEndpoint call for the Endpoint with Path
"hids/quarantine" so its Write permission uses api.PermitAdmin instead of
api.PermitUser, and optionally add a required confirmation parameter (e.g., a
boolean "confirm" Parameter or require an additional confirmation middleware) to
the Endpoint.Parameters to ensure explicit confirmation before applying the
profile change.
- Around line 99-108: handleHidsAlert currently reads raw form values (pid,
binaryPath, score) without validation; update the function to validate pid is a
positive integer (parse pid into an int and ensure >0), parse score into a
float64 (handle strconv.ParseFloat errors), and treat binaryPath as a non-empty
string; on parse/validation failure return a clear error (or a structured
bad-request response) and log the normalized parsed values (use the parsed pid
and score in log messages) so downstream storage/broadcast code receives typed
data; reference function handleHidsAlert and variables pid, score, binaryPath
and ensure any errors from parsing are handled and returned instead of
proceeding with raw strings.
In `@sidecar/inference.py`:
- Around line 52-55: The except blocks in the connection-handling code (the
broad "except Exception as e" in the function handling connections) should log
the exception type as well as the message for better debugging; update the
logging.error calls in that handler (and the similar broad except at the other
site around lines 77-78) to include type(e).__name__ alongside e, preserving the
finally conn.close() behavior and not changing the exception handling semantics.
- Line 13: UDS_SOCKET_PATH set to "/tmp/portmaster_telemetry.sock" is insecure;
change it to a restricted, configurable runtime path (e.g. use XDG_RUNTIME_DIR
or "/run/portmaster/portmaster_telemetry.sock") and ensure the directory is
created with secure permissions before binding. Update the UDS_SOCKET_PATH
constant in inference.py to read from configuration/env (fallback to a secure
default), and add code to create the parent directory with os.makedirs(...,
exist_ok=True) and set permissions (os.chmod) or use a umask so only the service
user can write the socket.
In `@sidecar/requirements.txt`:
- Around line 1-2: The requirements file lists unpinned packages "torch" and
"requests" which makes builds non-reproducible; update sidecar/requirements.txt
to pin each package to tested versions (e.g., replace "torch" with a specific
version like "torch==2.x.y" and "requests" with "requests==2.x.y") or add a
constraints file and reference it so installs are deterministic; ensure the
chosen versions match CI/tested matrix and update any build/dev docs or CI that
install these deps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cd776bd4-665b-4c53-9f59-f333f450f242
📒 Files selected for processing (15)
.gitignoreportmaster-ui/.gitignoreportmaster-ui/index.htmlportmaster-ui/package.jsonportmaster-ui/src/App.vueportmaster-ui/src/components/WarningCard.vueportmaster-ui/src/main.jsportmaster-ui/vite.config.jsservice/network/api.goservice/network/connection.goservice/network/telemetry.gosidecar/inference.pysidecar/main.pysidecar/model.pysidecar/requirements.txt
Comment on lines
+39
to
+47
| const formData = new FormData(); | ||
| // Since the UI doesn't inherently have the profile ID linked, we simulate passing it. | ||
| // In full impl, this would resolve the PID -> Profile ID. For now we pass the binary path as fallback. | ||
| formData.append('profile', this.processName); | ||
|
|
||
| const res = await fetch('http://127.0.0.1:817/api/v1/hids/quarantine', { | ||
| method: 'POST', | ||
| body: formData | ||
| }); |
Contributor
There was a problem hiding this comment.
Critical: Sending processName (binary path) instead of profile ID will cause quarantine to always fail.
The backend's handleHidsQuarantine (in service/network/api.go) calls profile.GetLocalProfile(profileID, nil, nil), which expects an actual profile identifier (e.g., "local/some-profile-id"), not a human-readable binary path like /usr/bin/curl.
As noted in the comment on lines 40-41, this needs to resolve PID → Profile ID. Without this, every quarantine attempt will fail with "profile not found".
Suggested approach
The alert data structure should include the actual profile ID from the backend. Modify the telemetry/alert flow to include profileID alongside binaryPath:
const formData = new FormData();
- // Since the UI doesn't inherently have the profile ID linked, we simulate passing it.
- // In full impl, this would resolve the PID -> Profile ID. For now we pass the binary path as fallback.
- formData.append('profile', this.processName);
+ formData.append('profile', this.profileId);This requires:
- Adding
profileIdprop to WarningCard - Including profile ID in the alert payload from the Python sidecar
- Including profile ID in the telemetry sent from Go
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@portmaster-ui/src/components/WarningCard.vue` around lines 39 - 47, The UI is
sending the binary path (this.processName) as the 'profile' field which causes
handleHidsQuarantine/profile.GetLocalProfile to fail; update WarningCard (and
the telemetry flow) to send the actual profile ID: add a profileId prop to
WarningCard, change the form payload to append('profile', this.profileId)
instead of this.processName in the POST to /api/v1/hids/quarantine, and ensure
the backend-facing telemetry/alert pipeline (Python sidecar and Go telemetry
sender) is updated to include profileId alongside binaryPath so consumers
calling profile.GetLocalProfile receive a valid profile identifier.
Comment on lines
+842
to
+844
| // Stream connection telemetry to HIDS sidecar before finalization | ||
| sendTelemetry(conn) | ||
|
|
Contributor
There was a problem hiding this comment.
Restrict telemetry emission to complete IP connections.
At Line 842, telemetry is emitted for all connection types during delete. That includes DNS request objects and incomplete records, which can contaminate anomaly inputs and increase sidecar noise.
Suggested guard
- // Stream connection telemetry to HIDS sidecar before finalization
- sendTelemetry(conn)
+ // Stream telemetry only for complete IP connections.
+ if conn.Type == IPConnection && conn.DataIsComplete() {
+ sendTelemetry(conn)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/network/connection.go` around lines 842 - 844, Telemetry is currently
emitted unconditionally via sendTelemetry(conn) during deletion, causing DNS and
incomplete records to be sent; wrap the sendTelemetry(conn) call with a guard
that only allows emission for finished IP connections (e.g., check the
connection object for an IP-type and a terminal/complete state such as conn.Type
== IP or conn.IsIP() and conn.State/Status == Complete/Closed or
conn.IsComplete()), or use any existing helper like conn.IsIP() or
conn.IsComplete() if present; place this conditional immediately before the
sendTelemetry(conn) invocation so only complete IP connections are streamed to
the HIDS sidecar.
Comment on lines
+24
to
+42
| func sendTelemetry(conn *Connection) { | ||
| if conn == nil { | ||
| return | ||
| } | ||
|
|
||
| ipStr := "" | ||
| if conn.Entity != nil { | ||
| ipStr = conn.Entity.IP.String() | ||
| } | ||
|
|
||
| telemetry := ConnectionTelemetry{ | ||
| PID: conn.PID, | ||
| BinaryPath: conn.ProcessContext.BinaryPath, | ||
| DestIP: ipStr, | ||
| BytesSent: conn.BytesSent, | ||
| BytesReceived: conn.BytesReceived, | ||
| Started: conn.Started, | ||
| Ended: conn.Ended, | ||
| } |
Contributor
There was a problem hiding this comment.
Data race: Reading lock-protected Connection fields without holding the lock.
Per the Connection struct documentation in service/network/connection.go:
Entity: "Access to entity must be guarded by the connection lock" (lines 116-120)Ended: "access must be guarded by the connection lock" (lines 139-143)BytesSent/BytesReceivedare also mutable counters
Although delete() comments state "Callers must still make sure to lock the connection itself," sendTelemetry reads these fields without verifying the lock is held. If the caller doesn't hold the lock, this creates a data race.
Recommended fix: Accept pre-extracted values or lock internally
Option 1: Lock within sendTelemetry (safest)
func sendTelemetry(conn *Connection) {
if conn == nil {
return
}
+ conn.Lock()
ipStr := ""
if conn.Entity != nil {
ipStr = conn.Entity.IP.String()
}
telemetry := ConnectionTelemetry{
PID: conn.PID,
BinaryPath: conn.ProcessContext.BinaryPath,
DestIP: ipStr,
BytesSent: conn.BytesSent,
BytesReceived: conn.BytesReceived,
Started: conn.Started,
Ended: conn.Ended,
}
+ conn.Unlock()Option 2: Pass pre-extracted telemetry struct from caller while lock is held
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/network/telemetry.go` around lines 24 - 42, sendTelemetry currently
reads mutable Connection fields (Entity, Ended, BytesSent, BytesReceived, etc.)
without holding the connection lock, causing data races; fix by either acquiring
the connection's lock inside sendTelemetry (lock/unlock around reads of
conn.Entity, conn.Ended, conn.BytesSent, conn.BytesReceived and any other
mutable fields before building ConnectionTelemetry) or change the API to accept
a pre-populated ConnectionTelemetry (or explicit values) that callers populate
while holding the connection lock; update signatures and call sites accordingly
(referencing sendTelemetry, Connection, ConnectionTelemetry and the fields
Entity/Ended/BytesSent/BytesReceived) so all mutable reads are protected.
Comment on lines
+87
to
+92
| url = f"http://127.0.0.1:{self.api_port}/api/v1/hids/alert" | ||
| try: | ||
| requests.post(url, json=payload, timeout=2) | ||
| logging.warning(f"ALERT TRIGGERED for PID {payload['pid']} (Score {score})") | ||
| except requests.exceptions.RequestException as e: | ||
| logging.error(f"Failed to send alert to Core: {e}") |
Contributor
There was a problem hiding this comment.
Critical: Content-Type mismatch - sending JSON but backend expects form data.
requests.post(url, json=payload) sends the payload as application/json, but the Go endpoint handleHidsAlert reads parameters using ar.Request.FormValue(), which expects application/x-www-form-urlencoded or multipart/form-data.
This will cause all alert parameters (pid, binaryPath, score) to be empty strings on the backend.
Fix: Send as form data instead of JSON
def trigger_alert(self, data: dict, score: float):
payload = {
"pid": data.get("pid"),
"binaryPath": data.get("binaryPath"),
"destIP": data.get("destIP"),
"score": score
}
url = f"http://127.0.0.1:{self.api_port}/api/v1/hids/alert"
try:
- requests.post(url, json=payload, timeout=2)
+ requests.post(url, data=payload, timeout=2)
logging.warning(f"ALERT TRIGGERED for PID {payload['pid']} (Score {score})")
except requests.exceptions.RequestException as e:
logging.error(f"Failed to send alert to Core: {e}")Note: data=payload sends as form-encoded. You may also need to convert numeric values to strings since form data is string-based.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sidecar/inference.py` around lines 87 - 92, The requests.post call currently
sends JSON (requests.post(url, json=payload)) but the Go handler handleHidsAlert
reads form values via Request.FormValue(), so change the send to use
form-encoded data (requests.post(url, data=payload, timeout=2)) and ensure
payload values (e.g., payload['pid'], payload['score'], payload['binaryPath'])
are strings (convert numeric score/pid to str) before sending; keep the same
try/except around the call and continue to log success/failure using
self.api_port, payload and score identifiers.
Comment on lines
+9
to
+15
| parser.add_argument("--api-port", type=int, required=True, help="Portmaster Core API Port") | ||
| parser.add_argument("--test-mode", action="store_true", help="Run model instantiation test and exit") | ||
|
|
||
| args = parser.parse_args() | ||
|
|
||
| engine = InferenceEngine(api_port=args.api_port, test_mode=args.test_mode) | ||
| engine.run() |
Contributor
There was a problem hiding this comment.
Make --api-port conditional when --test-mode is used.
At Line 9, --api-port is always required, but Line 10 describes a standalone test mode. This currently blocks --test-mode runs without a port.
Suggested CLI fix
- parser.add_argument("--api-port", type=int, required=True, help="Portmaster Core API Port")
+ parser.add_argument("--api-port", type=int, help="Portmaster Core API Port")
parser.add_argument("--test-mode", action="store_true", help="Run model instantiation test and exit")
args = parser.parse_args()
+ if not args.test_mode and args.api_port is None:
+ parser.error("--api-port is required unless --test-mode is set")
- engine = InferenceEngine(api_port=args.api_port, test_mode=args.test_mode)
+ engine = InferenceEngine(api_port=args.api_port or 0, test_mode=args.test_mode)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| parser.add_argument("--api-port", type=int, required=True, help="Portmaster Core API Port") | |
| parser.add_argument("--test-mode", action="store_true", help="Run model instantiation test and exit") | |
| args = parser.parse_args() | |
| engine = InferenceEngine(api_port=args.api_port, test_mode=args.test_mode) | |
| engine.run() | |
| parser.add_argument("--api-port", type=int, help="Portmaster Core API Port") | |
| parser.add_argument("--test-mode", action="store_true", help="Run model instantiation test and exit") | |
| args = parser.parse_args() | |
| if not args.test_mode and args.api_port is None: | |
| parser.error("--api-port is required unless --test-mode is set") | |
| engine = InferenceEngine(api_port=args.api_port or 0, test_mode=args.test_mode) | |
| engine.run() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sidecar/main.py` around lines 9 - 15, Change the CLI so --api-port is not
always required: make parser.add_argument("--api-port", ..., required=False) and
after args = parser.parse_args() add a validation that raises parser.error when
args.test_mode is False and args.api_port is None (e.g., if not args.test_mode
and args.api_port is None: parser.error("the --api-port argument is required
unless --test-mode is used")). Keep passing api_port=args.api_port into
InferenceEngine so test_mode can run without supplying a port.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements a full Host-based Intrusion Detection/Prevention System (HIDS/HIPS) for Portmaster across three domains:
Core (Go): Streaming local connection metrics over UDS sockets without blocking network filtration.
Sidecar (Python): A PyTorch anomaly detection daemon that learns tabular behavior metrics.
UI (Vue3): Frontend visualization of suspended states allowing one-click quarantine actions that propagate down through Portmaster's hierarchical configurations.
Summary by CodeRabbit